Bug 217735 – [GPU Process] Enable Document based ImageBitmap and OffscreenCanvas drawing to use GPU Process rendering

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Said Abou-Hallawa]

We need to create RemoteImageBuffer for ImageBitmap and OffscreenCanvas. Later we need to especially record drawing the ImageBitmap and OffscreenCanvas into another 2D canvas or RenderLayer such that the backend of the ImageBuffer is not accessed in the Web Process.

[Said Abou-Hallawa]

Created attachment 411382 [details]
Patch

[Said Abou-Hallawa]

Created attachment 411386 [details]
Patch

[Said Abou-Hallawa]

Created attachment 411389 [details]
Patch

[Said Abou-Hallawa]

Created attachment 411408 [details]
Patch

[Said Abou-Hallawa]

Created attachment 411416 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 411416 [details]
Patch

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

> Source/WebCore/html/ImageBitmap.cpp:84
> +    auto document = scriptExecutionContext.isDocument() ? &downcast<Document>(scriptExecutionContext) : nullptr;
> +    auto hostWindow = (document && document->view() && document->view()->root()) ? document->view()->root()->hostWindow() : nullptr;

ImageBitmap is:

Exposed=(Window,Worker),

so this needs to work in Workers too, where scriptExecutionContext.isDocument() will be false.

[Kenneth Russell]

Comment on attachment 411416 [details]
Patch

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

>> Source/WebCore/html/ImageBitmap.cpp:84
>> +    auto hostWindow = (document && document->view() && document->view()->root()) ? document->view()->root()->hostWindow() : nullptr;
> 
> ImageBitmap is:
> 
> Exposed=(Window,Worker),
> 
> so this needs to work in Workers too, where scriptExecutionContext.isDocument() will be false.

Yes - are there wpt or layout tests covering creation and use of ImageBitmaps on workers? If not, please add one.

[Radar WebKit Bug Importer]

<rdar://problem/70548262>

[Said Abou-Hallawa]

Created attachment 413003 [details]
Patch

[Said Abou-Hallawa]

Created attachment 413006 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 411416 [details]
Patch

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

>>> Source/WebCore/html/ImageBitmap.cpp:84
>>> +    auto hostWindow = (document && document->view() && document->view()->root()) ? document->view()->root()->hostWindow() : nullptr;
>> 
>> ImageBitmap is:
>> 
>> Exposed=(Window,Worker),
>> 
>> so this needs to work in Workers too, where scriptExecutionContext.isDocument() will be false.
> 
> Yes - are there wpt or layout tests covering creation and use of ImageBitmaps on workers? If not, please add one.

Getting the the worker ImageBitmap to create a remote ImageBuffer will need many changes in different areas of the code:

1. We need to have a way to create an ImageBuffer without having to have a HostWindow, for example PlatformStrategies::createImageBuffer().
2. The RemoteImageBufferProxy needs to be created on the main thread. WebProcess::ensureGPUProcessConnection() asserts  RELEASE_ASSERT(RunLoop::isMain());
3. Executing any operation on this RemoteImageBufferProxy has to be on the main thread also because it creates the WeakPtr<RemoteRenderingBackendProxy> on the main thread.

So I would suggest making this bug for the document based ImageBitmap. Enabling the GPU rendering for the worker ImageBitmap can be tracked by a separate bug.

These tests use ImageBitmaps on workers:
imported/w3c/web-platform-tests/html/canvas/element/imagebitmap/createImageBitmap-in-worker-transfer.html
imported/w3c/web-platform-tests/html/canvas/element/imagebitmap/createImageBitmap-serializable.html

[Said Abou-Hallawa]

See bug 218482

[Simon Fraser (smfr)]

Comment on attachment 413006 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [GPU Process] Enable Document based ImageBitmap and OffscreenCanvas drawing to go through GPU

"to go through GPU" -> "to use GPU Process rendering"

> Source/WebCore/html/ImageBitmap.cpp:84
> +    auto document = scriptExecutionContext.isDocument() ? &downcast<Document>(scriptExecutionContext) : nullptr;
> +    auto hostWindow = (document && document->view() && document->view()->root()) ? document->view()->root()->hostWindow() : nullptr;

Does this code ever run in a Worker thread? If so, this is very non-threadsafe.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3808
>      WebProcess::singleton().setUseGPUProcessForMedia(settings.useGPUProcessForMediaEnabled());
> +    WebProcess::singleton().setUseGPUProcessForCanvas(m_shouldRenderCanvasInGPUProcess);

Why are these two lines different?

[Said Abou-Hallawa]

Created attachment 413143 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 413006 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        [GPU Process] Enable Document based ImageBitmap and OffscreenCanvas drawing to go through GPU
> 
> "to go through GPU" -> "to use GPU Process rendering"

Title was changed.

>> Source/WebCore/html/ImageBitmap.cpp:84
>> +    auto hostWindow = (document && document->view() && document->view()->root()) ? document->view()->root()->hostWindow() : nullptr;
> 
> Does this code ever run in a Worker thread? If so, this is very non-threadsafe.

This code was cleaned up.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3808
>> +    WebProcess::singleton().setUseGPUProcessForCanvas(m_shouldRenderCanvasInGPUProcess);
> 
> Why are these two lines different?

This part was removed from this bug and is being tracked by bug 218549.

[EWS]

Committed r269381: <https://trac.webkit.org/changeset/269381>

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