Bug 94012 – [Chromium] Fix vsyncTick drought when vsync throttling is disabled

• Component: New Bugs
• Status: RESOLVED FIXED

[Brian Anderson]

Fix vsyncTick drought when vsync throttling is disabled

[Brian Anderson]

Created attachment 158390 [details]
Patch

[Brian Anderson]

Created attachment 158391 [details]
Patch

[James Robinson]

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?

[Brian Anderson]

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.

[John Bates]

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.

[Brian Anderson]

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.

[Brian Anderson]

Created attachment 158426 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Nat Duca]

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?

[Brian Anderson]

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.

[John Bates]

(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.

[John Bates]

Comment on attachment 158426 [details]
Patch

LGTM

[James Robinson]

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.

[Nat Duca]

Sounds like a plan.

[Brian Anderson]

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.

[David Reveman]

*** Bug 94175 has been marked as a duplicate of this bug. ***

[Brian Anderson]

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.

[David Reveman]

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

[Brian Anderson]

Created attachment 159188 [details]
Patch

[Brian Anderson]

Created attachment 159192 [details]
Patch

[Brian Anderson]

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.

[James Robinson]

Comment on attachment 159192 [details]
Patch

Great, thanks for investigating that.

[WebKit Review Bot]

Comment on attachment 159192 [details]
Patch

Clearing flags on attachment: 159192

Committed r125939: <http://trac.webkit.org/changeset/125939>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.