Skip to content

Don't invalidate the pixelpipe cache on every commit when a raster mask is used #21519

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
masterpiga wants to merge 2 commits into
base: master
Choose a base branch
from
+91 −42
Open

Don't invalidate the pixelpipe cache on every commit when a raster mask is used #21519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cec8ba9
Don't invalidate the pixelpipe cache on every commit when a raster ma…
masterpiga Jul 9, 2026
a297709
Invalidate raster-mask cachelines based on per-pipe state, not a shar…
masterpiga Jul 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/develop/develop.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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;
Expand Down
113 changes: 80 additions & 33 deletions src/develop/imageop.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1915,13 +1915,20 @@ 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, 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.
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.
*/
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)
Expand All @@ -1938,7 +1945,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))
Expand All @@ -1953,13 +1960,53 @@ 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 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,
module,
DT_DEVICE_NONE,
NULL,
NULL,
"from '%s%s' new",
candidate->op,
dt_iop_get_instance_id(candidate));

// 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: ");
}
return;
}
}
}
Expand All @@ -1979,7 +2026,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,
Expand Down Expand Up @@ -2177,13 +2223,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.
Expand Down Expand Up @@ -2221,13 +2278,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));
Expand All @@ -2237,9 +2287,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;
Expand Down Expand Up @@ -2327,7 +2374,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);
Expand Down
12 changes: 7 additions & 5 deletions src/develop/imageop.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,12 +385,14 @@ 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: 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.
*/
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. */
Expand Down
4 changes: 2 additions & 2 deletions src/gui/presets.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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);
Expand Down
Loading