Bug 217554

Created attachment 411005 [details]
Patch

Created attachment 411006 [details]
Patch for review

Comment on attachment 411006 [details]
Patch for review

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

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:74
> +void RemoteRenderingBackendProxy::remoteCreateImageBufferBackend(const FloatSize& logicalSize, const IntSize& backendSize, float resolutionScale, ColorSpace colorSpace, ImageBufferBackendHandle handle, RemoteResourceIdentifier remoteResourceIdentifier)

Or createRemoteImageBufferBackend?

Does this have to do IPC? Could we create the image buffer backend lazily the first time it's referenced by something?

> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:76
> +    if (auto image = flushDrawingContext(displayList, remoteResourceIdentifier)) {
> +        image->flushContext();

It's confusing that we have both flushDrawingContext() and flushContext(), but we also have the "commit" terminology for flushContext().

> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.h:50
> +    WebCore::ImageBuffer* flushDrawingContext(const WebCore::DisplayList::DisplayList&, WebCore::RemoteResourceIdentifier);
> +    WebCore::ImageBuffer* flushDrawingContextAndCommit(const WebCore::DisplayList::DisplayList&, WebCore::RemoteResourceIdentifier);

I'm a bit confused by flushDrawing() need to take a DisplayList argument. In my mind, "flush" means "execute drawing commands that you've already received".

> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.h:58
> +    RemoteImageBufferHashMap m_remoteImageBufferHashMap;

Don't put "HashMap" in the name. Maybe m_identifierToBufferMap, or just m_imageBuffers.

Created attachment 411199 [details]
Patch

Comment on attachment 411006 [details]
Patch for review

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

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:74
>> +void RemoteRenderingBackendProxy::remoteCreateImageBufferBackend(const FloatSize& logicalSize, const IntSize& backendSize, float resolutionScale, ColorSpace colorSpace, ImageBufferBackendHandle handle, RemoteResourceIdentifier remoteResourceIdentifier)
> 
> Or createRemoteImageBufferBackend?
> 
> Does this have to do IPC? Could we create the image buffer backend lazily the first time it's referenced by something?

Name was changed. Eventually we will not send this message to the Web Process because we do not want it to have access to the backend. We need it now because the canvas ImageBuffer is still drawn to the layer in Web Process which should not happen.

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:76
>> +        image->flushContext();
> 
> It's confusing that we have both flushDrawingContext() and flushContext(), but we also have the "commit" terminology for flushContext().

I renamed it to applyDisplayList() since all RemoteImageBufferProxy does is applying a DisplayList to the backend.

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.h:58
>> +    RemoteImageBufferHashMap m_remoteImageBufferHashMap;
> 
> Don't put "HashMap" in the name. Maybe m_identifierToBufferMap, or just m_imageBuffers.

Done.

Comment on attachment 411199 [details]
Patch

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

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:76
> +    send(Messages::RemoteRenderingBackend::CreateRemoteImageBufferBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(handle), remoteResourceIdentifier), m_renderingBackendIdentifier);

Do we need separate IPC to create the remote buffer backend, or can we just create them when first referenced, and send the IDs in a reply?

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:79
> +void RemoteRenderingBackendProxy::commitApplyDisplayList(ImageBufferFlushIdentifier flushIdentifier, RemoteResourceIdentifier remoteResourceIdentifier)

"commitApply" is weird naming. Can this just use 'flush"?

> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:41
> +void RemoteResourceCacheProxy::createImageBuffer(const FloatSize& logicalSize, RenderingMode renderingMode, float resolutionScale, ColorSpace colorSpace, RemoteResourceIdentifier remoteResourceIdentifier)

Should the cache be the one to create the buffer? Normally you just add things to caches, not have the cache create things.

> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:75
> +        image->flushContext();
> +        m_remoteRenderingBackendProxy.commitApplyDisplayList(flushIdentifier, remoteResourceIdentifier);

The mixture of "commit" and "flush" is confusing here.

Should the function on m_remoteRenderingBackendProxy be called didFlushDrawingForResource?

> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:79
> +RefPtr<ImageData> RemoteResourceCacheProxy::getImageData(AlphaPremultiplication outputFormat, const IntRect& srcRect, RemoteResourceIdentifier remoteResourceIdentifier) const

This doesn't seem like it should live on the resource cache.

Created attachment 411459 [details]
Patch

Comment on attachment 411199 [details]
Patch

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

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:76
>> +    send(Messages::RemoteRenderingBackend::CreateRemoteImageBufferBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(handle), remoteResourceIdentifier), m_renderingBackendIdentifier);
> 
> Do we need separate IPC to create the remote buffer backend, or can we just create them when first referenced, and send the IDs in a reply?

When ImageBuffer::create() is called for a GPU based ImageBuffer, we have to create an instance of RemoteImageBuffer. RemoteImageBuffer offers the GraphicsContext which is backed by a DisplayList::Recorder. Usually the caller will not need the backend immediately because it has to draw first. At this time RemoteImageBufferProxy is created in GPU Process and it sends its ImageBufferHandle to RemoteImageBuffer. So this workflow works nicely with the common case of using ImageBuffer and at the same time it is asynchronous. So I am not sure why do we have to delay sending this message till the backend is needed? This should cause a delay while sending the message, waiting for the replay and creating the backend.

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:79
>> +void RemoteRenderingBackendProxy::commitApplyDisplayList(ImageBufferFlushIdentifier flushIdentifier, RemoteResourceIdentifier remoteResourceIdentifier)
> 
> "commitApply" is weird naming. Can this just use 'flush"?

