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
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?
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.
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.
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.
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.
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.
Comment on attachment 147845 [details] Patch Clearing flags on attachment: 147845 Committed r120634: <http://trac.webkit.org/changeset/120634>
All reviewed patches have been landed. Closing bug.