Bug 236099

<rdar://88260597>

Created attachment 450810 [details] WIP

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.

Created attachment 450890 [details] Patch

Created attachment 450894 [details] Patch

> 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)

Created attachment 451102 [details] Patch

Created attachment 451103 [details] Patch

Created attachment 451106 [details] Patch

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/

Created attachment 451217 [details] Patch

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.

Created attachment 451346 [details] WIP with CaptureSampleManager

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.

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

Created attachment 451405 [details] Patch for landing

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].

Re-opened since this is blocked by bug 236467

Created attachment 451662 [details] Patch

Created attachment 451663 [details] Patch

Created attachment 451667 [details] Patch

Created attachment 451669 [details] Patch

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Created attachment 451682 [details] Patch

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].