Bug 217566

Summary:

[GPU Process][Resource caching 4/7]: Implement DisplayList::DrawImageBuffer item

Product:

WebKit

Reporter:

Said Abou-Hallawa <sabouhallawa>

Component:

Layout and Rendering

Assignee:

Said Abou-Hallawa <sabouhallawa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

annulen, bfulgham, eric.carlson, ews-watchlist, glenn, gyuyoung.kim, jer.noble, mmaxfield, philipj, ryuan.choi, sergio, simon.fraser, webkit-bug-importer, wenson_hsieh, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

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

Bug Depends on:

217558

Bug Blocks:

217342, 217573

Attachments:

Description	Flags
Patch	none
Patch for review	none
Patch	none
Patch for review	none
Patch	none
Patch	none

Said Abou-Hallawa

Reported 2020-10-10 21:55:09 PDT

For in-process drawing there is no need to record drawing an ImageBuffer to a GraphicsContext. The current implementation creates a NativeImage from the ImageBuffer and draw this NativeImage to the GraphicsContext. So we end up recording a DrawNativeImage item instead. But for remote drawing, drawing a RemoteImageBuffer is an expensive operation unless creating the NativeImage and drawing it to the remote client is moved to the GPU Process. To do that, we need to record the RemoteResourceIdentifier of the ImageBuffer and send it to the GPU Process.

Attachments
Patch (131.66 KB, patch) 2020-10-10 21:57 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch for review (39.98 KB, patch) 2020-10-10 22:40 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (85.14 KB, patch) 2020-10-20 16:22 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch for review (34.23 KB, patch) 2020-10-20 18:37 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (37.43 KB, patch) 2020-10-27 00:44 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (38.02 KB, patch) 2020-10-27 02:00 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Said Abou-Hallawa

Comment 1 2020-10-10 21:57:38 PDT

Created attachment 411031 [details] Patch

Said Abou-Hallawa

Comment 2 2020-10-10 22:40:40 PDT

Created attachment 411032 [details] Patch for review

Simon Fraser (smfr)

Comment 3 2020-10-12 10:36:54 PDT

Comment on attachment 411032 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=411032&action=review > Source/WebCore/ChangeLog:8 > + The sequence in the Web Process is the following: I'm a bit confused by what you're enabling in this patch. Is this an ImageBuffer whose contents are a display list? Didn't Wenson already do something like that?

Said Abou-Hallawa

Comment 4 2020-10-12 13:45:31 PDT

Comment on attachment 411032 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=411032&action=review >> Source/WebCore/ChangeLog:8 >> + The sequence in the Web Process is the following: > > I'm a bit confused by what you're enabling in this patch. Is this an ImageBuffer whose contents are a display list? Didn't Wenson already do something like that? I am fixing the scenario: <body> <script> const canvas1 = document.createElement("canvas"); canvas1.width = 100; canvas1.height = 100; const ctx1 = canvas1.getContext('2d'); ctx1.fillStyle = "green"; ctx1.fillRect(0, 0, 100, 100); const canvas2 = document.createElement("canvas"); canvas2.style.cssText = "width: 200px; height: 200px; border: 1px solid red"; canvas2.width = 200; canvas2.height = 200; document.body.appendChild(canvas2); const ctx2 = canvas2.getContext('2d'); ctx2.drawImage(canvas1, 50, 50, 100, 100); </script> </body> The current implementation draws a canvas into another canvas by the following steps: 1. GraphicsContext::drawImageBuffer() asks the RemoteImageBuffer to draw itself into a NativeImage. Then its draws this NativeImage to the context of the RemoteImageBuffer. 2. GraphicsContext::drawNativeImage() records a DrawNativeImage(). The NativeImage will be encoded to a ShareableBitmap and it will be sent to GPU when encoding the DisplayList. 3. The GPU Process decodes the ShareableBitmap into a NativeImage. 4. DrawNativeImage is replayed back and the nativeImage is drawn to the backend of RemoteImageBufferProxy. With this patch this is what is going to happen: 1. GraphicsContext::drawImageBuffer() will ask DisplayList::Recorder if it can record the DrawImageBuffer which will ask its delegate. The delegate in this case is RemoteImageBuffer. RemoteImageBuffer will ask the RemoteResourceCache through the RemoteRenderingBackend. If the ImageBuffer is remote, it will record the DrawImageBuffer with the RemoteResourceIdentifier. 2. When replaying back DrawImageBuffer in GPU process, the DisplayList::replayer will ask its delegate which is RemoteImageBufferProxy. RemoteImageBufferProxy::apply() will delegate the call to RemoteResourceCacheProxy through the RemoteRenderingBackendProxy. RemoteResourceCacheProxy will resolve the RemoteResourceIdentifier to an ImageBuffer and calls GraphicsContext::drawImageBuffer() 3. GraphicsContext::drawImageBuffer() will make ImageBuffer draw itself into a NativeImage and call GraphicsContext::drawNativeImage() 4. GraphicsContext::drawNativeImage() will draw the NativeImage to the context of the backend. So with patch, no NativeImage encoding/decoding is involved. And more importantly no access to the backend of the RemoteImageBuffer happens in the Web Process.

Wenson Hsieh

Comment 5 2020-10-12 14:23:24 PDT

