New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dt UCS: clip lightness to allowed upper limit #12233
dt UCS: clip lightness to allowed upper limit #12233
Conversation
There seems to be a diff in the integration test.
|
Oops, of course one needs to clip Y before calculating L_star from it. Will fix tomorrow… |
5ae5f66
to
b855b78
Compare
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.
@TurboGit this would be something for dt 4.0.1. Review from @aurelienpierre would be appreciated. |
b855b78
to
bec4710
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
@aurelienpierre : I'd like to move forward for 4.0.1, is that ok for you? TIA. |
@flannelhead : Thanks! |
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:
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).
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
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.
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.
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.
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 I created a branch where I reverted parts of the fix merged here and reran the integration test. Results below: 7874783 - this is the state of current
a37dfc3 - Reverting all the other changes from CPU path except clipping of J to the upper limit:
c2f4db0 - reverting also clipping of J to upper limit. The whole fix on CPU path is thus reverted.
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. |
Fixes #12163.