Skip to content

pixelpipe: don't convert the pipe input in place for blending - #22326

Open
masterpiga wants to merge 1 commit into
darktable-org:masterfrom
masterpiga:fix_colorspace
Open

masterpiga wants to merge 1 commit into
darktable-org:masterfrom
masterpiga:fix_colorspace

Conversation

@masterpiga

Copy link
Copy Markdown
Collaborator

Another bug that I stumbled upon while testing flexi masks.

When a module blends in a colorspace other than the one it processes in, the CPU path converts tmp to the blend colorspace in place. When no input transform was needed, tmp is the input itself, and the code relies on invalidating the cacheline. But for the first processed module the input can be pipe->input (pipe data: full in -d pipe), which every later run reads again. It stays converted, and each run converts it once more, so the image degrades into false-color garbage. This change makes screen pipes convert a copy instead, as they already do for the input transform. The same fix applies to the OpenCL path's CPU fallback. Export and thumbnail pipes keep the in-place conversion: they run once.

To reproduce: open a small TIFF, enable blending on exposure with the blend colorspace set to Lab, then trigger a second preview run, for example by moving a mask shape. The navigation thumbnail turns into saturated bands while the center view stays correct. Tested in the GUI with these steps.

Co-authored with Claude.

For the first processed module, the input can be pipe->input itself,
so the in-place conversion to the blend colorspace corrupted the
source buffer that every later run reads again. Screen pipes now
convert a copy, as they already do for the input transform.
@masterpiga masterpiga added this to the 5.8 milestone Sep 18, 2026
@masterpiga masterpiga added bugfix pull request fixing a bug priority: medium core features are degraded in a way that is still mostly usable, software stutters difficulty: average some changes across different parts of the code base scope: image processing correcting pixels labels Sep 18, 2026
@jenshannoschwalm

Copy link
Copy Markdown
Collaborator

First: the issue, it's analysis and the code spots where this commit modifies tmp are real/corrct and we have to fix that. Good catch!

Yet it comes at a cost and when looking at the _blend_transform_buffer i think there are a two points to be discussed:

  1. As if(!copy) return tmp; after all would return bad data that should not plain-exit but do a report in logs via -d pipe

  2. The if(tmp != input || !dt_pipe_is_screen(pipe)) return tmp; line is good but should be refined. The analysis correctly mentions "For the first module that input can be pipe->input ...". The first module in the pipe is either rawprepare (can't blend) for images that are dt_image_is_rawprepare_supported() or it's undefined and might have have blending (the issue case). So the test should check for that and only use the fresh tmp buffer if not raw_prepare. This would keep performance for all raw_prepare images :-)

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The only unresolved finding is a non-blocking release-notes nit.

Review effort: Lite
Findings: None

What changed in this PR

Fixes repeated in-place blend colorspace conversion that corrupted screen-pipe previews.

Changes:

  • Copies screen-pipe input before blend conversion.
  • Applies protection to CPU and OpenCL fallback paths.
  • Preserves existing export and thumbnail behavior.
File Description
src/​develop/​pixelpipe_hb.c Adds temporary buffering for screen-pipe blend transforms. Review noted a non-blocking release-notes documentation nit.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix pull request fixing a bug difficulty: average some changes across different parts of the code base priority: medium core features are degraded in a way that is still mostly usable, software stutters scope: image processing correcting pixels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants