RESOLVED FIXED 218401
[GPU Process] Flush canvas displayList from doAfterUpdateRendering
https://bugs.webkit.org/show_bug.cgi?id=218401
Summary [GPU Process] Flush canvas displayList from doAfterUpdateRendering
Rini Patel
Reported 2020-10-30 12:56:44 PDT
Attachments
Patch (5.77 KB, patch)
2020-10-30 13:38 PDT, Rini Patel
no flags
Patch (6.12 KB, patch)
2020-11-02 10:27 PST, Rini Patel
no flags
Patch (6.06 KB, patch)
2020-11-02 19:27 PST, Rini Patel
no flags
Patch (6.29 KB, patch)
2020-11-03 11:19 PST, Rini Patel
no flags
Rini Patel
Comment 1 2020-10-30 13:38:39 PDT
Simon Fraser (smfr)
Comment 2 2020-10-30 13:45:13 PDT
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
Comment 3 2020-11-02 10:27:11 PST
Rini Patel
Comment 4 2020-11-02 10:33:56 PST
(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 5 2020-11-02 10:49:54 PST
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 6 2020-11-02 11:04:28 PST
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
Comment 7 2020-11-02 19:27:43 PST
Simon Fraser (smfr)
Comment 8 2020-11-03 09:52:52 PST
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
Comment 9 2020-11-03 11:19:11 PST
Rini Patel
Comment 10 2020-11-03 11:19:54 PST
Okay. Changed the method name. I think this should be close to what we're looking for.
Simon Fraser (smfr)
Comment 11 2020-11-03 11:51:12 PST
Comment on attachment 413079 [details] Patch Let's wait for the Windows bot.
Rini Patel
Comment 12 2020-11-03 12:34:31 PST
Looks like it failed on first run and passed on retry.
Rini Patel
Comment 13 2020-11-03 12:35:44 PST
Is it concerning? (In reply to Rini Patel from comment #12) > Looks like it failed on first run and passed on retry.
Wenson Hsieh
Comment 14 2020-11-03 12:37:03 PST
(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
Comment 15 2020-11-03 13:24:22 PST
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
Comment 16 2020-11-03 13:25:18 PST
Note You need to log in before you can comment on or make changes to this bug.