Bug 235233 – Implement WebGL GPU buffer texture upload path for Cocoa getUserMedia camera streams

• Component: WebGL
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

Implement WebGL GPU buffer texture upload path for Cocoa getUserMedia camera streams

[Radar WebKit Bug Importer]

<rdar://problem/87601762>

[Kimmo Kinnunen]

Created attachment 449368 [details]
Patch

[Kimmo Kinnunen]

Created attachment 449369 [details]
Patch

[Kimmo Kinnunen]

Created attachment 449381 [details]
Patch

[Kimmo Kinnunen]

Created attachment 449385 [details]
Patch

[Eric Carlson]

Comment on attachment 449385 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Make full texture uploads from MSE camera captures use CVPixelBuffers

s/MSE/MediaStream

[Kimmo Kinnunen]

Created attachment 449407 [details]
Patch

[Kimmo Kinnunen]

Created attachment 449468 [details]
Patch

[Kimmo Kinnunen]

Created attachment 449471 [details]
Patch

[Kimmo Kinnunen]

Created attachment 449474 [details]
Patch

[Kimmo Kinnunen]

Created attachment 449476 [details]
Patch

[youenn fablet]

Comment on attachment 449476 [details]
Patch

Direction looks good to me though I do not see why introduce a new frame structure.
To limit the size of the patch, we could also think of introducing a rotation getter that we would only implement for MediaPlayerPrivateMediaStreamAVFObjC.

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

> Source/WebCore/ChangeLog:17
> +        with a rotation or a flip.

Why not using a MediaSample which is already the structure we use for MediaStream?
Creating a MediaSample from a CVPixelBuffer is done via MediaSampleAVFObjC::createImageSample(RetainPtr<CVPixelBufferRef>&&, VideoRotation, bool mirrored);

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:145
> +    std::optional<MediaPlayerVideoFrame> videoFrameForCurrentTime() override;

Should be final probably.

[Kimmo Kinnunen]

(In reply to youenn fablet from comment #12)
> Comment on attachment 449476 [details]
> Direction looks good to me though I do not see why introduce a new frame
> structure.
> To limit the size of the patch, we could also think of introducing a
> rotation getter that we would only implement for
> MediaPlayerPrivateMediaStreamAVFObjC.

So the raw video frame does not make sense without the orientation transform.
This means it does not make sense to say "pixelBufferForCurrentFrame()" as you cannot use it correctly for anything.

It also does not make sense to think that it somehow limits to MediaPlayerPrivateMediaStreamAVFObjC. Also MediaPlayerPrivateAVFoundationObjC does rotate the pixel buffer for no good reason from WebGL perspective. This is problematic for GPUP as this causes more memory pressure (and currently the extra pixel buffer is unattributed). So maybe this rotation is moved from pixel buffer generation to cgimage generation phase.
 
> Why not using a MediaSample which is already the structure we use for
> MediaStream?

So instead of:
std::optional<MediaPlayerVideoFrame> MediaPlayerPrivateInterface::videoFrameForCurrentTime();

We would have:
RefPtr<MediaSample> MediaPlayerPrivateInterface::mediaSampleForCurrentTime()?

It's a bit counterproductive to wrap each CVPixelBufferRef to CMSampleBufferRef via CMSampleBufferCreateReadyWithImageBuffer. OTOH, maybe some day it we can get it directly also from the normal video file decoding process in GPUP?
I can of course try it..

[youenn fablet]

Comment on attachment 449476 [details]
Patch

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

> Source/WebCore/platform/graphics/cv/GraphicsContextGLCV.h:43
> +    virtual bool copyVideoFrameToTexture(MediaPlayerVideoFrame&, PlatformGLObject outputTexture, GCGLint level, GCGLenum internalFormat, GCGLenum format, GCGLenum type, FlipY) = 0;

Should probably be const& if we keep passing a MediaPlayerVideoFrame.

> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.cpp:285
> +    completionHandler(contextCV->copyVideoFrameToTexture(*videoFrame, texture, level, internalFormat, format, type, GraphicsContextGL::FlipY(flipY)));

We could directly pass the pixel buffer and rotation here.

[youenn fablet]

Comment on attachment 449476 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2633
> +    return MediaPlayerVideoFrame { RetainPtr { m_lastPixelBuffer }, ImageOrientation::None };

Do we need the RetainPtr here?

[youenn fablet]

Comment on attachment 449476 [details]
Patch

Thinking a bit more, MediaSample is probably too heavyweight.
Using a more lightweight structure seems fine but we might want to rationalise all these video frame objects at some point.

[youenn fablet]

@Philn, any idea why gtk is not happy with the new test.
Are there known issues with mock rotation?
Looking at the test expectations, I do not see anything.

[Kimmo Kinnunen]

Created attachment 449560 [details]
Patch

[Kimmo Kinnunen]

Created attachment 449595 [details]
Patch

[EWS]

Committed r288315 (246231@main): <https://commits.webkit.org/246231@main>

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

[Philippe Normand]

Comment on attachment 449595 [details]
Patch

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

> LayoutTests/platform/gtk/TestExpectations:215
> +# setMockCameraOrientation seems to fail.
> +webkit.org/b/235396 fast/mediastream/getUserMedia-to-canvas-1.html [ Failure ]
> +webkit.org/b/235396 fast/mediastream/getUserMedia-to-canvas-2.html [ Failure ]

Can you move this to the glib TestExpectations please? That's the common place for WPE and GTK.