hyperion.ng Saturation & Brightness/Value Gain using Oklab color space

Saturation & Brightness/Value Gain using Oklab color space

Open xkns opened this issue 2 years ago • 18 comments

Summary

Adds Saturation & Value Gain settings to the UI under Image Processing and adjusts the colors accordingly using Okhsv. As discussed on Discord this was done using a new OkhsvTransform class.

Fixes #822, #1092 and partially fixes #1142.

What kind of change does this PR introduce? (check at least one)

[ ] Bugfix
[x] Feature
[ ] Code style update
[ ] Refactor
[ ] Docs
[ ] Build-related changes
[ ] Other, please describe:

If changing the UI of web configuration, please provide the before/after screenshot: Images

Does this PR introduce a breaking change? (check one)

[ ] Yes
[x] No

If yes, please describe the impact and migration path for existing setups:

The PR fulfills these requirements:

[x] When resolving a specific issue, it's referenced in the PR's body (e.g. Fixes: #xxx[,#xxx], where "xxx" is the issue number)

If adding a new feature, the PR's description includes:

[x] A convincing reason for adding this feature
[ ] Related documents have been updated (docs/docs/en)
[ ] Related tests have been updated

PLEASE DON'T FORGET TO ADD YOUR CHANGES TO CHANGELOG.MD

[x] Yes, CHANGELOG.md is also updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

Jun 11 '22 19:06 xkns

Hello @xkns :wave:

I'm the Hyperion Project Bot and I want to thank you for contributing to Hyperion with your pull requests!

To help you and other users test your pull requests faster, I'll create a link for you to your workflow artifacts.

:link: https://github.com/hyperion-project/hyperion.ng/actions/runs/2481085530

Of course, if you make changes to your PR, I will create a new link.

Best regards, Hyperion Project

Jun 11 '22 19:06 hyperion-project[bot]

This pull request introduces 5 alerts when merging b8744535557117ebc9ace83fb9bb1dd6202451d6 into 359618a4eefd4fc4a27d49993d9960e8f896c544 - view on LGTM.com

new alerts:

4 for Multiplication result converted to larger type
1 for Declaration hides parameter

Jun 11 '22 20:06 lgtm-com[bot]

This pull request introduces 5 alerts when merging b874453 into 359618a - view on LGTM.com

new alerts:
* 4 for Multiplication result converted to larger type

* 1 for Declaration hides parameter

These are all in the Oklab reference implementation. Not sure how easy it is license wise to try to fix them.

Edit: As the reference implementation is licensed under MIT it appears that we can modify and redistribute it under any license as long as the copyright is kept. But I'm not a lawyer.

Jun 11 '22 22:06 xkns

Hey @xkns I created a new link to your workflow artifacts: :link: https://github.com/hyperion-project/hyperion.ng/actions/runs/2565443187

Jun 26 '22 20:06 hyperion-project[bot]

This pull request introduces 5 alerts when merging 5a295b9fb2ba5508271292d3482a92b3a347d8e6 into 359618a4eefd4fc4a27d49993d9960e8f896c544 - view on LGTM.com

new alerts:

4 for Multiplication result converted to larger type
1 for Declaration hides parameter

Jun 26 '22 21:06 lgtm-com[bot]

I deeply hope that this PR is accepted. I compiled this and the saturation and brightness gain controls put Hyperion over the line for me.

Would the translation to OKLAB facilitate the implementation of ICC profiles for calibration? I find that my particular LEDs render blue midtones as a rich cyan in a way that cannot be corrected through Hyperion's calibration options without ruining the white balance.

Jul 07 '22 08:07 cjohnsto-nz

I deeply hope that this PR is accepted. I compiled this and the saturation and brightness gain controls put Hyperion over the line for me.

I've been in contact with the maintainer (@Lord-Grey) and I'm optimistic that this will get merged at some point. We'll see how the code review goes...

Would the translation to OKLAB facilitate the implementation of ICC profiles for calibration? I find that my particular LEDs render blue midtones as a rich cyan in a way that cannot be corrected through Hyperion's calibration options without ruining the white balance.

We have also started to look into this issue. While I believe that it is theoretically possible to get a good color correction profile set using the current settings (color setpoints + gamma curves) we agree that it isn't very intuitive to do so.

I don't think that importing ICC files is the solution here as that would require users to use some external software to generate those files. Fine tuning would also be very annoying as one would have to import the newly adjusted ICC file every single testing iteration.

I'm sure that we can find a more user friendly way but that will be in a new PR. Whether or not Oklab will play a part in that is to be seen but the necessary facilities will be present when this PR is merged.

Jul 07 '22 10:07 xkns

Awesome to hear. I notice that the cyan, yellow, magenta, and white values appear to no longer work. I'm guessing they're discarded during conversion. That might need to be made more clear in ui.

From: xkns @.> Sent: Thursday, July 7, 2022 10:51:47 PM To: hyperion-project/hyperion.ng @.> Cc: Chris Johnstone @.>; Comment @.> Subject: Re: [hyperion-project/hyperion.ng] Saturation & Brightness/Value Gain using Oklab color space (PR #1477)

I deeply hope that this PR is accepted. I compiled this and the saturation and brightness gain controls put Hyperion over the line for me. I've been in contact with the maintainer @.***) and I'm optimistic that this will get merged at ZjQcmQRYFpfptBannerStart CAUTION: This Email is from an EXTERNAL source. Ensure you trust this sender before clicking on any links or attachments.

ZjQcmQRYFpfptBannerEnd

I deeply hope that this PR is accepted. I compiled this and the saturation and brightness gain controls put Hyperion over the line for me.

I've been in contact with the maintainer @.***https://github.com/Lord-Grey) and I'm optimistic that this will get merged at some point. We'll see how the code review goes...

I'm sure that we can find a more user friendly way but that will be in a new PR. Whether or not Oklab will play a part in this is to be seen but the necessary facilities will be present when this PR is merged.

— Reply to this email directly, view it on GitHubhttps://github.com/hyperion-project/hyperion.ng/pull/1477#issuecomment-1177394241, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AFCPQMS7X6OGXF2OIEWN37LVS2ZEHANCNFSM5YQRPNGQ. You are receiving this because you commented.Message ID: @.***>

Chris Johnstone Production Print Specialist Ricoh New Zealand Limited Mobile: +64 21 849 115 www.ricoh.co.nz

ISO 14001 Environmentally certified | ISO 27001 Information Security Certified

Jul 07 '22 10:07 cjohnsto-nz

Edit: Sorry I misunderstood what you were saying. You're talking about the color setpoint settings and not that certain input colors go missing at some point.

The Okhsv transform is being applied before the color correction. It converts to Okhsv, then scales s (-aturation) and v (-alue) by the given factors and finally converts back to sRGB. The resulting color then runs through the color correction and all the other options before finally being output to the output device.

While the color correction should still work you might need to re-adjust it a bit since most colors reaching the color correction could now be in a different region of the color gamut (e.g. they are more saturated now) which you might not have tuned for previously. A new color correction system could definitely help out here.

The whole Okhsv transformation will be skipped when both saturation and brightness gain are set to their neutral value of 1.0. That will allow you to do some quick A/B testing.

Jul 07 '22 11:07 xkns

@xkns Sorry, for only coming back on this now.

a) As I am not too detailed in the Color transformation subject, maybe you could help me in understanding why the saturation and value gain are transformed before the RGB color transformations. By heart I would have expected that first Colors are mapped and then gains applied. Maybe I am missing some theory here....

b) What is the number range for saturation and value gain? Having 0-100 with a stepping of 0.1 does not seem to be reasonable. Or did you wanted to express a range of 0-100 percent? (Some definitions in the JSONs are currently not ok, but I would like to update them depending on the outcome of the above)

