Bug 207109 – Create a new ImageBuffer type for drawing on a DisplayList

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

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 389493 [details]
Patch

[Said Abou-Hallawa]

Created attachment 389506 [details]
Patch  for review

[Said Abou-Hallawa]

Created attachment 391136 [details]
Patch

[Said Abou-Hallawa]

Created attachment 391168 [details]
Patch

[Tim Horton]

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

[Said Abou-Hallawa]

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.

[Simon Fraser (smfr)]

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?

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 391298 [details]
Patch

[Said Abou-Hallawa]

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.

[Simon Fraser (smfr)]

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?

[Said Abou-Hallawa]

Created attachment 391406 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 391406 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 391406 [details]
Patch

Clearing flags on attachment: 391406

Committed r257156: <https://trac.webkit.org/changeset/257156>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59678215>

[Said Abou-Hallawa]

*** Bug 204803 has been marked as a duplicate of this bug. ***