Bug 220684 – Remove explicit clamp to SRGB for Lab colors on CG platforms that support wide color

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sam Weinig]

To break up the lab(), lch() change, I opted to initially land with an explicit clamp to sRGB to avoid the subtle rounding issues we were seeing with CG.

We should remove explicit clamp to SRGB by either:
- Explicit conversion to ExtendedSRGB (-Inf,Inf), which will avoid using CG's color conversion code, but won't be re-usable down the road if we want to use CG for things like gradients in the Lab color space.
- Passing the Lab color to CG and identifying and working around the precision issues in the tests.

[Sam Weinig]

Created attachment 417768 [details]
Test of CG Lab approach

[Sam Weinig]

Created attachment 417769 [details]
Test of ExtendedSRGB approach

[Sam Weinig]

Probably should take this moment to also figure out a way to actually test out of sRGB gamut rendering in layout tests.

[Sam Weinig]

(In reply to Sam Weinig from comment #1)
> Created attachment 417768 [details]
> Test of CG Lab approach

imported/w3c/web-platform-tests/css/css-color/lab-001.html
imported/w3c/web-platform-tests/css/css-color/lab-006.html
imported/w3c/web-platform-tests/css/css-color/lch-001.html
imported/w3c/web-platform-tests/css/css-color/lch-006.html

Ok, so these 4 still fail. 

Using Digital Color Meter, there is a clear (albeit tiny) difference between the top and bottom half of the green squares in lab-001 and lch-001. (4, 128, 1 vs. 0, 128, 1).

More confusingly though, I can't see any differences in the lab-006 and lch-006, so something funky is going on there.

[Sam Weinig]

(In reply to Sam Weinig from comment #2)
> Created attachment 417769 [details]
> Test of ExtendedSRGB approach

This one actually isn't right as the toSRGBA() call in ColorCG.cpp is actually clamping when linearizing, so we will need to add explicit support for ExtendedSRGBA conversion if we want to take this approach.

[Sam Weinig]

Created attachment 417771 [details]
Test Case

[Chris Lilley]

> Using Digital Color Meter, there is a clear (albeit tiny) difference between the top and bottom half of the green squares in lab-001 and lch-001. (4, 128, 1 vs. 0, 128, 1).

Is Digital Color Meter a physical measuring instrument or a software color picker? For Lab, does it use a D50 whitepoint (like ICC, and most hardware instruments) or D65?

Looking at lch-006 and double-checking the conversion, lch(70% 70 90) is lab(70% 0 70) is sRGB rgb(76.6254% 66.3607% 5.5775%)
https://colorjs.io/apps/convert/?color=lch(70%25%2070%2090)&precision=6

The conversion was originally done using the sample code I wrote for CSS Color 4; and my check just now was using Color.js which Lea and I wrote.

In lab-01, the color is lab(46.277% -47.562 48.583) which comes out as sRGB rgb(-0.0013% 50.1956% 0.0006%).
https://colorjs.io/apps/convert/?color=lab(46.277%25%20-47.562%2048.583)&precision=6

lab-01 and lch-01 are supposed to be the lab and lch values for sRGB #008000 which, re-checking, is lch(46.2775% 67.9892 134.3912)or lab(46.2775% -47.5621 48.5837)

Maybe I should update that test to 4 decimal places? Just to reduce round-tripping cumulative roundoff. Also, it would again be better for these tests as a colored rectangles rather than text.

I also want to make some P3-gamut Lab and LCH tests. For a manual test that is easy, just use a media query to tell the tester to skip that test if P3 isn't supported. I'm not sure how to tell WPT to skip rather than pass (or fail) if P3 isn't supported though.

[Sam Weinig]

Created attachment 417779 [details]
Test of ExtendedSRGB approach

[Sam Weinig]

Created attachment 417780 [details]
ExtendedSRGB approach

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

(In reply to Chris Lilley from comment #7)
> > Using Digital Color Meter, there is a clear (albeit tiny) difference between the top and bottom half of the green squares in lab-001 and lch-001. (4, 128, 1 vs. 0, 128, 1).
> 
> Is Digital Color Meter a physical measuring instrument or a software color
> picker? For Lab, does it use a D50 whitepoint (like ICC, and most hardware
> instruments) or D65?

It's not physical, it's just a little utility that comes default with macOS that tells you the pixel values at positions on the screen. In this case, what is being sampled has been converted to sRGB already when rendering, since the way these tests work is to draw the test.html and test-ref.html into sRGB png files and then compare the two png files. I'm pretty sure it's just tiny differences between how I am converting Lab to sRGB vs. how CoreGraphics is doing it, just need to debug it a bit.

> 
> Looking at lch-006 and double-checking the conversion, lch(70% 70 90) is
> lab(70% 0 70) is sRGB rgb(76.6254% 66.3607% 5.5775%)
> https://colorjs.io/apps/convert/?color=lch(70%25%2070%2090)&precision=6
> 
> The conversion was originally done using the sample code I wrote for CSS
> Color 4; and my check just now was using Color.js which Lea and I wrote.
> 
> In lab-01, the color is lab(46.277% -47.562 48.583) which comes out as sRGB
> rgb(-0.0013% 50.1956% 0.0006%).
> https://colorjs.io/apps/convert/?color=lab(46.277%25%20-47.562%2048.
> 583)&precision=6
> 
> lab-01 and lch-01 are supposed to be the lab and lch values for sRGB #008000
> which, re-checking, is lch(46.2775% 67.9892 134.3912)or lab(46.2775%
> -47.5621 48.5837)
> 
> Maybe I should update that test to 4 decimal places? Just to reduce
> round-tripping cumulative roundoff. Also, it would again be better for these
> tests as a colored rectangles rather than text.

I already updated these tests to rectangles in our local copy, so we are good there. I am not sure if more decimal places will help, but I will try that. Thanks.

> 
> I also want to make some P3-gamut Lab and LCH tests. For a manual test that
> is easy, just use a media query to tell the tester to skip that test if P3
> isn't supported. I'm not sure how to tell WPT to skip rather than pass (or
> fail) if P3 isn't supported though.

Having a way to do that for reference style tests in WPT would be great if it doesn't exist yet.

[Sam Weinig]

Created attachment 417784 [details]
CoreGraphics Lab approach

[Chris Lilley]

I recalculated all the rgb values to a higher precision and avoiding cumulative roundoff error, which in some cases made small differences. The new values are left as review comments on your WPT pull request

https://github.com/web-platform-tests/wpt/pull/27202

[Sam Weinig]

Created attachment 418059 [details]
Patch

[Sam Weinig]

Ok, this is ready for review now. Going with the ExtendedSRGB approach since it gives identical results and keeps Lab/LCH working on pre-Big Sur WebKit.

[EWS]

Committed r271712: <https://trac.webkit.org/changeset/271712>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418059 [details].

[Radar WebKit Bug Importer]

<rdar://problem/73467262>

[Darin Adler]

Comment on attachment 418059 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        While CoreGraphics does support the Lab colorspace on some systems (Big Sur and later),
> +        to ensure all versions of WebKit can support wide color Lab this approach provides the
> +        most coverage.

This comment confuses me about what occurs on newer Apple platforms. Do we end up successfully creating a color with the Lab colorspace, and this new code ends up having no effect? Or do we not even try to use the CG support for Lab? Is there a good reason to use the CG Lab support if it’s present?

> Source/WebCore/platform/graphics/ColorTypes.h:52
> +template<typename> struct ExtendedRGBModel;

I guess this is "RGBModel first, then the rest alphabetical"? Why not just stay alphabetical instead of using a logical order?

> Source/WebCore/platform/graphics/ColorTypes.h:81
> +        { -std::numeric_limits<float>::infinity(), std::numeric_limits<float>::infinity() },
> +        { -std::numeric_limits<float>::infinity(), std::numeric_limits<float>::infinity() },
> +        { -std::numeric_limits<float>::infinity(), std::numeric_limits<float>::infinity() }

Surprising that this even includes the infinities. But I’m not saying it’s wrong.

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:108
>      // Some CG ports don't support all the color spaces required and return
>      // sRGBColorSpaceRef() for unsupported color spaces. In those cases, we
> -    // need to eagerly and potentially lossily convert the color into sRGB
> -    // ourselves before creating the CGColorRef.
> +    // need to eagerly convert the color into extended sRGB ourselves before
> +    // creating the CGColorRef.
> +    // FIXME: This is not a good way to indicate lack of support. Make this
> +    // more explicit.

On some platforms, extendedSRGBColorSpaceRef() will just return sRGBColorSpaceRef(). On those, we’d need to call toSRGBA, not toExtendedSRGBA. All non-Cocoa platforms behave that way, so Windows is in that mode. We can detect that by noticing that extendedSRGBColorSpaceRef() is == sRGBColorSpaceRef() or come up with a fancier way to detect that case.

Separate but related issue: this code is not specific to Lab. We will create extended sRGB CG colors after this change for any non-sRGB color space; more precise than the sRGB would have been. Is this new strategy always better? For performance? For correctness?

[Sam Weinig]

(In reply to Darin Adler from comment #18)
> Comment on attachment 418059 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418059&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        While CoreGraphics does support the Lab colorspace on some systems (Big Sur and later),
> > +        to ensure all versions of WebKit can support wide color Lab this approach provides the
> > +        most coverage.
> 
> This comment confuses me about what occurs on newer Apple platforms. Do we
> end up successfully creating a color with the Lab colorspace, and this new
> code ends up having no effect? Or do we not even try to use the CG support
> for Lab? 

We don't even try to create a CGColorRef in the lab color space. 

> Is there a good reason to use the CG Lab support if it’s present?

At this point I don't believe there is any benefit to creating a CGColorRef in the Lab color space. I am going to chat with the CoreGraphics folks about this to find out more (there are some really weird peculiarities around creating a CIELab compliant CGColorSpace that I would like to understand better as well), but if it turns out there are benefits, the only change we need to make is to update the labColorSpaceRef() in ColorSpaceCG.cpp to look like this:

CGColorSpaceRef labColorSpaceRef()
{
    static CGColorSpaceRef labColorSpaceRef;
    static std::once_flag onceFlag;
    std::call_once(onceFlag, [] {
#if HAVE(LAB_SUPPORT_IN_CORE_GRAPHICS)
        static constexpr CGFloat d50WhitePoint[] { 0.96422f, 1.0f, 0.82521f };
        static constexpr CGFloat labRange[] { -127.5, 127.5, -127.5, 127.5 };
        labColorSpaceRef = CGColorSpaceCreateLab(d50WhitePoint, nullptr, labRange);
#else
        labColorSpaceRef = sRGBColorSpaceRef();
#endif
    });
    return labColorSpaceRef;
}

(Notice those really odd 127.5 range values, I don't get those, but they are required).

> 
> > Source/WebCore/platform/graphics/ColorTypes.h:52
> > +template<typename> struct ExtendedRGBModel;
> 
> I guess this is "RGBModel first, then the rest alphabetical"? Why not just
> stay alphabetical instead of using a logical order?

It was really just done in the order they were added, and then I wanted ExtendedRGBModel to be next to RGBModel, but probably just making it alphabetical would be better.

> 
> > Source/WebCore/platform/graphics/ColorTypes.h:81
> > +        { -std::numeric_limits<float>::infinity(), std::numeric_limits<float>::infinity() },
> > +        { -std::numeric_limits<float>::infinity(), std::numeric_limits<float>::infinity() },
> > +        { -std::numeric_limits<float>::infinity(), std::numeric_limits<float>::infinity() }
> 
> Surprising that this even includes the infinities. But I’m not saying it’s
> wrong.

Yeah, it turned out that having the infinities in the ranges as markers is really helpful in writing generic code using these ranges (like the code that clamps and asserts valid values). In the case of clamping, to avoid unnecessary comparisons I use constexpr if and check for these infinities to remove any clamping that might happen (the compiler might do this for us, but being explicit makes it clear these are markers).

> 
> > Source/WebCore/platform/graphics/cg/ColorCG.cpp:108
> >      // Some CG ports don't support all the color spaces required and return
> >      // sRGBColorSpaceRef() for unsupported color spaces. In those cases, we
> > -    // need to eagerly and potentially lossily convert the color into sRGB
> > -    // ourselves before creating the CGColorRef.
> > +    // need to eagerly convert the color into extended sRGB ourselves before
> > +    // creating the CGColorRef.
> > +    // FIXME: This is not a good way to indicate lack of support. Make this
> > +    // more explicit.
> 
> On some platforms, extendedSRGBColorSpaceRef() will just return
> sRGBColorSpaceRef(). On those, we’d need to call toSRGBA, not
> toExtendedSRGBA. All non-Cocoa platforms behave that way, so Windows is in
> that mode. We can detect that by noticing that extendedSRGBColorSpaceRef()
> is == sRGBColorSpaceRef() or come up with a fancier way to detect that case.

I thought about this, and I don't think it is actually a problem. When this happens (e.g. we are on a platform without ExtendedSRGB CGColorSpace support) we create a CGColor with the (non-extended) sRGB color space and some extended sRGB values. What CG then does is clamp those values to fit in the sRGB color space. If we special cased platforms that don't support ExtendedSRGB CGColorSpace, we would just do the clamping ourselves instead. Admittedly, its a bit gross. I have some ideas on how to make this cleaner, hence the FIXME I added.

> 
> Separate but related issue: this code is not specific to Lab. We will create
> extended sRGB CG colors after this change for any non-sRGB color space; more
> precise than the sRGB would have been. 

It's not more precise actually, just a larger total range of values. Rather than being clamped from 0 to 1, the allowed values now extend outside of that range to the infinities, but 0.0 through 1.0 still mean exactly the same thing.

The code is written to be color type agnostic, but in reality, at least right now, the only color that can hit this case (at least on non-windows CoreGraphics) is Lab.

> Is this new strategy always better? For performance? For correctness?

I think it is always better, though I am not clear on the performance costs within CoreGraphics. At least from the memory side it's the same, CG always stores 4 floats no matter what. Whether they have some fast paths for non-extended SRGB I don't know. I'll check with the CoreGraphics team about this.

Thanks for the review.