Bug 229025

Summary: Support drawImage with a wide gamut video source painting into a display-p3 canvas
Product: WebKit Reporter: Cameron McCormack (:heycam) <heycam>
Component: CanvasAssignee: Cameron McCormack (:heycam) <heycam>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, benjamin, calvaris, cdumez, cgarcia, changseok, clopez, cmarcelo, dino, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, glenn, gustavo, gyuyoung.kim, jer.noble, jsbell, kkinnunen, kondapallykalyan, menard, pdr, philipj, pnormand, sabouhallawa, sam, schenney, sergio, vjaquez, 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/31226
https://bugs.webkit.org/show_bug.cgi?id=231959
https://bugs.webkit.org/show_bug.cgi?id=232346
Bug Depends on: 231353    
Bug Blocks: 231645, 225140    
Attachments:
Description Flags
Patch for EWS
none
Patch with squashed dependencies for EWS
none
Patch with squashed dependencies for EWS
none
Patch with squashed dependencies for EWS
none
Patch for EWS
ews-feeder: commit-queue-
Patch for EWS
none
Patch for EWS with squashed dependencies
none
Patch for EWS with squashed dependencies
none
Patch
none
Patch
none
Patch
sam: review+, ews-feeder: commit-queue-
try
none
Patch for landing none

Description Cameron McCormack (:heycam) 2021-08-11 21:39:30 PDT
This doesn't work for sRGB or display-p3 canvases currently.
Comment 1 Radar WebKit Bug Importer 2021-08-11 21:41:36 PDT
<rdar://problem/81828490>
Comment 2 Cameron McCormack (:heycam) 2021-09-21 23:56:47 PDT
Created attachment 438925 [details]
Patch for EWS
Comment 3 Cameron McCormack (:heycam) 2021-09-22 00:00:41 PDT
Created attachment 438926 [details]
Patch with squashed dependencies for EWS
Comment 4 Cameron McCormack (:heycam) 2021-09-22 00:28:03 PDT
Created attachment 438928 [details]
Patch with squashed dependencies for EWS
Comment 5 EWS Watchlist 2021-09-22 00:29:01 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 6 Eric Carlson 2021-09-22 09:17:14 PDT
Comment on attachment 438928 [details]
Patch with squashed dependencies for EWS

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

> Source/WebCore/ChangeLog:61
> +2021-09-19  Cameron McCormack  <heycam@apple.com>
> +
> +        Preserve color space when structured cloning ImageBitmaps
> +        https://bugs.webkit.org/show_bug.cgi?id=230429
> +        <rdar://problem/83293533>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        The logic for serializing the CGColorSpace is copied and adpated from
> +        the ArgumentCoder<CGColorSpaceRef> specialization, which we cannot
> +        easily re-use.
> +
> +        * bindings/js/SerializedScriptValue.cpp:
> +        (WebCore::CloneSerializer::dumpImageBitmap):
> +        (WebCore::CloneSerializer::write):
> +        (WebCore::CloneDeserializer::read):
> +        (WebCore::CloneDeserializer::readImageBitmap):
> +

Double ChangeLog

> Source/WebKit/GPUProcess/media/cocoa/RemoteMediaPlayerProxyCocoa.mm:111
> +    completionHandler(surface->createSendRight(), nativeImage->colorSpace());

If you call `IOSurface::migrateColorSpaceToProperties` to attach the image color space to the IOSurface before creating the send right, will the colorspace be moved cross process with the IOSurface automatically?

> Source/WebKit/WebProcess/GPU/media/cocoa/MediaPlayerPrivateRemoteCocoa.mm:76
> +    std::optional<DestinationColorSpace> colorSpace;
> +    if (!connection().sendSync(Messages::RemoteMediaPlayerProxy::ColorSpace(), Messages::RemoteMediaPlayerProxy::ColorSpace::Reply(colorSpace), m_id))
> +        return std::nullopt;
> +
> +    return colorSpace;

