Bug 237025 – RemoteSampleBufferDisplayLayer::enqueueSample should not change media samples owned by its object heap

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

[youenn fablet]

RemoteSampleBufferDisplayLayer::enqueueSample should not change media samples owned by its object heap

[youenn fablet]

Created attachment 452843 [details]
Patch

[Darin Adler]

Comment on attachment 452843 [details]
Patch

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

> Source/WebKit/WebProcess/GPU/webrtc/RemoteVideoFrameObjectHeapProxyProcessor.cpp: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.

[youenn fablet]

(In reply to Darin Adler from comment #2)
> Comment on attachment 452843 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452843&action=review
> 
> > Source/WebKit/WebProcess/GPU/webrtc/RemoteVideoFrameObjectHeapProxyProcessor.cpp:46
> > +    , m_sharedVideoFrameReader(nullptr)
> 
> This should be initialized in the header, not the .cpp file.

This requires including RemoteVideoFrameObjectHeap.h in SharedVideoFrame.h header file.
I guess I can change the constructor to take a RemoteVideoFrameObjectHeap* instead of a RefPtr<RemoteVideoFrameObjectHeap>&&.


> 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.

I did not know about the encoder variant, will use it.

[Darin Adler]

Comment on attachment 452843 [details]
Patch

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

>>> Source/WebKit/WebProcess/GPU/webrtc/RemoteVideoFrameObjectHeapProxyProcessor.cpp:46
>>> +    , m_sharedVideoFrameReader(nullptr)
>> 
>> This should be initialized in the header, not the .cpp file.
> 
> This requires including RemoteVideoFrameObjectHeap.h in SharedVideoFrame.h header file.
> I guess I can change the constructor to take a RemoteVideoFrameObjectHeap* instead of a RefPtr<RemoteVideoFrameObjectHeap>&&.

Or add a default constructor.

[youenn fablet]

Created attachment 452943 [details]
Patch for landing

[EWS]

Committed r290358 (247676@main): <https://commits.webkit.org/247676@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/89343447>

[youenn fablet]

(In reply to youenn fablet from comment #5)
> Created attachment 452943 [details]
> Patch for landing

I forgot to commit the changes to use variant encoder before landing the patch...
I will upload a follow-up patch to fix that.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 452965 [details]
Post-commit changes

[EWS]

Committed r290373 (247689@main): <https://commits.webkit.org/247689@main>

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