Implement the remote ImageBuffer

• Component: Canvas
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Add the new interfaces RemoteImageBuffer and RemoteImageBufferProxy to WebKit. These classes will be responsible for handling the communications of the ImageBuffer parts between the WebProcess and the GPUProcess. No ImageBuffer backend operations will be handled by these objects.

[Said Abou-Hallawa]

Created attachment 389695 [details]
Patch

[Said Abou-Hallawa]

Created attachment 389701 [details]
Patch  for review

[Said Abou-Hallawa]

Created attachment 391709 [details]
Patch

[Said Abou-Hallawa]

Created attachment 391726 [details]
Patch

[Said Abou-Hallawa]

NOTE 1: The two parts of the remote ImageBuffer: RemoteImageBuffer and RemoteImageBufferProxy, are connected by the the RemoteImageBufferChannel and RemoteImageBufferChannelProxy.

NOTE 2: RemoteImageBufferChannel can be merged into RemoteImageBuffer and similarly RemoteImageBufferChannelProxy can be merged into RemoteImageBufferProxy. But in this case the IPC communications code will be in the header files of RemoteImageBuffer and RemoteImageBufferProxy because they are template classes.

NOTE 3: Two singletons can be created to handle the IPC communications of all the remote ImageBuffers. But this will be bring back RemoteRenderingBackend which Sam did not like in https://bugs.webkit.org/show_bug.cgi?id=207134. RemoteRenderingBackend will be the singleton for receiving the RemoteImageBuffer in the WebProcess side.

NOTE 4: I made GPUConnection and GPUConnectionToWebProcess be the singletons for receiving all remote ImageBuffer IPC communication. I added a MessageReceiverMap to each of them. Both RemoteImageBufferChannel and RemoteImageBufferChannelProxy register themselves as MessageReceivers in their constructors. And they both unregister themselves in their destructors. Both GPUConnection and GPUConnectionToWebProcess dispatch the messages to the appropriate remote ImageBuffer. So I think adding another dispatcher between the GPU connection and the remote ImageBuffer will not add any value. GPUConnection will dispatch messages to RemoteRenderingBackend and RemoteRenderingBackend will dispatch messages to RemoteImageBuffer.

NOTE 5: I was not sure if adding RemoteRenderingBackendProxy to GPUConnectionToWebProcess is the right thing to do or not since the WebProcess does not have RemoteRenderingBackend. I could have made GPUConnectionToWebProcess be responsible for creating and releasing the RemoteImageBufferProxy. But I followed what was done for media in GPUConnectionToWebProcess. But I think we should choose a different name for RemoteRenderingBackendProxy. Suggestions are: RemoteImageBufferProxyPool ImageBufferPool, RemoteImageBufferProxyFactory, ImageBufferFactory.

[Said Abou-Hallawa]

NOTE 6: RemoteImageBuffer and RemoteImageBufferProxy are backed by a dummy backend: ImageBufferIOSurfaceBackend, which is not the intended behavior. So in this patch RemoteImageBuffer and RemoteImageBufferProxy will create two separate backends and flushing the drawing context will happen in the WebProcess. This will be fixed after adding the shareable ImageBufferBackend.

[Tim Horton]

Comment on attachment 391726 [details]
Patch

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

Seems OK to me, let's see what smfr says.

The IPC changes are a bit surprising to be in this patch, but S_OK.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferChannel.cpp:73
> +    // FIXME: Delete this function when messages are addded to RemoteImageBufferChannel.messages.in.

Lots of "addded" (spelling!). But instead of fixing you can probably just elide this. The compiler will tell you!

[Simon Fraser (smfr)]

Comment on attachment 391726 [details]
Patch

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

> Source/WebCore/page/ChromeClient.h:308
> +    virtual std::unique_ptr<ImageBuffer> createImageBuffer(const FloatSize&, RenderingMode, float, ColorSpace) const { ASSERT_NOT_REACHED(); return nullptr; }

Can this be pure virtual instead of asserting?

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:213
>      if (decoder.messageReceiverName() == Messages::RemoteMediaPlayerManagerProxy::messageReceiverName()) {
>          remoteMediaPlayerManagerProxy().didReceiveMessageFromWebProcess(connection, decoder);
> -        return;
> +        return true;
>      } else if (decoder.messageReceiverName() == Messages::RemoteMediaPlayerProxy::messageReceiverName()) {
>          remoteMediaPlayerManagerProxy().didReceivePlayerMessage(connection, decoder);
> -        return;
> +        return true;
>      } else if (decoder.messageReceiverName() == Messages::RemoteMediaResourceManager::messageReceiverName()) {
>          remoteMediaResourceManager().didReceiveMessage(connection, decoder);
> -        return;
> +        return true;
>      }

