Bug 229025 – Support drawImage with a wide gamut video source painting into a display-p3 canvas

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Cameron McCormack (:heycam)]

This doesn't work for sRGB or display-p3 canvases currently.

[Radar WebKit Bug Importer]

<rdar://problem/81828490>

[Cameron McCormack (:heycam)]

Created attachment 438925 [details]
Patch for EWS

[Cameron McCormack (:heycam)]

Created attachment 438926 [details]
Patch with squashed dependencies for EWS

[Cameron McCormack (:heycam)]

Created attachment 438928 [details]
Patch with squashed dependencies for EWS

[EWS Watchlist]

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

[Eric Carlson]

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
> [phone]  Cameron McCormack  <[e-mail]>
> +
> +        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.

[Cameron McCormack (:heycam)]

Created attachment 439323 [details]
Patch with squashed dependencies for EWS

[Cameron McCormack (:heycam)]

Created attachment 439558 [details]
Patch for EWS

[Cameron McCormack (:heycam)]

Created attachment 439565 [details]
Patch for EWS

[Cameron McCormack (:heycam)]

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.

[Cameron McCormack (:heycam)]

Created attachment 439958 [details]
Patch for EWS with squashed dependencies

[Cameron McCormack (:heycam)]

Created attachment 440014 [details]
Patch for EWS with squashed dependencies

[Cameron McCormack (:heycam)]

Created attachment 440577 [details]
Patch

This can be reviewed before the WebRTC issue in bug 231353 is resolved.

[Kimmo Kinnunen]

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.

[Cameron McCormack (:heycam)]

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.

[Sam Weinig]

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).

[Cameron McCormack (:heycam)]

(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?

[Sam Weinig]

(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.

[Cameron McCormack (:heycam)]

Created attachment 441172 [details]
Patch

[Cameron McCormack (:heycam)]

Created attachment 441179 [details]
Patch

[Cameron McCormack (:heycam)]

Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/31226

[Cameron McCormack (:heycam)]

Created attachment 441479 [details]
try

[Cameron McCormack (:heycam)]

Comment on attachment 441179 [details]
Patch

Final test failure should be resolved by bug 231881.

[Cameron McCormack (:heycam)]

Created attachment 441687 [details]
Patch for landing

[EWS]

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].

[Philippe Normand]

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 :(

[Cameron McCormack (:heycam)]

(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.

[Philippe Normand]

Thanks Cameron! So it seems we're stuck because Cairo doesn't seem to have any API for colorspace handling...