Bug 229024

Summary: Support canvas text drawing with wide gamut colors
Product: WebKit Reporter: Cameron McCormack (:heycam) <heycam>
Component: CanvasAssignee: Cameron McCormack (:heycam) <heycam>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, clopez, dino, eric.carlson, esprehn+autocc, ews-watchlist, glenn, jer.noble, kondapallykalyan, pdr, philipj, sam, sergio, simon.fraser, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://github.com/web-platform-tests/wpt/pull/30098
Bug Depends on:    
Bug Blocks: 225140    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Cameron McCormack (:heycam) 2021-08-11 21:38:00 PDT
.
Comment 1 Radar WebKit Bug Importer 2021-08-11 21:41:16 PDT
<rdar://problem/81828477>
Comment 2 Cameron McCormack (:heycam) 2021-08-16 18:07:18 PDT
Created attachment 435651 [details]
Patch
Comment 3 EWS Watchlist 2021-08-16 18:08:09 PDT
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 4 Cameron McCormack (:heycam) 2021-08-16 18:26:17 PDT
Created attachment 435653 [details]
Patch
Comment 5 Sam Weinig 2021-08-17 14:21:38 PDT
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.
Comment 6 Cameron McCormack (:heycam) 2021-08-18 00:32:07 PDT
Created attachment 435752 [details]
Patch
Comment 7 Cameron McCormack (:heycam) 2021-08-18 00:36:08 PDT
(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.
Comment 8 Cameron McCormack (:heycam) 2021-08-18 01:48:46 PDT
Created attachment 435754 [details]
Patch
Comment 9 Cameron McCormack (:heycam) 2021-08-18 16:07:00 PDT
Created attachment 435811 [details]
Patch
Comment 10 Cameron McCormack (:heycam) 2021-08-18 16:07:23 PDT
Now with a couple of more tests and some iOS fixes.
Comment 11 Sam Weinig 2021-08-19 09:58:16 PDT
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().
Comment 12 Cameron McCormack (:heycam) 2021-08-19 16:40:25 PDT
Created attachment 435915 [details]
Patch
Comment 13 Cameron McCormack (:heycam) 2021-08-19 17:02:24 PDT
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/30098
Comment 14 Cameron McCormack (:heycam) 2021-08-22 15:03:33 PDT
Created attachment 436126 [details]
Patch

With Windows build fixes.
Comment 15 EWS 2021-08-23 14:36:46 PDT
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].