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

color_space_conversion: use exact rational matrix formulations #1089

Merged
merged 3 commits into from
Mar 21, 2022

Conversation

kainino0x
Copy link
Collaborator

On a few tests, the conversion result was exactly the same down to the ULP. But these make a nice reference implementation since they can be copied to other contexts with arbitrary precisions.

Unfortunately, since the results didn't change, this didn't fix relatively large deltas I saw in the G channel of these tests (not huge, 0.0007). It's unclear whether we can fix that or if we'll keep those thresholds (in #1055).

Issue: #913


Requirements for PR author:

  • All missing test coverage is tracked with "TODO" or .unimplemented().
  • New helpers are /** documented */ and new helper files are found in helper_index.txt.
  • Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)

Requirements for reviewer sign-off:

  • Tests are properly located in the test tree.
  • Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
  • Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
  • Helpers and types promote readability and maintainability.

When landing this PR, be sure to make any necessary issue status updates.

@shaoboyan
Copy link
Contributor

shaoboyan commented Mar 21, 2022

Recalled that the diff might also generated by the chromium implementation. It always use XYZ D50 color space as intermediate color space to calculate the conversion matrix. This might introduce D65 -> D50 conversion with Bradford chromatic adaptation(Not 100% sure uses this method).
And the reason is that ICC profiles are defined relative to a D50 reference white.

@shaoboyan
Copy link
Contributor

Bradford chromatic adaptation which is also used by CSS4 spec. (More info).

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
	// http://www.brucelindbloom.com/index.html?Eqn_ChromAdapt.html

Copy link
Contributor

@shaoboyan shaoboyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formulation LGTM!

@kainino0x
Copy link
Collaborator Author

Your idea about D50 makes sense! Do you think it's just due to precision loss in the D50/D65 conversions or an actual error in the computation?

@kainino0x kainino0x merged commit f66ce52 into gpuweb:main Mar 21, 2022
@kainino0x kainino0x deleted the exact-colorspaces branch March 21, 2022 23:49
@github-actions
Copy link

Previews, as seen when this build job started (326c027):
Run tests | View tsdoc

@shaoboyan
Copy link
Contributor

shaoboyan commented Mar 22, 2022

Do you think it's just due to precision loss in the D50/D65 conversions

I think it is due to precision loss considering the big diff happens around ~0 values. And the method to convert from D65/D50 are various. We cannot control the implementation in browser and I think chromium chooses the recommended one.

kainino0x added a commit to kainino0x/csswg-drafts that referenced this pull request May 27, 2022
These color space conversion matrices have exact rational values that
can be computed from the numbers provided in the spec. Using exact
values is more succinct for most of these matrices, and also makes it a
nice reference implementation for other languages. This example code
already uses exact inline formulations for a number of other things,
like D50 and D65 definitions, so this is similar to that.

I only did the XYZ conversion matrices for srgb, display-p3, a98-rgb,
and rec2020.
- I don't have code to easily compute the D65/D50 conversions or
  OKLab/OKLCH as I was only interested in the predefined color spaces.
- The rational forms of prophoto-rgb's matrices exceed the precision of
  JavaScript math. I could include them as comments though.

Source to compute these: https://github.com/kainino0x/exact_css_xyz_matrices
using this Rust crate: https://crates.io/crates/rgb_derivation
as described for sRGB on this page: https://mina86.com/2019/srgb-xyz-matrix/
but using the numbers from this spec.

I used these in the WebGPU conformance test suite:
gpuweb/cts#1089
WebGPU needed only srgb and display-p3, but it was easy to extend to the
other predefined color spaces. (WebGPU may add some of those color
spaces eventually anyway.)
svgeesus pushed a commit to w3c/csswg-drafts that referenced this pull request Aug 23, 2022
These color space conversion matrices have exact rational values that
can be computed from the numbers provided in the spec. Using exact
values is more succinct for most of these matrices, and also makes it a
nice reference implementation for other languages. This example code
already uses exact inline formulations for a number of other things,
like D50 and D65 definitions, so this is similar to that.

I only did the XYZ conversion matrices for srgb, display-p3, a98-rgb,
and rec2020.
- I don't have code to easily compute the D65/D50 conversions or
  OKLab/OKLCH as I was only interested in the predefined color spaces.
- The rational forms of prophoto-rgb's matrices exceed the precision of
  JavaScript math. I could include them as comments though.

Source to compute these: https://github.com/kainino0x/exact_css_xyz_matrices
using this Rust crate: https://crates.io/crates/rgb_derivation
as described for sRGB on this page: https://mina86.com/2019/srgb-xyz-matrix/
but using the numbers from this spec.

I used these in the WebGPU conformance test suite:
gpuweb/cts#1089
WebGPU needed only srgb and display-p3, but it was easy to extend to the
other predefined color spaces. (WebGPU may add some of those color
spaces eventually anyway.)
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