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 Rec. 709 and Rec. 2020 #410

Merged
merged 9 commits into from
Aug 11, 2024
Merged

Add Rec. 709 and Rec. 2020 #410

merged 9 commits into from
Aug 11, 2024

Conversation

Kirk-Fox
Copy link
Contributor

@Kirk-Fox Kirk-Fox commented Aug 9, 2024

I have added the Rec. 709 and Rec. 2020 standards to the crate. Rec. 709 is simply the same as Srgb with a different transfer function, called RecOetf. Rec. 2020 is a wide color gamut standard using this same transfer function.

For Rec. 2020, I am not sure how to best implement conversions from floating-point to unsigned ints since, due to the wide gamut, it typically operates with either 10 or 12 bits per color channel, making the typical use of u8's not ideal. Ideally, there could be a lookup table containing 4096 values that could be indexed into variably depending on the chosen bit depth (e.g. REC_TO_F64[4*value] for a 10-bit Rec. 2020 value, or REC_TO_F64[16*value] for an 8-bit Rec. 709 value. I do not know the best way to incorporate any sort of variable bit depth into the library, though.

Copy link

codspeed-hq bot commented Aug 10, 2024

CodSpeed Performance Report

Merging #410 will degrade performances by 12.46%

Comparing Kirk-Fox:master (b120695) with master (39a9c7a)

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

codecov bot commented Aug 10, 2024

Codecov Report

Attention: Patch coverage is 72.91667% with 39 lines in your changes missing coverage. Please review.

Project coverage is 82.74%. Comparing base (39a9c7a) to head (b120695).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #410      +/-   ##
==========================================
- Coverage   82.81%   82.74%   -0.08%     
==========================================
  Files         128      129       +1     
  Lines       19826    19970     +144     
  Branches    19826    19970     +144     
==========================================
+ Hits        16419    16524     +105     
- Misses       3302     3319      +17     
- Partials      105      127      +22     
Components Coverage Δ
palette 82.80% <72.91%> (-0.08%) ⬇️
palette_derive 81.98% <ø> (ø)

@Ogeon
Copy link
Owner

Ogeon commented Aug 10, 2024

Thanks a lot for taking the time to put this together! If you don't mind, I would appreciate a comment with a reference or link to where the matrices are taken from. It's good for potential bug hunting or correctness checking. Also, a simple unit test for the transfer functions would be nice.

For Rec. 2020, I am not sure how to best implement conversions from floating-point to unsigned ints since, due to the wide gamut, it typically operates with either 10 or 12 bits per color channel, making the typical use of u8's not ideal. Ideally, there could be a lookup table containing 4096 values that could be indexed into variably depending on the chosen bit depth (e.g. REC_TO_F64[4*value] for a 10-bit Rec. 2020 value, or REC_TO_F64[16*value] for an 8-bit Rec. 709 value. I do not know the best way to incorporate any sort of variable bit depth into the library, though.

This library doesn't have a good model for integer representations beyond the most naive option, so I'm not sure there's a good way of implementing it yet. The conversion should ideally depend on the RGB space/standard in some way, so it can include things like black levels as well. I think that needs a bit more design work and probably a few breaking changes.

@Kirk-Fox
Copy link
Contributor Author

I don't have a direct source for the matrix values since I calculated them independently using the formulas specified at http://www.brucelindbloom.com/index.html?Eqn_RGB_XYZ_Matrix.html, but I will include that fact in a comment.

Would it be good to include IntoLinear and FromLinear implementations between floats and u8's for 8-bit color for the time being to keep things consistent across the RGB spaces?

@Ogeon
Copy link
Owner

Ogeon commented Aug 10, 2024

I don't have a direct source for the matrix values since I calculated them independently using the formulas specified at http://www.brucelindbloom.com/index.html?Eqn_RGB_XYZ_Matrix.html, but I will include that fact in a comment.

Thanks, it would be great if it's mentioned somewhere. I try to keep a trail of how the numbers ended up how and where they are with newer code.