Does the colorspace of a video ever change? If not, it'd be a big perf win to cache the value.
Comment 7 Cameron McCormack (:heycam) 2021-09-26 23:57:32 PDT
Created attachment 439323 [details]
Patch with squashed dependencies for EWS
Comment 8 Cameron McCormack (:heycam) 2021-09-28 19:42:14 PDT
Created attachment 439558 [details]
Patch for EWS
Comment 9 Cameron McCormack (:heycam) 2021-09-28 22:07:28 PDT
Created attachment 439565 [details]
Patch for EWS
Comment 10 Cameron McCormack (:heycam) 2021-09-30 22:26:45 PDT
The iOS failures are due to the CGColorSpace attachment not existing on CVPixelBuffers on iOS.  We might have to find a way to work around that.
Comment 11 Cameron McCormack (:heycam) 2021-10-02 00:13:47 PDT
Created attachment 439958 [details]
Patch for EWS with squashed dependencies
Comment 12 Cameron McCormack (:heycam) 2021-10-03 13:51:03 PDT
Created attachment 440014 [details]
Patch for EWS with squashed dependencies
Comment 13 Cameron McCormack (:heycam) 2021-10-07 23:39:33 PDT
Created attachment 440577 [details]
Patch

This can be reviewed before the WebRTC issue in bug 231353 is resolved.
Comment 14 Kimmo Kinnunen 2021-10-08 00:32:47 PDT
Comment on attachment 440577 [details]
Patch

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

> Source/WebKit/ChangeLog:14
> +        the color space used.

Not making it your "fault" since the existing implementation was what it was:

Currently the operation is "NativeImageForCurrentTime -> <SendRight, ColorSpace>", which isn't obvious. It should be called "SendRightAndColorSpaceRepresentingNativeImageBecauseYouKnowItForCurrentTime", which obviously is better but a on the long side.

The good way (IMO anyway :) would be to have "NativeImageForCurrentTime -> NativeImage". Then you would:
- add WebCoreArgumentCoder for NativeImage
- move the encoding/decoding logic from the call sites to the argument coder
- add your colourspace encoding and decoding there.

This probably can be done as a hobby project of mine later, too.
Comment 15 Cameron McCormack (:heycam) 2021-10-08 00:48:31 PDT
I'll update texture-corner-case-videos.html to be expected fail, rather than add updated expectations, and link to https://github.com/KhronosGroup/WebGL/issues/3341, which Kimmo just filed.
Comment 16 Sam Weinig 2021-10-08 09:45:22 PDT
Comment on attachment 440577 [details]
Patch

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

> Source/WebCore/html/ImageBitmap.cpp:517
> +    if (!colorSpace)
> +        colorSpace = DestinationColorSpace::SRGB();

In what cases is there no colorspace? It seems like one case is when there is no player, in which case paintCurrentFrameInContext() is going to bail, so it doesn't matter. But what are the others?

> Source/WebCore/platform/graphics/cv/CVUtilities.mm:124
> +    return sRGBColorSpaceRef();

