Summary: | LibWebRTCCodecs should not need to create IOSurfaces | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||
Component: | WebRTC | Assignee: | youenn fablet <youennf> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | eric.carlson, ews-watchlist, fpizlo, ggaren, glenn, jer.noble, kkinnunen, philipj, sergio, webkit-bug-importer, youennf | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 235952, 235953, 235954 | ||||||||||||||
Attachments: |
|
Description
youenn fablet
2022-02-01 07:20:04 PST
Created attachment 450528 [details]
Patch
Created attachment 450535 [details]
Patch
Comment on attachment 450528 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450528&action=review > Source/ThirdParty/libwebrtc/ChangeLog:9 > + Introduce helper routines to copy a webrtc VideoFrame in a bufer and get a CVPixelBuffer from a webrtc VideoFrame. s/bufer/buffer/ > Source/WebCore/platform/cocoa/SharedVideoFrameInfo.mm:187 > + if (CVPixelBufferGetWidthOfPlane(rawPixelBuffer, 1) != m_widthPlaneB || CVPixelBufferGetHeightOfPlane(rawPixelBuffer, 1) != m_heightPlaneB) This leaves the buffer base address locked. Maybe use `makeScopeExit` to unlock instead? Created attachment 450539 [details]
Patch
(In reply to Eric Carlson from comment #4) > Comment on attachment 450528 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=450528&action=review > > > Source/ThirdParty/libwebrtc/ChangeLog:9 > > + Introduce helper routines to copy a webrtc VideoFrame in a bufer and get a CVPixelBuffer from a webrtc VideoFrame. > > s/bufer/buffer/ Fixed > > Source/WebCore/platform/cocoa/SharedVideoFrameInfo.mm:187 > > + if (CVPixelBufferGetWidthOfPlane(rawPixelBuffer, 1) != m_widthPlaneB || CVPixelBufferGetHeightOfPlane(rawPixelBuffer, 1) != m_heightPlaneB) > > This leaves the buffer base address locked. Maybe use `makeScopeExit` to > unlock instead? Right, fixed! Comment on attachment 450539 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450539&action=review > Source/ThirdParty/libwebrtc/ChangeLog:9 > + Introduce helper routines to copy a webrtc VideoFrame in a memory buffer and get a CVPixelBuffer from a webrtc VideoFrame. s/in a memory buffer/into a memory buffer/ > Source/WebCore/platform/cocoa/SharedVideoFrameInfo.h:79 > + Nit: extra blank line. > Source/WebCore/platform/cocoa/SharedVideoFrameInfo.mm:210 > + CVPixelBufferUnlockBaseAddress(rawPixelBuffer, 0); This is unnecessary. > Source/WebKit/GPUProcess/webrtc/LibWebRTCCodecsProxy.mm:70 > for (auto decoder : decoders.values()) `auto&`? > Source/WebKit/WebProcess/GPU/webrtc/SharedVideoFrame.cpp:71 > + if (!m_storage || m_storage->size() < size) { Should we also reallocate if m_storage is (significantly?) larger than needed? > Source/WebKit/WebProcess/GPU/webrtc/SharedVideoFrame.cpp:135 > + Nit: extra blank > Source/WebKit/WebProcess/GPU/webrtc/SharedVideoFrame.h:96 > + Ditto > > Source/WebCore/platform/cocoa/SharedVideoFrameInfo.mm:210 > > + CVPixelBufferUnlockBaseAddress(rawPixelBuffer, 0); > > This is unnecessary. OK > > Source/WebKit/GPUProcess/webrtc/LibWebRTCCodecsProxy.mm:70 > > for (auto decoder : decoders.values()) > > `auto&`? decoder is a pointer so auto is fine here. > > Source/WebKit/WebProcess/GPU/webrtc/SharedVideoFrame.cpp:71 > > + if (!m_storage || m_storage->size() < size) { > > Should we also reallocate if m_storage is (significantly?) larger than > needed? Good question. I guess we should consider it. What would be a reasonable heuristic? Created attachment 450616 [details]
Patch for landing
Created attachment 450628 [details]
Patch for landing
Committed r288951 (246680@main): <https://commits.webkit.org/246680@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 450628 [details]. |