Bug 92825 – Add CCDelayBasedTimeSource::setTimebaseAndInterval

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Brian Anderson]

Add CCDelayBasedTimeSource::setTimebaseAndInterval

[Brian Anderson]

Created attachment 155703 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155703 [details]
Patch

Attachment 155703 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13404281

New failing tests:
CCDelayBasedTimeSource.NextDelaySaneWhenHalfAfterRequestedTime

[WebKit Review Bot]

Created attachment 155708 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Brian Anderson]

Created attachment 155872 [details]
Patch

[Brian Anderson]

Comment on attachment 155872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155872&action=review

> Source/WebKit/chromium/tests/CCDelayBasedTimeSourceTest.cpp:233
> +TEST(CCDelayBasedTimeSource, HanldlesSignificantTimebaseOrIntervalChangesImmediately)

Planning to split this test into separate tests for timebase and interval.  Currently, it only tests changes in one +/- "direction"; I should be testing both.

> Source/WebKit/chromium/tests/CCDelayBasedTimeSourceTest.cpp:252
> +    thread.allowPendingTaskOverwrite(true);

I allow pending task overwrite in this test because it is expected, but do not verify that the original task is canceled.  Might punt on that if it's too hard to verify.

[Brian Anderson]

Created attachment 155905 [details]
Patch

[James Robinson]

Comment on attachment 155872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155872&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.cpp:38
> +    const double kDoubleTickThreshold = 0.25;
> +    const double kIntervalChangeThreshold = 0.25;
> +    const double kPhaseChangeThreshold = 0.25;

WebKit style nits:

Constants have the same naming rules as normal variables - no "k" prefix
No indentation for code in a namespace in WebKit

[Brian Anderson]

Created attachment 155918 [details]
Patch

[Brian Anderson]

Addressed syntax for constants.

Also, fixed tests to verify that pending tasks were canceled on an immediate clock change.

[Nat Duca]

Comment on attachment 155918 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155918&action=review

So as far as I can tell, the *Next states exist to optimize for the case where the new timebase/frequency has changed substantially, you reset the timer rather than fix it on the next tick.

I guess my question, is that worth the complexity? Is there something we can't do when we change the timebase on the next tick?

> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.cpp:36
> +const double doubleTickThreshold = 0.25;

Add unit or explanation. This is percent since last tick? Or ms...?

> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.cpp:117
> +    if (m_state == STATE_ACTIVE) {

if (!active)
  return;
... unindented body

> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.cpp:120
> +        // Detect change in frequency.

This comment isn't very helpful. You could, however, add an explanation of what the logic *inside* the branch is trying to do. Or put an explanation at the top of the function that explains the logic of this code. I'm honestly struggling a bit to grok the new states that have been added.

> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.cpp:211
> +    ASSERT(delay <= m_intervalSeconds * (1.0 + doubleTickThreshold));

do you think we need this assertion anymore?

> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.h:44
>  

We probably need to expose this setTimebaseAndInterval out to the TimeSource base class. Or get rid of the CCTimeSource base class if that makes more sense.

> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.h:77
> +    // m_intervalSecondsNext and m_tickTargetNext take effect on

Would a struct Parameters { ... } be useful? Then you'd just have m_current and m_pending?

> Source/WebCore/platform/graphics/chromium/cc/CCTimer.cpp:94
> +    const double epsilon = 1e-3; // 1 microsecond

How does this epsilon guard work? I'm not quite grokking..

[Brian Anderson]

Comment on attachment 155918 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155918&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.cpp:36
>> +const double doubleTickThreshold = 0.25;
> 
> Add unit or explanation. This is percent since last tick? Or ms...?

ok

>> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.cpp:117
>> +    if (m_state == STATE_ACTIVE) {
> 
> if (!active)
>   return;
> ... unindented body

Good idea.

>> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.cpp:120
>> +        // Detect change in frequency.
> 
> This comment isn't very helpful. You could, however, add an explanation of what the logic *inside* the branch is trying to do. Or put an explanation at the top of the function that explains the logic of this code. I'm honestly struggling a bit to grok the new states that have been added.

I don't like unhelpful comments either.  Will explain logic.

>> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.cpp:211
>> +    ASSERT(delay <= m_intervalSeconds * (1.0 + doubleTickThreshold));
> 
> do you think we need this assertion anymore?

It helped me catch a bug, so I like it.  There are two asserts in this function, the first one enforces a lower bound and this one enforces an upper bound.

>> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.h:44
>>  
> 
> We probably need to expose this setTimebaseAndInterval out to the TimeSource base class. Or get rid of the CCTimeSource base class if that makes more sense.

Will expose to TimeSource.

>> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.h:77
>> +    // m_intervalSecondsNext and m_tickTargetNext take effect on
> 
> Would a struct Parameters { ... } be useful? Then you'd just have m_current and m_pending?

That would make it very clear what variables are grouped together.  Will do.

>> Source/WebCore/platform/graphics/chromium/cc/CCTimer.cpp:94
>> +    const double epsilon = 1e-3; // 1 microsecond
> 
> How does this epsilon guard work? I'm not quite grokking..

For tests, I got actual values that were off by one than what I was expecting.  It turned out that the logic here was truncating down a millisecond if, for example, 7ms was represented as 6.999...ms.

The way it's implemented now shouldn't affect real use cases much.  So I should probably remove it and change test expectations, or make it round up even more for production if we actually want to round up.  Will remove the epsilon unless someone else feels strongly that we should round up more.

[Brian Anderson]

Created attachment 156197 [details]
Patch

[Brian Anderson]

Created attachment 156501 [details]
Patch

[Brian Anderson]

Roled https://bugs.webkit.org/show_bug.cgi?id=93064 into this patch and added lastTickTime and nextTickTime methods for use by future frame rate controller work.

[Brian Anderson]

Any new feedback on this patch?  Would like to get it in before the branch point if possible.

[James Robinson]

Comment on attachment 156501 [details]
Patch

Looks good to me, double-check that Nat's happy before landing.

[Nat Duca]

Comment on attachment 156501 [details]
Patch

LGTM. nice work!

[WebKit Review Bot]

Comment on attachment 156501 [details]
Patch

Clearing flags on attachment: 156501

Committed r124898: <http://trac.webkit.org/changeset/124898>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.