Bug 87766 – [chromium] Ensure that skipping frames during an accelerated animation doesn't cause starvation

• Component: WebKit Misc.
• Status: RESOLVED FIXED

When an accelerated animation is about to checkerboard, we currently drop frames*, but we are in danger of starvation. We should put a limit on the number of frames that we drop.

*MT compositor only. This patch will enable dropping frames in single threaded mode.

Created attachment 144599 [details]
Patch

[Dana Jansens]

Comment on attachment 144599 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:648
> +        drawFrame = m_layerTreeHostImpl->prepareToDraw(frame);

why do forced frames not count as non-skipped frames?

[Nat Duca]

Comment on attachment 144599 [details]
Patch

This should be a CCSchedulerStateMachine option. When prepareToDraw doesn't create a frame, you should bump a counter. When that is >k you should forcedraw.

[Nat Duca]

s/option/capability/

There shouldn't be any changes to the proxy.

Created attachment 144831 [details]
Patch

(In reply to comment #3)
> (From update of attachment 144599 [details])
> This should be a CCSchedulerStateMachine option. When prepareToDraw doesn't create a frame, you should bump a counter. When that is >k you should forcedraw.
Done.

Also, this patch will no longer turn on dropping frames in single threaded mode.

[Nat Duca]

Comment on attachment 144831 [details]
Patch

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

Can you piggy back or unify this code with the m_lastFrameNumberWhereDrawWasCalled? I'm confused about what the hell that code does anyway.

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:30
> +static const int maximumNumberOfFailedDrawsBeforeForce = 8;

thats a hitch of 8*16ms = 133ms. You sure you want to wait that long?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:79
> +    virtual void prepareToDrawOnCCThread(CCLayerTreeHostImpl*, bool& shouldForceFailure) { }

This hook is pretty confusing to me... why not make this return a bool (default to true) and make the real function down around line 120 return the value here.

Created attachment 144860 [details]
Patch

(In reply to comment #6)
> (From update of attachment 144831 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144831&action=review
>
> Can you piggy back or unify this code with the m_lastFrameNumberWhereDrawWasCalled? I'm confused about what the hell that code does anyway.
That won't work here, unfortunately. m_lastFrameNumberWhereDrawWasCalled is reset to -1 every commit, but it's certainly possible to continue to checkerboard before and after a commit, so I think we'll need another counter.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:30
> > +static const int maximumNumberOfFailedDrawsBeforeForce = 8;
>
> thats a hitch of 8*16ms = 133ms. You sure you want to wait that long?
Yeah, that is a bit too long. Switched to 3 frames.
>
> > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:79
> > +    virtual void prepareToDrawOnCCThread(CCLayerTreeHostImpl*, bool& shouldForceFailure) { }
>
> This hook is pretty confusing to me... why not make this return a bool (default to true) and make the real function down around line 120 return the value here.
Done.

Created attachment 145126 [details]
Patch

I've discussed m_lastFrameNumberWhereDrawWasCalled with Dana and concluded that
it can't be used in this patch, nor can we share state to accomplish our goals.

Dana's goal: detect if we've already attempted to draw this frame (so that we don't try twice).

My goal: count the number of failed draws (to eventually force a draw).

We need different state to accomplish these two goals. Another point that came
up in our discussion was that when it comes time to force a draw, it doesn't
makes sense to try drawing until the next commit when we have, hopefully, some
new textures. So instead of setting m_needsForcedDraw, I instead set a new
member m_needsForcedDrawAfterNextCommit, which does what you'd expect.

[Nat Duca]

Comment on attachment 145126 [details]
Patch

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

I buy that. Thanks for digging into this, I know it was moar work, I really appreciate it.

This patch LGTM but you need ccschedulerstatemachine tests.

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:192
> +            m_needsForcedRedrawAfterNextCommit = false;

what happens if we don't have a commit pending? Just thinking out loud... I think we're fine?

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:275
>      if (m_drawIfPossibleFailed) {

WebKit style bot wonders if we restructure this branch to say

if (success) { do stuff and return; }
... the stuff we do if it failed...

Dunno

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1277
> +

You should have ccschedulerstatemachine tests too.

We have a test for the starvation cases, including the one that moviated adding the "After next commit" state.

[Dana Jansens]

Comment on attachment 145126 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:277
>          m_needsCommit = true;

This promises that we have a commit pending when we fail a frame :) (Aside: which I think makes the call in CCLTH::prepareToDraw redundant)

Created attachment 147845 [details]
Patch

Added the CCSchedulerStateMachineTest.

[WebKit Review Bot]

Comment on attachment 147845 [details]
Patch

Clearing flags on attachment: 147845

Committed r120634: <http://trac.webkit.org/changeset/120634>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.