mirror/aseprite
1
0
Fork 0
mirror of https://github.com/aseprite/aseprite.git synced 2024-10-04 13:59:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix crashes with SpriteJob(s) that weren't locking the doc correctly (fix #4315)

Browse Source 
This was mainly found in SpriteSizeJob crash reports. In these reports
deleted image buffers were still used to paint the Editor canvas
because the doc was write-locked in the main thread (same thread where
the canvas is painted). This produced a re-entrant lock in the
Editor::onPaint() as we can still read-lock from the same thread where
we write-locked the doc.

With this change we write-lock the doc from the SpriteJob background
thread (not the main thread) only if it's necessary (i.e. when the doc
is not already locked in the main thread, e.g. when running a script).
This makes that the main thread (Editor::onPaint) cannot read the doc
until we finish the whole SpriteJob transaction/Tx.
This commit is contained in:
David Capello 2024-03-25 15:37:11 -03:00
parent 427ee6f5b5
commit ec4e82bdc0
8 changed files with 125 additions and 73 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
src/app/commands/cmd_change_pixel_format.cpp
View File

@ -1,5 +1,5 @@
// Aseprite
// Copyright (C) 2019-2023 Igara Studio S.A.
// Copyright (C) 2019-2024 Igara Studio S.A.
// Copyright (C) 2001-2018 David Capello
//
// This program is distributed under the terms of
@ -646,12 +646,12 @@ void ChangePixelFormatCommand::onExecute(Context* context)
#endif // ENABLE_UI
// No conversion needed
if (context->activeDocument()->sprite()->pixelFormat() == m_format)
Doc* doc = context->activeDocument();
if (doc->sprite()->pixelFormat() == m_format)
return;
{
const ContextReader reader(context);
SpriteJob job(reader, Strings::color_mode_title().c_str());
SpriteJob job(context, doc, Strings::color_mode_title());
Sprite* sprite(job.sprite());
// TODO this was moved in the main UI thread because
@ -662,16 +662,17 @@ void ChangePixelFormatCommand::onExecute(Context* context)
// https://github.com/aseprite/aseprite/issues/509
// https://github.com/aseprite/aseprite/issues/378
if (flatten) {
Tx tx(Tx::LockDoc, context, doc);
const bool newBlend = Preferences::instance().experimental.newBlend();
SelectedLayers selLayers;
for (auto layer : sprite->root()->layers())
selLayers.insert(layer);
job.tx()(new cmd::FlattenLayers(sprite, selLayers, newBlend));
tx(new cmd::FlattenLayers(sprite, selLayers, newBlend));
}
job.startJobWithCallback(
[this, &job, sprite] {
job.tx()(
[this, &job, sprite](Tx& tx) {
tx(
new cmd::SetPixelFormat(
sprite, m_format,
m_dithering,

47
src/app/commands/cmd_color_quantization.cpp
View File

@ -1,5 +1,5 @@
// Aseprite
// Copyright (C) 2019-2023 Igara Studio S.A.
// Copyright (C) 2019-2024 Igara Studio S.A.
// Copyright (C) 2001-2018 David Capello
//
// This program is distributed under the terms of
@ -177,45 +177,46 @@ void ColorQuantizationCommand::onExecute(Context* ctx)
return;
try {
ContextReader reader(ctx);
Doc* doc = site.document();
Sprite* sprite = site.sprite();
frame_t frame = site.frame();
const Palette* curPalette = site.sprite()->palette(frame);
Palette tmpPalette(frame, entries.picks());
SpriteJob job(reader, "Color Quantization");
SpriteJob job(ctx, doc, "Color Quantization");
const bool newBlend = pref.experimental.newBlend();
job.startJobWithCallback(
[sprite, withAlpha, &tmpPalette, &job, newBlend, algorithm]{
[sprite, withAlpha, curPalette, &tmpPalette, &job, &entries,
newBlend, algorithm, createPal, site, frame](Tx& tx) {
render::create_palette_from_sprite(
sprite, 0, sprite->lastFrame(),
withAlpha, &tmpPalette,
&job, // SpriteJob is a render::TaskDelegate
newBlend,
algorithm);
std::unique_ptr<Palette> newPalette(
new Palette(createPal ? tmpPalette:
*site.palette()));
if (createPal) {
entries = PalettePicks(newPalette->size());
entries.all();
}
int i = 0, j = 0;
for (bool state : entries) {
if (state)
newPalette->setEntry(i, tmpPalette.getEntry(j++));
++i;
}
if (*curPalette != *newPalette)
tx(new cmd::SetPalette(sprite, frame, newPalette.get()));
});
job.waitJob();
if (job.isCanceled())
return;
std::unique_ptr<Palette> newPalette(
new Palette(createPal ? tmpPalette:
*site.palette()));
if (createPal) {
entries = PalettePicks(newPalette->size());
entries.all();
}
int i = 0, j = 0;
for (bool state : entries) {
if (state)
newPalette->setEntry(i, tmpPalette.getEntry(j++));
++i;
}
if (*curPalette != *newPalette)
job.tx()(new cmd::SetPalette(sprite, frame, newPalette.get()));
}
catch (const base::Exception& e) {
Console::showException(e);

21
src/app/commands/cmd_rotate.cpp
View File

@ -1,5 +1,5 @@
// Aseprite
// Copyright (C) 2019-2022 Igara Studio S.A.
// Copyright (C) 2019-2024 Igara Studio S.A.
// Copyright (C) 2001-2018 David Capello
//
// This program is distributed under the terms of
@ -13,7 +13,6 @@
#include "app/cmd/set_cel_bounds.h"
#include "app/commands/cmd_rotate.h"
#include "app/commands/params.h"
#include "app/context_access.h"
#include "app/doc_api.h"
#include "app/doc_range.h"
#include "app/i18n/strings.h"
@ -45,10 +44,10 @@ class RotateJob : public SpriteJob {
public:
RotateJob(const ContextReader& reader,
RotateJob(Context* ctx, Doc* doc,
const std::string& jobName,
int angle, const CelList& cels, bool rotateSprite)
: SpriteJob(reader, jobName.c_str())
: SpriteJob(ctx, doc, jobName)
, m_cels(cels)
, m_rotateSprite(rotateSprite) {
m_angle = angle;
@ -80,8 +79,8 @@ protected:
}
// [working thread]
void onJob() override {
DocApi api = document()->getApi(tx());
void onSpriteJob(Tx& tx) override {
DocApi api = document()->getApi(tx);
// 1) Rotate cel positions
for (Cel* cel : m_cels) {
@ -93,7 +92,7 @@ protected:
gfx::RectF bounds = cel->boundsF();
rotate_rect(bounds);
if (cel->boundsF() != bounds)
tx()(new cmd::SetCelBoundsF(cel, bounds));
tx(new cmd::SetCelBoundsF(cel, bounds));
}
else {
gfx::Rect bounds = cel->bounds();
@ -192,6 +191,7 @@ void RotateCommand::onExecute(Context* context)
{
{
Site site = context->activeSite();
Doc* doc = site.document();
CelList cels;
bool rotateSprite = false;
@ -203,7 +203,7 @@ void RotateCommand::onExecute(Context* context)
// If we want to rotate the visible mask, we can go to
// MovingPixelsState (even when the range is enabled, because
// now PixelsMovement support ranges).
if (site.document()->isMaskVisible()) {
if (doc->isMaskVisible()) {
// Select marquee tool
if (tools::Tool* tool = App::instance()->toolBox()
->getToolById(tools::WellKnownTools::RectangularMarquee)) {
@ -237,13 +237,12 @@ void RotateCommand::onExecute(Context* context)
rotateSprite = true;
}
ContextReader reader(context);
{
RotateJob job(reader, friendlyName(), m_angle, cels, rotateSprite);
RotateJob job(context, doc, friendlyName(), m_angle, cels, rotateSprite);
job.startJob();
job.waitJob();
}
update_screen_for_document(reader.document());
update_screen_for_document(doc);
}
}

30
src/app/commands/cmd_sprite_size.cpp
View File

@ -1,5 +1,5 @@
// Aseprite
// Copyright (C) 2019-2023 Igara Studio S.A.
// Copyright (C) 2019-2024 Igara Studio S.A.
// Copyright (C) 2001-2018 David Capello
//
// This program is distributed under the terms of
@ -80,8 +80,11 @@ class SpriteSizeJob : public SpriteJob {
public:
SpriteSizeJob(const ContextReader& reader, int new_width, int new_height, ResizeMethod resize_method)
: SpriteJob(reader, Strings::sprite_size_title().c_str()) {
SpriteSizeJob(Context* ctx, Doc* doc,
const int new_width,
const int new_height,
const ResizeMethod resize_method)
: SpriteJob(ctx, doc, Strings::sprite_size_title()) {
m_new_width = new_width;
m_new_height = new_height;
m_resize_method = resize_method;
@ -90,8 +93,8 @@ public:
protected:
// [working thread]
void onJob() override {
DocApi api = writer().document()->getApi(tx());
void onSpriteJob(Tx& tx) override {
DocApi api = document()->getApi(tx);
Tilesets* tilesets = sprite()->tilesets();
int img_count = 0;
@ -147,7 +150,7 @@ protected:
++progress;
++idx;
}
tx()(new cmd::ReplaceTileset(sprite(), tsi, newTileset));
tx(new cmd::ReplaceTileset(sprite(), tsi, newTileset));
// Cancel all the operation?
if (isCanceled())
@ -170,11 +173,11 @@ protected:
cel->y()*scale.h,
canvasSize.w,
canvasSize.h);
tx()(new cmd::SetCelBoundsF(cel, newBounds));
tx(new cmd::SetCelBoundsF(cel, newBounds));
}
else {
resize_cel_image(
tx(), cel, scale,
tx, cel, scale,
m_resize_method,
cel->layer()->isReference() ?
-cel->boundsF().origin():
@ -239,7 +242,7 @@ protected:
newKey.setPivot(gfx::Point(scale_x(newKey.pivot().x),
scale_y(newKey.pivot().y)));
tx()(new cmd::SetSliceKey(slice, k.frame(), newKey));
tx(new cmd::SetSliceKey(slice, k.frame(), newKey));
}
}
@ -373,8 +376,9 @@ void SpriteSizeCommand::onExecute(Context* context)
#ifdef ENABLE_UI
const bool ui = (params().ui() && context->isUIAvailable());
#endif
const ContextReader reader(context);
const Sprite* sprite(reader.sprite());
const Site site = context->activeSite();
Doc* doc = site.document();
Sprite* sprite = site.sprite();
auto& params = this->params();
double ratio = sprite->width() / double(sprite->height());
@ -461,13 +465,13 @@ void SpriteSizeCommand::onExecute(Context* context)
new_height = std::clamp(new_height, 1, DOC_SPRITE_MAX_HEIGHT);
{
SpriteSizeJob job(reader, new_width, new_height, resize_method);
SpriteSizeJob job(context, doc, new_width, new_height, resize_method);
job.startJob();
job.waitJob();
}
#ifdef ENABLE_UI
update_screen_for_document(reader.document());
update_screen_for_document(doc);
#endif
}

4
src/app/job.cpp
View File

@ -1,5 +1,5 @@
// Aseprite
// Copyright (C) 2021 Igara Studio S.A.
// Copyright (C) 2021-2024 Igara Studio S.A.
// Copyright (C) 2001-2018 David Capello
//
// This program is distributed under the terms of
@ -33,7 +33,7 @@ int Job::runningJobs()
return g_runningJobs;
}
Job::Job(const char* jobName)
Job::Job(const std::string& jobName)
{
m_last_progress = 0.0;
m_done_flag = false;

5
src/app/job.h
View File

@ -1,5 +1,5 @@
// Aseprite
// Copyright (C) 2021 Igara Studio S.A.
// Copyright (C) 2021-2024 Igara Studio S.A.
// Copyright (C) 2001-2018 David Capello
//
// This program is distributed under the terms of
@ -15,6 +15,7 @@
#include <atomic>
#include <exception>
#include <mutex>
#include <string>
#include <thread>
namespace app {
@ -23,7 +24,7 @@ namespace app {
public:
static int runningJobs();
Job(const char* jobName);
Job(const std::string& jobName);
virtual ~Job();
// Starts the job calling onJob() event in another thread and

40
src/app/sprite_job.cpp
View File

@ -1,4 +1,5 @@
// Aseprite
// Copyright (C) 2024 Igara Studio S.A.
// Copyright (C) 2017 David Capello
//
// This program is distributed under the terms of
@ -10,26 +11,49 @@
#include "app/sprite_job.h"
#include "base/log.h"
namespace app {
SpriteJob::SpriteJob(const ContextReader& reader, const char* jobName)
SpriteJob::SpriteJob(Context* ctx, Doc* doc,
const std::string& jobName)
: Job(jobName)
, m_writer(reader, 500)
, m_document(m_writer.document())
, m_sprite(m_writer.sprite())
, m_tx(m_writer, jobName, ModifyDocument)
, m_doc(doc)
, m_sprite(doc->sprite())
, m_tx(Tx::DontLockDoc, ctx, doc, jobName, ModifyDocument)
, m_lockAction(Tx::LockDoc)
{
// Try to write-lock the document to see if we have to lock the
// document in the background thread.
auto lockResult = m_doc->writeLock(500);
if (lockResult != Doc::LockResult::Fail) {
if (lockResult == Doc::LockResult::Reentrant)
m_lockAction = Tx::DontLockDoc;
m_doc->unlock(lockResult);
}
}
SpriteJob::~SpriteJob()
{
if (!isCanceled())
m_tx.commit();
try {
if (!isCanceled())
m_tx.commit();
}
catch (const std::exception& ex) {
LOG(ERROR, "Error committing changes: %s\n", ex.what());
}
}
void SpriteJob::onSpriteJob(Tx& tx)
{
if (m_callback)
m_callback(tx);
}
void SpriteJob::onJob()
{
m_callback();
Tx subtx(m_lockAction, m_ctx, m_doc);
onSpriteJob(subtx);
}
bool SpriteJob::continueTask()

36
src/app/sprite_job.h
View File

@ -1,4 +1,5 @@
// Aseprite
// Copyright (C) 2024 Igara Studio S.A.
// Copyright (C) 2017-2018 David Capello
//
// This program is distributed under the terms of
@ -15,19 +16,32 @@
#include "render/task_delegate.h"
#include <functional>
#include <memory>
#include <string>
namespace app {
class Context;
// Creates a Job to run a task in a background thread. At the same
// time it creates a new Tx in the main thread (to group all sub-Txs)
// without locking the sprite. You have to lock the sprite with a sub
// Tx (the onSpriteJob(Tx) already has the sprite locked for write
// access).
//
// This class takes care to lock the sprite in the background thread
// for write access, or to avoid re-locking the sprite in case it's
// already locked from the main thread (where SpriteJob was created,
// generally true when we're running a script).
class SpriteJob : public Job,
public render::TaskDelegate {
public:
SpriteJob(const ContextReader& reader, const char* jobName);
SpriteJob(Context* ctx, Doc* doc,
const std::string& jobName);
~SpriteJob();
ContextWriter& writer() { return m_writer; }
Doc* document() const { return m_document; }
Doc* document() const { return m_doc; }
Sprite* sprite() const { return m_sprite; }
Tx& tx() { return m_tx; }
template<typename T>
void startJobWithCallback(T&& callback) {
@ -36,6 +50,8 @@ public:
}
private:
virtual void onSpriteJob(Tx& tx);
// Job impl
void onJob() override;
@ -44,15 +60,21 @@ private:
bool continueTask() override;
void notifyTaskProgress(double progress) override;
ContextWriter m_writer;
Doc* m_document;
Context* m_ctx;
Doc* m_doc;
Sprite* m_sprite;
Tx m_tx;
// What action to do with the sub-Tx inside the background thread.
// This is required to check if the sprite is already locked for
// write access in the main thread, in that case we don't need to
// lock it again from the background thread.
Tx::LockAction m_lockAction;
// Default implementation calls the given function in
// startJob(). Anyway you can just extended the SpriteJob and
// override onJob().
std::function<void()> m_callback;
std::function<void(Tx&)> m_callback;
};
} // namespace app