All these string comparisons are nuts.

> Source/WebKit/GPUProcess/graphics/RemoteImageBufferProxy.h:36
> +class RemoteImageBufferProxy : public WebCore::ConcreteImageBuffer<BackendType>, public RemoteImageBufferChannelProxy {

Does this mean a per-image-buffer thing is an IPC message receiver? I thought we were going to avoid this.

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:85
> +    if (!m_imageBufferMap.contains(imageBufferIdentifier)) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }
> +    m_imageBufferMap.remove(imageBufferIdentifier);

This is two hash lookups. You could reduce it to one using find().

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.h:37
> +class RemoteRenderingBackendProxy : private IPC::MessageReceiver {

The "backend" in the name makes me confused with ImageBufferBackend. This is more like a RemoteRenderingController, right? Or RemoteImageBufferRenderingController. It knows about ImageBuffers, not ImageBufferBackends.

It's also hard to tell from the name (and the file location) whether this is the GPU process side or the Web process side. I wonder if we should make that explicit in the name (GPURemote... WebRemote...).

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBuffer.h:36
> +class RemoteImageBuffer : public WebCore::DisplayList::ImageBuffer<BackendType>, public RemoteImageBufferChannel {

Same comment about this being a channel.

[Sam Weinig]

Comment on attachment 391726 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:899
> +    bool usesRemoteDrawing = false;

It seems really unfortunate that this leaks down to WebCore like this.

[Said Abou-Hallawa]

Created attachment 392081 [details]
Patch

[Said Abou-Hallawa]

Created attachment 392168 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 392168 [details]:

editing/spelling/spellcheck-paste-continuous-disabled.html bug 208016 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 392168 [details]
Patch

Clearing flags on attachment: 392168

Committed r257730: <https://trac.webkit.org/changeset/257730>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59964463>

[Said Abou-Hallawa]

Comment on attachment 391726 [details]
Patch

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

>> Source/WebCore/html/HTMLCanvasElement.cpp:899
>> +    bool usesRemoteDrawing = false;
> 
> It seems really unfortunate that this leaks down to WebCore like this.

This has been fixed in <https://trac.webkit.org/changeset/257677>. WebCore does not have to know whether rendering on canvas is remote or local.

>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:213
>>      }
> 
> All these string comparisons are nuts.

I think the problem is the media messages receivers are lazily created. So receiving the message for the first time will cause the appropriate receiver to be created.

>> Source/WebKit/GPUProcess/graphics/RemoteImageBufferProxy.h:36
>> +class RemoteImageBufferProxy : public WebCore::ConcreteImageBuffer<BackendType>, public RemoteImageBufferChannelProxy {
> 
> Does this mean a per-image-buffer thing is an IPC message receiver? I thought we were going to avoid this.

This was fixed in <https://trac.webkit.org/changeset/257730>. RemoteRenderingBackend and RemoteRenderingBackendProxy became the entry points for all the RemoteImageBuffer/RemoteImageBufferProxy messages. But this adds a new level of message dispatching. The flow of messages now is:

GPUProcessConnection -> RemoteRenderingBackend -> RemoteImageBuffer
GPUConnectionToWebProcess -> RemoteRenderingBackendProxy -> RemoteImageBufferProxy

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:85
>> +    m_imageBufferMap.remove(imageBufferIdentifier);
> 
> This is two hash lookups. You could reduce it to one using find().

This is fixed in <https://trac.webkit.org/changeset/257542>. We use the return of the remove method to know whether the element was in the hash map or not.

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.h:37
>> +class RemoteRenderingBackendProxy : private IPC::MessageReceiver {
> 
> The "backend" in the name makes me confused with ImageBufferBackend. This is more like a RemoteRenderingController, right? Or RemoteImageBufferRenderingController. It knows about ImageBuffers, not ImageBufferBackends.
> 
> It's also hard to tell from the name (and the file location) whether this is the GPU process side or the Web process side. I wonder if we should make that explicit in the name (GPURemote... WebRemote...).

I think having a name not tide to the ImageBuffer is more appropriate. The plan is to cache the decoded image and the fonts on the GPUProcess side and later refer to them by IDs or handles. RemoteRenderingBackend will be the place to manage this caching mechanism.

Also after bringing back the RemoteRenderingBack in <https://trac.webkit.org/changeset/257583>, there should be no confusion about which owns this class. I think the notion is: RemoteClassName means it belongs to WebProcess and RemoteClassNameProxy means it belongs to the GPUProcess.

>> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBuffer.h:36
>> +class RemoteImageBuffer : public WebCore::DisplayList::ImageBuffer<BackendType>, public RemoteImageBufferChannel {
> 
> Same comment about this being a channel.

Fixed.