Skip to content

toneequal: OpenCL follow-up improvements - #22444

Merged
TurboGit merged 5 commits into
darktable-org:masterfrom
da-phil:pl/improve_toneequal_opencl_impl
Sep 30, 2026
Merged

toneequal: OpenCL follow-up improvements#22444
TurboGit merged 5 commits into
darktable-org:masterfrom
da-phil:pl/improve_toneequal_opencl_impl

Conversation

@da-phil

@da-phil da-phil commented Sep 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Here are some follow-up improvements to the tone equalizer OpenCL code path added earlier in #22051.

  • Reuse the luminance mask on the full pipe. The OpenCL path rebuilt the mask, guided filter included, on every run, so each frame of a band slider drag paid the full cost. It now keeps a host copy, shared with the CPU path. When the mask hasn't changed, it uploads that copy instead of rebuilding.
  • Fix a silent CPU fallback. With iterations <= 0, which a preset, style or Lua script can store, the OpenCL blurs returned an allocation error even though every buffer was allocated.
  • Skip the quantization pass when it is off. With quantization at 0, the guide is the image itself. The copy kernel and its buffer are dropped, matching what the EIGF path already did.
  • Sample the guided filter parameters at the downscaled size. The blend kernels now read the downscaled parameters directly instead of upsampling them to full size first. This removes the largest temporaries (0.5× and 1× the input) and one full-size write and read per blend. The bilinear arithmetic moves into a shared data/kernels/bilinear.h, so all cached OpenCL kernels are rebuilt once.
  • Typo fix in RELEASE_NOTES.md.

Testing

  • Every commit builds with OpenCL enabled.
  • Integration tests 0079-toneequal-gf and 0080-toneequal-eigf on AMD gfx1103 (ROCm):
    • they pass against expected.png, and CPU vs GPU stays within tolerance (0.00 % of pixels above it);
    • the number of pixels differing between CPU and GPU (1894 and 1675) exceeds the stored cpugpu.maxpix limits (450 and 520). I haven't yet compared this against master on the same GPU.
  • No integration test uses quantization > 0 or more than one iteration, so those paths are untested.

Checklist

  • I have read CONTRIBUTING.md and the coding style.
  • I have not merged master into the topic branch.
  • The pull request is one logical change, and every commit compiles on its own.
  • I ran the relevant tests: unit tests, src/tests/integration/ where the pixelpipe is touched, or darktable-cli as a headless smoke test.
  • New user-visible strings use _(), new preferences are registered in data/darktableconfig.xml.in.
  • A RELEASE_NOTES.md entry was added (only needed if fixing an issue in a release). Do not reference GitHub issues.

AI assistance

This PR was co-created with Claude.

@da-phil
toneequal: reuse the host luminance mask on the OpenCL full pipe
9f350e7 
toneeq_process() keeps the full pipe luminance mask in
g->full_preview_buf and, while g->ui_preview_hash still matches, skips
compute_luminance_mask() and only applies the correction. That is what
makes a band slider drag cheap: the mask is the expensive part of the
module and the band factors are not in its hash.

process_cl() had no such cache. The device to host copy of the full pipe mask
was dropped because no GUI code consumes it, and only
invalidated g->ui_preview_hash so a later CPU run would recompute.
But the consumer would have been the next OpenCL run: without the copy every
full pipe run rebuilt the mask, guided filter included, so dragging a
band paid the whole extraction on every frame where the CPU path paid
the lookup only.

Keep the host copy on both darkroom pipes, shared with the CPU path so a
fallback stays in sync, and use it in both directions: when the key matches,
upload it into the device buffer instead of rebuilding the mask.
When it does not, rebuild and copy back. The transfers are blocking,
as in lens and overlay: later runs rewrite and reallocate the
host copy, and the copy back is only issued when the mask changed, when
the device just did the expensive part anyway. The key is invalidated
before the copy back and committed after it, so a failed transfer can
never pass a half filled buffer off as the mask.

