Summary: | [Chromium] Fix vsyncTick drought when vsync throttling is disabled | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brian Anderson <brianderson> | ||||||||||||||
Component: | New Bugs | Assignee: | Brian Anderson <brianderson> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | cc-bugs, darin, dglazkov, jamesr, jbates, nduca, reveman, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 84281 | ||||||||||||||||
Attachments: |
|
Description
Brian Anderson
2012-08-14 12:03:23 PDT
Created attachment 158390 [details]
Patch
Created attachment 158391 [details]
Patch
Comment on attachment 158391 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158391&action=review > Source/WebCore/ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). You'll have to do something about this patch or an svn presubmit check will barf on the (OOPS!). A brief description of what the bug was and how this fixes it would be good. > Source/WebCore/ChangeLog:10 > + No new tests (OOPS!). Can we have tests? Wanted to get this up quickly to share and get feedback. Will provide a better description in next patch. Definitely planning to add a test that will cause a vsyncTick drought if not handled properly. Comment on attachment 158391 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158391&action=review > Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:130 > + if (prevFramesPending == m_numFramesPending) { > + // We must manually tick if we aren't expecting a swap ack. > + postManualTick(); > + } else if (m_numFramesPending < m_maxFramesPending) { > + // We do expect a swap ack in this case, but manually tick > + // anyway to fill the swap queue up to m_maxFramesPending. > + postManualTick(); > + } Looks like these two could be combined to just: if (m_numFramesPending < m_maxFramesPending) postManualTick(); (If prevFramesPending == m_maxFramesPending, we would have dropped out at line 114.) I don't think we should support the m_maxFramesPending == 0 case. Comment on attachment 158391 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158391&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:130 >> + } > > Looks like these two could be combined to just: > if (m_numFramesPending < m_maxFramesPending) > postManualTick(); > > (If prevFramesPending == m_maxFramesPending, we would have dropped out at line 114.) > > I don't think we should support the m_maxFramesPending == 0 case. Good point. Will change. Created attachment 158426 [details]
Patch
Comment on attachment 158426 [details] Patch Attachment 158426 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13505112 New failing tests: CCLayerTreeHostTestFractionalScroll.runMultiThread CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread Canvas2DLayerBridgeTest.testFullLifecycleSingleThreadedDeferred CCLayerTreeHostTestEvictTextures.runMultiThread Created attachment 158454 [details]
Archive of layout-test-results from gce-cr-linux-01
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 158426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158426&action=review > Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:120 > + if (!m_isTimeSourceThrottling && m_numFramesPending < m_maxFramesPending) If you have numFrames=1 then you'll get an ack and you'll tick. This causes a double tick? Comment on attachment 158426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158426&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:120 >> + if (!m_isTimeSourceThrottling && m_numFramesPending < m_maxFramesPending) > > If you have numFrames=1 then you'll get an ack and you'll tick. This causes a double tick? The current goal is to attempt to go m_maxFramesPending deep if vsync is disabled. I was hoping to get some extra concurrency that way and wasn't worried about latency since vsync is disabled. Should I be? Maybe we should have a m_targetFramesPending that is independent of m_maxFramesPending. (In reply to comment #11) > (From update of attachment 158426 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158426&action=review > > >> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:120 > >> + if (!m_isTimeSourceThrottling && m_numFramesPending < m_maxFramesPending) > > > > If you have numFrames=1 then you'll get an ack and you'll tick. This causes a double tick? Same as old --disable-gpu-vsync behavior? > > The current goal is to attempt to go m_maxFramesPending deep if vsync is disabled. I was hoping to get some extra concurrency that way and wasn't worried about latency since vsync is disabled. Should I be? > > Maybe we should have a m_targetFramesPending that is independent of m_maxFramesPending. That seems beyond the scope of this fix. This bug is to fix --disable-gpu-vsync. Comment on attachment 158426 [details]
Patch
LGTM
Comment on attachment 158426 [details]
Patch
R=me. We can iterate further on the disable vsync behavior if we want, but this seems like a strict improvement.
Sounds like a plan. Comment on attachment 158426 [details]
Patch
Some of the test failures appear to be due to this patch, though it's not immediately clear why. Trying to sort it out.
*** Bug 94175 has been marked as a duplicate of this bug. *** Comment on attachment 158426 [details] Patch The test failures are due to the removal of the infinite max frames pending. There are no swap acks during tests, which prevents the frame rate controller from advancing. If I add fake swap acks to the tests, all is well again. See: https://bugs.webkit.org/show_bug.cgi?id=94254 My current plan is to make this patch work with infinite pending frames first, so it's not blocked on the patch above and so we can fix the rendering bug sooner than later. Comment on attachment 158426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158426&action=review not sure how much we care but I noticed a few trailing whitespaces when applying this patch locally > Source/WebKit/chromium/tests/CCFrameRateControllerTest.cpp:156 > + trailing whitespaces here > Source/WebKit/chromium/tests/CCFrameRateControllerTest.cpp:162 > + client.reset(); and here > Source/WebKit/chromium/tests/CCFrameRateControllerTest.cpp:166 > + client.reset(); and here > Source/WebKit/chromium/tests/CCFrameRateControllerTest.cpp:179 > + and here Created attachment 159188 [details]
Patch
Created attachment 159192 [details]
Patch
Comment on attachment 159192 [details]
Patch
This should be the final version. It addresses the whitespaces and keeps the infinite pending frames behavior.
Note: I was worried that this patch was causing crashes yesterday, but it ended up being a completely different bug, for which jbates already submitted a patch.
Comment on attachment 159192 [details]
Patch
Great, thanks for investigating that.
Comment on attachment 159192 [details] Patch Clearing flags on attachment: 159192 Committed r125939: <http://trac.webkit.org/changeset/125939> All reviewed patches have been landed. Closing bug. |