Bug 235233

Summary: Implement WebGL GPU buffer texture upload path for Cocoa getUserMedia camera streams
Product: WebKit Reporter: Kimmo Kinnunen <kkinnunen>
Component: WebGLAssignee: Kimmo Kinnunen <kkinnunen>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, dino, dpino, eric.carlson, ews-watchlist, glenn, gyuyoung.kim, hta, jer.noble, kbr, kkinnunen, kondapallykalyan, philipj, pnormand, ryuan.choi, sergio, tommyw, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=235396
Bug Depends on:    
Bug Blocks: 235352, 232727    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch ews-feeder: commit-queue-

Description Kimmo Kinnunen 2022-01-14 08:25:53 PST
Implement WebGL GPU buffer texture upload path for Cocoa getUserMedia camera streams
Comment 1 Radar WebKit Bug Importer 2022-01-14 08:26:19 PST
<rdar://problem/87601762>
Comment 2 Kimmo Kinnunen 2022-01-18 01:11:03 PST
Created attachment 449368 [details]
Patch
Comment 3 Kimmo Kinnunen 2022-01-18 01:27:55 PST
Created attachment 449369 [details]
Patch
Comment 4 Kimmo Kinnunen 2022-01-18 06:11:57 PST
Created attachment 449381 [details]
Patch
Comment 5 Kimmo Kinnunen 2022-01-18 06:57:25 PST
Created attachment 449385 [details]
Patch
Comment 6 Eric Carlson 2022-01-18 07:21:57 PST
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
Comment 7 Kimmo Kinnunen 2022-01-18 11:54:30 PST
Created attachment 449407 [details]
Patch
Comment 8 Kimmo Kinnunen 2022-01-18 23:39:34 PST
Created attachment 449468 [details]
Patch
Comment 9 Kimmo Kinnunen 2022-01-19 02:58:31 PST
Created attachment 449471 [details]
Patch
Comment 10 Kimmo Kinnunen 2022-01-19 04:37:13 PST
Created attachment 449474 [details]
Patch
Comment 11 Kimmo Kinnunen 2022-01-19 05:05:34 PST
Created attachment 449476 [details]
Patch
Comment 12 youenn fablet 2022-01-19 08:45:05 PST
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.
Comment 13 Kimmo Kinnunen 2022-01-19 11:15:59 PST
(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..
Comment 14 youenn fablet 2022-01-20 00:32:01 PST
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.
Comment 15 youenn fablet 2022-01-20 00:34:56 PST
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?
Comment 16 youenn fablet 2022-01-20 01:00:25 PST
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.
Comment 17 youenn fablet 2022-01-20 02:35:20 PST
@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.
Comment 18 Kimmo Kinnunen 2022-01-20 02:49:36 PST
Created attachment 449560 [details]
Patch
Comment 19 Kimmo Kinnunen 2022-01-20 11:08:12 PST
Created attachment 449595 [details]
Patch
Comment 20 EWS 2022-01-20 12:49:54 PST
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].
Comment 21 Philippe Normand 2022-01-21 00:27:54 PST
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.