c) Should the RGB+SatGain+ValGain an alternative way to the existing calibration rather then one on top? Would a user be able to control 11+ values to achieve a good calibration?

d) What would be the guidance for a standard user going forward? What are the elements that should be shown on the screen by default? (With input I will make it happen) Via "expert" level all elements could be shown...

Sorry for those many questions, but I would like to get a better feels in which direction the target design should go.

Jul 26 '22 19:07 Lord-Grey

Edit: As the reference implementation is licensed under MIT it appears that we can modify and redistribute it under any license as long as the copyright is kept. But I'm not a lawyer.

As this is MIT, we can modify the code. Good practice is to keep the original copyright, plus adding info on change e.g.:

Jul 26 '22 19:07 Lord-Grey

a) As I am not too detailed in the Color transformation subject, maybe you could help me in understanding why the saturation and value gain are transformed before the RGB color transformations. By heart I would have expected that first Colors are mapped and then gains applied. Maybe I am missing some theory here....

The order of operations should be irrelevant for hue and saturation. If you were to first saturate a color and then map it or first map it and then saturate the mapped color shouldn't make a significant difference (some rounding errors might occur).

What does make a difference is when you are limiting the brightness through the existing mapper and were to then apply some brightness gain (>= 1.0). In that case you would exceed the maximum brightness as set by your mapping.

