8000 Fix failures in relative-color-out-of-gamut-expected due to rgbToHwb algorithm by squelart · Pull Request #31636 · WebKit/WebKit · GitHub
[go: up one dir, main page]
Skip to content

Fix failures in relative-color-out-of-gamut-expected due to rgbToHwb algorithm #31636

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 11, 2024
Merged

Fix failures in relative-color-out-of-gamut-expected due to rgbToHwb algorithm #31636

merged 1 commit into from
Aug 11, 2024

Conversation

squelart
Copy link
Contributor
@squelart squelart commented Aug 2, 2024 •
edited by webkit-early-warning-system
Loading

d26cb4a

Fix failures in relative-color-out-of-gamut-expected due to rgbToHwb algorithm
https://bugs.webkit.org/show_bug.cgi?id=277538
rdar://133045692

Reviewed by Tim Nguyen.

The suggested rgbToHwb algorithm at https://drafts.csswg.org/css-color-4/#rgb-to-hwb
used to use rgbToHsl to compute the hue, discarding the saturation and lightness.
But rgbToHsl special-cases negative saturations (possible with out-of-gamut colors),
and flips the hue in that case, resulting in incorrect out-of-gamut HWB values.
This was fixed in w3c/csswg-drafts#10718

This patch here replicates w3c/csswg-drafts#10718 in the
equivalent WebKit function, it fixes failures in
relative-color-out-of-gamut-expected.html, but also in
color-mix-out-of-gamut-expected.html.

* LayoutTests/imported/w3c/web-platform-tests/css/css-color/parsing/color-mix-out-of-gamut-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-color/parsing/relative-color-out-of-gamut-expected.txt:
* Source/WebCore/platform/graphics/ColorConversion.cpp:
(WebCore::ExtendedSRGBA<float>>::convert):

Canonical link: https://commits.webkit.org/282101@main

f6a1b27

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2
✅ 🛠 tv
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@squelart squelart self-assigned this Aug 2, 2024
@squelart squelart added the CSS Cascading Style Sheets implementation label Aug 2, 2024
@webkit-early-warning-system
Copy link
Collaborator
webkit-early-warning-system commented Aug 2, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 3c5acad)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2
✅ 🛠 tv
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@squelart
Copy link
Contributor Author
squelart commented Aug 2, 2024 •
edited
Loading

Note: This patch does not reverse (parts of) https://commits.webkit.org/277999@main, because I wrote it before I looked for a cause. Reviewer(s), let me know if you'd prefer me to actually try and reverse (assuming the whole course of action is the right way to go, of course).

@squelart squelart requested a review from weinig August 2, 2024 09:03
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 2, 2024
@nt1m nt1m removed the merging-blocked Applied to prevent a change from being merged label Aug 2, 2024
@weinig
Copy link
Contributor
weinig commented Aug 4, 2024

Thanks for looking into this @squelart!

Is this fix purely based on what the current WPT tests are testing, or was there some conversion with the WG on this?

Perhaps I am misunderstanding standing you, but it seems like you are saying WebKit is doing what the spec suggests. If that is the case, should the test be updated instead?

@squelart
Copy link
Contributor Author
squelart commented Aug 4, 2024

Is this fix purely based on what the current WPT tests are testing, or was there some conversation with the WG on this?

Yes this PR intends to fix what the WPT tests are testing, but I think it's particular to WebKit because of how it handles hwb(from lab(100 104.3 -50.9) h w b) by closely following the specs (more below).

No conversation with WG, maybe we should start one, especially if the specs need updating.

Perhaps I am misunderstanding you, but it seems like you are saying WebKit is doing what the spec suggests. If that is the case, should the test be updated instead?

tl;dr: I believe WebKit follows the specs very closely, and it results in out-of-gamut errors.

The first failing test is:

  fuzzy_test_computed_color(`hwb(from lab(100 104.3 -50.9) h w b)`, `color(srgb 1.59343 0.58802 1.40564)`);

Now we have similar tests above that are not failing, e.g.:

  fuzzy_test_computed_color(`rgb(from lab(0 104.3 -50.9) r g b)`, `color(srgb 0.351376 -0.213938 0.299501)`);

Notice that the "from" colors are the same, there is a straight colorspace conversion, and the resulting relative colors are compared against the same srgb color.
This tells me that the test is most probably correct, especially since the whole test is about out-of-gamut colors, I would assume we want these to be preserved.
I've also verified this conversion in other places (other browsers, apps, and websites with their own js implementation).

Now, WebKit handles hwb(from lab(100 104.3 -50.9) h w b) internally by first converting to RGB (correctly), and then to HWB. I don't believe the specs suggest this, this is an implementation detail inside WebKit.

In any case, as extra proof that this is the point of failure, I can add the following test line:

  fuzzy_test_computed_color(`hwb(from color(srgb 1.59343 0.58802 1.40564) h w b / alpha)`, `color(srgb 1.59343 0.58802 1.40564)`);

And this fails the same way, pointing at the rgb-to-hwb conversion.

Now about the specs, we have https://drafts.csswg.org/css-color-4/#rgb-to-hwb

8.2. Converting sRGB Colors to HWB

Conversion in the reverse direction proceeds similarly. [from above: "Converting an HWB color to sRGB is straightforward, and related to how one converts HSL to RGB"]
[...]

function rgbToHwb(red, green, blue) {
    var hsl = rgbToHsl(red, green, blue);
    var white = Math.min(red, green, blue);
    var black = 1 - Math.max(red, green, blue);
    return([hsl[0], white*100, black*100]);
}

As you can see this uses rgbToHsl as-is, but then only uses the hue component.

rgbToHsl is defined at https://drafts.csswg.org/css-color-4/#rgb-to-hsl, and there it says:

Special care is taken to deal with intermediate negative values of saturation, which can be produced by colors far outside the sRGB gamut.

With the corresponding bit of code:

    // Very out of gamut colors can produce negative saturation
    // If so, just rotate the hue by 180 and use a positive saturation
    // see https://github.com/w3c/csswg-drafts/issues/9222
    if (sat < 0) {
        hue += 180;
        sat = Math.abs(sat);
    }

As it says, the hue is rotated and the saturation is made positive. This is probably the correct thing to do for HSL colors; my naive thinking is that it's like normalizing a vector with negative length by flipping its direction and negating the length, the resulting vector is the same.
But remember that rgbToHwb above only keeps the hue and drops the saturation, and I believe that's where the error ultimately comes from, we've effectively lost information.
Hence my proposed solution here: rgbToHwb should only use the parts of rgbToHsl that compute the pure hue, without influence from saturation (nor lightness).

Would you like me to open an issue against the specs? My suggestion for a spec change would be similar to this PR: Rewrite the rgbToHwb function so that it doesn't use rgbToHsl, but instead it should just do the minimum hue computation.

Sam, do you agree with my analysis?
And if you agree that the tests are correct (as they match results from multiple sources), I think this WebKit-specific fix could be merged before any potential WG decisions.

@weinig
Copy link
Contributor
weinig commented Aug 5, 2024

I think the tests are wrong, given they are supposed to test the spec.

Let’s take this to CSS WG GitHub for discussion.

If we want to make a short term fix, I would propose updating the test to match the spec, and pushing that to WPT.

@squelart squelart mentioned this pull request Aug 6, 2024
Closed
@squelart
Copy link
Contributor Author
squelart commented Aug 6, 2024

Alright, I've filed w3c/csswg-drafts#10695
While writing that I've found another argument for fixing the specs: The round trip hwbToRgb(rgbToHwb(rgb(...))) is broken for some out-of-gamut colors, and my suggestion fixes that. See https://codepen.io/squelart/pen/MWMoeoB for PoC.
Anyway, we can argue over at CSS WG before coming back here. 😄

@squelart squelart force-pushed the eng/fix-css-color-out-of-gamut-lab-or-lch-to-hwb branch from 3c5acad to f6a1b27 Compare August 9, 2024 07:24
@webkit-early-warning-system
Copy link
Collaborator
webkit-early-warning-system commented Aug 9, 2024 •
edited
Loading

EWS run on current version of this PR (hash f6a1b27)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2
✅ 🛠 tv
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 9, 2024
nt1m
nt1m approved these changes Aug 9, 2024
View reviewed changes
Copy link
Member
@nt1m nt1m left a comment

Choose a reason for hiding this comment

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

Matches the spec change at: w3c/csswg-drafts@ead8668

@nt1m nt1m removed the merging-blocked Applied to prevent a change from being merged label Aug 9, 2024
@squelart squelart added the merge-queue Applied to send a pull request to merge-queue label Aug 11, 2024
@squelart
Fix failures in relative-color-out-of-gamut-expected due to rgbToHwb …
d26cb4a 
…algorithm

https://bugs.webkit.org/show_bug.cgi?id=277538
rdar://133045692

Reviewed by Tim Nguyen.

The suggested rgbToHwb algorithm at https://drafts.csswg.org/css-color-4/#rgb-to-hwb
used to use rgbToHsl to compute the hue, discarding the saturation and lightness.
But rgbToHsl special-cases negative saturations (possible with out-of-gamut colors),
and flips the hue in that case, resulting in incorrect out-of-gamut HWB values.
This was fixed in w3c/csswg-drafts#10718

This patch here replicates w3c/csswg-drafts#10718 in the
equivalent WebKit function, it fixes failures in
relative-color-out-of-gamut-expected.html, but also in
color-mix-out-of-gamut-expected.html.

* LayoutTests/imported/w3c/web-platform-tests/css/css-color/parsing/color-mix-out-of-gamut-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-color/parsing/relative-color-out-of-gamut-expected.txt:
* Source/WebCore/platform/graphics/ColorConversion.cpp:
(WebCore::ExtendedSRGBA<float>>::convert):

Canonical link: https://commits.webkit.org/282101@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/fix-css-color-out-of-gamut-lab-or-lch-to-hwb branch from f6a1b27 to d26cb4a Compare August 11, 2024 22:29
@webkit-commit-queue
Copy link
Collaborator

Committed 282101@main (d26cb4a): https://commits.webkit.org/282101@main

Reviewed commits have been landed. Closing PR #31636 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit d26cb4a into WebKit:main Aug 11, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Aug 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nt1m nt1m nt1m approved these changes

@weinig weinig Awaiting requested review from weinig

Assignees

@squelart squelart

Labels

CSS Cascading Style Sheets implementation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@squelart @webkit-early-warning-system @weinig @webkit-commit-queue @nt1m @webkit-ews-buildbot
0