The test has been consistently failing on our bots (with some flakiness though). Talking with David, he feels that the test makes the wrong assumption: hm, it looks like the unit test is wrong it assumes that drawLayersOnCCThread has been called when commitNumber has advanced Let's disable it until someone comes and fix it.
Disabled in http://trac.webkit.org/changeset/127013
The test is failing whenever two commits are performed in between two vsync ticks as this causes beginCommitOnCCThread to be called twice without any calls to drawLayersOnCCThread and the test seem to assumes that this can't happen.
(In reply to comment #2) > The test is failing whenever two commits are performed in between two vsync ticks as this causes beginCommitOnCCThread to be called twice without any calls to drawLayersOnCCThread and the test seem to assumes that this can't happen. But we should never do 2 commits within pair of vsyncs - that's a valid assumption to make.
(In reply to comment #3) > (In reply to comment #2) > > The test is failing whenever two commits are performed in between two vsync ticks as this causes beginCommitOnCCThread to be called twice without any calls to drawLayersOnCCThread and the test seem to assumes that this can't happen. > > But we should never do 2 commits within pair of vsyncs - that's a valid assumption to make. It's 2 commit completions that happen within a pair of vsync ticks and not necessarily 2 "begin frame" in case that makes any difference. If we should never do 2 commits, what's the mechanism in place that controls this?
Sorry, I still don't follow. Could you walk me through the scenario where we complete 2 commits during one vsync period? The mechanism to control this is the scheduler.
I'm going to revert http://trac.webkit.org/changeset/126956 and re-enable the test until we get to the bottom of this, this is also breaking other threaded tests. The very first comment on https://bugs.webkit.org/show_bug.cgi?id=94721 was "Be careful to not allow more than one commit per frame" and it seems like this broke exactly that. This will regress the threaded latency - we need to figure out how to get latency back without breaking the commit flow.
Ok, I understand now. Thanks for reverting that change, and sorry for breaking this and not paying attention to Nat's comment on bug 94721. (In reply to comment #5) > Sorry, I still don't follow. Could you walk me through the scenario where we complete 2 commits during one vsync period? This is the sequence I was seeing: 1. beginFrameComplete 2. scheduleUpdateMoreResource 3. enter vsync (nothing to draw as still updating resources) 4. leave vsync 5. updateResourcesComplete 6. commit 7. scheduleBeginFrame 8. beginFrameComplete 9. scheduleUpdateMoreResource 10. updateResourcesComplete 12. commit 13. enter vsync 14. draw 15. leave vsync If I understand this correctly we need to delay "7. scheduleBeginFrame" until after "14. draw", right? > > The mechanism to control this is the scheduler. I now see how we relied on vsync tick being the only time texture uploads can complete to maintain this behavior. I'll fix so https://bugs.webkit.org/show_bug.cgi?id=94721 doesn't break this 1-draw-per-commit behavior.
Another thought Nat had that might be even simpler would be to hold off on completing the commit until we've finished updating resources. Then the scheduler wouldn't have to know about this at all, it'd just be waiting for the beginFrameComplete until the texture uploader finished up.
David, if you want to gvc today, happy to do so. More and more, I'm thinking we should move updating completely out of the scheduler. Basically, have the proxy do all the uploading state managment, remove the "updating resources" state from the scheduler, and only tell the compositor the beginFrame is complete when we've done the updates. Wdyt?
(In reply to comment #7) > Ok, I understand now. Thanks for reverting that change, and sorry for breaking this and not paying attention to Nat's comment on bug 94721. > > (In reply to comment #5) > > Sorry, I still don't follow. Could you walk me through the scenario where we complete 2 commits during one vsync period? > > This is the sequence I was seeing: > > 1. beginFrameComplete > 2. scheduleUpdateMoreResource > 3. enter vsync > (nothing to draw as still updating resources) > 4. leave vsync > 5. updateResourcesComplete > 6. commit > 7. scheduleBeginFrame > 8. beginFrameComplete > 9. scheduleUpdateMoreResource > 10. updateResourcesComplete > 12. commit > 13. enter vsync > 14. draw > 15. leave vsync > > If I understand this correctly we need to delay "7. scheduleBeginFrame" until after "14. draw", right? Yes, exactly. After a commit completes we need to go into COMMIT_STATE_WAITING_FOR_FIRST_DRAW and stay there until we draw or are informed that we can't draw at all for some reason (not visible, for instance) and just skip ahead. If you think about what's going on this makes perfect sense - we've already done a lot of work to get a new frame ready on the main thread and on the impl thread and we need to make sure the user actually sees what happened. Imagine a WebGL game that in the animate call on the main thread queues up 10ms of GPU work every time. The scheduler needs to make sure that we run that (via beginFrame) and draw exactly one time each every frame. If we double up beginFrames, we'll drop a frame.
Here's a short update on the issue. The threaded compositor currently prevents more than one commit with resource updates per redraw. Commits without resource updates are not restricted in this way. We can have an infinite number of commits per redraw as long as they don't include resource updates. My current understanding is that this is wrong. I've created https://bugs.webkit.org/show_bug.cgi?id=95661 for tracking this issues and uploaded a patch that enforcing only one commit per draw. https://bugs.webkit.org/show_bug.cgi?id=94721 has also been updated to not allow more than one commit with resource updates per redraw.
All issues related to this are now fixed.