From cec8ba95f0ed5c58b4e7cf49109f549da5d49183 Mon Sep 17 00:00:00 2001 From: masterpiga Date: Thu, 9 Jul 2026 21:08:35 +0200 Subject: [PATCH 1/2] Don't invalidate the pixelpipe cache on every commit when a raster mask is used MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dt_iop_commit_blend_params() registers a module as a user of a raster-mask source, and is supposed to report only genuinely new registrations so the caller can invalidate that source's cache once. Instead it returned the source module unconditionally on every call, so any history commit anywhere in the pipe was treated as "a source just gained a raster user," discarding every cacheline from the source onward — even though almost all of those calls were re-registering an existing user, not adding a new one. The invalidation itself is legitimate for the one case it exists for: a source that gains a new user doesn't change its own params, so its cacheline hash stays the same and it could otherwise be served from cache without ever writing the mask the new user needs. But firing on every commit instead of once means that a raster source sitting early in the pipe — a common setup, e.g. a mask painted on an early exposure/tone module and consumed by a later one — forces a full re-render of the pipe tail on every unrelated slider move downstream. This is invisible on small images and short pipes, and increasingly costly as either grows, since a full-resolution intermediate buffer can be hundreds of MB and won't re-establish itself between edits. The fix makes the "new" check (already computed via g_hash_table_insert's return value, but previously used only for a debug print) gate the return value, so the function does what its own doc comment already promised: return NULL except for a genuinely new source registration. Found while profiling a downstream fork, where a raster-masked pipe re-executed from the source on every mask edit (~2.8s) instead of only recomputing the touched module (~0.17s). The invalidation mechanism and the -d pipe evidence are identical on master, so stock darktable pays the same unnecessary cost, scaled to image size and how early the raster source sits in the pipe. --- src/develop/develop.c | 4 +-- src/develop/imageop.c | 83 ++++++++++++++++++++++++++----------------- src/develop/imageop.h | 13 ++++--- src/gui/presets.c | 4 +-- 4 files changed, 63 insertions(+), 41 deletions(-) diff --git a/src/develop/develop.c b/src/develop/develop.c index 89296ba62fcb..e264f34002f1 100644 --- a/src/develop/develop.c +++ b/src/develop/develop.c @@ -1624,7 +1624,7 @@ void dt_dev_pop_history_items_ext(dt_develop_t *dev, const int32_t cnt) { dt_iop_module_t *module = modules->data; memcpy(module->params, module->default_params, module->params_size); - dt_iop_commit_blend_params(module, module->default_blendop_params); + dt_iop_commit_blend_params(module, module->default_blendop_params, NULL); module->enabled = module->default_enabled; if(module->multi_priority == 0) @@ -1646,7 +1646,7 @@ void dt_dev_pop_history_items_ext(dt_develop_t *dev, const int32_t cnt) memcpy(hist->module->params, hist->module->default_params, hist->module->params_size); else memcpy(hist->module->params, hist->params, hist->module->params_size); - dt_iop_commit_blend_params(hist->module, hist->blend_params); + dt_iop_commit_blend_params(hist->module, hist->blend_params, NULL); hist->module->iop_order = hist->iop_order; hist->module->enabled = hist->enabled; diff --git a/src/develop/imageop.c b/src/develop/imageop.c index 6fe982d0bbb6..092cb79d81df 100644 --- a/src/develop/imageop.c +++ b/src/develop/imageop.c @@ -79,7 +79,7 @@ void dt_iop_load_default_params(dt_iop_module_t *module) dt_develop_blend_colorspace_t cst = dt_develop_blend_default_module_blend_colorspace(module); dt_develop_blend_init_blend_parameters(module->default_blendop_params, cst); - dt_iop_commit_blend_params(module, module->default_blendop_params); + dt_iop_commit_blend_params(module, module->default_blendop_params, NULL); dt_iop_gui_blending_reload_defaults(module); } @@ -428,7 +428,7 @@ gboolean dt_iop_load_module_by_so(dt_iop_module_t *module, dt_develop_blend_colorspace_t cst = dt_develop_blend_default_module_blend_colorspace(module); dt_develop_blend_init_blend_parameters(module->default_blendop_params, cst); - dt_iop_commit_blend_params(module, module->default_blendop_params); + dt_iop_commit_blend_params(module, module->default_blendop_params, NULL); if(module->params_size == 0) { @@ -759,7 +759,7 @@ dt_iop_module_t *dt_iop_gui_duplicate(dt_iop_module_t *base, memcpy(module->params, base->params, module->params_size); if(module->flags() & IOP_FLAGS_SUPPORTS_BLENDING) { - dt_iop_commit_blend_params(module, base->blend_params); + dt_iop_commit_blend_params(module, base->blend_params, NULL); if(dt_is_valid_maskid(base->blend_params->mask_id)) { module->blend_params->mask_id = NO_MASKID; @@ -1916,12 +1916,17 @@ void dt_iop_advertise_rastermask(dt_iop_module_t *module, const int mask_mode) 1. Handling of raster mask users must only be done if we don't use module's default blending parameters. 2. Also watch out for a raster mask source module to get it's first `target` - entry, if so we should invalidate all cachelines from modules with a higher iop order - in dt_iop_commit_blend_params() callers. - To support this, dt_iop_commit_blend_params() either returns NULL or the source module. + entry: a source that gains a new user doesn't change its own params, so its + cacheline hash is unchanged and it could be served from cache without ever + writing the raster mask the new user depends on. When that happens and `pipe` + is given, invalidate that source's cachelines onward so it re-runs and writes + the mask. This must fire only for a genuinely new registration, not on every + commit, or every edit downstream of an early raster source forces a full + re-render of the pipe tail. */ -dt_iop_module_t *dt_iop_commit_blend_params(dt_iop_module_t *module, - const dt_develop_blend_params_t *blendop_params) +void dt_iop_commit_blend_params(dt_iop_module_t *module, + const dt_develop_blend_params_t *blendop_params, + dt_dev_pixelpipe_t *pipe) { memcpy(module->blend_params, blendop_params, sizeof(dt_develop_blend_params_t)); if(blendop_params->blend_cst == DEVELOP_BLEND_CS_NONE) @@ -1938,7 +1943,7 @@ dt_iop_module_t *dt_iop_commit_blend_params(dt_iop_module_t *module, { module->raster_mask.sink.source = NULL; module->raster_mask.sink.id = INVALID_MASKID; - return NULL; + return; } for(GList *iter = module->dev->iop; iter; iter = g_list_next(iter)) @@ -1953,13 +1958,27 @@ dt_iop_module_t *dt_iop_commit_blend_params(dt_iop_module_t *module, GINT_TO_POINTER(blendop_params->raster_mask_id)); module->raster_mask.sink.source = candidate; module->raster_mask.sink.id = blendop_params->raster_mask_id; - dt_print_pipe(DT_DEBUG_PIPE | DT_DEBUG_MASKS | DT_DEBUG_VERBOSE, - "request raster mask", - NULL, module, DT_DEVICE_NONE, NULL, NULL, "from '%s%s' %s", - candidate->op, dt_iop_get_instance_id(candidate), - new ? "new" : "replaced"); - return candidate; + // Only the genuinely-new case is interesting: it's the one time this + // source's cache needs invalidating. Logging every "replaced" would just + // repeat on every commit for as long as the raster mask is in use. + if(new) + { + dt_print_pipe(DT_DEBUG_PIPE | DT_DEBUG_MASKS | DT_DEBUG_VERBOSE, + "request raster mask", + NULL, + module, + DT_DEVICE_NONE, + NULL, + NULL, + "from '%s%s' new", + candidate->op, + dt_iop_get_instance_id(candidate)); + if(pipe) + dt_dev_pixelpipe_cache_invalidate_later( + pipe, candidate->iop_order, "blend new raster: "); + } + return; } } } @@ -1979,7 +1998,6 @@ dt_iop_module_t *dt_iop_commit_blend_params(dt_iop_module_t *module, } module->raster_mask.sink.source = NULL; module->raster_mask.sink.id = INVALID_MASKID; - return NULL; } gboolean _iop_validate_params(dt_introspection_field_t *field, @@ -2177,13 +2195,24 @@ void dt_iop_commit_params(dt_iop_module_t *module, { memcpy(piece->blendop_data, blendop_params, sizeof(dt_develop_blend_params_t)); + /* We have to take blending parameters into account for the hash if + a) there is some blending active detected via the mask_mode or + b) we have a blending module in focus so we have valid cachelines + */ + const gboolean is_blending = + piece->enabled && module->flags() & IOP_FLAGS_SUPPORTS_BLENDING && + (blendop_params->mask_mode != DEVELOP_MASK_DISABLED || dt_dev_gui_module() == module); + /* 1. make blendop_params available dt_iop_module_t struct. - Also checks for a new source raster mask module. - This is important as we can't know from here if that raster mask is actually - available. - We handle this case by partly invalidating the cache to enforce a valid raster. + Also checks for a new source raster mask module. This is important as we + can't know from here if that raster mask is actually available: we handle + this case by having dt_iop_commit_blend_params() partly invalidate the cache + to enforce a valid raster, but only when the raster mask is actually in use. */ - dt_iop_module_t *new_raster = dt_iop_commit_blend_params(module, blendop_params); + dt_iop_commit_blend_params( + module, + blendop_params, + (blendop_params->mask_mode & DEVELOP_MASK_RASTER) && is_blending ? pipe : NULL); #ifdef HAVE_OPENCL // assume process_cl is ready, commit_params can overwrite this. @@ -2221,13 +2250,6 @@ void dt_iop_commit_params(dt_iop_module_t *module, phash = dt_hash(phash, &module->instance, sizeof(int32_t)); phash = dt_hash(phash, module->params, module->params_size); - /* We have to take blending parameters into account for the hash if - a) there is some blending active detected via the mask_mode or - b) we have a blending module in focus so we have valid cachelines - */ - const gboolean is_blending = module->flags() & IOP_FLAGS_SUPPORTS_BLENDING - && (blendop_params->mask_mode != DEVELOP_MASK_DISABLED - || dt_dev_gui_module() == module); if(is_blending) { phash = dt_hash(phash, blendop_params, sizeof(dt_develop_blend_params_t)); @@ -2237,9 +2259,6 @@ void dt_iop_commit_params(dt_iop_module_t *module, { phash = dt_masks_group_hash(phash, grp); } - - if(blendop_params->mask_mode & DEVELOP_MASK_RASTER && new_raster) - dt_dev_pixelpipe_cache_invalidate_later(pipe, new_raster->iop_order, "blend new raster: "); } } piece->hash = phash; @@ -2327,7 +2346,7 @@ static gboolean _gui_reset_callback(GtkButton *button, } /* reset to default params */ dt_iop_reload_defaults(module); - dt_iop_commit_blend_params(module, module->default_blendop_params); + dt_iop_commit_blend_params(module, module->default_blendop_params, NULL); /* reset ui to its defaults */ dt_iop_gui_reset(module); diff --git a/src/develop/imageop.h b/src/develop/imageop.h index d8408ca8767c..2ba2c88db015 100644 --- a/src/develop/imageop.h +++ b/src/develop/imageop.h @@ -385,12 +385,15 @@ void dt_iop_commit_params(dt_iop_module_t *module, struct dt_dev_pixelpipe_t *pipe, struct dt_dev_pixelpipe_iop_t *piece); -/** make sure that blend_params are in sync with the iop struct - Also watch out for a raster mask source module to get it's first `target`, - dt_iop_commit_blend_params() either returns NULL or the source module. +/** make sure that blend_params are in sync with the iop struct. + Also watch out for a raster mask source module to get its first `target`: if `pipe` + is non-NULL and the registration is genuinely new, invalidates that source's + cachelines onward, since the source's own hash didn't change but it now has to + write a mask for the new user. */ -dt_iop_module_t *dt_iop_commit_blend_params(dt_iop_module_t *module, - const struct dt_develop_blend_params_t *blendop_params); +void dt_iop_commit_blend_params(dt_iop_module_t *module, + const struct dt_develop_blend_params_t *blendop_params, + struct dt_dev_pixelpipe_t *pipe); /** make sure the raster mask is advertised if available */ void dt_iop_advertise_rastermask(dt_iop_module_t *module, const int mask_mode); /** creates a label widget for the expander, with callback to enable/disable this module. */ diff --git a/src/gui/presets.c b/src/gui/presets.c index 1727a5b037a0..c3dcf1591503 100644 --- a/src/gui/presets.c +++ b/src/gui/presets.c @@ -1088,7 +1088,7 @@ void dt_gui_presets_apply_preset(const gchar* name, && (blendop_version == dt_develop_blend_version()) && (bl_length == sizeof(dt_develop_blend_params_t))) { - dt_iop_commit_blend_params(module, blendop_params); + dt_iop_commit_blend_params(module, blendop_params, NULL); } else if(blendop_params && dt_develop_blend_legacy_params(module, blendop_params, @@ -1099,7 +1099,7 @@ void dt_gui_presets_apply_preset(const gchar* name, } else { - dt_iop_commit_blend_params(module, module->default_blendop_params); + dt_iop_commit_blend_params(module, module->default_blendop_params, NULL); } DT_CONTROL_SIGNAL_RAISE(DT_SIGNAL_PRESET_APPLIED, module); From a2977094fb75576dd293588ee20c8cd28efdc5c5 Mon Sep 17 00:00:00 2001 From: Daniele Pighin Date: Mon, 13 Jul 2026 16:18:43 +0200 Subject: [PATCH 2/2] Invalidate raster-mask cachelines based on per-pipe state, not a shared registration flag The previous fix (cec8ba95f0ed5c58b4e7cf49109f549da5d49183) decided whether to invalidate a raster-mask source's cacheline by checking whether the consumer's registration in the source module's shared users table was brand new. That flag lives on the module and is shared across every pipe (full preview, thumbnail preview, second window), and it is consumed the first time anything touches that table. In practice, the consumer usually gets added to that shared table by GUI code, at the moment the user picks a raster mask from the blend panel combo box, before any pixel pipe actually commits the new blend parameters. By the time each pipe's own commit runs afterward, the registration is no longer new, so the flag is already false for every pipe. None of them invalidate the source's cacheline. If the source's own parameters have not changed, its cacheline is still warm, so the source is never reprocessed, and it never writes the mask the consumer needs. The consumer silently blends with no mask at all until something unrelated forces a full reprocessing later. The same problem shows up when raster mask assignments are replayed from history, such as during undo, redo, or loading an image with existing history: the registration is consumed while replaying history, before the real per-pipe commit that would need to react to it. The fix stops relying on that shared, one-shot flag entirely. When a pipe commits a module that consumes a raster mask, it now looks at that specific pipe's own copy of the source module and checks whether a mask has already been stored there for the requested mask id. If not, only that pipe's cacheline for the source is invalidated, so the source reprocesses and writes the mask on this pipe. If the mask is already present, nothing is invalidated, which keeps the original goal of not forcing a full reprocess on every unrelated edit. Because the check is now local to each pipe instead of a single flag shared across all of them, every pipe correctly detects and recovers from the case where it is missing a mask, regardless of whether the registration happened through the GUI, through history replay, or through a normal pipe commit, and regardless of which pipe reaches the code first. Verified interactively with pipe and mask debug logging: assigning a raster mask to a module for the first time now triggers a cacheline invalidation on both the full and preview pipes at the moment of assignment, the source module reprocesses and writes the mask immediately, and the consuming module picks it up in the same run without requiring any further unrelated edit. --- src/develop/imageop.c | 54 ++++++++++++++++++++++++++++++++----------- src/develop/imageop.h | 7 +++--- 2 files changed, 44 insertions(+), 17 deletions(-) diff --git a/src/develop/imageop.c b/src/develop/imageop.c index 092cb79d81df..da6518b71c55 100644 --- a/src/develop/imageop.c +++ b/src/develop/imageop.c @@ -1915,14 +1915,16 @@ void dt_iop_advertise_rastermask(dt_iop_module_t *module, const int mask_mode) /* make sure that blend_params are in sync with the iop struct 1. Handling of raster mask users must only be done if we don't use module's default blending parameters. - 2. Also watch out for a raster mask source module to get it's first `target` - entry: a source that gains a new user doesn't change its own params, so its - cacheline hash is unchanged and it could be served from cache without ever - writing the raster mask the new user depends on. When that happens and `pipe` - is given, invalidate that source's cachelines onward so it re-runs and writes - the mask. This must fire only for a genuinely new registration, not on every - commit, or every edit downstream of an early raster source forces a full - re-render of the pipe tail. + 2. Also watch out for a raster mask source module: gaining a user doesn't change + the source's own params, so its cacheline hash is unchanged and it could be + served from cache without ever writing the raster mask its consumer needs. + When `pipe` is given, check that pipe's own source piece: if it has never + stored a mask for this id, invalidate the source's cachelines onward so it + re-runs and writes one. This is deliberately per-pipe state, not the + module-global "is this registration new" flag from the users hash table: + GUI code and history replay register a consumer in that shared table before + any pipe commits, so a global flag is already consumed by the time a real + per-pipe commit happens and would never fire. */ void dt_iop_commit_blend_params(dt_iop_module_t *module, const dt_develop_blend_params_t *blendop_params, @@ -1959,11 +1961,11 @@ void dt_iop_commit_blend_params(dt_iop_module_t *module, module->raster_mask.sink.source = candidate; module->raster_mask.sink.id = blendop_params->raster_mask_id; - // Only the genuinely-new case is interesting: it's the one time this - // source's cache needs invalidating. Logging every "replaced" would just - // repeat on every commit for as long as the raster mask is in use. + // Only the genuinely-new case is interesting for the debug log: it's the + // one time this exact user/source pairing gets registered. Logging every + // "replaced" would just repeat on every commit for as long as the raster + // mask is in use. if(new) - { dt_print_pipe(DT_DEBUG_PIPE | DT_DEBUG_MASKS | DT_DEBUG_VERBOSE, "request raster mask", NULL, @@ -1974,7 +1976,33 @@ void dt_iop_commit_blend_params(dt_iop_module_t *module, "from '%s%s' new", candidate->op, dt_iop_get_instance_id(candidate)); - if(pipe) + + // Whether *this pipe* needs to invalidate the source's cacheline must + // not be decided from `new`: that flag is shared across all pipes via + // `candidate->raster_mask.source.users`, and GUI code + // (_raster_value_changed_callback) as well as history replay register + // the consumer there before any pipe ever commits, so by the time a + // real per-pipe commit runs, `new` is already false everywhere and no + // pipe would invalidate -- silently starving the consumer of a mask + // that was never actually computed. Instead check this pipe's own + // state: has its source piece already stored a mask for this id? If + // not, the source must (re-)run so it writes one. + if(pipe) + { + dt_dev_pixelpipe_iop_t *source_piece = NULL; + for(GList *n = pipe->nodes; n; n = g_list_next(n)) + { + dt_dev_pixelpipe_iop_t *p = n->data; + if(p->module == candidate) + { + source_piece = p; + break; + } + } + const gboolean mask_missing = + !source_piece || !g_hash_table_lookup(source_piece->raster_masks, + GINT_TO_POINTER(blendop_params->raster_mask_id)); + if(mask_missing) dt_dev_pixelpipe_cache_invalidate_later( pipe, candidate->iop_order, "blend new raster: "); } diff --git a/src/develop/imageop.h b/src/develop/imageop.h index 2ba2c88db015..56e868e29078 100644 --- a/src/develop/imageop.h +++ b/src/develop/imageop.h @@ -386,10 +386,9 @@ void dt_iop_commit_params(dt_iop_module_t *module, struct dt_dev_pixelpipe_iop_t *piece); /** make sure that blend_params are in sync with the iop struct. - Also watch out for a raster mask source module to get its first `target`: if `pipe` - is non-NULL and the registration is genuinely new, invalidates that source's - cachelines onward, since the source's own hash didn't change but it now has to - write a mask for the new user. + Also watch out for a raster mask source module: if `pipe` is given and that + pipe's own source piece has not yet stored a mask for the requested id, + invalidates the source's cachelines onward so it re-runs and writes one. */ void dt_iop_commit_blend_params(dt_iop_module_t *module, const struct dt_develop_blend_params_t *blendop_params,