darktable
darktable copied to clipboard
darktable-org
dt UCS: clip lightness to allowed upper limit
There seems to be a diff in the integration test.
Expected CPU vs. current CPU report :
----------------------------------
Max dE : 21.60480
Avg dE : 0.00018
Std dE : 0.03013
----------------------------------
Pixels below avg + 0 std : 99.97 %
Pixels below avg + 1 std : 99.97 %
Pixels below avg + 3 std : 99.97 %
Pixels below avg + 6 std : 99.98 %
Pixels below avg + 9 std : 99.98 %
----------------------------------
Pixels above tolerance : 0.00 %
FAILS: image visually changed
see diff.png for visual difference
(732 pixels changed)
Doesn't look particularly bad, though:
Oops, of course one needs to clip Y before calculating L_star from it. Will fix tomorrow…
I pushed fixed code that clamps the Y early when going to xyY to dt UCS JCH. Also I clamped both Y and L* to 0 at the lower side.
Test seems to be ok now.
Test 0093-colorbalancergb-ucs
Image mire1.cr2
CPU & GPU version differ by 32490 pixels
CPU vs. GPU report :
----------------------------------
Max dE : 1.64225
Avg dE : 0.00501
Std dE : 0.04980
----------------------------------
Pixels below avg + 0 std : 98.84 %
Pixels below avg + 1 std : 98.85 %
Pixels below avg + 3 std : 98.90 %
Pixels below avg + 6 std : 99.08 %
Pixels below avg + 9 std : 99.56 %
----------------------------------
Pixels above tolerance : 0.00 %
Expected CPU vs. current CPU report :
----------------------------------
Max dE : 1.03536
Avg dE : 0.00009
Std dE : 0.00646
----------------------------------
Pixels below avg + 0 std : 99.98 %
Pixels below avg + 1 std : 99.98 %
Pixels below avg + 3 std : 99.98 %
Pixels below avg + 6 std : 99.98 %
Pixels below avg + 9 std : 99.98 %
----------------------------------
Pixels above tolerance : 0.00 %
OK
@TurboGit this would be something for dt 4.0.1. Review from @aurelienpierre would be appreciated.
@aurelienpierre : I'd like to move forward for 4.0.1, is that ok for you? TIA.
There are maths to check here that were not checked, but sure, keep merging ASAP, we don't have enough bugs and stability is a bourgeois concept.
Clipping the relative luminance Y to 13237757000 should have zero effect on the low dynamic range studio shot used as a test here. Since the test image peak luminance is expected well below 300, the max delta E expected is 0 ± 1e-15.
The result obtained here shows a possible interaction with something else and is not normal. As such, master is to be considered broken, the fix might be worse than the original bug, and @TurboGit just showed his incompetence by merging it without raising an eyebrow and bypassing my review.
This is shitty work all around and properly unacceptable.
@aurelienpierre there's no need to be rude here.
I checked out master and reverted the fix that was merged here. Resulting delta E for 0093-colorbalancergb-ucs:
Expected CPU vs. current CPU report :
----------------------------------
Max dE : 1.03536
Avg dE : 0.00009
Std dE : 0.00646
----------------------------------
Pixels below avg + 0 std : 99.98 %
Pixels below avg + 1 std : 99.98 %
Pixels below avg + 3 std : 99.98 %
Pixels below avg + 6 std : 99.98 %
Pixels below avg + 9 std : 99.98 %
----------------------------------
Pixels above tolerance : 0.00 %
OK
So there's something about different environments (probably different compiler, I'm using GCC 12) that makes me always get this small delta E for most of the integration tests. You'll most likely be able to verify similar results (no change in delta E with or without the fix here).
Clipping the relative luminance Y to 13237757000 should have zero effect on the low dynamic range studio shot used as a test here.
It's good that you highlighted this. I took a further look at this. Turns out removing the clamping of Y going from xyY to JCH doesn't affect the test result at all. It was probably something else in the WIP state of this fix. Removing the Y clamping (and also clipping of J to zero when going back, so only clipping of J to the upper bound in the JCH -> xyY direction), I now still get
Expected CPU vs. current CPU report :
----------------------------------
Max dE : 1.03536
Avg dE : 0.00009
Std dE : 0.00646
----------------------------------
Pixels below avg + 0 std : 99.98 %
Pixels below avg + 1 std : 99.98 %
Pixels below avg + 3 std : 99.98 %
Pixels below avg + 6 std : 99.98 %
Pixels below avg + 9 std : 99.98 %
----------------------------------
Pixels above tolerance : 0.00 %
OK
so everything should be OK and the bug is still gone. Apologies for not paying enough attention to this at first as it definitely caused confusion. The initial big delta E must have been just some glitch in my environment.
Anyway regarding the clipping of Y, I believe it should be still done for the sake of correctness since the xyY -> JCH conversion isn't really well-defined at and above that highest Y value.
@TurboGit just showed his incompetence by merging it without raising an eyebrow and bypassing my review.
You had plenty of time to review this and were notified for a couple of times. To me it started to seem like you have no interest in reviewing this at all. I agree that code should be carefully reviewed before merging, but I can't really blame Pascal here for doing so, given the circumstances.
This is shitty work all around and properly unacceptable.
While I appreciate the work you do and share some of your worries with code quality and UX degradation, what I find unacceptable is your behaviour towards other contributors and maintainers. Your messages written in this tone of voice will not serve towards any goal you might have (except if the goal is losing contributors). At present I'm feeling very discouraged to submit any further contributions to dt that might be subject to your review. It's a shame, since all of the earlier discussions I have had with you have been very pleasant and resulted in good outcomes.
@flannelhead : Just don't pay any attention to messages with crude words and insults. And don't be discouraged otherwise the project will be dead at some point and I'm wondering if this is not the goal here just for the sake of his own fork. That's what I do now. So until Aurélien learn to speak to others I won't be given a single though to what he has to say, period.
@TurboGit the destruction of the project comes from all the bugs and regressions you irresponsibly merge before due process is done, and the proof was made once again here that you merge shit you don't understand with no problem. Don't blame it on me. If you don't like my words, change the reality they describe.
You had plenty of time to review this and were notified for a couple of times. To me it started to seem like you have no interest in reviewing this at all. I agree that code should be carefully reviewed before merging, but I can't really blame Pascal here for doing so, given the circumstances.
As I said, it's the first semi-normal summer since Covid and people being away from computer during July and August is not unheard of. What was the rush anyway ?
So just coming back to the concerning delta E in the first post. That was a glitch in my build environment, probably due to some unrelated changed that coexisted with this fix in my environment (I probably had built with Release instead of RelWithDebInfo, and I also was experimenting with some other optimization flags at the time of writing this fix).
I created a branch where I reverted parts of the fix merged here and reran the integration test. Results below:
78747838c2a4d47d23dfba779f76abaf3c4caf2a - this is the state of current master as of writing this:
Test 0093-colorbalancergb-ucs
Image mire1.cr2
Expected CPU vs. current CPU report :
----------------------------------
Max dE : 1.03536
Avg dE : 0.00009
Std dE : 0.00646
----------------------------------
Pixels below avg + 0 std : 99.98 %
Pixels below avg + 1 std : 99.98 %
Pixels below avg + 3 std : 99.98 %
Pixels below avg + 6 std : 99.98 %
Pixels below avg + 9 std : 99.98 %
----------------------------------
Pixels above tolerance : 0.00 %
OK
a37dfc393d1ddf0f14bc06fa64cebbe88758464f - Reverting all the other changes from CPU path except clipping of J to the upper limit:
Test 0093-colorbalancergb-ucs
Image mire1.cr2
Expected CPU vs. current CPU report :
----------------------------------
Max dE : 1.03536
Avg dE : 0.00009
Std dE : 0.00646
----------------------------------
Pixels below avg + 0 std : 99.98 %
Pixels below avg + 1 std : 99.98 %
Pixels below avg + 3 std : 99.98 %
Pixels below avg + 6 std : 99.98 %
Pixels below avg + 9 std : 99.98 %
----------------------------------
Pixels above tolerance : 0.00 %
OK
c2f4db07ab9bf5e3f9c48775ffc3b3e07b641c55 - reverting also clipping of J to upper limit. The whole fix on CPU path is thus reverted.
Test 0093-colorbalancergb-ucs
Image mire1.cr2
Expected CPU vs. current CPU report :
----------------------------------
Max dE : 1.03536
Avg dE : 0.00009
Std dE : 0.00646
----------------------------------
Pixels below avg + 0 std : 99.98 %
Pixels below avg + 1 std : 99.98 %
Pixels below avg + 3 std : 99.98 %
Pixels below avg + 6 std : 99.98 %
Pixels below avg + 9 std : 99.98 %
----------------------------------
Pixels above tolerance : 0.00 %
OK
Notice that there is always a small delta E - this is probably due to some difference between my environment and the one that was used to generate the expected images in integration test repo (different compiler or something like that). But the delta E here is unchanged between the revisions, so there's no issue and the code that was merged is correct. Sorry about posting the initial delta E results which were clearly wrong and just caused by some glitch in my dev environment.