Function was renamed commitFlushDisplayList().

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:41
>> +void RemoteResourceCacheProxy::createImageBuffer(const FloatSize& logicalSize, RenderingMode renderingMode, float resolutionScale, ColorSpace colorSpace, RemoteResourceIdentifier remoteResourceIdentifier)
> 
> Should the cache be the one to create the buffer? Normally you just add things to caches, not have the cache create things.

I was seeing RemoteResourceCacheProxy not only a resource cache but also a resource manager. But having the "Cache" in the name makes it confusing to make it create the objects as well. So I moved creating the ImageBuffers back to RemoteRenderingBackendProxy.

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:75
>> +        m_remoteRenderingBackendProxy.commitApplyDisplayList(flushIdentifier, remoteResourceIdentifier);
> 
> The mixture of "commit" and "flush" is confusing here.
> 
> Should the function on m_remoteRenderingBackendProxy be called didFlushDrawingForResource?

I moved this function back to RemoteRenderingBackendProxy.

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:79
>> +RefPtr<ImageData> RemoteResourceCacheProxy::getImageData(AlphaPremultiplication outputFormat, const IntRect& srcRect, RemoteResourceIdentifier remoteResourceIdentifier) const
> 
> This doesn't seem like it should live on the resource cache.

I moved this function back to RemoteRenderingBackendProxy.

Created attachment 411634 [details]
Patch

(In reply to Said Abou-Hallawa from comment #8)
> Comment on attachment 411199 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411199&action=review
> 
> >> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:76
> >> +    send(Messages::RemoteRenderingBackend::CreateRemoteImageBufferBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(handle), remoteResourceIdentifier), m_renderingBackendIdentifier);
> > 
> > Do we need separate IPC to create the remote buffer backend, or can we just create them when first referenced, and send the IDs in a reply?
> 
> When ImageBuffer::create() is called for a GPU based ImageBuffer, we have to
> create an instance of RemoteImageBuffer. RemoteImageBuffer offers the
> GraphicsContext which is backed by a DisplayList::Recorder. Usually the
> caller will not need the backend immediately because it has to draw first.
> At this time RemoteImageBufferProxy is created in GPU Process and it sends
> its ImageBufferHandle to RemoteImageBuffer. So this workflow works nicely
> with the common case of using ImageBuffer and at the same time it is
> asynchronous. So I am not sure why do we have to delay sending this message
> till the backend is needed? This should cause a delay while sending the
> message, waiting for the replay and creating the backend.

This is in GPUProcess/graphics/RemoteRenderingBackendProxy, so I assume it's sending IPC from GPU to WebProcess, which is confusing because the WebProcess should be the one creating buffer proxies which are then materialized in the GPU process.

> 
> >> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:79
> >> +void RemoteRenderingBackendProxy::commitApplyDisplayList(ImageBufferFlushIdentifier flushIdentifier, RemoteResourceIdentifier remoteResourceIdentifier)
> > 
> > "commitApply" is weird naming. Can this just use 'flush"?
> 
> Function was renamed commitFlushDisplayList().

Not ideal but we can wordsmith later.

> >> Source/WebKit/GPUProcess/graphics/RemoteResourceCacheProxy.cpp:41
> >> +void RemoteResourceCacheProxy::createImageBuffer(const FloatSize& logicalSize, RenderingMode renderingMode, float resolutionScale, ColorSpace colorSpace, RemoteResourceIdentifier remoteResourceIdentifier)
> > 
> > Should the cache be the one to create the buffer? Normally you just add things to caches, not have the cache create things.
> 
> I was seeing RemoteResourceCacheProxy not only a resource cache but also a
> resource manager. But having the "Cache" in the name makes it confusing to
> make it create the objects as well. So I moved creating the ImageBuffers
> back to RemoteRenderingBackendProxy.

Seems better.

Created attachment 411645 [details]
Patch

Comment on attachment 411199 [details]
Patch

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

>>>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:76
>>>> +    send(Messages::RemoteRenderingBackend::CreateRemoteImageBufferBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(handle), remoteResourceIdentifier), m_renderingBackendIdentifier);
>>> 
>>> Do we need separate IPC to create the remote buffer backend, or can we just create them when first referenced, and send the IDs in a reply?
>> 
>> When ImageBuffer::create() is called for a GPU based ImageBuffer, we have to create an instance of RemoteImageBuffer. RemoteImageBuffer offers the GraphicsContext which is backed by a DisplayList::Recorder. Usually the caller will not need the backend immediately because it has to draw first. At this time RemoteImageBufferProxy is created in GPU Process and it sends its ImageBufferHandle to RemoteImageBuffer. So this workflow works nicely with the common case of using ImageBuffer and at the same time it is asynchronous. So I am not sure why do we have to delay sending this message till the backend is needed? This should cause a delay while sending the message, waiting for the replay and creating the backend.
> 
> This is in GPUProcess/graphics/RemoteRenderingBackendProxy, so I assume it's sending IPC from GPU to WebProcess, which is confusing because the WebProcess should be the one creating buffer proxies which are then materialized in the GPU process.

RemoteRenderingBackendProxy messages were renamed: ImageBufferBackendWasCreated and FlushDisplayListWasCommitted.

<rdar://problem/70401828>

Created attachment 411656 [details]
Patch

Committed r268637: <https://trac.webkit.org/changeset/268637>

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