Bug 229806 - RemoteVideoSample needs CVPixelBufferRef to be kept alive, but relies on caller to retain it
Summary: RemoteVideoSample needs CVPixelBufferRef to be kept alive, but relies on call...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-02 07:47 PDT by David Kilzer (:ddkilzer)
Modified: 2021-09-03 03:51 PDT (History)
9 users (show)

See Also:


Attachments
Patch v1 (6.54 KB, patch)
2021-09-02 11:31 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (8.79 KB, patch)
2021-09-02 12:29 PDT, David Kilzer (:ddkilzer)
darin: review+
Details | Formatted Diff | Diff
Patch for landing (8.73 KB, patch)
2021-09-02 12:49 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2021-09-02 07:47:59 PDT
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>
Comment 1 Radar WebKit Bug Importer 2021-09-02 11:18:42 PDT
<rdar://problem/82684479>
Comment 2 David Kilzer (:ddkilzer) 2021-09-02 11:31:49 PDT
Created attachment 437174 [details]
Patch v1
Comment 3 Darin Adler 2021-09-02 11:46:56 PDT
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()));
Comment 4 David Kilzer (:ddkilzer) 2021-09-02 12:29:45 PDT
Created attachment 437181 [details]
Patch v2
Comment 5 Darin Adler 2021-09-02 12:38:20 PDT
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.
Comment 6 David Kilzer (:ddkilzer) 2021-09-02 12:49:50 PDT
Created attachment 437184 [details]
Patch for landing
Comment 7 EWS 2021-09-03 03:51:29 PDT
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].