Bug 207109

Summary: Create a new ImageBuffer type for drawing on a DisplayList
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: CanvasAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, annulen, beidson, cdumez, commit-queue, dino, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, glenn, graouts, gyuyoung.kim, hta, jer.noble, jsbell, kondapallykalyan, noam, pdr, philipj, ryuan.choi, schenney, sergio, simon.fraser, thorton, tommyw, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 207048    
Bug Blocks: 204955, 207134    
Attachments:
Description Flags
Patch
none
Patch for review
none
Patch
none
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2020-02-03 00:11:04 PST
The drawing context will be the context of a display list. The ImageBuffer operations will ensure the recorded displaylist is replayed back before getting the pixels of the ImageBufferBackend.
Comment 1 Said Abou-Hallawa 2020-02-03 00:23:29 PST
Created attachment 389493 [details]
Patch
Comment 2 Said Abou-Hallawa 2020-02-03 07:41:37 PST
Created attachment 389506 [details]
Patch  for review
Comment 3 Said Abou-Hallawa 2020-02-18 20:27:50 PST
Created attachment 391136 [details]
Patch
Comment 4 Said Abou-Hallawa 2020-02-19 09:08:16 PST
Created attachment 391168 [details]
Patch
Comment 5 Tim Horton 2020-02-19 11:20:50 PST
Comment on attachment 391168 [details]
Patch

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

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.h:49
> +    const DisplayList* replayDisplayList() const { return m_replayDisplayList.get(); }

"replay" sounds like a verb, but it's just a getter
Comment 6 Said Abou-Hallawa 2020-02-19 12:23:17 PST
Comment on attachment 391168 [details]
Patch

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

>> Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.h:49
>> +    const DisplayList* replayDisplayList() const { return m_replayDisplayList.get(); }
> 
> "replay" sounds like a verb, but it's just a getter

Should I change it to replayedDisplayList()? The reason I chose this name is the caller is HTMLCanvasElement::replayDisplayListAsText() which is called from Internals::replayDisplayListForElement(). So I think if I am going to change the name of this function, I have to change all the callers with the prefix replayDisplayList to start with replayedDisplayList.
Comment 7 Simon Fraser (smfr) 2020-02-19 13:00:22 PST
Comment on attachment 391168 [details]
Patch

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

>>> Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.h:49
>>> +    const DisplayList* replayDisplayList() const { return m_replayDisplayList.get(); }
>> 
>> "replay" sounds like a verb, but it's just a getter
> 
> Should I change it to replayedDisplayList()? The reason I chose this name is the caller is HTMLCanvasElement::replayDisplayListAsText() which is called from Internals::replayDisplayListForElement(). So I think if I am going to change the name of this function, I have to change all the callers with the prefix replayDisplayList to start with replayedDisplayList.

replayedDisplayList is fine.

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.h:69
> +    void replayDisplayList(GraphicsContext& destContext)
> +    {
> +        if (!m_displayList.itemCount())
> +            return;
> +
> +        Replayer replayer(destContext, m_displayList);
> +        if (m_tracksDisplayListReplay)
> +            m_replayDisplayList = replayer.replay({ }, m_tracksDisplayListReplay);
> +        else
> +            replayer.replay();
> +        m_displayList.clear();
> +    }
> +

Add a .cpp file; I don't think this should be inline.

> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:35
> +class ImageBuffer : public ConcreteImageBuffer<BackendType> , public DrawingContext {

Extra space after >

Should DrawingContext be added via composition rather than inheritance?

> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:58
> +    ~ImageBuffer()
> +    {
> +        flushDrawingContext();
> +    }

Hmm, why flush? No-one can use the buffer after this. If flush triggers any callbacks on |this| we could have a bad time.

> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:60
> +    DrawingContext* drawingContext() override { return this; }

Weird.

> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:67
> +    GraphicsContext& context() const override
> +    {
> +        return DrawingContext::context();
> +    }
> +
> +    void flushDrawingContext() override

override -> final?
Comment 8 Said Abou-Hallawa 2020-02-19 13:15:49 PST
Comment on attachment 391168 [details]
Patch

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

>> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:35
>> +class ImageBuffer : public ConcreteImageBuffer<BackendType> , public DrawingContext {
> 
> Extra space after >
> 
> Should DrawingContext be added via composition rather than inheritance?

It can but this will make this class just a bridge to the DrawingContext public methods. I will try doing it.

>> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:58
>> +    }
> 
> Hmm, why flush? No-one can use the buffer after this. If flush triggers any callbacks on |this| we could have a bad time.

I hit an assertion in the destructor of GraphicsContext because ASSERT(m_stack.isEmpty()). This call fixed this assertion. I will try to find a way to flush the GraphicsContext without having to flush the DrawingContext.
Comment 9 Said Abou-Hallawa 2020-02-20 10:01:45 PST
Created attachment 391298 [details]
Patch
Comment 10 Said Abou-Hallawa 2020-02-20 10:31:28 PST
Comment on attachment 391168 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.h:49
>>>> +    const DisplayList* replayDisplayList() const { return m_replayDisplayList.get(); }
>>> 
>>> "replay" sounds like a verb, but it's just a getter
>> 
>> Should I change it to replayedDisplayList()? The reason I chose this name is the caller is HTMLCanvasElement::replayDisplayListAsText() which is called from Internals::replayDisplayListForElement(). So I think if I am going to change the name of this function, I have to change all the callers with the prefix replayDisplayList to start with replayedDisplayList.
> 
> replayedDisplayList is fine.

Done.

>> Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.h:69
>> +
> 
> Add a .cpp file; I don't think this should be inline.

Added.

>>> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:35
>>> +class ImageBuffer : public ConcreteImageBuffer<BackendType> , public DrawingContext {
>> 
>> Extra space after >
>> 
>> Should DrawingContext be added via composition rather than inheritance?
> 
> It can but this will make this class just a bridge to the DrawingContext public methods. I will try doing it.

Done.

>>> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:58
>>> +    }
>> 
>> Hmm, why flush? No-one can use the buffer after this. If flush triggers any callbacks on |this| we could have a bad time.
> 
> I hit an assertion in the destructor of GraphicsContext because ASSERT(m_stack.isEmpty()). This call fixed this assertion. I will try to find a way to flush the GraphicsContext without having to flush the DrawingContext.

I think calling flushDrawingContext() from the destructor is the right sequence. The destructor of HTMLCanvasElement calls setImageBuffer(nullptr) which nullifies m_contextStateSaver. The destructor of GraphicsContextStateSaver calls m_context.restore(). With the DisplayList::ImageBuffer, GraphicsContext::restore() will add a Restore element to the DisplayList. When setImageBuffer() nullifies the m_imageBuffer, DisplayList::ImageBuffer::~ImageBuffer() replays back the Restore item to the ImageBufferBackend which is equivalent to the case of the none DisplayList case.

>> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:60
>> +    DrawingContext* drawingContext() override { return this; }
> 
> Weird.

Fixed by having a member of type DrawingContext.

>> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:67
>> +    void flushDrawingContext() override
> 
> override -> final?

No because this method is going to be overridden by RemoteImageBuffer.
Comment 11 Simon Fraser (smfr) 2020-02-20 17:41:50 PST
Comment on attachment 391298 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:845
> +    if (buffer() && buffer()->drawingContext() && buffer()->drawingContext()->replayedDisplayList())
> +        return buffer()->drawingContext()->replayedDisplayList()->asText(flags);

Can we if (!buffer()) return and use a buffer ref for the rest of the function?

> Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.h:28
> +#include "AffineTransform.h"

Not needed?
Comment 12 Said Abou-Hallawa 2020-02-21 08:57:16 PST
Created attachment 391406 [details]
Patch
Comment 13 WebKit Commit Bot 2020-02-21 12:25:54 PST
The commit-queue encountered the following flaky tests while processing attachment 391406 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org)
The commit-queue is continuing to process your patch.
Comment 14 WebKit Commit Bot 2020-02-21 12:26:37 PST
Comment on attachment 391406 [details]
Patch

Clearing flags on attachment: 391406

Committed r257156: <https://trac.webkit.org/changeset/257156>
Comment 15 WebKit Commit Bot 2020-02-21 12:26:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2020-02-21 12:27:12 PST
<rdar://problem/59678215>
Comment 17 Said Abou-Hallawa 2020-03-03 17:04:50 PST
*** Bug 204803 has been marked as a duplicate of this bug. ***