Bug 219558 – [GPU Process] Rename imageBuffer::flushDrawingContextAndCommit() to commitDrawingContext()

• Component: Canvas
• Status: RESOLVED DUPLICATE

[Said Abou-Hallawa]

To make the name of replaying back the DisplayList functions consistent and more clear, we will adopt the following scheme:

-- submitDisplayList(): Replays back a DisplayList. If the replay back happens in GPUP, no message confirming the replay back will be sent to WebP.

-- commitDrawingContext(): Replays back the DisplayList of DrawingContext. If the replay back happens in GPUP, a message confirming the replay back will be sent to WebP. WebP will not be blocked till this message is sent.

-- flushDrawingContext(): Same as commitDrawingContext() but if the commit happens in GPUP, and WebP will wait for the confirming message.

[Said Abou-Hallawa]

Created attachment 415468 [details]
Patch

[Tim Horton]

Comment on attachment 415468 [details]
Patch

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

Regardless of which way you go, this is better than the current name.

> Source/WebCore/platform/graphics/ImageBuffer.h:82
> +    virtual void commitDrawingContext() { }
>      virtual void submitDisplayList(const DisplayList::DisplayList&) { }

should we go with submit, like submitDisplayList? That is what this is, right? Submit outstanding pending work? There's no synchronization here, so not sure about the "commit" word.

[Simon Fraser (smfr)]

Comment on attachment 415468 [details]
Patch

I think we can improve these names.

[Simon Fraser (smfr)]

Comment on attachment 415468 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        -- submitDisplayList(): Replays back a DisplayList. If the replay back
> +           happens in GPUP, no message confirming the replay back will be sent
> +           to WebP.

Can this just be submit()? The argument is a display list.

Does this ever involve IPC of the display list to the GPUP?

> Source/WebCore/ChangeLog:21
> +        -- commitDrawingContext(): Replays back the DisplayList of DrawingContext.
> +           If the replay back happens in GPUP, a message confirming the replay
> +           back will be sent to WebP. WebP will not be blocked till this message
> +           is sent.
> +
> +        -- flushDrawingContext(): Same as commitDrawingContext() but if the commit
> +           happens in GPUP, and WebP will wait for the confirming message.

If these are similar they should have similar names. Maybe flush() and flushSync()? And are we flushing display lists to the GPUP, flushing display lists in the GPUP, or doing CGContextFlush, or all of those things?

[Said Abou-Hallawa]

Comment on attachment 415468 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +           to WebP.
> 
> Can this just be submit()? The argument is a display list.
> 
> Does this ever involve IPC of the display list to the GPUP?

Yes. See RemoteImageBufferProxy::submitDisplayList() and RemoteRenderingBackendProxy::submitDisplayList().

>> Source/WebCore/ChangeLog:21
>> +           happens in GPUP, and WebP will wait for the confirming message.
> 
> If these are similar they should have similar names. Maybe flush() and flushSync()? And are we flushing display lists to the GPUP, flushing display lists in the GPUP, or doing CGContextFlush, or all of those things?

They does flush the DisplayList only to the GPU.

CGContextFlush() is called automatically in the GPUP side when you want the pixels of the backend. For example RemoteRenderingBackend::getImageData() calls RemoteImageBuffer::getImageData() which inherits the implementation from ConcreteImageBuffer. ConcreteImageBuffer::getImageData() eventually calls ImageBufferIOSurfaceBackend::flushContext() via ConcreteImageBuffer::flushContext().

[Radar WebKit Bug Importer]

<rdar://problem/72241697>

[Said Abou-Hallawa]

submitDisplayList() was deleted so we need to rename flushDrawingContextAndCommit() only. Renaming this function is part of the patch in bug 220126.

*** This bug has been marked as a duplicate of bug 220126 ***