There are basically three options here:

Okhsv transform last: You would need to limit the max brightness again and by the same value as given for the other mapping.
Okhsv transform somewhere in the middle: The RgbTransform gets compiled into what is effectively a mapping matrix. This is very efficient as it can do all operations in one go. Splitting that matrix up into two in order to fit the Okhsv transform in the middle would lose some of that benefit.
Okhsv transform first: Let RgbTransform do its thing.

In theory you could construct some "super" matrix that does everything at once but I don't have a good enough understanding of the Okhsv definition / reference implementation to attempt such a thing.

b) What is the number range for saturation and value gain? Having 0-100 with a stepping of 0.1 does not seem to be reasonable. Or did you wanted to express a range of 0-100 percent? (Some definitions in the JSONs are currently not ok, but I would like to update them depending on the outcome of the above)

1.0 is the neutral and (intended) default value for both settings. A brightness gain of 0.0 doesn't make too much sense to me as that would always result in total 0, 0, 0 blackness. Desaturation gain of 0.0 would completely desaturate leaving only grayscale colors which is a plausible input. Values smaller than 0.0 have no valid result. On the opposite end gains of 255 and larger would probably always result in perfect white 255, 255, 255 although I haven't tested this extrema. 100 brightness gain is effectively the current brightness times 100 (clipped to the configured maximum brightness). I wasn't sure exactly where to put the upper limit.

Should you want to use percent instead all values would simply have to be multiplied by 100 (including the upper limits).

Using other scales would also be possible but a simple multiplier is quite intuitive in my opinion. Brightness gain for example could use the same input curve as gamma which would make them act identical given that all three gamma values are the same. You would however lose the ability to "overdrive" the brightness (which might not even be a bad thing). I would be happy to implement something else instead if you have an idea.

c) Should the RGB+SatGain+ValGain an alternative way to the existing calibration rather then one on top? Would a user be able to control 11+ values to achieve a good calibration?

ValGain is redundant in this sense because you should theoretically be able achieve the same effect by setting all three gamma values just right. What it can't do however is account for different physical light output by various color channels on the hardware. This is important for getting mixed colors (including gray tones) right.

I'm not sure if I would count SatGain as a calibration measure or simply an aesthetic effect. It's not one of the "calibration settings" you would find in other pieces of software but you could argue that (especially in HDR situations) the LED color output might be lacking in saturation / looking washed out. In that sense it would calibrate something that other existing settings cannot account for.

d) What would be the guidance for a standard user going forward? What are the elements that should be shown on the screen by default? (With input I will make it happen) Via "expert" level all elements could be shown...

This probably needs some additional discussion but I would say that saturation is something that a standard user has some intuition for. As it isn't really redundant there isn't really an alternative expert/non-expert setting to show instead.

Brightness gain on the other hand is mostly identical (the curves diverge more as you get farther away from the neutral value) to gamma settings if all gamma settings have the same value. So the gamma settings could be regarded as the expert brightness gain. This is especially the case if we were to change the input curve to match the gamma formula.

Sorry for those many questions, but I would like to get a better feels in which direction the target design should go.

Thanks for taking the time and sorry for messing up the JSON definitions.

Jul 26 '22 19:07 xkns

Hey @xkns I created a new link to your workflow artifacts: :link: https://github.com/hyperion-project/hyperion.ng/actions/runs/2761779884

Jul 29 '22 15:07 hyperion-project[bot]

This pull request introduces 2 alerts when merging b2b06c035691977cb51c21e44fe03ce0b3308999 into 8000c3e8b73e6c33a9cd7086bb0a0b0ef9d4604f - view on LGTM.com

new alerts:

1 for Useless assignment to local variable
1 for Declaration hides parameter

Jul 29 '22 16:07 lgtm-com[bot]

Hey @xkns I created a new link to your workflow artifacts: :link: https://github.com/hyperion-project/hyperion.ng/actions/runs/2762745343

Jul 29 '22 19:07 hyperion-project[bot]

This pull request introduces 1 alert when merging f181d9bca4703ff14d6f6854c3ea1d87413b671e into 8000c3e8b73e6c33a9cd7086bb0a0b0ef9d4604f - view on LGTM.com

new alerts: