Bug 230139

Summary: Work around bug 225873 (canvas color space rendering problems) or disable the feature on older OSes
Product: WebKit Reporter: Cameron McCormack (:heycam) <heycam>
Component: CanvasAssignee: Cameron McCormack (:heycam) <heycam>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, benjamin, cdumez, changseok, cmarcelo, dino, esprehn+autocc, ews-watchlist, gyuyoung.kim, jsbell, kondapallykalyan, sam, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 229021    
Attachments:
Description Flags
Patch
none
Patch
none
Patch that works around the bug on older OSes
none
Timing test
none
Patch for EWS that disables the feature on unsupported platforms
ews-feeder: commit-queue-
Patch for EWS that disables the feature on supported platforms
ews-feeder: commit-queue-
Patch that disables the feature on unsupported platforms
none
Patch none

Description Cameron McCormack (:heycam) 2021-09-09 17:28:11 PDT
We should be able to work around the framework bug on Apple platforms that caused the canvas rendering problems with different color spaces, identified in bug 225873.  The workaround is needed on Big Sur and earlier.
Comment 1 Radar WebKit Bug Importer 2021-09-09 17:34:18 PDT
<rdar://problem/82951700>
Comment 2 Cameron McCormack (:heycam) 2021-09-09 17:34:59 PDT
I ran Motionmark locally with the workaround in place, and didn't get notably different results.
Comment 3 Cameron McCormack (:heycam) 2021-09-09 17:46:55 PDT
<rdar://81828819>
Comment 4 Cameron McCormack (:heycam) 2021-09-09 18:24:54 PDT
Created attachment 437819 [details]
Patch
Comment 5 Cameron McCormack (:heycam) 2021-09-09 23:53:30 PDT
Created attachment 437845 [details]
Patch
Comment 6 Cameron McCormack (:heycam) 2021-09-09 23:54:56 PDT
(In reply to Cameron McCormack (:heycam) from comment #2)
> I ran Motionmark locally with the workaround in place, and didn't get
> notably different results.

That is measuring the overhead of doing the color space check, when we don't need to purge the caches.  I haven't measured how much it can slow things down when repeatedly switching between color spaces.
Comment 7 Cameron McCormack (:heycam) 2021-09-10 16:03:37 PDT
Created attachment 437918 [details]
Patch that works around the bug on older OSes

Mark Windows failure since Display P3 canvas is not available there.
Comment 8 Cameron McCormack (:heycam) 2021-09-10 18:25:25 PDT
(In reply to Cameron McCormack (:heycam) from comment #6)
> I haven't measured how much it can slow things
> down when repeatedly switching between color spaces.

I just tested this.  The overhead of the context flush and cache purge, when it's needed is about 2 ms on my machine.  If drawing operations are done on canvases of alternating color spaces, then this is substantial.  So now I am not sure that we want to do this.
Comment 9 Cameron McCormack (:heycam) 2021-09-10 18:26:56 PDT
Created attachment 437934 [details]
Timing test

This is what I tested with.  Got these results:

(A and B are srgb canvases, C and D are display-p3 canvases)
AAAA: 9 ms
CCCC: 23 ms
ABAB: 20 ms
CDCD: 12 ms
ACAC: 42739 ms
Comment 10 Cameron McCormack (:heycam) 2021-09-11 16:44:05 PDT
Created attachment 437970 [details]
Patch for EWS that disables the feature on unsupported platforms
Comment 11 Cameron McCormack (:heycam) 2021-09-11 20:54:24 PDT
Created attachment 437978 [details]
Patch for EWS that disables the feature on supported platforms
Comment 12 Cameron McCormack (:heycam) 2021-09-12 00:14:02 PDT
Created attachment 437981 [details]
Patch that disables the feature on unsupported platforms
Comment 13 EWS 2021-09-15 21:34:00 PDT
Tools/Scripts/svn-apply failed to apply attachment 437981 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 14 Cameron McCormack (:heycam) 2021-09-15 22:49:48 PDT
Created attachment 438318 [details]
Patch
Comment 15 EWS 2021-09-16 01:57:04 PDT
Committed r282513 (241738@main): <https://commits.webkit.org/241738@main>

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