[webkit-reviews] review granted: [Bug 237083] LibWebRTCCodecs, -Proxy create and communicate the RemoteVideoFrameProxy incorrectly : [Attachment 453081] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 237083: LibWebRTCCodecs, -Proxy create and communicate the
RemoteVideoFrameProxy incorrectly
https://bugs.webkit.org/show_bug.cgi?id=237083

Attachment 453081: Patch

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




--- Comment #3 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 453081
  --> https://bugs.webkit.org/attachment.cgi?id=453081
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.