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..da6518b71c55 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; @@ -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) @@ -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)) @@ -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; } } } @@ -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, @@ -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. @@ -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)); @@ -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; @@ -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); diff --git a/src/develop/imageop.h b/src/develop/imageop.h index d8408ca8767c..56e868e29078 100644 --- a/src/develop/imageop.h +++ b/src/develop/imageop.h @@ -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. */ 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);