WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
<
rdar://problem/70891766
>
Attachments
Patch
(5.77 KB, patch)
2020-10-30 13:38 PDT
,
Rini Patel
no flags
Details
Formatted Diff
Diff
Patch
(6.12 KB, patch)
2020-11-02 10:27 PST
,
Rini Patel
no flags
Details
Formatted Diff
Diff
Patch
(6.06 KB, patch)
2020-11-02 19:27 PST
,
Rini Patel
no flags
Details
Formatted Diff
Diff
Patch
(6.29 KB, patch)
2020-11-03 11:19 PST
,
Rini Patel
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Rini Patel
Comment 1
2020-10-30 13:38:39 PDT
Created
attachment 412791
[details]
Patch
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
Created
attachment 412939
[details]
Patch
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
Created
attachment 412996
[details]
Patch
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
Created
attachment 413079
[details]
Patch
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
<
rdar://problem/71008643
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug