Bug 236200 - Conversion to a color space with a smaller gamut should perform gamut mapping
Summary: Conversion to a color space with a smaller gamut should perform gamut mapping
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-06 09:49 PST by Sam Weinig
Modified: 2022-02-08 10:59 PST (History)
5 users (show)

See Also:


Attachments
Patch (70.68 KB, patch)
2022-02-06 10:22 PST, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (76.14 KB, patch)
2022-02-06 14:01 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (74.26 KB, patch)
2022-02-07 17:11 PST, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (74.25 KB, patch)
2022-02-08 09:55 PST, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2022-02-06 09:49:28 PST
Conversion to a color space with a smaller gamut should perform gamut mapping
Comment 1 Sam Weinig 2022-02-06 10:22:03 PST
Created attachment 451043 [details]
Patch
Comment 2 EWS Watchlist 2022-02-06 10:23:23 PST
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 3 Sam Weinig 2022-02-06 10:24:04 PST
(You will have to look at the non-formatted diff to see the unicode artwork changes, as the prettifier does not seem to like them).
Comment 4 Sam Weinig 2022-02-06 14:01:24 PST
Created attachment 451051 [details]
Patch
Comment 5 Darin Adler 2022-02-06 15:05:02 PST
Comment on attachment 451051 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451051&action=review

r=me but I assume you’ll be looking into the test failure on gtk-wk2

> Source/WebCore/platform/graphics/ColorConversion.cpp:44
>      float hue = rad2deg(atan2(b, a));

This converts to double and then back to float. Maybe we should use std::rad2deg and std::atan2 to keep things as float?

> Source/WebCore/platform/graphics/ColorConversion.h:162
> +    template<typename ColorType> static auto mapToBoundedGamut(const ColorType& color) -> typename ColorType::BoundedCounterpart

Do you need the "->" part? Maybe auto can deduce the type from the return value in the function without it?

> Source/WebCore/platform/graphics/ColorConversion.h:180
> +    static constexpr const float JND = 0.02f;

No need for "const" here after "constexpr".

> Source/WebCore/platform/graphics/ColorConversion.h:198
> +        if (WTF::areEssentiallyEqual(colorInOKLCHColorSpace.lightness, 100.0f) || colorInOKLCHColorSpace.lightness > 100.0f)
> +            return { 1.0, 1.0, 1.0, resolvedColor.alpha };
> +        else if (WTF::areEssentiallyEqual(colorInOKLCHColorSpace.lightness, 0.0f))
> +            return { 0.0f, 0.0f, 0.0f, resolvedColor.alpha };

Surprised at the need for the "WTF::" prefix here.
Comment 6 Sam Weinig 2022-02-06 19:23:05 PST
(In reply to Darin Adler from comment #5)
> Comment on attachment 451051 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451051&action=review
> 
> r=me but I assume you’ll be looking into the test failure on gtk-wk2

Yep. 

> 
> > Source/WebCore/platform/graphics/ColorConversion.cpp:44
> >      float hue = rad2deg(atan2(b, a));
> 
> This converts to double and then back to float. Maybe we should use
> std::rad2deg and std::atan2 to keep things as float?

The reason this was using the unprefixed atan2 was for windows, where we have a macro that redefines atan2 on windows to wtf_atan2() which implements some bridging behavior (this is all in wtf/MathExtras.h).

I think there are two possibilities here:

1. Using std::atan2 on windows is compatible these days, and too just use it and remove the wtf_atan2() stuff.
2. We can't use std::atan2 but we should add a WTF::atan2() that we can overload ourselves.

(probably other options I can't think of).

As for rad2deg, that is a WTF thing and is overloaded for both double and float already.

> 
> > Source/WebCore/platform/graphics/ColorConversion.h:162
> > +    template<typename ColorType> static auto mapToBoundedGamut(const ColorType& color) -> typename ColorType::BoundedCounterpart
> 
> Do you need the "->" part? Maybe auto can deduce the type from the return
> value in the function without it?

It absolutely can, but I wanted to keep around to illustrate the contract of the gamut mapping functions, that they take in an extended color type, and return that extended color types, bounded counterpart. Since the body of the function also requires `typename ColorType::BoundedCounterpart` to be well defined, its not strictly necessary, but I think is still useful.

> 
> > Source/WebCore/platform/graphics/ColorConversion.h:180
> > +    static constexpr const float JND = 0.02f;
> 
> No need for "const" here after "constexpr".

Fixed.

> 
> > Source/WebCore/platform/graphics/ColorConversion.h:198
> > +        if (WTF::areEssentiallyEqual(colorInOKLCHColorSpace.lightness, 100.0f) || colorInOKLCHColorSpace.lightness > 100.0f)
> > +            return { 1.0, 1.0, 1.0, resolvedColor.alpha };
> > +        else if (WTF::areEssentiallyEqual(colorInOKLCHColorSpace.lightness, 0.0f))
> > +            return { 0.0f, 0.0f, 0.0f, resolvedColor.alpha };
> 
> Surprised at the need for the "WTF::" prefix here.

I was too. 

Also had me wondering if it worth adding a WTF::areEssentiallyGreaterThanOrEqual / WTF::areEssentiallyLessThanOrEqual/


Thanks for the review.
Comment 7 Sam Weinig 2022-02-07 17:11:43 PST
Created attachment 451178 [details]
Patch
Comment 8 EWS 2022-02-08 08:13:01 PST
ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.
Comment 9 Sam Weinig 2022-02-08 09:55:31 PST
Created attachment 451266 [details]
Patch
Comment 10 EWS 2022-02-08 10:58:44 PST
Committed r289396 (246973@main): <https://commits.webkit.org/246973@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451266 [details].
Comment 11 Radar WebKit Bug Importer 2022-02-08 10:59:18 PST
<rdar://problem/88640354>