Bug 236356 – Support remote video frames in WebRTC video pipeline

• Component: WebRTC
• Status: RESOLVED FIXED

[youenn fablet]

Support remote video frames in WebRTC video pipeline

[youenn fablet]

Created attachment 451352 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 451352 [details]
Patch

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

Looks good overall.
I'm not 100% sure I am satisfied the process qualified identifier, but I can appreciate how good of an invention that was!

> Source/WebKit/GPUProcess/media/RemoteVideoFrameObjectHeap.cpp:75
> +RemoteVideoFrameIdentifier RemoteVideoFrameObjectHeap::createVideoFrame(Ref<WebCore::MediaSample>&& frame)

You could name this createRemoteVideoFrame() and explicitly call the retire the way described below.

> Source/WebKit/Shared/ThreadSafeObjectHeap.h:193
> +void ThreadSafeObjectHeap<Identifier, HeldType>::add(Identifier identifier, HeldType&& object)

This hides the reference semantics.
There should not be such a functions at this level.
It's a low-level class for managing the reference semantics.

So it would be better that caller uses:

auto theNewObject = ....;
auto write = WriteReference { { identifier, 0 }, 0}; 
auto newReference = write.retiredReference();
heap->retire(write, theNewObject);

sendTheWrittenReference(newReference)

> Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:212
> +        m_connection->send(Messages::RemoteCaptureSampleManager::VideoSampleAvailable(m_id, WTFMove(*remoteSample), remoteIdentifier, metadata), 0);

This is maybe a bit messier than I'd prefer.
To me it would make sense to have two different messages. Example naming:
 VideoSampleAvailable for sending RemoteVideoFrameProxy
 VideoSampleAvailableCV for sending RemoteVideoSample (CV as in Core Video).

> Source/WebKit/WebProcess/GPU/media/RemoteVideoFrameProxy.cpp:61
> +    , m_connectionID(gpuProcessConnection.connection().uniqueID())

Not sure what is the purpose of this.
If we want to store thread-safe connection, we just get Ref<IPC::Connection. Connection is thread-safe to hold, gpuProcessConnection is not.

> Source/WebKit/WebProcess/GPU/media/RemoteVideoFrameProxy.cpp:152
> +        IPC::Connection::send(*m_connectionID, Messages::RemoteVideoFrameObjectHeap::ReleaseVideoFrame(write()), 0, IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);

This message does not make sense to be sent if we do not have a GPU process connection anymore.

> Source/WebKit/WebProcess/GPU/media/RemoteVideoFrameProxy.h:71
>  

I don't think these should be moved to private.
In private, it means that these functions cannot be called through RemoteViedoFrameProxy. This is incorrect, as they're the features the class supports and we want the client to call them through the MediaSample. Similarly if client holds the VideoFrame, we want it to be able to call.

[youenn fablet]

