Summary: | Implement WebGL GPU buffer texture upload path for Cocoa getUserMedia camera streams | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kimmo Kinnunen <kkinnunen> | ||||||||||||||||||||||||
Component: | WebGL | Assignee: | 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
Kimmo Kinnunen
2022-01-14 08:25:53 PST
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. |