Bug 92825 - Add CCDelayBasedTimeSource::setTimebaseAndInterval
Summary: Add CCDelayBasedTimeSource::setTimebaseAndInterval
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:
Depends on:
Blocks:
 
Reported: 2012-07-31 19:19 PDT by Brian Anderson
Modified: 2012-08-07 11:30 PDT (History)
6 users (show)

See Also:


Attachments
Patch (7.71 KB, patch)
2012-07-31 19:24 PDT, Brian Anderson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (965.80 KB, application/zip)
2012-07-31 19:59 PDT, WebKit Review Bot
no flags Details
Patch (12.07 KB, patch)
2012-08-01 13:32 PDT, Brian Anderson
no flags Details | Formatted Diff | Diff
Patch (14.49 KB, patch)
2012-08-01 15:35 PDT, Brian Anderson
no flags Details | Formatted Diff | Diff
Patch (14.47 KB, patch)
2012-08-01 16:24 PDT, Brian Anderson
no flags Details | Formatted Diff | Diff
Patch (16.58 KB, patch)
2012-08-02 16:29 PDT, Brian Anderson
no flags Details | Formatted Diff | Diff
Patch (19.34 KB, patch)
2012-08-03 18:46 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-07-31 19:19:07 PDT
Add CCDelayBasedTimeSource::setTimebaseAndInterval
Comment 1 Brian Anderson 2012-07-31 19:24:31 PDT
Created attachment 155703 [details]
Patch
Comment 2 WebKit Review Bot 2012-07-31 19:59:39 PDT
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
Comment 3 WebKit Review Bot 2012-07-31 19:59:43 PDT
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
Comment 4 Brian Anderson 2012-08-01 13:32:02 PDT
Created attachment 155872 [details]
Patch
Comment 5 Brian Anderson 2012-08-01 13:58:39 PDT
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.
Comment 6 Brian Anderson 2012-08-01 15:35:02 PDT
Created attachment 155905 [details]
Patch
Comment 7 James Robinson 2012-08-01 15:36:34 PDT
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
Comment 8 Brian Anderson 2012-08-01 16:24:48 PDT
Created attachment 155918 [details]
Patch
Comment 9 Brian Anderson 2012-08-01 16:27:10 PDT
Addressed syntax for constants.

Also, fixed tests to verify that pending tasks were canceled on an immediate clock change.
Comment 10 Nat Duca 2012-08-02 00:09:33 PDT
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..
Comment 11 Brian Anderson 2012-08-02 11:19:35 PDT
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.
Comment 12 Brian Anderson 2012-08-02 16:29:33 PDT
Created attachment 156197 [details]
Patch
Comment 13 Brian Anderson 2012-08-03 18:46:59 PDT
Created attachment 156501 [details]
Patch
Comment 14 Brian Anderson 2012-08-03 18:49:59 PDT
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.
Comment 15 Brian Anderson 2012-08-06 14:09:50 PDT
Any new feedback on this patch?  Would like to get it in before the branch point if possible.
Comment 16 James Robinson 2012-08-06 15:34:19 PDT
Comment on attachment 156501 [details]
Patch

Looks good to me, double-check that Nat's happy before landing.
Comment 17 Nat Duca 2012-08-06 22:05:43 PDT
Comment on attachment 156501 [details]
Patch

LGTM. nice work!
Comment 18 WebKit Review Bot 2012-08-07 11:30:05 PDT
Comment on attachment 156501 [details]
Patch

Clearing flags on attachment: 156501

Committed r124898: <http://trac.webkit.org/changeset/124898>
Comment 19 WebKit Review Bot 2012-08-07 11:30:10 PDT
All reviewed patches have been landed.  Closing bug.