Bug 94012 - [Chromium] Fix vsyncTick drought when vsync throttling is disabled
Summary: [Chromium] Fix vsyncTick drought when vsync throttling is disabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brian Anderson
URL:
Keywords:
: 94175 (view as bug list)
Depends on:
Blocks: 84281
  Show dependency treegraph
 
Reported: 2012-08-14 12:03 PDT by Brian Anderson
Modified: 2012-08-17 14:50 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.68 KB, patch)
2012-08-14 12:07 PDT, Brian Anderson
no flags Details | Formatted Diff | Diff
Patch (2.74 KB, patch)
2012-08-14 12:12 PDT, Brian Anderson
no flags Details | Formatted Diff | Diff
Patch (5.00 KB, patch)
2012-08-14 15:39 PDT, Brian Anderson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 (332.25 KB, application/zip)
2012-08-14 17:22 PDT, WebKit Review Bot
no flags Details
Patch (3.71 KB, patch)
2012-08-17 13:24 PDT, Brian Anderson
no flags Details | Formatted Diff | Diff
Patch (3.70 KB, patch)
2012-08-17 13:42 PDT, Brian Anderson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Anderson 2012-08-14 12:03:23 PDT
Fix vsyncTick drought when vsync throttling is disabled
Comment 1 Brian Anderson 2012-08-14 12:07:36 PDT
Created attachment 158390 [details]
Patch
Comment 2 Brian Anderson 2012-08-14 12:12:27 PDT
Created attachment 158391 [details]
Patch
Comment 3 James Robinson 2012-08-14 12:15:29 PDT
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?
Comment 4 Brian Anderson 2012-08-14 12:22:23 PDT
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 5 John Bates 2012-08-14 12:52:02 PDT
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 6 Brian Anderson 2012-08-14 13:43:20 PDT
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.
Comment 7 Brian Anderson 2012-08-14 15:39:36 PDT
Created attachment 158426 [details]
Patch
Comment 8 WebKit Review Bot 2012-08-14 17:22:04 PDT
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
Comment 9 WebKit Review Bot 2012-08-14 17:22:07 PDT
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 10 Nat Duca 2012-08-15 10:07:32 PDT
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 11 Brian Anderson 2012-08-15 10:36:11 PDT
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.
Comment 12 John Bates 2012-08-15 10:50:07 PDT
(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 13 John Bates 2012-08-15 10:54:01 PDT
Comment on attachment 158426 [details]
Patch

LGTM
Comment 14 James Robinson 2012-08-15 10:59:03 PDT
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.
Comment 15 Nat Duca 2012-08-15 11:02:53 PDT
Sounds like a plan.
Comment 16 Brian Anderson 2012-08-15 13:33:08 PDT
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.
Comment 17 David Reveman 2012-08-15 20:47:00 PDT
*** Bug 94175 has been marked as a duplicate of this bug. ***
Comment 18 Brian Anderson 2012-08-16 15:32:38 PDT
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 19 David Reveman 2012-08-17 08:44:24 PDT
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
Comment 20 Brian Anderson 2012-08-17 13:24:50 PDT
Created attachment 159188 [details]
Patch
Comment 21 Brian Anderson 2012-08-17 13:42:36 PDT
Created attachment 159192 [details]
Patch
Comment 22 Brian Anderson 2012-08-17 13:56:27 PDT
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 23 James Robinson 2012-08-17 13:59:46 PDT
Comment on attachment 159192 [details]
Patch

Great, thanks for investigating that.
Comment 24 WebKit Review Bot 2012-08-17 14:50:38 PDT
Comment on attachment 159192 [details]
Patch

Clearing flags on attachment: 159192

Committed r125939: <http://trac.webkit.org/changeset/125939>
Comment 25 WebKit Review Bot 2012-08-17 14:50:43 PDT
All reviewed patches have been landed.  Closing bug.