Bug 95661 – [Chromium] Multiple commits without invalidation allowed per redraw.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Reveman]

There's no mechanism in place to prevent multiple commits per redraw. Multiple commits with resource updates are prevented as a side-effect of how the current incremental resource update mechanism works.

[David Reveman]

Created attachment 161855 [details]
Patch

Unit tests and change that prevents all types of commits from happening twice between vsync ticks

[WebKit Review Bot]

Comment on attachment 161855 [details]
Patch

Attachment 161855 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13733388

[Peter Beverloo (cr-android ews)]

Comment on attachment 161855 [details]
Patch

Attachment 161855 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13734298

[David Reveman]

The current patch breaks some existing unit tests but I'll fix that before landing. The important part of the patch is the new unit tests. My current understanding is that all these tests should pass and the change included enforces that. Is that correct?

[James Robinson]

Comment on attachment 161855 [details]
Patch

Setting this bit on the scheduler feels redundant with the state transitions we already have.  We should go from commit -> waiting for draw -> draw -> ready for commit again.  How are we falling out of waiting_for_first_draw without drawing?

Why 50 in the unit tests?  Is there anything in particular interesting about that number, or is it meant as a blind fuzz test of sorts?  It doesn't really feel appropriate as a unit test.

[David Reveman]

Created attachment 161856 [details]
Patch

Only call setCanBeginFrame(true) when no more resource updates

[WebKit Review Bot]

Comment on attachment 161856 [details]
Patch

Attachment 161856 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13726920

[Peter Beverloo (cr-android ews)]

Comment on attachment 161856 [details]
Patch

Attachment 161856 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13741013

[James Robinson]

Comment on attachment 161856 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:94
>      m_stateMachine.beginFrameComplete();
> +    m_stateMachine.setCanBeginFrame(false);

These still seem redundant to me. Telling the state machine beginFrameComplete should put us into the waiting for draw state.  Once we get into this state, how are we returning ACTION_BEGIN_FRAME before we actually draw?  I think fixing that will fix this class of bugs without needing redundant state sets.

[David Reveman]

Created attachment 162622 [details]
Patch

Use COMMIT_STATE_WAITING_FOR_FIRST_DRAW to enforce a draw after each commit.

[James Robinson]

Comment on attachment 162622 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:209
> +        if (!m_needsForcedCommit)
>              m_commitState = COMMIT_STATE_WAITING_FOR_FIRST_DRAW;
>          else
>              m_commitState = COMMIT_STATE_IDLE;

After ACTION_COMMIT you should unconditionally enter COMMIT_STATE_WAITING_FOR_FIRST_DRAW.  If we need a forced commit, we'll go ahead and emit ACTION_BEGIN_FRAME even if we're in COMMIT_STATE_WAITING_FOR_FIRST_DRAW.

I can't figure out for the life of me what the conditions "(m_needsCommit || !m_visible) && !m_needsForcedCommit" were supposed to do

[Nat Duca]

Comment on attachment 162622 [details]
Patch

Lets give this a try. I really think we need to do a patch to hoist the impl-side "dont actually commit now" out of the scheduler completely, but thats for another day.

[James Robinson]

Comment on attachment 162622 [details]
Patch

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

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:2915
> +        if (m_numDidCommit < 50) {

this is not a good unit test. a unit test should test for a specific case, not just blindly loop a bunch.  run through the states you care about and then end the test - you definitely don't care about 50 different states

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:2972
> +        if (m_numAnimate < 50) {

same here. make the test specific

[David Reveman]

Created attachment 162873 [details]
Patch

Check number of commits between first and second draw instead of ending the tests after a random number of commits.

[James Robinson]

Comment on attachment 162873 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:209
> +        if (!m_needsForcedCommit)
>              m_commitState = COMMIT_STATE_WAITING_FOR_FIRST_DRAW;
>          else
>              m_commitState = COMMIT_STATE_IDLE;

Why didn't you fix this?

[David Reveman]

Created attachment 162894 [details]
Patch

Always update commit state to COMMIT_STATE_WAITING_FOR_FIRST_DRAW from ACTION_COMMIT

[James Robinson]

Comment on attachment 162894 [details]
Patch

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

Great!

> Source/WebCore/ChangeLog:9
> +        Enter COMMIT_STATE_WAITING_FOR_FIRST_DRAW after commit unless we need
> +        a forced commit. This ensures we draw before beginning a new frame.

update this - I think the "unless..." is stale now

[David Reveman]

Created attachment 162896 [details]
Patch

Fix changelog

[WebKit Review Bot]

Comment on attachment 162896 [details]
Patch

Clearing flags on attachment: 162896

Committed r127940: <http://trac.webkit.org/changeset/127940>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.