(In reply to Kimmo Kinnunen from comment #2)
> Comment on attachment 451352 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451352&action=review
> 
> Looks good overall.
> I'm not 100% sure I am satisfied the process qualified identifier, but I can
> appreciate how good of an invention that was!

Right, I think we want to generate IDs where the frames are created, i.e. in GPUProcess solely. I hope we can make it that way so that we can go back to the remove ID being a simple ObjectIdentifier at some point after we make the whole thing working.

[youenn fablet]

Created attachment 451520 [details]
Patch

[youenn fablet]

We probably need to add a backup in case we cannot retrieve a remote CVPixelBuffer, libwebrtc might not like null buffers. We can probably use a black frame in that case.

[youenn fablet]

Created attachment 451610 [details]
Patch

[youenn fablet]

Created attachment 451611 [details]
Patch

[youenn fablet]

Created attachment 451652 [details]
Combined patch

[youenn fablet]

(In reply to Kimmo Kinnunen from comment #2)
> Comment on attachment 451352 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451352&action=review
> 
> Looks good overall.
> I'm not 100% sure I am satisfied the process qualified identifier, but I can
> appreciate how good of an invention that was!

Yes, we might want to make it back to a pure ObjectIdentifier if/when GPUProcess is the only creating the IDs.

> > Source/WebKit/GPUProcess/media/RemoteVideoFrameObjectHeap.cpp:75
> > +RemoteVideoFrameIdentifier RemoteVideoFrameObjectHeap::createVideoFrame(Ref<WebCore::MediaSample>&& frame)
> 
> You could name this createRemoteVideoFrame() and explicitly call the retire
> the way described below.

OK

> > Source/WebKit/Shared/ThreadSafeObjectHeap.h:193
> > +void ThreadSafeObjectHeap<Identifier, HeldType>::add(Identifier identifier, HeldType&& object)
> 
> This hides the reference semantics.
> There should not be such a functions at this level.
> It's a low-level class for managing the reference semantics.
> 
> So it would be better that caller uses:
> 
> auto theNewObject = ....;
> auto write = WriteReference { { identifier, 0 }, 0}; 
> auto newReference = write.retiredReference();
> heap->retire(write, theNewObject);
> 
> sendTheWrittenReference(newReference)

retire has a lot of logic that are unnecessary in our case where we know the ID is brand new.

> > Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:212
> > +        m_connection->send(Messages::RemoteCaptureSampleManager::VideoSampleAvailable(m_id, WTFMove(*remoteSample), remoteIdentifier, metadata), 0);
> 
> This is maybe a bit messier than I'd prefer.
> To me it would make sense to have two different messages. Example naming:
>  VideoSampleAvailable for sending RemoteVideoFrameProxy
>  VideoSampleAvailableCV for sending RemoteVideoSample (CV as in Core Video).

I like having just one IPC message.
The message is a bit bigger but this makes it easier to follow to me.

> > Source/WebKit/WebProcess/GPU/media/RemoteVideoFrameProxy.cpp:61
> > +    , m_connectionID(gpuProcessConnection.connection().uniqueID())
> 
> Not sure what is the purpose of this.
> If we want to store thread-safe connection, we just get Ref<IPC::Connection.
> Connection is thread-safe to hold, gpuProcessConnection is not.

We could, but connectionID is equally good.

> > Source/WebKit/WebProcess/GPU/media/RemoteVideoFrameProxy.cpp:152
> > +        IPC::Connection::send(*m_connectionID, Messages::RemoteVideoFrameObjectHeap::ReleaseVideoFrame(write()), 0, IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);
> 
> This message does not make sense to be sent if we do not have a GPU process
> connection anymore.

When video frame is created as already initialised, it no longer has an initialized m_gpuProcessConnection (it does not need to be registered as a message receiver) but it has to say that it is being released.

> 
> > Source/WebKit/WebProcess/GPU/media/RemoteVideoFrameProxy.h:71
> >  
> 
> I don't think these should be moved to private.
> In private, it means that these functions cannot be called through
> RemoteViedoFrameProxy. This is incorrect, as they're the features the class
> supports and we want the client to call them through the MediaSample.
> Similarly if client holds the VideoFrame, we want it to be able to call.

It is mostly a style change, in general I prefer reducing exposure of overriden methods, nothing prevents to make them public if we want.
I think it is nice to keep it this way:
- WebCore works on MediaSample API.
- WebKit works on RemoteViedoFrameProxy specific API.

At some point, I would like RemoteViedoFrameProxy (or part of it) to be back to a WebCore object and WebKit would only deal with IPC stuff.

[youenn fablet]

Created attachment 451654 [details]
Combined patch

[youenn fablet]

Created attachment 451673 [details]
Patch

[youenn fablet]

Created attachment 451692 [details]
Combined patch

[youenn fablet]

Created attachment 451694 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 451694 [details]
Patch

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

Good, a bit to improve later in our overall architecture.
few nits

> Source/WebKit/GPUProcess/media/RemoteVideoFrameObjectHeap.cpp:71
> +RemoteVideoFrameIdentifier RemoteVideoFrameObjectHeap::createRemoteVideoFrame(Ref<WebCore::MediaSample>&& frame)

maybe don't add this.
see the call site.

> Source/WebKit/GPUProcess/webrtc/LibWebRTCCodecsProxy.mm:100
> +            remoteIdentifier = videoFrameObjectHeap->createRemoteVideoFrame(sample.releaseNonNull());

Use the same pattern as RemoteGraphicsContextGL::paintCompositedResultsToMediaSample()

> Source/WebKit/Shared/ThreadSafeObjectHeap.h:192
> +template<typename Identifier, typename HeldType>

don't add this, it's a bit redundant and hard to use correctly.

[Kimmo Kinnunen]

Comment on attachment 451694 [details]
Patch

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

> Source/WebKit/Shared/ThreadSafeObjectHeap.h:198
> +    m_objects.add(reference, ReferenceState { WTFMove(object) });

actually, you have to remove this, it's incorrect.

The add needs to be done with retiring a write reference, otherwise it won't work.
So just remove this function (and the calling function)

[Kimmo Kinnunen]

Comment on attachment 451694 [details]
Patch

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

> Source/WebKit/Shared/ThreadSafeObjectHeap.h:198
> +    m_objects.add(reference, ReferenceState { WTFMove(object) });

actually, you have to remove this, it's incorrect.

The add needs to be done with retiring a write reference, otherwise it won't work.
So just remove this function (and the calling function)

[youenn fablet]

(In reply to Kimmo Kinnunen from comment #16)
> Comment on attachment 451694 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451694&action=review
> 
> > Source/WebKit/Shared/ThreadSafeObjectHeap.h:198
> > +    m_objects.add(reference, ReferenceState { WTFMove(object) });
> 
> actually, you have to remove this, it's incorrect.
> 
> The add needs to be done with retiring a write reference, otherwise it won't
> work.
> So just remove this function (and the calling function)

It should be safe with the current usage (we always provide a new ID generated by GPUProcess), but I see your point about being consistent.
I am landing as is right now, but will follow up with improvements according your suggestions.

[youenn fablet]

<rdar://88260659>

[EWS]

Committed r289628 (247138@main): <https://commits.webkit.org/247138@main>

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

[Brent Fulgham]

*** Bug 232879 has been marked as a duplicate of this bug. ***