[webkit-reviews] review granted: [Bug 237025] RemoteSampleBufferDisplayLayer::enqueueSample should not change media samples owned by its object heap : [Attachment 452843] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 237025: RemoteSampleBufferDisplayLayer::enqueueSample should not change
media samples owned by its object heap
https://bugs.webkit.org/show_bug.cgi?id=237025

Attachment 452843: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 452843
  --> https://bugs.webkit.org/attachment.cgi?id=452843
Patch

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

>
Source/WebKit/WebProcess/GPU/webrtc/RemoteVideoFrameObjectHeapProxyProcessor.cp
p:46
> +    , m_sharedVideoFrameReader(nullptr)

This should be initialized in the header, not the .cpp file.

> Source/WebKit/WebProcess/GPU/webrtc/SharedVideoFrame.cpp:101
> +	   IOSurfaceRef surface = pixelBuffer ?
CVPixelBufferGetIOSurface(pixelBuffer) : nullptr;
> +	   if (surface) {

This looks better to me:

    if (auto pixelBuffer = downcast<MediaSampleAVFObjC>(frame).pixelBuffer()) {
	if (auto surface = CVPixelBufferGetIOSurface(pixelBuffer)) {

> Source/WebKit/WebProcess/GPU/webrtc/SharedVideoFrame.h:127
> +    switchOn(buffer,
> +    [&](std::nullptr_t representation) {
> +	   encoder << (uint8_t)0;
> +    }, [&](const RemoteVideoFrameReadReference& reference) {
> +	   encoder << (uint8_t)1;
> +	   encoder << reference;
> +    } , [&](const MachSendRight& sendRight) {
> +	   encoder << (uint8_t)2;
> +	   encoder << sendRight;
> +    });

Not sure the indentation here is right. But also, variant should have an encode
overload that does this automatically. Why doesn’t this work?

    encoder << buffer;

If it doesn't, we can fix that.

> Source/WebKit/WebProcess/GPU/webrtc/SharedVideoFrame.h:161
> +    uint8_t bufferType;
> +    if (!decoder.decode(bufferType))
> +	   return { };
> +
> +    if (bufferType > 2)
> +	   return { };
> +
> +    if (bufferType == 1) {
> +	   std::optional<RemoteVideoFrameReadReference> reference;
> +	   decoder >> reference;
> +	   if (!reference)
> +	       return { };
> +	   frame.buffer = WTFMove(*reference);
> +    } else if (bufferType == 2) {
> +	   MachSendRight sendRight;
> +	   if (!decoder.decode(sendRight))
> +	       return { };
> +	   frame.buffer = WTFMove(sendRight);
> +    }
> +    return frame;

This would be *so* much better in the Variant version.