Possibly auto-disable highlights for non-altered images - #22095
Possibly auto-disable highlights for non-altered images#22095jenshannoschwalm wants to merge 1 commit into
Conversation
jenshannoschwalm
added
scope: image processing
f809af1 to
e8f7e15
Compare
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a cross-thread data race on the new disable flag and has a stale-flag edge case (plus UI enable-state sync) that can incorrectly disable the module.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds an optimization to automatically disable the default-on highlights (opposed) reconstruction in darkroom when a full-pipe run detects there are no clipped photosites, but only for images whose history has not been altered (to avoid interfering with subsequent user edits).
Changes:
- Track whether the opposed algorithm found any clipped photosites and set a flag to indicate the highlights module can be disabled.
- Add a
DT_SIGNAL_DEVELOP_UI_PIPE_FINISHEDcallback in the highlights module UI to auto-disable the module on unaltered images when no clipping is detected. - Document the behavior in the release notes.
File summaries
| File | Description |
|---|---|
| src/iop/hlreconstruct/opposed.c | Sets an internal flag when the opposed analysis finds no clipped photosites in the full pipe. |
| src/iop/highlights.c | Adds the disable flag state and a UI-pipe-finished callback to auto-disable the module for unaltered images. |
| RELEASE_NOTES.md | Documents the new automatic disable behavior for performance. |
Review details
Suppressed comments (1)
src/iop/highlights.c:845
- Same issue as the OpenCL path:
disable_highlightsis reset only after the early passthrough return, so a stale TRUE can disable the module on the next UI-pipe-finished signal without recomputing clipping for the current image. Reset it before the passthrough branch and keep a singlefullpipevariable.
const gboolean fullpipe = dt_pipe_is_full(pipe);
const gboolean fastmode = dt_pipe_is_fast(pipe);
const dt_iop_highlights_mode_t dmode = fastmode && (d->mode == DT_IOP_HIGHLIGHTS_SEGMENTS)
? DT_IOP_HIGHLIGHTS_OPPOSED
: d->mode;
const gboolean scaled = filters == 0 && dmode != DT_IOP_HIGHLIGHTS_CLIP;
if(fullpipe)
disable_highlights = FALSE;
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e8f7e15 to
2f0a368
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Asynchronous and persisted-state handling can disable highlights for the wrong image or override recent user edits.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
| static dt_aligned_pixel_t img_oppchroma; | ||
| static gboolean img_oppclipped = TRUE; | ||
| static dt_hash_t img_opphash = ULLONG_MAX; | ||
| static gboolean disable_highlights = FALSE; |
|
|
||
| static void _ui_pipe_done(gpointer instance, dt_iop_module_t *self) | ||
| { | ||
| if(disable_highlights && self && self->enabled && !dt_image_altered(self->dev->image_storage.id)) |
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
|
@jenshannoschwalm : I'm not sure a comment work, you need to click the button "Fix with Copilot" and enter the prompt in the dialog. |
|
I am trying out this AI thing the first time myself. Currently i'm not sure the current approach is the right way at all. Already i "learned" that the after UI pipe callback could still be running when a next piperun has started. Hmmm... The question to me: how can i correctly change any history parameters - in this example just disabling - from data derived while processing the full pipe not doing crazy workarounds :-) |
I'm not sure there is a way. The pipe has it's own copy the history, so you can certainly skip a module (as if it was disabled), but this won't make it disabled in the image's history. Frankly, this is going to be very tricky and will certainly hinder portability. That being said you know more about the pipe than I, so you may well be the magician here :) |
7ec80bd to
2faeb04
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The linear/sRAW opposed path never records the result needed to trigger automatic disabling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
2faeb04 to
9180218
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Linear RAW handling, cache reuse, and segmentation mode can produce incorrect or missed auto-disable behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Balanced
We have highlights module enabled using the opposed algorithm for a hassle-free import of raw and linear sraw images by default. Yet this comes with some superfluous processing burden for images without any clipped photosites. As the opposed algorithm checks this anyway, we can keep the result of a full pipe process and possibly disable the module automatically via a DT_SIGNAL_DEVELOP_UI_PIPE_FINISHED callback. Please note: 1. This happens only in darkroom if the image history is not marked as altered in the database as we don't want to interfere later with user actions. 2. Possibly we avoid module processing and might have smaller ROI 3. In some unlikely conditions we might miss the auto-disabling (that won't hurt)
9180218 to
a8b4503
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Incomplete clipping scans and stale persisted-history checks can incorrectly disable highlights.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
src/iop/hlreconstruct/opposed.c:353
anyclippedis not a complete no-clipping test: the scan at lines 276-278 stops atmheight - 1/mwidth - 1, omitting at least the final 3x3 block along the bottom and right edges. If clipping exists only there, this publishesTRUEand the next render loses the reconstruction that the output loop would otherwise apply. Use a full-frame clipping check for the auto-disable decision rather than the deliberately cropped chrominance-mask scan.
disable_highlights = !anyclipped;
tested_id = self->dev->image_storage.id;
src/iop/hlreconstruct/opposed.c:556
clippedis derived fromhighlights_initmask, which only fills mask values from source pixels whenmcol < mwidth - 1 && mrow < mheight - 1(data/kernels/basic.cl:375-386). Clipping confined to the omitted bottom/right blocks therefore yields zero here and incorrectly disables highlights. The OpenCL path needs a complete source clipping reduction before publishing this result.
disable_highlights = clipped == 0.0f;
tested_id = self->dev->image_storage.id;
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
| && self->dev->image_storage.id == tested_id | ||
| && !dt_image_altered(tested_id)) |
| if(fullpipe && !anyclipped) | ||
| { | ||
| disable_highlights = TRUE; | ||
| tested_id = self->dev->image_storage.id; |
We have highlights module enabled using the opposed algorithm for a hassle-free import of raw images by default.
Yet this comes with some superfluous processing burden for images without any clipped photosites. As the opposed algorithm checks this anyway, we can keep the result of a full pipe process and possibly disable the module automatically via a DT_SIGNAL_DEVELOP_UI_PIPE_FINISHED callback.
Please note: