From f430b9ce776c89587abb7d93082bf9247bff7ceb Mon Sep 17 00:00:00 2001 From: David Capello Date: Tue, 4 Jun 2019 10:58:19 -0300 Subject: [PATCH] Avoid accessing Preferences from background thread when loading preset palettes --- src/app/CMakeLists.txt | 1 + src/app/file/file.cpp | 42 +++++++++++++++--------- src/app/file/file.h | 23 ++++++------- src/app/file/file_op_config.cpp | 27 +++++++++++++++ src/app/file/file_op_config.h | 40 ++++++++++++++++++++++ src/app/file/palette_file.cpp | 8 +++-- src/app/file/palette_file.h | 5 ++- src/app/res/palettes_loader_delegate.cpp | 11 ++++++- src/app/res/palettes_loader_delegate.h | 7 ++++ 9 files changed, 131 insertions(+), 33 deletions(-) create mode 100644 src/app/file/file_op_config.cpp create mode 100644 src/app/file/file_op_config.h diff --git a/src/app/CMakeLists.txt b/src/app/CMakeLists.txt index fdc1630c4..d379f8aa3 100644 --- a/src/app/CMakeLists.txt +++ b/src/app/CMakeLists.txt @@ -542,6 +542,7 @@ add_library(app-lib file/file_data.cpp file/file_format.cpp file/file_formats_manager.cpp + file/file_op_config.cpp file/palette_file.cpp file/split_filename.cpp file_system.cpp diff --git a/src/app/file/file.cpp b/src/app/file/file.cpp index ea906d1e3..758d6f253 100644 --- a/src/app/file/file.cpp +++ b/src/app/file/file.cpp @@ -41,6 +41,7 @@ #include "render/render.h" #include "ui/alert.h" #include "ui/listitem.h" +#include "ui/system.h" #include "ask_for_color_profile.xml.h" #include "open_sequence.xml.h" @@ -177,10 +178,13 @@ FileOpROI::FileOpROI(const Doc* doc, } // static -FileOp* FileOp::createLoadDocumentOperation(Context* context, const std::string& filename, int flags) +FileOp* FileOp::createLoadDocumentOperation(Context* context, + const std::string& filename, + const int flags, + const FileOpConfig* config) { std::unique_ptr fop( - new FileOp(FileOpLoad, context)); + new FileOp(FileOpLoad, context, config)); if (!fop) return nullptr; @@ -329,7 +333,7 @@ FileOp* FileOp::createSaveDocumentOperation(const Context* context, const bool ignoreEmptyFrames) { std::unique_ptr fop( - new FileOp(FileOpSave, const_cast(context))); + new FileOp(FileOpSave, const_cast(context), nullptr)); // Document to save fop->m_document = const_cast(roi.document()); @@ -711,7 +715,7 @@ void FileOp::operate(IFileOpProgress* progress) try { load_aseprite_data_file(m_dataFilename, m_document, - m_defaultSliceColor); + m_config.defaultSliceColor); } catch (const std::exception& ex) { setError("Error loading data file: %s\n", ex.what()); @@ -739,7 +743,7 @@ void FileOp::operate(IFileOpProgress* progress) // For each frame in the sprite. render::Render render; - render.setNewBlend(m_newBlend); + render.setNewBlend(m_config.newBlend); frame_t outputFrame = 0; for (frame_t frame : m_roi.selectedFrames()) { @@ -903,7 +907,7 @@ void FileOp::postLoad() base::SharedPtr palette( render::create_palette_from_sprite( sprite, frame_t(0), sprite->lastFrame(), true, - nullptr, nullptr, Preferences::instance().experimental.newBlend())); + nullptr, nullptr, m_config.newBlend)); sprite->resetPalettes(); sprite->setPalette(palette.get(), false); @@ -915,10 +919,10 @@ void FileOp::postLoad() app::gen::ColorProfileBehavior behavior = app::gen::ColorProfileBehavior::DISABLE; - if (m_preserveColorProfile) { + if (m_config.preserveColorProfile) { // Embedded color profile if (this->hasEmbeddedColorProfile()) { - behavior = Preferences::instance().color.filesWithProfile(); + behavior = m_config.filesWithProfile; if (behavior == app::gen::ColorProfileBehavior::ASK) { #ifdef ENABLE_UI if (m_context && m_context->isUIAvailable()) { @@ -944,7 +948,7 @@ void FileOp::postLoad() } // Missing color space else { - behavior = Preferences::instance().color.missingProfile(); + behavior = m_config.missingProfile; if (behavior == app::gen::ColorProfileBehavior::ASK) { #ifdef ENABLE_UI if (m_context && m_context->isUIAvailable()) { @@ -982,7 +986,7 @@ void FileOp::postLoad() case app::gen::ColorProfileBehavior::CONVERT: { // Convert to the working color profile - auto gfxCS = get_working_rgb_space_from_preferences(); + auto gfxCS = m_config.workingCS; if (!gfxCS->nearlyEqual(*spriteCS)) cmd::convert_color_profile(sprite, gfxCS); break; @@ -990,7 +994,7 @@ void FileOp::postLoad() case app::gen::ColorProfileBehavior::ASSIGN: { // Convert to the working color profile - auto gfxCS = get_working_rgb_space_from_preferences(); + auto gfxCS = m_config.workingCS; sprite->setColorSpace(gfxCS); m_document->notifyColorSpaceChanged(); break; @@ -1178,7 +1182,9 @@ bool FileOp::isStop() const return stop; } -FileOp::FileOp(FileOpType type, Context* context) +FileOp::FileOp(FileOpType type, + Context* context, + const FileOpConfig* config) : m_type(type) , m_format(nullptr) , m_context(context) @@ -1190,11 +1196,17 @@ FileOp::FileOp(FileOpType type, Context* context) , m_oneframe(false) , m_createPaletteFromRgba(false) , m_ignoreEmpty(false) - , m_preserveColorProfile(Preferences::instance().color.manage()) , m_embeddedColorProfile(false) - , m_newBlend(Preferences::instance().experimental.newBlend()) - , m_defaultSliceColor(Preferences::instance().slices.defaultColor()) { + if (config) + m_config = *config; + else if (ui::is_ui_thread()) + m_config.fillFromPreferences(); + else { + LOG(VERBOSE, "FILE: Using a file operation with default configuration\n"); + ASSERT(false); + } + m_seq.palette = nullptr; m_seq.image.reset(); m_seq.progress_offset = 0.0f; diff --git a/src/app/file/file.h b/src/app/file/file.h index 250616654..792c157ae 100644 --- a/src/app/file/file.h +++ b/src/app/file/file.h @@ -10,6 +10,8 @@ #pragma once #include "app/color.h" +#include "app/file/file_op_config.h" +#include "app/pref/preferences.h" #include "base/mutex.h" #include "base/paths.h" #include "base/shared_ptr.h" @@ -102,7 +104,8 @@ namespace app { public: static FileOp* createLoadDocumentOperation(Context* context, const std::string& filename, - int flags); + const int flags, + const FileOpConfig* config = nullptr); static FileOp* createSaveDocumentOperation(const Context* context, const FileOpROI& roi, @@ -114,7 +117,7 @@ namespace app { bool isSequence() const { return !m_seq.filename_list.empty(); } bool isOneFrame() const { return m_oneframe; } - bool preserveColorProfile() const { return m_preserveColorProfile; } + bool preserveColorProfile() const { return m_config.preserveColorProfile; } const std::string& filename() const { return m_filename; } const base::paths& filenames() const { return m_seq.filename_list; } @@ -175,11 +178,13 @@ namespace app { void setEmbeddedColorProfile() { m_embeddedColorProfile = true; } bool hasEmbeddedColorProfile() const { return m_embeddedColorProfile; } - bool newBlend() const { return m_newBlend; } + bool newBlend() const { return m_config.newBlend; } private: FileOp(); // Undefined - FileOp(FileOpType type, Context* context); + FileOp(FileOpType type, + Context* context, + const FileOpConfig* config); FileOpType m_type; // Operation type: 0=load, 1=save. FileFormat* m_format; @@ -204,18 +209,10 @@ namespace app { bool m_createPaletteFromRgba; bool m_ignoreEmpty; - // Return if we've to save/embed the color space of the document - // in the file. - bool m_preserveColorProfile; - // True if the file contained a color profile when it was loaded. bool m_embeddedColorProfile; - // True if we should render each frame to save it with the new - // blend mode. - bool m_newBlend; - - app::Color m_defaultSliceColor; + FileOpConfig m_config; base::SharedPtr m_formatOptions; diff --git a/src/app/file/file_op_config.cpp b/src/app/file/file_op_config.cpp new file mode 100644 index 000000000..b64a6f15f --- /dev/null +++ b/src/app/file/file_op_config.cpp @@ -0,0 +1,27 @@ +// Aseprite +// Copyright (C) 2019 Igara Studio S.A. +// +// This program is distributed under the terms of +// the End-User License Agreement for Aseprite. + +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include "app/file/file_op_config.h" + +#include "app/color_spaces.h" + +namespace app { + +void FileOpConfig::fillFromPreferences() +{ + preserveColorProfile = Preferences::instance().color.manage(); + filesWithProfile = Preferences::instance().color.filesWithProfile(); + missingProfile = Preferences::instance().color.missingProfile(); + newBlend = Preferences::instance().experimental.newBlend(); + defaultSliceColor = Preferences::instance().slices.defaultColor(); + workingCS = get_working_rgb_space_from_preferences(); +} + +} // namespace app diff --git a/src/app/file/file_op_config.h b/src/app/file/file_op_config.h new file mode 100644 index 000000000..2f31a69df --- /dev/null +++ b/src/app/file/file_op_config.h @@ -0,0 +1,40 @@ +// Aseprite +// Copyright (C) 2019 Igara Studio S.A. +// +// This program is distributed under the terms of +// the End-User License Agreement for Aseprite. + +#ifndef APP_FILE_FILE_OP_CONFIG_H_INCLUDED +#define APP_FILE_FILE_OP_CONFIG_H_INCLUDED +#pragma once + +#include "app/color.h" +#include "app/pref/preferences.h" +#include "gfx/color_space.h" + +namespace app { + + // Options that came from Preferences but can be used in the non-UI thread. + struct FileOpConfig { + // True if we have to save/embed the color space of the document + // in the file. + bool preserveColorProfile = true; + + // Configuration of what to do when we load a file with a color + // profile or without a color profile. + app::gen::ColorProfileBehavior filesWithProfile = app::gen::ColorProfileBehavior::EMBEDDED; + app::gen::ColorProfileBehavior missingProfile = app::gen::ColorProfileBehavior::ASSIGN; + gfx::ColorSpacePtr workingCS = gfx::ColorSpace::MakeSRGB(); + + // True if we should render each frame to save it with the new + // blend mode.h + bool newBlend = true; + + app::Color defaultSliceColor = app::Color::fromRgb(0, 0, 255); + + void fillFromPreferences(); + }; + +} // namespace app + +#endif diff --git a/src/app/file/palette_file.cpp b/src/app/file/palette_file.cpp index eac5323ad..766241ce6 100644 --- a/src/app/file/palette_file.cpp +++ b/src/app/file/palette_file.cpp @@ -1,5 +1,5 @@ // Aseprite -// Copyright (C) 2018 Igara Studio S.A. +// Copyright (C) 2018-2019 Igara Studio S.A. // Copyright (C) 2001-2018 David Capello // // This program is distributed under the terms of @@ -52,7 +52,8 @@ base::paths get_writable_palette_extensions() return paths; } -Palette* load_palette(const char* filename) +Palette* load_palette(const char* filename, + const FileOpConfig* config) { dio::FileFormat dioFormat = dio::detect_format(filename); Palette* pal = nullptr; @@ -89,7 +90,8 @@ Palette* load_palette(const char* filename) nullptr, filename, FILE_LOAD_CREATE_PALETTE | FILE_LOAD_SEQUENCE_NONE | - FILE_LOAD_ONE_FRAME)); + FILE_LOAD_ONE_FRAME, + config)); if (fop && !fop->hasError()) { fop->operate(nullptr); diff --git a/src/app/file/palette_file.h b/src/app/file/palette_file.h index fce529df1..43a6aa44a 100644 --- a/src/app/file/palette_file.h +++ b/src/app/file/palette_file.h @@ -1,4 +1,5 @@ // Aseprite +// Copyright (C) 2019 Igara Studio S.A. // Copyright (C) 2001-2018 David Capello // // This program is distributed under the terms of @@ -15,11 +16,13 @@ namespace doc { } namespace app { + struct FileOpConfig; base::paths get_readable_palette_extensions(); base::paths get_writable_palette_extensions(); - doc::Palette* load_palette(const char *filename); + doc::Palette* load_palette(const char *filename, + const FileOpConfig* config = nullptr); bool save_palette(const char *filename, const doc::Palette* pal, int columns); diff --git a/src/app/res/palettes_loader_delegate.cpp b/src/app/res/palettes_loader_delegate.cpp index 76ee60245..4c6ea81a9 100644 --- a/src/app/res/palettes_loader_delegate.cpp +++ b/src/app/res/palettes_loader_delegate.cpp @@ -1,4 +1,5 @@ // Aseprite +// Copyright (C) 2019 Igara Studio S.A. // Copyright (C) 2001-2017 David Capello // // This program is distributed under the terms of @@ -20,9 +21,17 @@ #include "base/fs.h" #include "base/scoped_value.h" #include "doc/palette.h" +#include "ui/system.h" namespace app { +PalettesLoaderDelegate::PalettesLoaderDelegate() +{ + // Necessary to load preferences in the UI-thread which will be used + // in a FileOp executed in a background thread. + m_config.fillFromPreferences(); +} + void PalettesLoaderDelegate::getResourcesPaths(std::map& idAndPath) const { // Include extension palettes @@ -55,7 +64,7 @@ void PalettesLoaderDelegate::getResourcesPaths(std::map& idAndPath) const override; virtual Resource* loadResource(const std::string& id, const std::string& path) override; + + private: + FileOpConfig m_config; }; } // namespace app