Add CCDelayBasedTimeSource::setTimebaseAndInterval
Created attachment 155703 [details] Patch
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
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
Created attachment 155872 [details] Patch
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.
Created attachment 155905 [details] Patch
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
Created attachment 155918 [details] Patch
Addressed syntax for constants. Also, fixed tests to verify that pending tasks were canceled on an immediate clock change.
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 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.
Created attachment 156197 [details] Patch
Created attachment 156501 [details] Patch
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.
Any new feedback on this patch? Would like to get it in before the branch point if possible.
Comment on attachment 156501 [details] Patch Looks good to me, double-check that Nat's happy before landing.
Comment on attachment 156501 [details] Patch LGTM. nice work!
Comment on attachment 156501 [details] Patch Clearing flags on attachment: 156501 Committed r124898: <http://trac.webkit.org/changeset/124898>
All reviewed patches have been landed. Closing bug.