Bug 225286

Summary: Add preliminary support for specifying a color space for 2D canvas
Product: WebKit Reporter: Sam Weinig <sam>
Component: CanvasAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, cdumez, changseok, dino, drousso, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, kondapallykalyan, philipj, ryuan.choi, sergio, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 225140    
Attachments:
Description Flags
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch ews-feeder: commit-queue-

Description Sam Weinig 2021-05-02 17:58:20 PDT
As a first step toward implementing https://github.com/WICG/canvas-color-space/blob/main/CanvasColorSpaceProposal.md, let's start with adding support for specifying a color space via the settings object and rendering using that color space. To keep things small, we will leave out ImageData access from this first change.
Comment 1 Sam Weinig 2021-05-03 08:48:15 PDT Comment hidden (obsolete)
Comment 2 Sam Weinig 2021-05-03 16:38:47 PDT Comment hidden (obsolete)
Comment 3 Sam Weinig 2021-05-03 16:43:38 PDT Comment hidden (obsolete)
Comment 4 Sam Weinig 2021-05-03 16:45:12 PDT Comment hidden (obsolete)
Comment 5 Sam Weinig 2021-05-03 18:29:36 PDT Comment hidden (obsolete)
Comment 6 Sam Weinig 2021-05-04 09:06:17 PDT
Created attachment 427674 [details]
Patch
Comment 7 Dean Jackson 2021-05-04 09:16:02 PDT
Comment on attachment 427674 [details]
Patch

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

> LayoutTests/fast/canvas/CanvasRenderingContext2DSettings-colorSpace-disabled.html:11
> +        let canvas = document.createElement("canvas");
> +        let context = canvas.getContext("2d", settings);

Nit: const
(and elsewhere)

> LayoutTests/fast/canvas/canvas-color-space-display-p3.html:5
> +    <p>This test passes if the square below looks to be all one color</p>

Nice test!
Comment 8 Simon Fraser (smfr) 2021-05-04 09:42:48 PDT
Comment on attachment 427674 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:951
> +        else
> +            return std::pair { DestinationColorSpace::SRGB, PixelFormat::BGRA8 };

No else after return.

> Source/WebCore/html/canvas/PredefinedColorSpace.h:34
> +enum class PredefinedColorSpace {

Maybe this should be CanvasPredefinedColorSpace?
Comment 9 Sam Weinig 2021-05-05 08:46:01 PDT
(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 427674 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427674&action=review
> 
> > Source/WebCore/html/HTMLCanvasElement.cpp:951
> > +        else
> > +            return std::pair { DestinationColorSpace::SRGB, PixelFormat::BGRA8 };
> 
> No else after return.

Yeah. (I kind of hate this rule. The symmetry seems so much nicer in cases like this, but if I want to change this rule I will bring it up separately). Fixed.

> 
> > Source/WebCore/html/canvas/PredefinedColorSpace.h:34
> > +enum class PredefinedColorSpace {
> 
> Maybe this should be CanvasPredefinedColorSpace?

I'd like to stick to the spec name if I can. Perhaps we should suggest changing it HTML?
Comment 10 Sam Weinig 2021-05-05 08:53:47 PDT
Created attachment 427770 [details]
Patch
Comment 11 EWS 2021-05-05 09:37:03 PDT
Committed r277024 (237338@main): <https://commits.webkit.org/237338@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427770 [details].
Comment 12 Radar WebKit Bug Importer 2021-05-05 09:38:31 PDT
<rdar://problem/77561169>