Bug 235951 – LibWebRTCCodecs should not need to create IOSurfaces

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

LibWebRTCCodecs should not need to create IOSurfaces

[Radar WebKit Bug Importer]

<rdar://problem/88326654>

[youenn fablet]

Created attachment 450528 [details]
Patch

[youenn fablet]

Created attachment 450535 [details]
Patch

[Eric Carlson]

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?

[youenn fablet]

Created attachment 450539 [details]
Patch

[youenn fablet]

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

[Eric Carlson]

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

[youenn fablet]

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

[youenn fablet]

Created attachment 450616 [details]
Patch for landing

[youenn fablet]

Created attachment 450628 [details]
Patch for landing

[EWS]

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