tone equalizer: add OpenCL implementation - #22051
Conversation
f39f2c2 to
6dab75d
Compare
|
There is still one open question to w.r.t. the luminance mask cache keying on the wrong hash. Both below items affect the CPU and the OpenCL path equally, which is why they are kept out of that PR, as it is purely about OpenCL. 1. The cache key includes the module's own parameters
const dt_hash_t hash = dt_dev_pixelpipe_piece_hash(piece, roi_out, TRUE);With The luminance mask does not depend on the nine band factors or on
Potential fix. Key the mask on
That set is already enumerated in 2. The preview readback can then be made ~2 KB instead of a whole bufferOnce (1) lands, what remains is one device-to-host copy of the whole mask per actual mask change. The two GUI consumers need far less than that:
Potential fix. Compute the log histogram on the device and read back only the bins (~2 KB); read the 3x3 region on demand when the cursor moves, rather than the whole buffer on every pipe run. That removes the last bulk device-to-host transfer from the darkroom path, which is the substance of the long-standing objection to giving this module a GPU code path at all. Notes
Would be great to hear your thoughts on this @jenshannoschwalm |
About what exactly - or in general?
|
|
maybe carve out the fast guided filter OpenCl code into a separate kernel so it can be used for further modules (see #21596) |
Didn't you just do it in your PR： https://github.com/darktable-org/darktable/pull/21596/changes#diff-6f734258091f34041ce75b0beff1edca67f1941d5bf5ce86dde7347ee9f77dc0 ？ |
7d36509 to
c2483c5
Compare
TurboGit
added
feature: enhancement
There was a problem hiding this comment.
🔵 Needs a closer look
The change introduces a substantial new GPU implementation path (multiple kernels + memory/tiling behavior) that is difficult to fully validate for correctness and device-compatibility via automated review alone.
Pull request overview
Adds an OpenCL (GPU) execution path to toneequal to avoid repeated host/device roundtrips and accelerate interactive editing + export when multiple instances are used.
Changes:
- Implement
process_cl()for tone equalizer, including luminance mask extraction, guided/EIGF filtering, and correction application (with host-side cache synchronization for GUI histogram/cursor sampling). - Add a dedicated OpenCL program (
data/kernels/toneequal.cl) containing kernels for luminance mask generation, guided filter steps, EIGF steps, and mask display. - Register the new OpenCL program in
data/kernels/programs.confand add atiling_callback()to model intermediate-buffer memory needs.
File summaries
| File | Description |
|---|---|
| src/iop/toneequal.c | Adds kernel IDs, OpenCL processing path, and tiling memory accounting for GPU execution while mirroring existing CPU behavior/caching. |
| data/kernels/toneequal.cl | New OpenCL kernels implementing the tone equalizer luminance mask + guided/EIGF filtering + apply/display steps. |
| data/kernels/programs.conf | Registers toneequal.cl as OpenCL program id 43 so the module can create kernels at init time. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d48c6f8 to
442ecf3
Compare
Changes were done in the following commits Does this look good to you?
Need to dig into that first... |
442ecf3 to
4857d8c
Compare
|
@da-phil : What is the status of this, is that ready for review/integration? |
Yes, it's ready for review/integration. I've been using/testing this code for a while. |
The tone equalizer was CPU only, so it pulled the pixelpipe back to the
host on every zoom, pan and slider move.
Add data/kernels/toneequal.cl and a process_cl() mirroring the CPU path:
- luminance mask extraction for all seven estimators,
- the fast guided filter (quantization, separable box average, linear
and geometric-mean blending),
- the exposure independent guided filter, reusing the existing
dt_gaussian_*_cl() buffer blurs,
- the exposure correction and the luminance mask display.
The correction is evaluated as the sum of the eight octave gaussians
rather than through d->correction_lut: the transcendentals are cheaper
on a GPU than uploading a 320 kB table per invocation, and this is the
reference formulation the lut approximates.
The GUI reads the luminance mask from host memory to build its histogram
and to report the luminance under the cursor, so process_cl() mirrors
the caching logic of toneeq_process() and copies the mask back whenever
the upstream pipe hash changes.
Also add a tiling_callback() so the pixelpipe accounts for the
intermediate buffers and falls back to the CPU rather than failing to
allocate. The module stays untileable, the guided filter being global.
Checked against the CPU path with the integration tests 0079, 0080 and
0129 to 0134, plus mask quantization and multi-iteration variants: the
8-bit outputs differ by at most 1 LSB on less than 0.1% of the pixels.
Giving tone equalizer an OpenCL path moves its luminance mask into device memory. That mask is not a private intermediate: the CPU code publishes it to the GUI, which reads it from another thread. The GUI cannot read device memory, so a device to host copy has to appear somewhere, and such a copy is a synchronisation point in an otherwise asynchronous command queue. This seam, interleaving the pixelpipe and GUI threads with a GPU queue now in between, is the long standing objection to giving this module a GPU code path at all, so it is worth keeping as narrow as it can be. process_cl() mirrored toneeq_process() and kept both host caches, but only the preview pipe one is ever read. Both GUI consumers take their data from it: update_histogram() through g->pd.buf, and _luminance_from_module_buffer() for the exposure under the cursor. The full pipe copy landed in g->full_preview_buf, which no GUI code reads, and on the OpenCL path the correction is applied on the device from dev_luminance, so no later GPU run read it back either. It was a blocking multi-megabyte transfer into a buffer nobody consumes, issued whenever the roi or the upstream pipe state changed: every pan and zoom step, and every motion event of a feathering or iterations drag, since those invalidate the luminance cache. Drop it, and let the full pipe mask stay on the device. The copy could not simply be deleted. toneeq_process() skips compute_luminance_mask() while g->ui_preview_hash still matches, so a GPU run that advanced that hash without filling the host buffer would let a later CPU run apply a stale mask. That is reachable by turning OpenCL off mid session, by cldevice_..._nocl=toneequal, and by any late fallback to the CPU path. The hash is therefore invalidated rather than advanced, so such a transition recomputes the mask once. That costs about one CPU process() run. The preview pipe copy stays as it is. The GUI genuinely needs that buffer in host memory, it goes through the shared dt_preview_data_t service, that pipe is small, and the blocking read is issued outside the module GUI lock so it cannot stall a GUI thread. Pixel output is unchanged: both kernels read dev_luminance either way, and gui_attached is FALSE on export, so this block is darkroom only.
dt_preview_data_set_hash() records dt_dev_pixelpipe_piece_hash(piece, roi, TRUE), which covers the owning module's own params. That is the right key for a buffer that is a copy of the module output, but not for one that only depends on a part of the module state. Add dt_preview_data_set_hash_value() so a module can commit a freshness key it computed itself, and document that dt_preview_data_is_fresh(), which recomputes the cumulative piece hash, does not apply to buffers stored that way. No functional change: no caller uses it yet.
toneeq_process() and process_cl() both gated mask recomputation on dt_dev_pixelpipe_piece_hash(piece, roi_out, TRUE) With include = TRUE that hashes nodes[0 .. position-1], and module->position is the module's own 1-based index in the pipe, so the module's own params were part of the key. The luminance mask does not depend on the nine band factors nor on `smoothing`; those only feed d->factors and the correction LUT. But moving any of them changed the key, so every frame of a band slider drag recomputed the whole mask on the CPU and on the GPU alike, and on the preview pipe copied a bit-identical mask device to host again. The mask dominates process(): a full CPU run measures ~60 ms/Mpix (-d perf, four cores, mire1.cr2 at 3908x2602 -> 0.575-0.694 s). Key it instead on the upstream pipe only (include = FALSE, which still folds in the roi) plus the params compute_luminance_mask() and _compute_luminance_mask_cl() actually read: method, details, radius (and the blending it derives from), feathering, iterations, quantization, scale, exposure_boost and contrast_boost. That is the same set gui_changed() calls invalidate_luminance_cache() for.
4857d8c to
5053a89
Compare
Motivation
I'm frequently using multiple instances of the tone equalizer module and I love it for taming HDR contrast, however, if you have multiple instances of it, exporting images can easily take up to 25s on my desktop PC or even up to 40s on my laptop.
The module has only a CPU execution path, so it pulled the pixelpipe back to the host on every zoom, pan and slider move, which also made it an overall slow editing experience.
Fixes #4805
Changes
Add data/kernels/toneequal.cl and a process_cl() mirroring the CPU path:
dt_gaussian_*_cl() buffer blurs,
The correction is evaluated as the sum of the eight octave gaussians rather than through d->correction_lut: the transcendentals are cheaper on a GPU than uploading a 320 kB table per invocation, and this is the reference formulation the lut approximates.
The GUI reads the luminance mask from host memory to build its histogram and to report the luminance under the cursor, so process_cl() mirrors the caching logic of toneeq_process() and copies the mask back whenever the upstream pipe hash changes.
Also add a tiling_callback() so the pixelpipe accounts for the intermediate buffers and falls back to the CPU rather than failing to allocate. The module stays untileable, the guided filter being global.
Results
Integration tests
Tone equalizer relevant integration tests (CPU vs GPU) look fine, see test-20260829-201614.log.
Benchmarks
Comparison of darktable export performance with the CPU-only tone equalizer
(current master) against an OpenCL implementation, measured on two AMD systems.
Test setup
gfx1103, 6 CU)gfx1200, 16 CU)toneequalinstances (two with blending), twodiffuseinstances,colorequal,agx.darktable -d opencl -d perf -d tiling, export to JPEG.Full pipeline
Per-instance tone equalizer timings
toneequaltoneequal.2(blended)toneequal.1(blended)Disclaimer: this change was co-created with Claude.