Bug 208597 – Canvas drawing commands have to be flushed to the GPUProcess in batches

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

[Said Abou-Hallawa]

For GPU canvas rendering, the drawing commands are recorded in a DisplayList. These drawing commands are flushed out to the backend in the GPUProcess only when the backend pixels are needed. Delaying the flushing makes us lose the parallelization that CoreGraphics does when the drawing commands are issued directly to the GraphicsContext.

Flushing the drawing commands earlier makes the commit time to get the pixels updated shorter. But a tradeoff has to be made between sending many messages and delaying the flushing. So we need to send the drawing commands to the GPU Process in "batches". The goal is to make CoreGraphics starts its work earlier but not to encode, decode and send many messages.

[Said Abou-Hallawa]

Created attachment 392479 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/60178756>

[Myles C. Maxfield]

Comment on attachment 392479 [details]
Patch

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

Elegant.

> Source/WebCore/platform/graphics/displaylists/DisplayListObserver.h:31
> +class Observer {

Calling this by the general name DisplayList::Observer seems misleading as it really only observes the recorder. Please consider either giving it a longer name, or making it an inner class (DisplayList::Recorder::Observer).

Personally, I prefer DisplayList::Recorder::Observer because it keeps the two classes close together, rather than being spread to two separate files.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBuffer.h:85
> +            RemoteImageBufferMessageHandler::flushDrawingContextAndWaitCommit(displayList);

I like the design of this better than what I was doing in https://bugs.webkit.org/show_bug.cgi?id=208560. I've updated that patch to match this design.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBuffer.h:94
> +        if (displayList.itemCount() >= DisplayListBatchSize) {

This doesn't really matter much, but shouldn't this condition be checked after the item gets appended? The action of appending the item is the trigger which can cause this condition to become true.

[Said Abou-Hallawa]

Created attachment 392945 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 392479 [details]
Patch

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

>> Source/WebCore/platform/graphics/displaylists/DisplayListObserver.h:31
>> +class Observer {
> 
> Calling this by the general name DisplayList::Observer seems misleading as it really only observes the recorder. Please consider either giving it a longer name, or making it an inner class (DisplayList::Recorder::Observer).
> 
> Personally, I prefer DisplayList::Recorder::Observer because it keeps the two classes close together, rather than being spread to two separate files.

I made Observer an inner class of DisplayList::Recorder.

>> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBuffer.h:94
>> +        if (displayList.itemCount() >= DisplayListBatchSize) {
> 
> This doesn't really matter much, but shouldn't this condition be checked after the item gets appended? The action of appending the item is the trigger which can cause this condition to become true.

I wanted to ensure there is a least one DisplayList::Item left after the batching flush context message is sent. This item will guarantee flushDrawingContextAndWaitCommit() is going to be called by flushDrawingContext(). If I make this condition be checked after the item gets appended and this item is the last item, we will need to call flushDrawingContextAndWaitCommit() even if displayList is empty.

[WebKit Commit Bot]

Comment on attachment 392945 [details]
Patch

Clearing flags on attachment: 392945

Committed r258104: <https://trac.webkit.org/changeset/258104>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.