Bug 208560 - [GPU Process] Implement CanvasRenderingContext2D.getImageData()
Summary: [GPU Process] Implement CanvasRenderingContext2D.getImageData()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-03 22:48 PST by Myles C. Maxfield
Modified: 2020-03-07 09:27 PST (History)
9 users (show)

See Also:


Attachments
WIP (4.69 KB, patch)
2020-03-03 22:48 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (5.93 KB, patch)
2020-03-03 22:52 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (15.95 KB, patch)
2020-03-04 14:44 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (17.39 KB, patch)
2020-03-04 15:16 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (16.45 KB, patch)
2020-03-04 15:50 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (16.39 KB, patch)
2020-03-04 16:53 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (16.40 KB, patch)
2020-03-05 19:24 PST, Myles C. Maxfield
sabouhallawa: review+
Details | Formatted Diff | Diff
WIP (10.29 KB, text/plain)
2020-03-06 19:50 PST, Myles C. Maxfield
no flags Details
Patch for committing (22.58 KB, patch)
2020-03-07 00:42 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2020-03-03 22:48:40 PST
[GPU Process] Implement CanvasRenderingContext2D.getImageData()
Comment 1 Myles C. Maxfield 2020-03-03 22:48:55 PST
Created attachment 392373 [details]
WIP
Comment 2 Myles C. Maxfield 2020-03-03 22:52:56 PST
Created attachment 392374 [details]
WIP
Comment 3 Myles C. Maxfield 2020-03-04 14:44:35 PST
Created attachment 392488 [details]
WIP
Comment 4 Myles C. Maxfield 2020-03-04 15:16:57 PST
Created attachment 392494 [details]
WIP
Comment 5 Radar WebKit Bug Importer 2020-03-04 15:48:45 PST
<rdar://problem/60060618>
Comment 6 Myles C. Maxfield 2020-03-04 15:50:59 PST
Created attachment 392498 [details]
Patch
Comment 7 Myles C. Maxfield 2020-03-04 16:53:48 PST
Created attachment 392511 [details]
Patch
Comment 8 Jon Lee 2020-03-05 11:01:14 PST
Comment on attachment 392511 [details]
Patch

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

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBuffer.h:89
> +    bool asyncronouslyFlushDrawingContext()

sp: asynchronously
Comment 9 Myles C. Maxfield 2020-03-05 19:24:30 PST
Created attachment 392659 [details]
Patch
Comment 10 Said Abou-Hallawa 2020-03-06 17:43:58 PST
Comment on attachment 392659 [details]
Patch

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

> Source/WebKit/GPUProcess/graphics/RemoteImageBufferMessageHandlerProxy.h:51
> +    virtual RefPtr<WebCore::ImageData> performGetImageData(WebCore::AlphaPremultiplication outputFormat, WebCore::IntRect srcRect) = 0;

const WebCore::IntRect&

> Source/WebKit/GPUProcess/graphics/RemoteImageBufferProxy.h:60
> +    RefPtr<WebCore::ImageData> performGetImageData(WebCore::AlphaPremultiplication outputFormat, WebCore::IntRect srcRect) override
> +    {
> +        return BaseConcreteImageBuffer::getImageData(outputFormat, srcRect);
> +    }

You could have the same name getImageData() instead of performGetImageData(). Just define RemoteImageBufferMessageHandlerProxy::getImageData() exactly the same as ImageBuffer::getImageData() is defined. Change the above one to be 

    RefPtr<WebCore::ImageData> getImageData(WebCore::AlphaPremultiplication outputFormat, const WebCore::IntRect& srcRect) const override
    {
        return BaseConcreteImageBuffer::getImageData(outputFormat, srcRect);
    }

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:108
> +            completionHandler(WebCore::IntSize(), IPC::SharedBufferDataReference());

Should not we do a better job here? Can't we return an ImageData with the required size with zero bytes?

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.messages.in:29
> +    CreateImageBuffer(WebCore::FloatSize logicalSize, WebCore::RenderingMode renderingMode, float resolutionScale, WebCore::ColorSpace colorSpace, WebKit::ImageBufferIdentifier imageBufferIdentifier)
> +    ReleaseImageBuffer(WebKit::ImageBufferIdentifier imageBufferIdentifier)
> +    FlushImageBufferDrawingContext(WebCore::DisplayList::DisplayList displayList, WebKit::ImageBufferFlushIdentifier flushIdentifier, WebKit::ImageBufferIdentifier imageBufferIdentifier)
> +    GetImageData(enum:uint8_t WebCore::AlphaPremultiplication outputFormat, WebCore::IntRect srcRect, WebKit::ImageBufferIdentifier imageBufferIdentifier) -> (WebCore::IntSize size, IPC::SharedBufferDataReference data) Synchronous

Is there a reason for removing the void? I am asking because Tim added asked me to add them verbally and I did not ask why.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBuffer.h:89
> +    bool asynchronouslyFlushDrawingContext()

asynchronously is a little bit confusing since the message is asynchronous message anyway. Can't we keep this function flushDrawingContext() and name the other flushDrawingContextAndWait()?

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferMessageHandler.cpp:77
> +    return ImageData::create(size, Uint8ClampedArray::create(reinterpret_cast<const uint8_t*>(data.data()), data.size()));

The ImageData should be encoded and decoded as one unit. right?
Comment 11 Myles C. Maxfield 2020-03-06 19:50:07 PST
Created attachment 392842 [details]
WIP
Comment 12 Myles C. Maxfield 2020-03-07 00:42:37 PST
Created attachment 392855 [details]
Patch for committing
Comment 13 WebKit Commit Bot 2020-03-07 01:28:12 PST
Comment on attachment 392855 [details]
Patch for committing

Clearing flags on attachment: 392855

Committed r258069: <https://trac.webkit.org/changeset/258069>
Comment 14 Jer Noble 2020-03-07 09:27:35 PST
Follow up unified-build fix: Committed r258072: <https://trac.webkit.org/changeset/258072>