<rdar://problem/87601762>

Created attachment 449368 [details] Patch

Created attachment 449369 [details] Patch

Created attachment 449381 [details] Patch

Created attachment 449385 [details] Patch

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

Created attachment 449407 [details] Patch

Created attachment 449468 [details] Patch

Created attachment 449471 [details] Patch

Created attachment 449474 [details] Patch

Created attachment 449476 [details] Patch

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.

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

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

Created attachment 449560 [details] Patch

Created attachment 449595 [details] Patch

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