Would be good to have some comment here explaining why this makes sense (I assume it is what the system does if no color space is there, but stating it could help in the future).
Comment 17 Cameron McCormack (:heycam) 2021-10-08 15:48:55 PDT
(In reply to Sam Weinig from comment #16)
> > Source/WebCore/html/ImageBitmap.cpp:517
> > +    if (!colorSpace)
> > +        colorSpace = DestinationColorSpace::SRGB();
> 
> In what cases is there no colorspace? It seems like one case is when there
> is no player, in which case paintCurrentFrameInContext() is going to bail,
> so it doesn't matter. But what are the others?

Other ports where MediaPlayerPrivate::colorSpace hasn't been overridden.  Do you think it makes sense instead to either make MediaPlayerPrivate::colorSpace return SRGB, or make all the ports override it and return SRGB from them too?

> > Source/WebCore/platform/graphics/cv/CVUtilities.mm:124
> > +    return sRGBColorSpaceRef();
> 
> Would be good to have some comment here explaining why this makes sense (I
> assume it is what the system does if no color space is there, but stating it
> could help in the future).

It's not a principled fallback value.  Apart from some real error conditions like OOM, I think CVImageBufferCreateColorSpaceFromAttachments could return null if:

* the image buffer has an ICC profile attached to it, and the profile is invalid
* it doesn't have color primaries / transfer function attachments

I think the latter is unexpected.  The former could happen, but embedded ICC profiles are uncommon, and I haven't tried creating a video with one to see how it's handled.  Probably depends on exactly what framework function you pass the CVImageBuffer to.

So I guess we can't declare this case as ASSERT_NOT_REACHED(), since content could cause us to reach it.  But the content is already broken.  So I'm not sure how important it is to try to match some other functions' handling of the broken content.

Do you think it's worth spending the effort to find out how malformed videos like this end up getting treated in e.g. QuickTime Player?
Comment 18 Sam Weinig 2021-10-13 09:11:38 PDT
(In reply to Cameron McCormack (:heycam) from comment #17)
> (In reply to Sam Weinig from comment #16)
> > > Source/WebCore/html/ImageBitmap.cpp:517
> > > +    if (!colorSpace)
> > > +        colorSpace = DestinationColorSpace::SRGB();
> > 
> > In what cases is there no colorspace? It seems like one case is when there
> > is no player, in which case paintCurrentFrameInContext() is going to bail,
> > so it doesn't matter. But what are the others?
> 
> Other ports where MediaPlayerPrivate::colorSpace hasn't been overridden.  Do
> you think it makes sense instead to either make
> MediaPlayerPrivate::colorSpace return SRGB, or make all the ports override
> it and return SRGB from them too?

I think having the ports override it and return SRGB would be good, since that is really what is happening. The other ports only support SRGB so any time we can make that really explicit is good.


> 
> > > Source/WebCore/platform/graphics/cv/CVUtilities.mm:124
> > > +    return sRGBColorSpaceRef();
> > 
> > Would be good to have some comment here explaining why this makes sense (I
> > assume it is what the system does if no color space is there, but stating it
> > could help in the future).
> 
> It's not a principled fallback value.  Apart from some real error conditions
> like OOM, I think CVImageBufferCreateColorSpaceFromAttachments could return
> null if:
> 
> * the image buffer has an ICC profile attached to it, and the profile is
> invalid
> * it doesn't have color primaries / transfer function attachments
> 
> I think the latter is unexpected.  The former could happen, but embedded ICC
> profiles are uncommon, and I haven't tried creating a video with one to see
> how it's handled.  Probably depends on exactly what framework function you
> pass the CVImageBuffer to.
> 
> So I guess we can't declare this case as ASSERT_NOT_REACHED(), since content
> could cause us to reach it.  But the content is already broken.  So I'm not
> sure how important it is to try to match some other functions' handling of
> the broken content.
> 
> Do you think it's worth spending the effort to find out how malformed videos
> like this end up getting treated in e.g. QuickTime Player?

I think just a comment saying something along these lines would be good enough. Like, we shouldn't reach here in the case of content that is not broken, but if we do, sRGB is the best, and most backward compatible guess we have.
Comment 19 Cameron McCormack (:heycam) 2021-10-13 18:33:30 PDT
Created attachment 441172 [details]
Patch
Comment 20 Cameron McCormack (:heycam) 2021-10-13 20:25:07 PDT
Created attachment 441179 [details]
Patch
Comment 21 Cameron McCormack (:heycam) 2021-10-13 20:26:47 PDT
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/31226
Comment 22 Cameron McCormack (:heycam) 2021-10-15 23:59:53 PDT
Created attachment 441479 [details]
try
Comment 23 Cameron McCormack (:heycam) 2021-10-17 14:39:21 PDT
Comment on attachment 441179 [details]
Patch

Final test failure should be resolved by bug 231881.
Comment 24 Cameron McCormack (:heycam) 2021-10-18 21:03:00 PDT
Created attachment 441687 [details]
Patch for landing
Comment 25 EWS 2021-10-19 01:35:48 PDT
Committed r284439 (243201@main): <https://commits.webkit.org/243201@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441687 [details].
Comment 26 Philippe Normand 2021-10-26 07:59:15 PDT
Comment on attachment 441687 [details]
Patch for landing

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

> LayoutTests/media/video-canvas-createPattern.html:-16
> -                [355, 403, 194, 193, 14], // Represents yellow south east tile.

Are these baselines expected to work when sRGB is used? This patch broke a bunch of tests in WPE/GTK :(
Comment 27 Cameron McCormack (:heycam) 2021-10-27 15:56:32 PDT
(In reply to Philippe Normand from comment #26)
> Are these baselines expected to work when sRGB is used? This patch broke a
> bunch of tests in WPE/GTK :(

Sorry about that!  The patch changes the expected behavior even for sRGB canvases, where the color space of the video should now be converted from the video's native color space to sRGB when doing a drawImage().  I didn't make any WPE/GTK port changes, so I guess the correct thing to do is to mark the test as expected fail.

I think there are a few things that need looking at to get this working on these ports:

* MediaPlayerPrivateGStreamer::colorSpace() will need to return an appropriate the video color space as a DestinationColorSpace
* that will also mean PlatformColorSpace will probably need to gain the ability to represent arbitrary color spaces on non-CG platforms
* MediaPlayerPrivateGStreamer::paint() might need looking at.  The BitmapImage that ImageGStreamer::image() returns probably needs to be annotated with the right color space too.
Comment 28 Philippe Normand 2021-10-28 06:17:45 PDT
Thanks Cameron! So it seems we're stuck because Cairo doesn't seem to have any API for colorspace handling...