Bug 237083

Summary: LibWebRTCCodecs, -Proxy create and communicate the RemoteVideoFrameProxy incorrectly
Product: WebKit Reporter: Kimmo Kinnunen <kkinnunen>
Component: MediaAssignee: Kimmo Kinnunen <kkinnunen>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Kimmo Kinnunen 2022-02-23 06:02:36 PST
LibWebRTCCodecs, -Proxy create and communicate the RemoteVideoFrameProxy incorrectly
Comment 1 Kimmo Kinnunen 2022-02-23 06:13:11 PST
Created attachment 452972 [details]
Patch
Comment 2 Kimmo Kinnunen 2022-02-24 00:37:31 PST
Created attachment 453081 [details]
Patch
Comment 3 youenn fablet 2022-02-24 01:43:12 PST
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.
Comment 4 Kimmo Kinnunen 2022-02-24 02:30:40 PST
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.
Comment 5 EWS 2022-02-24 04:12:55 PST
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].
Comment 6 Radar WebKit Bug Importer 2022-02-24 04:13:22 PST
<rdar://problem/89411003>