Bug 237083 – LibWebRTCCodecs, -Proxy create and communicate the RemoteVideoFrameProxy incorrectly

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Kimmo Kinnunen]

LibWebRTCCodecs, -Proxy create and communicate the RemoteVideoFrameProxy incorrectly

[Kimmo Kinnunen]

Created attachment 452972 [details]
Patch

[Kimmo Kinnunen]

Created attachment 453081 [details]
Patch

[youenn fablet]

Comment on attachment 453081 [details]
Patch

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

> Source/WebCore/platform/MediaSample.h:115
> +    virtual void setOwnershipIdentity(const ProcessIdentity&) { }

It seems unnecessary to add a virtual method here, how about just reusing pixelBuffer()?
Also, there is only one call site in Cocoa specific code, so maybe we do not need to add such MediaSample method since we might want to move away from MediaSample for VideoFrames.

> Source/WebKit/GPUProcess/media/RemoteVideoFrameObjectHeap.cpp:87
> +{

We will probably always want to set the owner to the web process currently tied to RemoteVideoFrameObjectHeap.
Maybe we should consider doing that here, otherwise we might forget about doing that in new code paths.

> Source/WebKit/GPUProcess/webrtc/LibWebRTCCodecsProxy.mm:91
> +            sample->setOwnershipIdentity(resourceOwner);

If we were doing that in addVideoFrame, we would need to do that call for the RemoteVideoSample code path.
This is probably fine, since at some point we will remove the RemoteVideoSample code path.

> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:327
> +    // needs to be implemented.

I am not sure this comment helps much. Ideally if we were going into that situation, having some kind of debug assert would be better.

> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.h:136
> +    void completedDecodingCV(RTCDecoderIdentifier, uint32_t timeStamp, WebCore::RemoteVideoSample&&);

Maybe add a FIXME, with something like:
FIXME: Remove when RemoteVideoFrameProxy is enabled.

[Kimmo Kinnunen]

Comment on attachment 453081 [details]
Patch

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

>> Source/WebKit/GPUProcess/webrtc/LibWebRTCCodecsProxy.mm:91
>> +            sample->setOwnershipIdentity(resourceOwner);
> 
> If we were doing that in addVideoFrame, we would need to do that call for the RemoteVideoSample code path.
> This is probably fine, since at some point we will remove the RemoteVideoSample code path.

This is doing it for the RemoteVideoSample too.

>> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:327
>> +    // needs to be implemented.
> 
> I am not sure this comment helps much. Ideally if we were going into that situation, having some kind of debug assert would be better.

So currently there is nowhere to put the assertion, as it cannot happen.

In the scenario that it would happen, it is expected and business as usual. GPUP cannot know when WP unsubscribes a source from the messages. This means GPUP sends a message to some destination *at the same time* WP removes the destination the message will be undelivered. This is business as usual, and needs to be taken care of for the RemoteVideoFrameProxy::Properties, since the Properties owns the reference.

>> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.h:136
>> +    void completedDecodingCV(RTCDecoderIdentifier, uint32_t timeStamp, WebCore::RemoteVideoSample&&);
> 
> Maybe add a FIXME, with something like:
> FIXME: Remove when RemoteVideoFrameProxy is enabled.

I'll do another similar patch for another call site, I'll add the fixme in that patch, to overcome the patch submission delay.

[EWS]

Committed r290423 (247731@main): <https://commits.webkit.org/247731@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/89411003>