Bug 240113

<rdar://92629845>

Created attachment 458862 [details]
Patch

Created attachment 458863 [details]
Patch

Created attachment 458864 [details]
Patch

Created attachment 458865 [details]
Patch

Comment on attachment 458865 [details]
Patch

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

r=me once the bots are happy

> Source/WebKit/GPUProcess/media/RemoteVideoFrameObjectHeap.cpp:142
> +        m_connection->send(Messages::RemoteVideoFrameObjectHeapProxyProcessor::NewConvertedVideoFrameBuffer { { } }, 0);

It looks like SharedVideoFrameReader doesn't log errors, so it might be worth logging an error here to help debug unexpected failures.

> Source/WebKit/GPUProcess/media/RemoteVideoFrameObjectHeap.cpp:149
> +        m_connection->send(Messages::RemoteVideoFrameObjectHeapProxyProcessor::NewConvertedVideoFrameBuffer { { } }, 0);

Ditto, PixelBufferConformerCV also doesn't log failures.

> Source/WebKit/GPUProcess/media/RemoteVideoFrameObjectHeap.mm:32
> +

Nit: unneeded white space

> It looks like SharedVideoFrameReader doesn't log errors, so it might be
> worth logging an error here to help debug unexpected failures.

Let's fix this in https://bugs.webkit.org/show_bug.cgi?id=236066, I just uploaded a patch.

> > Source/WebKit/GPUProcess/media/RemoteVideoFrameObjectHeap.cpp:149
> > +        m_connection->send(Messages::RemoteVideoFrameObjectHeapProxyProcessor::NewConvertedVideoFrameBuffer { { } }, 0);
> 
> Ditto, PixelBufferConformerCV also doesn't log failures.

Will add logging for this one.

Comment on attachment 458865 [details]
Patch

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

> Source/WebKit/WebProcess/GPU/webrtc/RemoteVideoFrameObjectHeapProxyProcessor.cpp:145
> +    connection.send(Messages::RemoteVideoFrameObjectHeap::ConvertBuffer { WTFMove(*buffer) }, 0);
> +    m_conversionSemaphore.wait();

Why this and not just a sync message? Sync IPC has logic to avoid deadlocking, but this doesn't.

Created attachment 458882 [details]
Patch for landing

Created attachment 458888 [details]
Patch for landing

(In reply to Simon Fraser (smfr) from comment #9)
> Comment on attachment 458865 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458865&action=review
> 
> > Source/WebKit/WebProcess/GPU/webrtc/RemoteVideoFrameObjectHeapProxyProcessor.cpp:145
> > +    connection.send(Messages::RemoteVideoFrameObjectHeap::ConvertBuffer { WTFMove(*buffer) }, 0);
> > +    m_conversionSemaphore.wait();
> 
> Why this and not just a sync message? Sync IPC has logic to avoid
> deadlocking, but this doesn't.

We send a message from main thread but we receive the answer from a work queue.

Created attachment 458941 [details]
Patch for landing

(In reply to youenn fablet from comment #12)
> (In reply to Simon Fraser (smfr) from comment #9)
> > Comment on attachment 458865 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=458865&action=review
> > 
> > > Source/WebKit/WebProcess/GPU/webrtc/RemoteVideoFrameObjectHeapProxyProcessor.cpp:145
> > > +    connection.send(Messages::RemoteVideoFrameObjectHeap::ConvertBuffer { WTFMove(*buffer) }, 0);
> > > +    m_conversionSemaphore.wait();
> > 
> > Why this and not just a sync message? Sync IPC has logic to avoid
> > deadlocking, but this doesn't.
> 
> We send a message from main thread but we receive the answer from a work
> queue.

Thinking more about it, this will create a deadlock in case of GPUProcess crash, so we might need to use sync IPC, though I am unsure how sync IPC and work queue message receivers do things consistently.
I'll file a bug to keep track of this.

I filed https://bugs.webkit.org/show_bug.cgi?id=240161.

Committed r293886 (250344@main): <https://commits.webkit.org/250344@main>

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