Bug 231062 - SVG images drawn onto display-p3 canvas are flattened to sRGB
Summary: SVG images drawn onto display-p3 canvas are flattened to sRGB
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Cameron McCormack (:heycam)
URL:
Keywords: InRadar
Depends on:
Blocks: 225140
  Show dependency treegraph
 
Reported: 2021-09-30 22:03 PDT by Cameron McCormack (:heycam)
Modified: 2022-02-07 15:30 PST (History)
19 users (show)

See Also:


Attachments
Patch (75.62 KB, patch)
2021-09-30 22:25 PDT, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch with Image::draw / GraphicsContext::drawImage argument (89.53 KB, patch)
2021-10-01 15:36 PDT, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch (76.95 KB, patch)
2021-10-04 18:36 PDT, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack (:heycam) 2021-09-30 22:03:16 PDT
.
Comment 1 Radar WebKit Bug Importer 2021-09-30 22:03:36 PDT
<rdar://problem/83752451>
Comment 2 Cameron McCormack (:heycam) 2021-09-30 22:03:59 PDT
We draw the SVG image into a NativeImage so it can be sent to the GPU process.
Comment 3 Cameron McCormack (:heycam) 2021-09-30 22:25:52 PDT
Created attachment 439822 [details]
Patch
Comment 4 EWS Watchlist 2021-09-30 22:27:06 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 5 Simon Fraser (smfr) 2021-10-01 14:14:26 PDT
Comment on attachment 439822 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.h:433
> +    virtual ImageDrawResult drawImageForCanvas(Image&, const FloatRect& destination, const FloatRect& source, const ImagePaintingOptions&, DestinationColorSpace canvasColorSpace);

Not a fan of "for canvas" down here in GraphicsContext.

Can we just an std::optional<DestinationColorSpace> to an existing function?

> Source/WebCore/svg/graphics/SVGImage.h:102
> +    ImageDrawResult drawForCanvas(GraphicsContext&, const FloatRect& destination, const FloatRect& source, const ImagePaintingOptions&, DestinationColorSpace) final;
> +    ImageDrawResult drawAsNativeImage(GraphicsContext&, const FloatRect& destination, const FloatRect& source, const ImagePaintingOptions&, DestinationColorSpace);
>      ImageDrawResult drawForContainer(GraphicsContext&, const FloatSize containerSize, float containerZoom, const URL& initialFragmentURL, const FloatRect& dstRect, const FloatRect& srcRect, const ImagePaintingOptions& = { });
> +    ImageDrawResult drawForContainerInternal(GraphicsContext&, const FloatSize containerSize, float containerZoom, const URL& initialFragmentURL, const FloatRect& dstRect, const FloatRect& srcRect, const ImagePaintingOptions&, DestinationColorSpace);
> +    ImageDrawResult drawInternal(GraphicsContext&, const FloatRect& destination, const FloatRect& source, const ImagePaintingOptions&, DestinationColorSpace);
> +    ImageDrawResult drawForCanvasForContainer(GraphicsContext&, const FloatSize containerSize, float containerZoom, const URL& initialFragmentURL, const FloatRect& dstRect, const FloatRect& srcRect, const ImagePaintingOptions&, DestinationColorSpace);

That's quite a bit of proliferation too. Can std::optional<DestinationColorSpace> help?
Comment 6 Cameron McCormack (:heycam) 2021-10-01 14:22:06 PDT
(In reply to Simon Fraser (smfr) from comment #5)
> Can we just an std::optional<DestinationColorSpace> to an existing function?

That was my first thought.  There were many subclasses to add that to.  Maybe that's better than adding the separate functions here.
Comment 7 Cameron McCormack (:heycam) 2021-10-01 15:36:41 PDT
Created attachment 439920 [details]
Patch with Image::draw / GraphicsContext::drawImage argument
Comment 8 Cameron McCormack (:heycam) 2021-10-01 15:40:57 PDT
There's a version of the patch with a std::optional<DestinationColorSpace> argument.  And I could be convinced not to make it optional (or std::optional<>) so that all drawImage/draw callers have to pass in an intermediate color space.
Comment 9 Cameron McCormack (:heycam) 2021-10-04 18:35:50 PDT
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/31108
Comment 10 Cameron McCormack (:heycam) 2021-10-04 18:36:34 PDT
Created attachment 440139 [details]
Patch
Comment 11 EWS 2021-10-04 19:33:03 PDT
Committed r283531 (242497@main): <https://commits.webkit.org/242497@main>

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