[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 #7 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 418630
  --> https://bugs.webkit.org/attachment.cgi?id=418630
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().

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.

> Source/WebCore/platform/graphics/ImageBufferBackend.cpp:262
> +void ImageBufferBackend::copyImageData(void* destData, AlphaPremultiplication outputFormat, const IntRect& srcRect, void* srcData) const

ImageBufferBackend::getImageData() above can be written in in terms of this function.

RefPtr<ImageData> ImageBufferBackend::getImageData(AlphaPremultiplication outputFormat, const IntRect& srcRect, void* data) const
{
    auto imageData = ImageData::create(srcRectScaled.size());
    copyImageData(imageData->data()->data(), outputFormat, srcRect, data);
    return imageData;
}

> Source/WebCore/platform/graphics/cairo/ImageBufferCairoSurfaceBackend.cpp:128
> +RefPtr<ImageData> ImageBufferCairoSurfaceBackend::copyImageData(void* buffer, AlphaPremultiplication outputFormat, const IntRect& srcRect) const

The return of this function is "void". And its prototype is not defined in the file ImageBufferCairoSurfaceBackend.h

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

> 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().
    Creates an ImageData and copies from m_imageDataBuffer to this new ImageData.

RemoteRenderingBackend::getImageData()
    Maps the handle of the SharedMemory in GetImageData to a SharedMemory
    Calls copyImageData()
    Signal the semaphore.

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

-- 
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/20210128/ea316cf3/attachment-0001.htm>