[Webkit-unassigned] [Bug 220649] [GPU Process] Make getImageData use display list with shared memory instead of sending a sync IPC

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=220649

--- Comment #8 from Ryosuke Niwa <rniwa at webkit.org> ---
(In reply to Said Abou-Hallawa from comment #7)
> Comment on attachment 418630 [details]
> Updated to use semaphore
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418630&action=review
> 
> > Source/WebCore/platform/graphics/ImageBuffer.h:131
> > +    virtual void copyImageData(void*, AlphaPremultiplication outputFormat, const IntRect& srcRect) const = 0;
> 
> It is unclear whether this ImageBuffer is the source or the destination of
> the copy operation. I would suggest make it an override of getImageData().

I'd rather avoid function overrides as they could be confusing. I'd probably call this function copyImageDataToBuffer or something.

> Also this call looks insecure since we do not know the size off the "void*"
> buffer. We use "void *" when calling private methods like copyImagePixels().
> But these calls for buffers that we allocate or we sure they are sufficient
> for the data to be be copied to or from. I would suggest passing the number
> of bytes of the destination buffer.

Well, you reviewed this way too early. There is a reason I didn't put this up for review.

> > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:304
> > +void RemoteRenderingBackend::getImageData(const WebCore::IntSize& size, CompletionHandler<void(const SharedMemory::IPCHandle& handle)>&& completionHandler)
> 
> No need to WebCore:: in "onst WebCore::IntSize& size".
> 
> Also what is the use of this function now after deleting the ImageBuffer
> renderingResourceIdentifier and the outputFormat? What does it exactly
> return in the completionHandler?

We need toe IPC to get a new shared memory buffer when the size of getImageData is larger than the current buffer size or the shared memory hasn't been allocated yet. I just re-used the old IPC to implement that. Again, a pre-emptive code review :)

> > Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:171
> > +    IntSize newSize = m_imageDataBufferSize.expandedTo(desiredSize);
> > +    SharedMemory::IPCHandle handle;
> > +    sendSync(Messages::RemoteRenderingBackend::GetImageData(newSize), Messages::RemoteRenderingBackend::GetImageData::Reply(handle), m_renderingBackendIdentifier, 1_s);
> > +
> > +    m_imageDataBuffer = SharedMemory::map(handle.handle, SharedMemory::Protection::ReadWrite);
> > +    m_imageDataBufferSize = newSize;
> 
> Can't we make managing the SharedMemory be handled completely by WebP? I
> think this scenario might work:
> 
> RemoteRenderingBackendProxy::getImageData():
>     Ensures the size of m_imageDataBuffer is sufficient for the request
> image data
>     Records getImageData() and passes the handle of m_imageDataBuffer in the
> DL item
>     waitForImageData().

No, you can't pass a shared memory in display list. There is no mechanism to share a new shared memory by sharing some bytes in a shared memory. We need to send it as a mach port attachment via IPC.

We also want to allocate this memory in GPU Process so that only GPU process will have read-write access to the memory while WebContent will have readonly access to it.

There is an alternative design which is for GPU content process to send a new shared memory buffer when it detects that the newly requested getImageData doesn't fit in the existing buffer but this is kind of tricky because then Web content process will now have to wait for both the semaphore and potentially an IPC to arrive. In practice, this would mean that there needs to be some data indicating that the new IPC is coming and then WebContent has to send a sync IPC to GPU Process. This will be a lot of back & forth between GPU & WebContent processes, which will incur extra overhead.

> > Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.h:134
> > +    RefPtr<SharedMemory> m_imageDataBuffer;
> > +    WebCore::IntSize m_imageDataBufferSize;
> 
> Why do we need to store the size of the SharedMemory in
> m_imageDataBufferSize? Can't we use m_imageDataBuffer->size() instead?

This was really for expedience to get things going but you're right that we don't really need it.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210213/76cbdb90/attachment.htm>