colorspaces: compute JzAzBz's Az and Bz without cancellation - #22093
Conversation
The A matrix rows for az and bz each sum to zero -- az is
3.524L' - 4.066708M' + 0.542708S' -- so for a near-neutral colour, where
L', M' and S' are nearly the same O(1) number, the result is whatever
survives an almost total cancellation. It is also the only input to hue,
since hz = atan2(Bz, Az). And the PQ exponent 134.034375 multiplies any
relative error in its argument by 134, so a single ulp anywhere upstream
arrives at that subtraction magnified.
Evaluated directly, Az and Bz keep barely any correct digits for neutral
colours, and no two conforming float32 implementations can agree on them.
That is not hypothetical: it is why darktable renders such pixels
differently with OpenCL enabled than without. Measured on a standalone
differential carrying verbatim copies of both implementations, hue
disagrees between the CPU and OpenCL paths by up to 1.68e-03 overall, and
by up to 0.045 below Cz 1e-5.
Rewrite az and bz over the differences L'-M', S'-M' and L'-S'. This is
the same zero-sum rows regrouped, so it is algebraically identical, and
each difference is carried through both power laws and the rational PQ
step without ever subtracting two computed values:
- the raw differences come from the matrix rows, not the computed LMS;
- through each power law by x^k - y^k = y^k * expm1(k * log1p((x-y)/y)),
which is accurate because log1p and expm1 are accurate near zero;
- through the PQ step by
y_a - y_b = (c2 - c1 c3)(t_a - t_b) / ((1 + c3 t_a)(1 + c3 t_b)).
Iz is a sum rather than a difference and is untouched. Inputs with a
non-positive LMS component take the original path, where the components
are not near-equal and cancellation is not a concern.
Measured on 200,000 samples, CPU versus OpenCL: Az/Bz max difference
6.27e-06 -> 4.49e-07, hue 1.68e-03 -> 3.13e-05. A 14x improvement on
Az/Bz and 54x on hue. The result is also closer to the true value on both
paths, not merely more consistent.
This changes renders, on both paths, because it changes a conversion
shared with colorbalancergb, colorequal, diffuse and the JzCzhz picker.
The change is small and confined to near-neutral colours: mean hue change
1.43e-05, max 0.0068, with 3 of 199,773 samples (0.002%) moving by more
than 1/255 -- smaller than the CPU/OpenCL disagreement it removes. For
the pixels most affected there is no stable rendering to preserve, since
today they render one way with OpenCL and another way without.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@TurboGit this changes both the CPU and OpenCL rendering paths to make them both more correct and agree with each other. I labeled this as a bugfix because the two paths diverging when this could be avoided reads as a bug to me. |
|
Just mentioning;
Otherwise nice. Could you observe/check changes with the integration test results? |
|
Thanks, @jenshannoschwalm. I am assessing the impact of the proposed changes (and their effect on the integration tests) empirically. I will report back when it's done. |
|
I'm running the integration tests right now, I'm at 0085. |
|
Thanks @jenshannoschwalm — took both suggestions after measuring. Divisions. Changed to The Cost. Worth stating plainly: the rewrite is not free.
Three Integration tests. All 179, both binaries, CPU and OpenCL, under the
162 of 179 tests are untouched; the rest move by a handful of pixels either Against master, 15 tests change output. Nine by exactly one 8-bit level. Two One caveat on methodology: |
|
Here are the results (only one failure): But as we can see the actual diff between CPU & GPU is now smaller. So going in the right direction. |
|
Found one regression: CPU & GPU version differ by 458602 pixels The number of pixel diff on current master is exactly 94780 so the regression is quite important. |
|
I think that is fixed by my latest opencl pr, i had that before doing that and and i have around 50k diff pixels now here. |
Indeed, we have more than 150k pixel diff, then you patch fixed that to 48k and then this patch move it back to almost 460k. |
|
@TurboGit Is that on both commits or just the 1st one? |
|
I have only tested on both commits: Current master: This PR: As you can see I have messed something :) I suppose my previous test was with a build without @jenshannoschwalm work. I'm sorry for the noise. |
|
@masterpiga : We just need a release notes entry for merging. TIA. |
@TurboGit Just to clarify. I am referring to both commits in this PR. I pushed a new commit after your commented that you were running the integration tests. Maybe you restarted the test afterwards, I am just asking for confirmation. TIA! |
|
Sorry to be clear my last message is with the last version of this PR. So in any case I there is no regression now. |
|
Wonderful, thanks for confirming 👍 |
bf72672 to
bfa5f87
Compare
|
@TurboGit Added entry to release notes. TIA! |
|
@masterpiga : I don't see the release notes. |
The cancellation-free path added for Az and Bz did more work than it needs to. Scaling to the paper's 1/10000 nits was six divisions; 1e-4f is not the exact reciprocal, but the extra rounding is orders below what the difference machinery resolves. On 400k near-neutral samples against a double-precision reference the mean hue error moves from 1.069e-07 to 1.070e-07, with the maximum unchanged. Both the host and the kernel also recomputed powers they already held: the y^k factor of each difference is t_m or t_s, and y_m^p is needed three times but was evaluated three times. Hoisting those removes five pow() calls per pixel on each path. _dt_pow_diff() therefore takes y^k from the caller rather than computing it itself. The results are unchanged: every integration test renders identically on CPU, and the one test that moves by a level on GPU moves by the same level between two runs of the same binary. Measured on colorbalancergb at 10mpix, this recovers about a third of what the rewrite costs on the GPU. Against master, its module time goes from +19% to +12% (median of 20 runs, 0.021s -> 0.025s -> 0.024s); on the CPU from +14% to +12% (min of 7, 0.506s -> 0.576s -> 0.568s). The remaining cost is inherent: three log1p and three expm1 per pixel are the price of the precision.
bfa5f87 to
17c17a0
Compare
|
Right, that's what happens when one amends without adding first :) Done, sorry |
The JzAzBz fix went in as PR darktable-org#22093, in two commits: the cancellation-free rewrite, then the review follow-up that made the nit scaling a multiply and stopped both paths recomputing powers they already held. Adds what the follow-up measured on the integration suite, including the cost -- 12% of colorbalancergb's module time on both paths -- and the two tests whose references need regenerating.
The JzAzBz fix went in as PR darktable-org#22093, in two commits: the cancellation-free rewrite, then the review follow-up that made the nit scaling a multiply and stopped both paths recomputing powers they already held. Adds what the follow-up measured on the integration suite, including the cost -- 12% of colorbalancergb's module time on both paths -- and the two tests whose references need regenerating.
Background
This is another OpenCL/CPU code inconsistency found working on flexi masks. I built a tool that replays mask edits from user libraries four ways (two code paths × CPU/OpenCL). I used it to processed >70K edits from a dozen darktable users. This made the pipeline disagreeing with itself very visible.
The issue
dt_XYZ_2_JzAzBz()computesaz = 3.524L' − 4.066708M' + 0.542708S', whose coefficients sum to zero. For a near-neutral colour L', M' and S' are nearly the same O(1) number, so Az and Bz are whatever survives an almost total cancellation — and they are the only input to hue, sincehz = atan2(Bz, Az). The PQ exponent 134.034375 makes it worse: it multiplies any relative error in its argument by 134, so a single ulp anywhere upstream reaches that subtraction magnified.The consequence is that no two conforming float32 implementations can agree here, and darktable has two: the host conversion and its OpenCL twin. The same edit renders differently depending on whether OpenCL is enabled. Measured on a standalone differential carrying verbatim copies of both, hue disagrees by up to 1.68e-03 overall, and by up to 0.045 below Cz 1e-5.
The fix
Rewrite az and bz over the differences
L'−M',S'−M',L'−S'— the same zero-sum rows regrouped, so algebraically identical — and carry each difference through both power laws and the rational PQ step without ever subtracting two computed values:x^k − y^k = y^k·expm1(k·log1p((x−y)/y)), accurate becauselog1p/expm1are accurate near zero;y_a − y_b = (c2 − c1·c3)(t_a − t_b) / ((1 + c3·t_a)(1 + c3·t_b)).Izis a sum, not a difference, and is untouched. Inputs with a non-positive LMS component take the original path, where the components are not near-equal and cancellation is not a concern.Evidence. On 200,000 samples, CPU vs OpenCL: Az/Bz max difference 6.27e-06 → 4.49e-07, hue 1.68e-03 → 3.13e-05 — 14× and 54×. Replaying two contributed libraries of real edits, counting those where the CPU and OpenCL renders of the same edit disagree by more than 1/255:
No errors, and no edit whose own CPU/OpenCL gap widened. The residual is not this conversion: part is the guided filter used for mask feathering, part is
_blendif_compute_factor's hard<=against a slider limit, which swings 0 → 1 on an input difference of 5e-07. Both are separate issues.Does it change existing renders? Yes, on both paths, because it changes a conversion shared with
colorbalancergb,colorequal,diffuseand the JzCzhz picker. The change is small and confined to near-neutral colours: mean hue change 1.43e-05, max 0.0068, with 3 of 199,773 samples (0.002%) moving by more than 1/255 — smaller than the CPU/OpenCL disagreement it removes. For the pixels most affected there is no stable rendering to preserve, since today they already render one way with OpenCL and another way without. It is also closer to the true value on both paths, not merely more consistent.More details
There is a more extended version of this writeup here.
Co-authored with Claude.