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.
Created attachment 161855 [details] Patch Unit tests and change that prevents all types of commits from happening twice between vsync ticks
Comment on attachment 161855 [details] Patch Attachment 161855 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13733388
Comment on attachment 161855 [details] Patch Attachment 161855 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13734298
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?
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.
Created attachment 161856 [details] Patch Only call setCanBeginFrame(true) when no more resource updates
Comment on attachment 161856 [details] Patch Attachment 161856 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13726920
Comment on attachment 161856 [details] Patch Attachment 161856 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13741013
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.
Created attachment 162622 [details] Patch Use COMMIT_STATE_WAITING_FOR_FIRST_DRAW to enforce a draw after each commit.
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
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.
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
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.
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?
Created attachment 162894 [details] Patch Always update commit state to COMMIT_STATE_WAITING_FOR_FIRST_DRAW from ACTION_COMMIT
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
Created attachment 162896 [details] Patch Fix changelog
Comment on attachment 162896 [details] Patch Clearing flags on attachment: 162896 Committed r127940: <http://trac.webkit.org/changeset/127940>
All reviewed patches have been landed. Closing bug.