Bug 219558 - [GPU Process] Rename imageBuffer::flushDrawingContextAndCommit() to commitDrawingContext()
Summary: [GPU Process] Rename imageBuffer::flushDrawingContextAndCommit() to commitDra...
Status: RESOLVED DUPLICATE of bug 220126
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-04 15:40 PST by Said Abou-Hallawa
Modified: 2020-12-23 12:11 PST (History)
10 users (show)

See Also:


Attachments
Patch (5.45 KB, patch)
2020-12-04 15:42 PST, Said Abou-Hallawa
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2020-12-04 15:40:50 PST
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.
Comment 1 Said Abou-Hallawa 2020-12-04 15:42:42 PST
Created attachment 415468 [details]
Patch
Comment 2 Tim Horton 2020-12-04 15:58:48 PST
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.
Comment 3 Simon Fraser (smfr) 2020-12-04 16:08:32 PST
Comment on attachment 415468 [details]
Patch

I think we can improve these names.
Comment 4 Simon Fraser (smfr) 2020-12-04 16:14:07 PST
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?
Comment 5 Said Abou-Hallawa 2020-12-04 19:31:56 PST
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().
Comment 6 Radar WebKit Bug Importer 2020-12-11 15:41:14 PST
<rdar://problem/72241697>
Comment 7 Said Abou-Hallawa 2020-12-23 12:11:59 PST
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 ***