(In reply to Simon Fraser (smfr) from comment #3) > Comment on attachment 411032 [details] > Patch for review > > View in context: > https://bugs.webkit.org/attachment.cgi?id=411032&action=review > > > Source/WebCore/ChangeLog:8 > > + The sequence in the Web Process is the following: > > I'm a bit confused by what you're enabling in this patch. Is this an > ImageBuffer whose contents are a display list? Didn't Wenson already do > something like that? If you're referring to the ClipToImageBuffer bug, we fixed that by adding a ClipToDrawingCommands command instead and having the call site use that (i.e. refactor the code so that the call site does its drawing on a separate compatible GraphicsContext, created by the GraphicsContext).

Radar WebKit Bug Importer

Comment 6 2020-10-17 21:56:18 PDT

<rdar://problem/70413918>

Said Abou-Hallawa

Comment 7 2020-10-20 16:22:17 PDT

Created attachment 411934 [details] Patch

Said Abou-Hallawa

Comment 8 2020-10-20 18:37:20 PDT

Created attachment 411947 [details] Patch for review

Said Abou-Hallawa

Comment 9 2020-10-27 00:44:53 PDT

Created attachment 412398 [details] Patch

Said Abou-Hallawa

Comment 10 2020-10-27 02:00:59 PDT

Created attachment 412403 [details] Patch

Simon Fraser (smfr)

Comment 11 2020-10-27 10:15:31 PDT

Comment on attachment 412403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412403&action=review > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h:62 > + RemoteResourceClientsHashMap m_lockedRemoteResourceForRemoteClients; > + RemoteResourceClientsHashMap m_pendingUnlockRemoteResourceForRemoteClients; Don't these need to be counted, since a single item could be referenced by multiple clients? Can we simplify to avoid the need for "locked" and "pending unlock"?

Said Abou-Hallawa

Comment 12 2020-10-27 11:43:25 PDT

Comment on attachment 412403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412403&action=review >> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h:62 >> + RemoteResourceClientsHashMap m_pendingUnlockRemoteResourceForRemoteClients; > > Don't these need to be counted, since a single item could be referenced by multiple clients? > > Can we simplify to avoid the need for "locked" and "pending unlock"? These are HashMaps from a RenderingResourceIdentifier to a HashSet<RenderingResourceIdentifier>. So it is one-to-many relationship: resource -> clients. Every time a resource is referenced by a remote client, the RenderingResourceIdentifier of the remote client is added to the HashSet of this resource. The resource is released when its HashSet is empty. Please see the GraphicsContext::drawConsumingImageBuffer() above. If an source ImageBuffer is drawing-consuming into a destination ImageBuffer, the source ImageBuffer will be deleted before the destination ImageBuffer flushes its DrawingContext. For the drawConsumingImageBuffer() scenario this is what happens. 1. bufferDest.context().drawConsumingImageBuffer(WTFMove(bufferSrc)...) is called. 2. DisplayList::Recorder::drawImageBuffer() gets called which calls lockRemoteImageBuffer() of its delegate. 3. RemoteImageBufferProxy::lockRemoteImageBuffer() gets called because it's a DisplayList::Recorder::Delegate. 4. RemoteResourceCacheProxy::lockRemoteImageBufferForRemoteClient() gets called. 5. The relationship bufferSrc -> bufferDest is added to m_lockedRemoteResourceForRemoteClients. 6. A DrawImageBuffer item is created and GraphicsContext::drawConsumingImageBuffer() exists. 7. The destructor of RemoteImageBufferProxy for bufferSrc is called. 8. RemoteResourceCacheProxy::releaseImageBuffer() gets called. 9. RemoteResourceCacheProxy::releaseRemoteResource() is called which realizes that the resource is locked. 10. So it moves the entry of bufferSrc from m_lockedRemoteResourceForRemoteClients to m_pendingUnlockRemoteResourceForRemoteClients. 11. Eventually RemoteImageBufferProxy::flushDrawingContext() is called for destBuffer. 12.RemoteResourceCacheProxy::unlockRemoteResourcesForRemoteClient() is called. The entry of the RenderingResourceIdentifier of bufferDest is removed from m_lockedRemoteResourceForRemoteClients and m_pendingUnlockRemoteResourceForRemoteClients. 13. All the entries in m_pendingUnlockRemoteResourceForRemoteClients whose HashSets are empty will be released from the GPU.

EWS

Comment 13 2020-10-27 12:53:27 PDT

Committed r269065: <https://trac.webkit.org/changeset/269065> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412403 [details].

Myles C. Maxfield

Comment 14 2020-11-02 22:07:25 PST

Comment on attachment 412403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412403&action=review > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:71 > + lockRemoteResourceForRemoteClient(imageBuffer.renderingResourceIdentifier(), remoteClientIdentifier); I don't quite understand how this works. The destructor of the ImageBuffer (presumably the RemoteImageBufferProxy subclass) sends the ReleaseRemoteResource message, but this lock operation doesn't retain the ImageBuffer. Doesn't this mean that the ImageBuffer could be deleted from the GPU Process before the GPU Process receives the DrawImageBuffer DL item?

Myles C. Maxfield

Comment 15 2020-11-02 22:08:58 PST

Comment on attachment 412403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412403&action=review >> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:71 >> + lockRemoteResourceForRemoteClient(imageBuffer.renderingResourceIdentifier(), remoteClientIdentifier); > > I don't quite understand how this works. The destructor of the ImageBuffer (presumably the RemoteImageBufferProxy subclass) sends the ReleaseRemoteResource message, but this lock operation doesn't retain the ImageBuffer. Doesn't this mean that the ImageBuffer could be deleted from the GPU Process before the GPU Process receives the DrawImageBuffer DL item? Oh, never mind, you explain this in https://bugs.webkit.org/show_bug.cgi?id=217566#c12.

Note You need to log in before you can comment on or make changes to this bug.