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.
Created attachment 411031 [details] Patch
Created attachment 411032 [details] Patch for review
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?
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.
(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).
<rdar://problem/70413918>
Created attachment 411934 [details] Patch
Created attachment 411947 [details] Patch for review
Created attachment 412398 [details] Patch
Created attachment 412403 [details] Patch
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"?
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.
Committed r269065: <https://trac.webkit.org/changeset/269065> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412403 [details].
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?
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.