Summary: | [GPU Process] Enable Document based ImageBitmap and OffscreenCanvas drawing to use GPU Process rendering | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||||||
Component: | Canvas | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | alecflett, beidson, cdumez, changseok, dino, esprehn+autocc, ews-watchlist, gyuyoung.kim, jsbell, kbr, simon.fraser, webkit-bug-importer, wenson_hsieh | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=218482 | ||||||||||||||||||||
Bug Depends on: | 218264 | ||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2020-10-14 15:35:16 PDT
Created attachment 411382 [details]
Patch
Created attachment 411386 [details]
Patch
Created attachment 411389 [details]
Patch
Created attachment 411408 [details]
Patch
Created attachment 411416 [details]
Patch
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. 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. Created attachment 413003 [details]
Patch
Created attachment 413006 [details]
Patch
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 See bug 218482 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? Created attachment 413143 [details]
Patch
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. Committed r269381: <https://trac.webkit.org/changeset/269381> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413143 [details]. |