Bug 229806 – RemoteVideoSample needs CVPixelBufferRef to be kept alive, but relies on caller to retain it

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

[David Kilzer (:ddkilzer)]

RemoteVideoSample needs CVPixelBufferRef to be kept alive, but relies on caller to retain it.

While working on Bug 229661, I discovered that when creating a RemoteVideoSample using a CVPixelBufferRef, the CVPixelBufferRef needs to be kept alive for the duration of the RemoteVideoSample.

if this is not done, then these two tests fail:

    webrtc/video-mute.html
    webrtc/video-unmute.html

See 229661, Comment #15 for how this was discovered.
<https://bugs.webkit.org/show_bug.cgi?id=229661#c15>

[Radar WebKit Bug Importer]

<rdar://problem/82684479>

[David Kilzer (:ddkilzer)]

Created attachment 437174 [details]
Patch v1

[Darin Adler]

Comment on attachment 437174 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=437174&action=review

> Source/WebCore/platform/graphics/RemoteVideoSample.cpp:118
>  std::unique_ptr<RemoteVideoSample> RemoteVideoSample::create(CVPixelBufferRef imageBuffer, MediaTime&& presentationTime, MediaSample::VideoRotation rotation)

Seems like the argument should be RetainPtr<CVPixelBufferRef>&&; we could avoid retain count churn by handing in ownership explicitly and then the caller doesn’t need to call get(), although they do need to call WTFMove if they are using a local variable.

> Source/WebCore/platform/graphics/RemoteVideoSample.h:106
> +    RemoteVideoSample(IOSurfaceRef, CVPixelBufferRef, const DestinationColorSpace&, MediaTime&&, MediaSample::VideoRotation, bool);

Then this could be RetainPtr<CVPixelBufferRef>&&.

> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:457
> +        auto convertedPixelBuffer = convertToBGRA(pixelBuffer.get());
> +        sample = RemoteVideoSample::create(convertedPixelBuffer.get(), MediaTime(frame.timestamp_us() * 1000, 1000000), toMediaSampleVideoRotation(frame.rotation()));

I’d like to see this:

    sample = RemoteVideoSample::create(convertToBGRA(pixelBuffer.get()), MediaTime(frame.timestamp_us() * 1000, 1000000), toMediaSampleVideoRotation(frame.rotation()));

[David Kilzer (:ddkilzer)]

Created attachment 437181 [details]
Patch v2

[Darin Adler]

Comment on attachment 437181 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=437181&action=review

> Source/WebCore/platform/graphics/RemoteVideoSample.cpp:92
> +    RetainPtr<CVPixelBufferRef> imageBuffer = PAL::CMSampleBufferGetImageBuffer(sample.platformSample().sample.cmSampleBuffer);

Should be able to write just:

    RetainPtr imageBuffer = ...

> Source/WebCore/platform/graphics/RemoteVideoSample.cpp:115
> +    return std::unique_ptr<RemoteVideoSample>(new RemoteVideoSample(surface, WTFMove(imageBuffer), DestinationColorSpace::SRGB(), sample.presentationTime(), sample.videoRotation(), sample.videoMirrored()));

Another option would be to write:

    return std::unique_ptr<RemoteVideoSample>(new RemoteVideoSample(surface, RetainPtr { imageBuffer }, DestinationColorSpace::SRGB(), sample.presentationTime(), sample.videoRotation(), sample.videoMirrored()));

As alternatives to RetainPtr { }, could choose RetainPtr() syntax or call the retainPtr() function.

This could be done without changing the type of the local variable. Not sure I prefer this to what you have done, though.

[David Kilzer (:ddkilzer)]

Created attachment 437184 [details]
Patch for landing

[EWS]

Committed r281984 (241291@main): <https://commits.webkit.org/241291@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437184 [details].