The full pipe buffer handling moves into _full_preview_buffer(), used
by both paths. It also resets the recorded size when the allocation
fails, so the next run retries instead of returning NULL forever, and
invalidates the key on a resize.
@da-phil
toneequal: report success from the OpenCL blurs before the first enqueue
02449e2 
_fast_surface_blur_cl() and _fast_eigf_surface_blur_cl() start err at
CL_MEM_OBJECT_ALLOCATION_FAILURE so the shared cleanup label can be
used from the allocation checks, and relied on the first enqueue to
overwrite it. The EIGF loop body is the only place that does so there,
so with iterations <= 0, which the slider forbids but a preset, style
or Lua script can still store, the function returned an allocation
failure with every buffer allocated and the pipe silently fell back to
the CPU. Set err once the allocations are known good.
@da-phil
toneequal: skip the guided filter quantization pass when it is off
5de0818 
With `mask quantization` at zero, quantize() is a plain copy of the
downscaled image into the guide buffer, and _fast_surface_blur_cl()
launched that copy kernel on every iteration. The guide is then the
image itself, so pack the image twice and do not allocate the guide
buffer at all, as _fast_eigf_surface_blur_cl() already does.
@da-phil
toneequal: sample the guided filter parameters at the downscaled size
b2f637f 
The OpenCL guided filter and EIGF blend kernels read their per pixel
parameters from full size buffers upsampled with
dt_interpolate_bilinear_cl(). Sample the downscaled parameters
bilinearly in the blend kernels instead. This drops the largest
temporaries (0.5 and 1.0 times the input in tiling_callback() terms)
and a full size write plus read per blend. Output is unchanged.

To keep the arithmetic in one place, it moves from bilinear.cl into a
new data/kernels/bilinear.h (dt_bilinear_sample1/2/4), used by both
the bilinear kernels and toneequal.cl. The header is installed and
added to the kernel cache key includes in src/common/opencl.c, so all
cached kernel binaries are rebuilt once.

Also drop the unused TONEEQ_PIXEL_CHAN define.
@da-phil
Fix typo in RELEASE_NOTES.md and reformat
8b34347
@TurboGit TurboGit added this to the 5.8 milestone Sep 30, 2026
@TurboGit

TurboGit commented Sep 30, 2026

Copy link
Copy Markdown
Member

the number of pixels differing between CPU and GPU (1894 and 1675)

Also the case on current master, a recent change I suppose. Let me know if you have a hint about the possible culprit, there is many OpenCL changes recently.

@TurboGit TurboGit added feature: enhancement current features to improve priority: medium core features are degraded in a way that is still mostly usable, software stutters scope: image processing correcting pixels scope: performance doing everything the same but faster OpenCL Related to darktable OpenCL code labels Sep 30, 2026
TurboGit
TurboGit approved these changes Sep 30, 2026
View reviewed changes

@TurboGit TurboGit left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, thanks!

@TurboGit
TurboGit merged commit 477d0b5 into darktable-org:master Sep 30, 2026
5 checks passed
@da-phil
da-phil deleted the pl/improve_toneequal_opencl_impl branch September 30, 2026 15:31
@da-phil

da-phil commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

the number of pixels differing between CPU and GPU (1894 and 1675)

Also the case on current master, a recent change I suppose. Let me know if you have a hint about the possible culprit, there is many OpenCL changes recently.

Yeah, that would be my guess too. I'll try to dig deeper to understand this difference.

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

Reviewers

@TurboGit TurboGit TurboGit approved these changes

Assignees

No one assigned

Labels

feature: enhancement current features to improve OpenCL Related to darktable OpenCL code priority: medium core features are degraded in a way that is still mostly usable, software stutters scope: image processing correcting pixels scope: performance doing everything the same but faster

Projects

None yet

Milestone

5.8

Development

Successfully merging this pull request may close these issues.

2 participants

@da-phil @TurboGit