Bug 218401 – [GPU Process] Flush canvas displayList from doAfterUpdateRendering

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

[Rini Patel]

<rdar://problem/70891766>

[Rini Patel]

Created attachment 412791 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 412791 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +
> +        No new tests (OOPS!).

This needs some explanation for the change. Remove the "No new tests" line.

> Source/WebCore/html/HTMLCanvasElement.cpp:373
> +    if (m_context)
> +        addObserver(document());

I don't think you should do this unconditionally. You should only do this when GPU Process is active. In fact, you should only do this when GPU Process is active, and drawing has occurred since the last flush.

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:310
> +    bool needsPreparationForDisplay() const final { return true; }

This can't be unconditional. You should only return true when using the GPU backend. Ideally it would only return true if there is new drawing.

[Rini Patel]

Created attachment 412939 [details]
Patch

[Rini Patel]

(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 412791 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412791&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +
> > +        No new tests (OOPS!).
> 
> This needs some explanation for the change. Remove the "No new tests" line.
> 
> > Source/WebCore/html/HTMLCanvasElement.cpp:373
> > +    if (m_context)
> > +        addObserver(document());
> 
> I don't think you should do this unconditionally. You should only do this
> when GPU Process is active. In fact, you should only do this when GPU
> Process is active, and drawing has occurred since the last flush.
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:310
> > +    bool needsPreparationForDisplay() const final { return true; }
> 
> This can't be unconditional. You should only return true when using the GPU
> backend. Ideally it would only return true if there is new drawing.

We only flush from flushDrawingContextAndCommit() if there're new items in display list after the last flush. Should that be enough?

[Sam Weinig]

Comment on attachment 412939 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:375
> +#if ENABLE(GPU_PROCESS)
> +    if (m_context)
> +        addObserver(document());
> +#endif

It seems inherently wrong for WebCore to have to care about the GPU Process, so this #ifdef should really never be used in WebCore.

[Simon Fraser (smfr)]

Comment on attachment 412939 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:375
> +#if ENABLE(GPU_PROCESS)
> +    if (m_context)
> +        addObserver(document());
> +#endif

This needs to be a runtime check not a compile-time check, and should only happen when we make a remote image buffer.

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2051
> +    if (buffer && buffer->renderingResourceIdentifier())

I don't think checking for a renderingResourceIdentifier is the right approach. Maybe add ImageBuffer::flushContextAsync() and just call it always, then specialize the remote image buffer to flush the display list etc.

[Rini Patel]

Created attachment 412996 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 412996 [details]
Patch

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

Almost there! Let's do one more round.

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:310
> +    bool needsPreparationForDisplay() const final { return true; }

This needs to also consult the buffer's shouldEagerlyFlush(), otherwise you've changed the behavior at Document::canvasChanged().

> Source/WebCore/platform/graphics/ImageBuffer.h:71
> +    virtual bool shouldEagerlyFlush() { return false; }

I'm trying to think of a better name for this. How about prefersPreparationForDisplay().

[Rini Patel]

Created attachment 413079 [details]
Patch

[Rini Patel]

Okay. Changed the method name. I think this should be close to what we're looking for.

[Simon Fraser (smfr)]

Comment on attachment 413079 [details]
Patch

Let's wait for the Windows bot.

[Rini Patel]

Looks like it failed on first run and passed on retry.

[Rini Patel]

Is it concerning? 
(In reply to Rini Patel from comment #12)
> Looks like it failed on first run and passed on retry.

[Wenson Hsieh]

(In reply to Rini Patel from comment #13)
> Is it concerning? 
> (In reply to Rini Patel from comment #12)
> > Looks like it failed on first run and passed on retry.

The Windows test failure (fast/visual-viewport/resize-event-fired-window-resized.html) looks unrelated — we're probably okay.

[EWS]

Committed r269331: <https://trac.webkit.org/changeset/269331>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413079 [details].

[Radar WebKit Bug Importer]

<rdar://problem/71008643>