-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add Adobe RGB #411
Add Adobe RGB #411
Conversation
CodSpeed Performance ReportMerging #411 will degrade performances by 12.46%Comparing Summary
Benchmarks breakdown
|
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.
Ah, round two! Thank you, you know now what I'm usually looking for, so there's not much to discuss this time. :D There's only the issue with GammaFn
. I had somehow manged to get it backwards, probably when I refactored the transfer function traits.
palette/src/encoding/adobe.rs
Outdated
|
||
impl RgbStandard for AdobeRgb { | ||
type Space = AdobeRgb; | ||
type TransferFn = GammaFn; |
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.
The GammaFn
isn't going to be correct here. I noticed last weekend that there's a bug in it that makes it do the opposite transform. I would recommend implementing the traits on AdobeRgb
and use T::from_f64(256.0) / T::from_f64(563.0)
and its inverse. A small unit test to make sure it's not also backwards would be nice too.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #411 +/- ##
==========================================
+ Coverage 82.74% 82.80% +0.05%
==========================================
Files 129 130 +1
Lines 19970 20039 +69
Branches 19970 20039 +69
==========================================
+ Hits 16524 16593 +69
Misses 3319 3319
Partials 127 127
|
Wow, yeah, that GammaFn bug is hard to spot. I added the custom implementation for |
Great, thank you! I appreciate the help with these! By the way, I started looking into P3 before I got distracted by the backwards gamma function. Feel free to continue there if you feel like checking off more from the list, but it's enough to implement DCI-P3 and Display P3 for now. They are straight forward, as opposed to the HDR and D60/ACES cans of worms.
Hard to spot and easy to get wrong. I'll probably deprecate parts of it and replace it with something more explicit. Or replace it completely. There's little to gain with it, compared to just calling the |
This adds the Adobe RGB (1998) color space and standard (which uses a gamma 2.2 transfer function).
a98-rgb
task of CSS color module level 5 color space support #408 by implementing Adobe RGB (1998).