Bug 236099 – Introduce a RemoteMediaSampleProxy to represent captured video frames used in Media Streams and present in GPUP

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

[Kimmo Kinnunen]

Introduce a RemoteMediaSampleProxy to represent captured video frames used in Media Streams and present in GPUP

[Kimmo Kinnunen]

<rdar://88260597>

[Kimmo Kinnunen]

Created attachment 450810 [details]
WIP

[youenn fablet]

Comment on attachment 450810 [details]
WIP

Quickly glancing at it, the WebKit part of it looks great to me.
At some point, we will need to be able to read remote video frame proxy pixels from WebProcess but this should be easy to add using SharedVideoFrameReader/Writer.
I am less sure about the integration with MediaSample.
Could you take a look at these bits in https://bugs.webkit.org/show_bug.cgi?id=236131 and tell me what you think.
I am in particular wondering whether we could not have VideoFrame being a wrapper around a Variant<CVPixelBufferRef, WebRTCBuffer, RemoteVideoFrameProxy>.

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

> Source/WebCore/platform/MediaSample.h:53
> +        RemoteVideoFrameProxyType, // FIXME: To be removed when VideoFrame is not MediaSample.

Normally, if we add RemoteVideoFrameProxyType, we should probably add a new type in the union below.
In https://bugs.webkit.org/show_bug.cgi?id=236131, I am introducing VideoFrameType as a member and VideoFrame* as a member of the union.
Ideally, VideoFrame would be able to encapsulate the RemoteVideoFrameProxyType case.

> Source/WebCore/platform/MediaSample.h:114
> +    virtual RefPtr<ImageBuffer> imageBuffer() const { return nullptr; }

I would be tempted to move imageBuffer() to RemoteVideoFrameProxyType branch code if possible.

[Kimmo Kinnunen]

Created attachment 450890 [details]
Patch

[Kimmo Kinnunen]

Created attachment 450894 [details]
Patch

[Kimmo Kinnunen]

> Normally, if we add RemoteVideoFrameProxyType, we should probably add a new type in the union below.

No, the union is pointless in this scenario.
The type is just used to identify that RemoteVideoFrameProxy is not a MediaSourceAVFObjC.

When VideoFrame is not a MediaSource, there will not be a union or variant, as all the instances are normal subclasses (like ImageBuffers and other abstractions)

[Kimmo Kinnunen]

Created attachment 451102 [details]
Patch

[Kimmo Kinnunen]

Created attachment 451103 [details]
Patch

[Kimmo Kinnunen]

Created attachment 451106 [details]
Patch

[Eric Carlson]

Comment on attachment 451106 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        currently are currently typed as:

s/currently are currently typed/currently are typed/

[Kimmo Kinnunen]

Created attachment 451217 [details]
Patch

[youenn fablet]

Comment on attachment 451217 [details]
Patch

After reading most of the patch, I know better understand it so there might be questions at the beginning that might not be needed.
Anyway, my main question is if we could stick to the model where:
- GPUProcess creates an IOSurface
- GPUProcess sends it to WebProcess (as an ID)
- WebProcess creates a MediaSample/VideoFrame based on the ID.
This model is what we use for libwebrtc and video capture, it should also be applicable to canvas capture.

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

> Source/WebKit/ChangeLog:12
> +        RemoteVideoFrameProxy can be sent to the GPUP and the the SampleBufferDisplayLayer. So

s/the the/the/

> Source/WebKit/ChangeLog:21
> +        map to normal MediaSampleAVFObjC.

It would make more sense to do a direct RemoteVideoFrameIdentifier -> IOSurface. WebProcess will need the other information (rotation, size, timestamps) but may not need the IOSurface raw bytes.

> Source/WebKit/ChangeLog:32
> +        received.

I am not sure how this applies to the typical webrtc case which is: a new video frame is created in GPUProcess, let's send an IPC to WebProcess.
The basic flow I had in mind was something like the following:
- At the moment we want to send it to WebProcess, we register the IOSurface in a map which provides us an Identifier. As part of the IPC message, we send the identifier.
- The map refs the IOSurface and will only release it when WebProcess asks to release it or when WebProcess crashes.
This way of doing things makes things very easy to integrate with existing code paths, it is very similar to MachSendRight.
It also allows if we think this is useful to have per-source or per-use maps.