I found the specification (https://www.itu.int/rec/R-REC-BT.2020), but it doesn't provide a pre-computed matrix. Also, this site has almost the same matrices, maybe with a bit more precision: https://antlerpost.com/colour-spaces/Rec2020.html.

Would it be good to include IntoLinear and FromLinear implementations between floats and u8's for 8-bit color for the time being to keep things consistent across the RGB spaces?

It would at least make sense for Rec. 709 to speed it up with a lookup table, so feel free to add them. Note, though, that Palette is currently relying on fast-srgb8 for the f32 to u8 conversion. I haven't decided if I want to attempt to add a generalized implementation, but it would make sense to support more transfer functions at some point...

@Kirk-Fox
Copy link
Contributor Author

I found the specification (https://www.itu.int/rec/R-REC-BT.2020), but it doesn't provide a pre-computed matrix. Also, this site has almost the same matrices, maybe with a bit more precision: https://antlerpost.com/colour-spaces/Rec2020.html.

It seems somehow that these have less precision since they fail the conversion tests. I used a program that eliminated rounding errors until the final step to calculate the matrices so I went ahead and kept the ones I calculated and included the comment mentioned.

It would at least make sense for Rec. 709 to speed it up with a lookup table, so feel free to add them.

Lookup tables for RecOetf have been added.

I also went ahead and added various unit tests to test the various implementations of the transfer function (which helped me find a sign error in the original code I had submitted).

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.

Thank you! I think it's pretty much good to go, except for the comment about float to uint conversion.

palette/src/encoding/rec_standards.rs Show resolved Hide resolved
@Kirk-Fox
Copy link
Contributor Author

It took a lot of effort and complicated math, but I was finally able to understand the fast-srgb8 algorithm and adapt it to the RecOetf transfer function. If it'd still be better to remove those implementations for now, though, just let me know.

@Ogeon
Copy link
Owner

Ogeon commented Aug 11, 2024

Oh, wow! It was already past midnight for me when you asked. I just came in now to say "go ahead", but you already did. 😅 Thanks again for all the work!

Something I was going to say with my "go ahead" was that my hope has been to generalize it enough to be able to generate tables "on the fly". I still haven't studied how it works or had a proper look at your implementation, so I'm still in that "concept" mindset, but it would be neat to be able to plug in a fn(u8) -> f32 function or similar and get a pair of tables out. Both because this surely isn't the last place where this will be useful, but also as a way of generating fast, user specified transforms. Now, I'll try to wrap my head around this too.

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.

Alright, I think I get the overall picture. The numbers in the table are split into bias and scale halves. These form a set of lines that touch or get close to the actual curve, right? I think the missing piece for me is where the table values come from. Going down on the bit level isn't my strongest side, so bear with me if I'm missing something.

I did find the source of the algorithm, so I will have a closer look at it: https://gist.github.com/rygorous/2246678 and https://gist.github.com/2203834

Edit: Ah, I see, the recipe is right there in make_tab4 👀

palette/src/encoding/rec_standards.rs Outdated Show resolved Hide resolved
palette/src/encoding/rec_standards.rs Outdated Show resolved Hide resolved
@Kirk-Fox
Copy link
Contributor Author

I went and made those changes you mentioned. As far as making the tables goes, I'm not sure if I used the same method, but it gives near equal results to the original formula. Here is my working through the reverse-engineering process in a formatted PDF, but be warned it involves a bit of calculus.

I could implement a table-generating function that takes the transfer function and its derivative as inputs, but since it can't be constant time (due to floating point math) and it isn't part of the public API, I'm not sure where to include it.

@Ogeon
Copy link
Owner

Ogeon commented Aug 11, 2024

Oh, that's neat! I think the current implementation is good enough for this time. I can generalize it later and try a few options. Once again, it was more a question of where the numbers come from, but you have thoroughly documented the process.

I think we can go ahead and merge this after my other two comments have been addressed. I was a bit tired yesterday and didn't notice the missing derives.

@Kirk-Fox
Copy link
Contributor Author

Thanks for pointing out the derives. I totally spaced on those. Hopefully everything is good now

@Ogeon Ogeon changed the title Adding Rec. 709 and Rec. 2020 (bit depth needs consideration) Add Rec. 709 and Rec. 2020 Aug 11, 2024
@Ogeon
Copy link
Owner

Ogeon commented Aug 11, 2024

Absolutely, the tests are happy and I'm happy. I took the liberty to edit the title for the release notes, in case you wonder. Thanks a lot! 🙏 Let's check these off in the list ✔️

@Ogeon Ogeon merged commit 366046b into Ogeon:master Aug 11, 2024
16 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