[webkit-reviews] review granted: [Bug 229806] RemoteVideoSample needs CVPixelBufferRef to be kept alive, but relies on caller to retain it : [Attachment 437181] Patch v2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 229806: RemoteVideoSample needs CVPixelBufferRef to be kept alive, but
relies on caller to retain it
https://bugs.webkit.org/show_bug.cgi?id=229806

Attachment 437181: Patch v2

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




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 437181
  --> https://bugs.webkit.org/attachment.cgi?id=437181
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.