The potential issues with this model are:
- If WebProcess does not process the message, we could leak the surface. We need to make sure we always decode IPC messages that contain such IDs (LibWebRTCCodecs and RemoteCaptureSampleManager guarantee this).
- When WebProcess releases the IOSurface, we need to make sure we correctly handle the race conditions between sinks wanting to access the IOSurface and the WebProcess message to release the IOSurface. One way of doing this would be to use AsyncReply, the completion handler in WebProcess holding a ref to the WebProcess object. Or we could use a BinarySemaphore to let WebProcess be notified when it is ok to unref the WebProcess object.

On WebRTC side, we also need to send MediaSample from WebProcess to GPUProcess (to HW encoders say).
The frames might not always be remote-IOSurface but in memory as well.
So it is best if we can use the existing IPC messages like LibWebRTCCodecsProxy::EncodeFrame with both kind of frames.

> Source/WebKit/ChangeLog:46
> +            "End the life-time of identifier 1, version 32 when all 77 reads have completed. Create version = 33"

What is the use case for version?
In the typical case, what we do is create-read-remove.
It makes sense to deal with read/remove concurrency but version seems to have another purpose.

> Source/WebCore/platform/VideoFrame.h:45
> +    // FIXME: These are not intended to be used for these objects.

I think it would be good to separate between APIs that will be needed to the final VideoFrame object and MediaSample specific methods that we do not care.
presentationSize() for instance is used in WebProcess.
Patch in https://bugs.webkit.org/attachment.cgi?id=450881&action=review is trying to highlight this separation.

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:958
> +

not needed

> Source/WebKit/WebProcess/GPU/graphics/RemoteGraphicsContextGLProxy.cpp:130
> +    auto mediaSample = RemoteVideoFrameProxy::create(*m_gpuProcessConnection);

Ah ok, now I see why you are doing it this way.
WebProcess is asking a MediaSample, so let's create one now in WebProcess then let GPUProcess deal with filling it at some point.
For Canvas Capture, we can change the synchronous call to an async call with a completion handler.
That way we could write something like:
void RemoteGraphicsContextGLProxy::paintCompositedResultsToMediaSample(CompletionHandler<void(RefPtr<MediaSample>&&)>&& callback)
{
    sendWithAsyncReply(Messages::RemoteGraphicsContextGL::PaintCompositedResultsToMediaSample(), WTFMove(callback));
}

This would simplify the logic by always making GPUProcess responsible for creating the IOSurface, the IDs...
I am not sure we have a use case for creating a synchronous MediaSample that would be later filled by GPUProcess data.
If so, maybe we could treat this using a dedicated sync IPC for now and see how we can move away from sync IPC as a follow-up?

> Source/WebKit/WebProcess/GPU/webrtc/SampleBufferDisplayLayer.cpp:146
> +        return;

Why do we need this check? It might probably be racy anyway given this happens in a background thread.

[Kimmo Kinnunen]

Created attachment 451346 [details]
WIP with CaptureSampleManager

[youenn fablet]

Comment on attachment 451217 [details]
Patch

OK, let's go with this version. I think we will want to move ID creation mostly or solely in GPUProcess which will allow simplifying things.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 451217 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Kimmo Kinnunen]

Created attachment 451405 [details]
Patch for landing

[EWS]

Committed r289525 (247056@main): <https://commits.webkit.org/247056@main>

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 236467

[Kimmo Kinnunen]

Created attachment 451662 [details]
Patch

[Kimmo Kinnunen]

Created attachment 451663 [details]
Patch

[Kimmo Kinnunen]

Created attachment 451667 [details]
Patch

[Kimmo Kinnunen]

Created attachment 451669 [details]
Patch

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[Kimmo Kinnunen]

Created attachment 451682 [details]
Patch

[EWS]

Committed r289622 (247133@main): <https://commits.webkit.org/247133@main>

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