Bug 236200 – Conversion to a color space with a smaller gamut should perform gamut mapping

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Conversion to a color space with a smaller gamut should perform gamut mapping

[Sam Weinig]

Created attachment 451043 [details]
Patch

[EWS Watchlist]

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

[Sam Weinig]

(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).

[Sam Weinig]

Created attachment 451051 [details]
Patch

[Darin Adler]

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.

[Sam Weinig]

(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.

[Sam Weinig]

Created attachment 451178 [details]
Patch

[EWS]

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

[Sam Weinig]

Created attachment 451266 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/88640354>