Bug 229024 – Support canvas text drawing with wide gamut colors

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Cameron McCormack (:heycam)]

.

[Radar WebKit Bug Importer]

<rdar://problem/81828477>

[Cameron McCormack (:heycam)]

Created attachment 435651 [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

[Cameron McCormack (:heycam)]

Created attachment 435653 [details]
Patch

[Sam Weinig]

Comment on attachment 435653 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:57
> +Color::Color(CGColorRef color, OptionSet<Flags> flags)

This will change the behavior of all sRGB CGColorRef -> Color creations to no longer clamp / use the inline form. The reason I had been holding off on doing this is I wanted to audit all the places where we use this constructor to see if it is what they really wanted.

What I would propose doing here is to make the Color(CGColorRef, OptionSet<Flags>) constructor (and any others that takes a CGColorRef) private, and replace the existing callers with an explicit roundAndClampToSRGBALossy() function. And then, I would add another function that does this color space aware conversion and update the one call site you are interested in here.

[Cameron McCormack (:heycam)]

Created attachment 435752 [details]
Patch

[Cameron McCormack (:heycam)]

(In reply to Sam Weinig from comment #5)
> This will change the behavior of all sRGB CGColorRef -> Color creations to
> no longer clamp / use the inline form. The reason I had been holding off on
> doing this is I wanted to audit all the places where we use this constructor
> to see if it is what they really wanted.
> 
> What I would propose doing here is to make the Color(CGColorRef,
> OptionSet<Flags>) constructor (and any others that takes a CGColorRef)
> private, and replace the existing callers with an explicit
> roundAndClampToSRGBALossy() function. And then, I would add another function
> that does this color space aware conversion and update the one call site you
> are interested in here.

That's reasonable.  I've done that in this updated patch.  (I still should write a separate test for text stroking and text shadows.) =

Let me know if you think it's worth having this extra code to try converting from CGColors with color spaces that aren't representable by WebCore::ColorSpace.  I chose to convert to XYZ since (I think?) that should avoid any clamping of colors.  Or if we should just convert to sRGB and be done with it, since it's unlikely to be a situation that comes up.

[Cameron McCormack (:heycam)]

Created attachment 435754 [details]
Patch

[Cameron McCormack (:heycam)]

Created attachment 435811 [details]
Patch

[Cameron McCormack (:heycam)]

Now with a couple of more tests and some iOS fixes.

[Sam Weinig]

Comment on attachment 435811 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:91
> +Color Color::createAndConvertToSRGBA(CGColorRef color, OptionSet<Flags> flags)

The name seems weird with the current behavior. Perhaps something like createAndLosslesslyConvertToSupportedColorSpace().

[Cameron McCormack (:heycam)]

Created attachment 435915 [details]
Patch

[Cameron McCormack (:heycam)]

Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/30098

[Cameron McCormack (:heycam)]

Created attachment 436126 [details]
Patch

With Windows build fixes.

[EWS]

Committed r281470 (240849@main): <https://commits.webkit.org/240849@main>

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