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>
<rdar://problem/82684479>
Created attachment 437174 [details] Patch v1
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()));
Created attachment 437181 [details] Patch v2
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.
Created attachment 437184 [details] Patch for landing
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].