Skip to content
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

Merged
merged 3 commits into from
Aug 11, 2024
Merged

Add Adobe RGB #411

merged 3 commits into from
Aug 11, 2024

Conversation

Kirk-Fox
Copy link
Contributor

This adds the Adobe RGB (1998) color space and standard (which uses a gamma 2.2 transfer function).

Copy link

codspeed-hq bot commented Aug 11, 2024

CodSpeed Performance Report

Merging #411 will degrade performances by 12.46%

Comparing Kirk-Fox:master (a06ce7a) with master (366046b)

Summary

❌ 1 regressions
✅ 46 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark master Kirk-Fox:master Change
matrix_inverse 409.7 ns 468.1 ns -12.46%

Copy link
Owner

@Ogeon Ogeon left a 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.


impl RgbStandard for AdobeRgb {
type Space = AdobeRgb;
type TransferFn = GammaFn;
Copy link
Owner

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.

Copy link

codecov bot commented Aug 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.80%. Comparing base (366046b) to head (a06ce7a).

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              
Components Coverage Δ
palette 82.86% <100.00%> (+0.06%) ⬆️
palette_derive 81.98% <ø> (ø)

@Kirk-Fox
Copy link
Contributor Author

Wow, yeah, that GammaFn bug is hard to spot. I added the custom implementation for AdobeRgb.

@Ogeon
Copy link
Owner

Ogeon commented Aug 11, 2024

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.

Wow, yeah, that GammaFn bug is hard to spot.

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 powf function, until we can have something like f32 in const generics.

@Ogeon Ogeon merged commit fab4412 into Ogeon:master Aug 11, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants