We're calling CCDelayBasedTimeSource::nextTickTime() before the timer is activated and nextTickTime() then returns 0.
Created attachment 159953 [details] Patch Simple fix. It might be more appropriate to re-factor CCScheduler to not call nextTickTime() on an inactive timer
Comment on attachment 159953 [details] Patch I like this. @brianderson?
Increasing priority as this bug is causing animations driven of requestAnimationFrame to run at half of the nominal frame rate.
This seems OK. The complexity here is getting scary. Is there any way we could have caught this regression more directly? I think Brian should take a look but we should land ASAP to get coverage back on the perf bots downstream.
I'm fine with getting this in, but would like to change it eventually, since it's lying about what the next tick time actually is. Also, the behavior for nextTickTime becomes intertwined with specific use cases of CCScheduler, which is only okay for now because CCDelayBasedTimeSource isn't used in many places. Note: There is a comment in CCDelayBasedTimeSource.h indicating nextTickTime will return 0 if not active. That comment should be changed to reflect the new behavior.
Comment on attachment 159953 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159953&action=review > Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.cpp:107 > + return monotonicTimeNow() + m_currentParameters.interval; jbates brought up a good point that this fake time will not be vsync aligned. Could this cause the texture uploading to run long and prevent us from going active again ASAP?
(In reply to comment #6) > (From update of attachment 159953 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159953&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.cpp:107 > > + return monotonicTimeNow() + m_currentParameters.interval; > > jbates brought up a good point that this fake time will not be vsync aligned. Could this cause the texture uploading to run long and prevent us from going active again ASAP? It looks to me as if tickTarget always becomes "now + interval" when the timer goes from inactive to active state. Maybe I'm missing something?
Created attachment 159996 [details] Patch Move comment to header
(In reply to comment #5) > I'm fine with getting this in, but would like to change it eventually, since it's lying about what the next tick time actually is. Also, the behavior for nextTickTime becomes intertwined with specific use cases of CCScheduler, which is only okay for now because CCDelayBasedTimeSource isn't used in many places. Would changing CCScheduler so that it always activates the timer before calling nextTickTime() be enough? > > Note: There is a comment in CCDelayBasedTimeSource.h indicating nextTickTime will return 0 if not active. That comment should be changed to reflect the new behavior. Fixed in latest patch.
> It looks to me as if tickTarget always becomes "now + interval" when the timer goes from inactive to active state. Maybe I'm missing something? There is some additional logic in CCDelayBasedTimeSource::postNextTickTask that aligns "now" to the nearest vsync when the timer is activated. > Would changing CCScheduler so that it always activates the timer before calling nextTickTime() be enough? That would prevent us from lying and would also automatically align nextTickTime to a vsync. We would end up with ticks that are no-ops, but the overhead should be negligible. Sounds like a good trade off to me.
Recapping verbal discussion, how about reusing the lastTickTime of the DBTS?
Created attachment 160016 [details] Patch Add nextTickTimeIfActivated and use postNextTickTask logic determine tick target when inactive
Comment on attachment 160016 [details] Patch Attachment 160016 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13561421
Created attachment 160021 [details] Patch Fix unit test and ASSERT in postNextTickTask
Comment on attachment 160021 [details] Patch New version looks good. It returns a vsync aligned timestamp, the function name doesn't lie about what it's doing, and it doesn't spuriously activate ticking.
Comment on attachment 160021 [details] Patch Clearing flags on attachment: 160021 Committed r126431: <http://trac.webkit.org/changeset/126431>
All reviewed patches have been landed. Closing bug.