-
Notifications
You must be signed in to change notification settings - Fork 30
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 support for ICtCp color space #216
Conversation
10a69a9
to
fe3de46
Compare
Thanks for the contribution! Before reviewing in detail I would like to get up to speed with (at least) CSS Color HDR Module. In the meantime, could you make an editorial change to rename this color space from |
@danburzo you're welcome! I will apply proposed naming change on weekend. If you want to run the formula for different colors, feel free to fork my notebook that uses minified version of the same code. BTW I've noted that there's a slight difference between spec-defined ICtCp values for
Proposed formula implementation is ported from |
Because I was tagged here, I've been following along. In case there is some concern about Basically, we are now using the more accurate rational number matrix from the ICtCp paper. It was given in the paper to convert the RGB values to LMS, but was not used in the calculation from XYZ via the authors. But since we are going from XYZ, you have to apply the appropriate XYZ matrix to get the more accurate XYZ to LMS matrix. This gives you values very close to zero for T and P for achromatic values. This of course improves accuracy for non-achromatic values as well. |
For the most accurate matrix, you can recalculate the matrix yourself with your own XYZ/Rec. 2020 matrix as shown here: https://github.com/facelessuser/coloraide/blob/main/tools/calc_ictcp_matrices.py (in case your XYZ/Rec. 2020 transform matrix is slightly different than Color.js). |
a9bcd67
to
88c4ec0
Compare
@danburzo I've applied proposed renames, now naming is consistent with |
@facelessuser thanks so much for pointing out these nuances. I've experimented a bit with within mentioned notebook and I saw that with matrices copied from |
I imagine you can get better than 1e-8. I get somewhere around 1e-16 to 2e-16. You can see live results for ColorAide here. Color.js gets close-ish to these results as well: There are obviously a number of factors that can affect the accuracy. We are introducing chromatic adaptation when going from Lab (D50) to ICtCp (D65), so the accuracy of those matrices are important. Second, the matrix can be more or less accurate depending on how good the Rec. 2020 to RGB matrix is and how well it aligns with the actual white point being used. I have not evaluated where and how Culori has come up with its matrices and specific white points being used. The most important thing is consistency. So, whatever white point Culori is using, it should be used to calculate the Rec.2020 RGB matrix, and then that matrix should be used to calculate the ICtCp matrix. |
Thanks @devgru for the updates, and thanks @facelessuser for the pointers! Reading the Dolby paper, I’m on the fence in regards to what constitutes the canonical conversion to LMS. Is it defined by the intent, ie. the Hunt-Pointer-Estevez XYZ to LMS adjusted with the crosstalk matrix? Or is it the direct rec2020-to-LMS matrix from BT.2100 which, in the words of the authors, was defined like that “for convenience”? In the PR on color.js Isaac mentions fixing For the PQ function, I’ve made a separate PR (now merged in The rest of the concerns are stylistic in nature, I’ll fix them post-merge. |
Here are my results for the XYZ from/to LMS matrices: import numpy as np;
np.set_printoptions(precision=16, sign='-', floatmode='fixed');
A = np.array([
[1.7166511879712683, -0.3556707837763925, -0.2533662813736599],
[-0.6666843518324893, 1.6164812366349395, 0.0157685458139111],
[0.0176398574453108, -0.0427706132578085, 0.9421031212354739]
]);
B = np.array([
[1688, 2146, 262],
[683, 2951, 262],
[99, 309, 3688]
]);
C = np.matmul(B / 4096, A);
# C
array([[ 0.3592832590121217, 0.6976051147779501, -0.0358915932320289],
[-0.1929421675348214, 1.1025652058879112, 0.0293737368853360],
[ 0.0070797844607477, 0.0748396662186366, 0.8433265453898765]])
# np.linalg.inv(C)
array([[ 2.0718247352617034, -1.3199721753353908, 0.1341516007515605],
[ 0.3638803308718196, 0.6772948457894078, -0.0081041392691667],
[-0.0496850897213268, -0.0490241910692021, 1.1853732722208565]]) |
Just make sure to dump the entire matrix up to 64 bits: np.set_printoptions(precision=16, sign='-', floatmode='fixed') |
If you are still getting only 1e-8 accuracy for achromatics, I would evaluate accuracy of CAT matrices or any matrices where the CAT was folded in, or just in general how the XYZ to RGB matrices were derived. There is no reason you shouldn't be able to get better than 1e-8 unless some of your matrices aren't quite accurate or you somehow used clamped some inverses to 32 bit or something like that. |
Oh, so that’s what was going on! Updated the results, thanks :-) |
One thing that may contribute to bad accuracy is the fact that the old CSS spec calculated the D50 <-> D65 matrices with a 32 bit inverse. If you incorporated that into your logic, it would also explain the poor accuracy. I think in November I informed them of the inaccuracy, and it has since been updated. The latest is: function D65_to_D50(XYZ) {
// Bradford chromatic adaptation from D65 to D50
// The matrix below is the result of three operations:
// - convert from XYZ to retinal cone domain
// - scale components from one reference white to another
// - convert back to XYZ
// see https://github.com/LeaVerou/color.js/pull/354/files
var M = [
[ 1.0479297925449969, 0.022946870601609652, -0.05019226628920524 ],
[ 0.02962780877005599, 0.9904344267538799, -0.017073799063418826 ],
[ -0.009243040646204504, 0.015055191490298152, 0.7518742814281371 ]
];
return multiplyMatrices(M, XYZ);
}
function D50_to_D65(XYZ) {
// Bradford chromatic adaptation from D50 to D65
// See https://github.com/LeaVerou/color.js/pull/360/files
var M = [
[ 0.955473421488075, -0.02309845494876471, 0.06325924320057072 ],
[ -0.0283697093338637, 1.0099953980813041, 0.021041441191917323 ],
[ 0.012314014864481998, -0.020507649298898964, 1.330365926242124 ]
];
return multiplyMatrices(M, XYZ);
} |
Wow, thanks for iterating on matrices. I was a bit confused with
as I am not 100% familiar with I'll also use new PQ implementation. Most likely I'll do that closer to weekend. Next week I'll have a vacation, so will have more spare time on weekdays. |
Here's what I found before updating matrices. We're pretty good with black representation in
That's not the case for white:
The reason is that
This reduces achievable accuracy in |
That's an indication that Coluri is likely using an inaccurate CAT matrix (conversion from Lab's D50 white point to D65 white point). I mentioned earlier that CSS updated their matrix because they were using an inverse of the CAT at only 32 bit precision. This is why you see inaccuracies after ~7 - 8 digits. |
Yes, that would be perfect! I will revise the chromatic adaptation matrices in a separate PR. |
As for matrices, there was a typo in one of them, Recalculated values are:
I've applied updated matrix and ran a test that verifies that The test fails in 17 cases out of 255 for matrices in current PR and 27 cases out of 255 for updated matrices — actually, I'm a bit surprised by this result. Here's the test that I used:
|
The typo notwithstanding (thank you for catching that) all other effects potentially stem from the matrix choices in various conversions. I’ve just pushed the (corrected) Python script to the repo so we have a reference, and I will incorporate updated matrices for |
I'm not surprised. The results are much better than what the HDR spec was showing. Results are better than 32 bit. ColorAide also has some cases that are slightly higher than 1e-14. These are still really good results compared to the previous results. Live example here. >>> LIMIT = 1e-14
>>> count = 0
>>> for i in range(256):
... c = i / 255
... color = Color('srgb', [c] * 3).convert('ictcp')
... if LIMIT < max(abs(color['cp']), abs(color['ct'])):
... print(color.coords())
... count += 1
...
[0.3217444983163378, 9.43689570931383e-15, 1.211530875622202e-14]
[0.36221688204506475, 1.0658141036401503e-14, 1.3579415369946446e-14]
[0.3888049160915181, 1.1879386363489175e-14, -9.645062526431047e-16]
[0.3906142708446122, -1.1102230246251565e-14, -3.019112737590035e-14]
[0.40124034856142143, 1.2212453270876722e-14, -8.743006318923108e-16]
[0.40297423934753707, -1.199040866595169e-14, -1.5432100042289676e-14]
[0.419767351862685, 2.4646951146678475e-14, 3.156502836887398e-14]
[0.44024139094244685, -1.3211653993039363e-14, 1.124100812432971e-15]
[0.452158773392722, 2.6645352591003757e-14, 3.39797634474337e-14]
[0.46500508165983473, -1.3211653993039363e-14, -3.6241842860107454e-14]
[0.4732631623179539, -1.4210854715202004e-14, -1.8041124150158794e-14]
[0.47461687454901896, -1.3877787807814457e-14, -1.765254609153999e-14]
[0.486523099789503, -6.994405055138486e-15, -1.9081958235744878e-14]
[0.5077539929125083, -1.4988010832439613e-14, -1.9373391779708982e-14]
[0.517227703532751, -1.554312234475219e-14, 1.0685896612017132e-15]
[0.5183902459102809, 7.216449660063518e-15, 1.9942381079829374e-14]
[0.5218498815349994, -1.4765966227514582e-14, -4.0190073491430667e-14]
[0.5673242146934809, -1.609823385706477e-14, -4.3950953987348385e-14]
>>> print(f'Total colors above threshold = {count}')
Total colors above threshold = 18 |
@danburzo that sounds great! BTW given current worse test results for updated matrices, would you like me to do any alterations to this PR or we're good to merge as is? |
I think we should, as they’re derived from the matrix we’re currently using. |
Alright, I've updated matrices in the PR. |
Perfect, thank you! |
I’ve made some code style / docs changes here: 53b4479 The |
@danburzo thanks so much! BTW what do you think of the state of types package? It's just slightly behind the current version, lacking maybe a couple of functions (I've noticed If you're planning to release v4 anytime soon, I think it might be a good idea to also update types for the package, I might invest some time into the update. |
As you can read in #128, I have not been involved in developing the types for Culori, and I lack the time and skill to manage them. I’m happy, however, if the community is willing to maintain a types package. |
Released in |
Happy New Year! 🎄
This PR introduces support for ICtCp color space.
Coloraide supports this color space for three years so I thought it would be safe to reuse @facelessuser's implementation.
The code is slightly simplified to be aligned with the rest of color spaces implementations supported by Culori, e.g. matrix-vector multiplication replaced with basic multiplication.
I've also updated docs, tests and API exports.
The PR also includes small fix of
tools/ranges.js
that should theoretically increase its precision. Currently for loops forr
,g
,b
never hit exact1
as step is not a power of 2.