Replace CurrentPhysicalTimeTicks with a fallback in CurrentFrameTimeTicks.

Replace CurrentPhysicalTimeTicks with a fallback in CurrentFrameTimeTicks.

CurrentPhysicalTimeTicks was needed because some tasks are scheduled
independently from frames.  They may fall hundreds of milliseconds after
the last frame, so it's not appropriate to use the last frame time for
them.  So we used CurrentFrameTimeTicks for mid-animation timestamps and
CurrentPhysicalTimeTicks for input/random tasks (including the
scheduling of delayed animations).  But there are two problems with this:
1) It's very easy to use the wrong one by accident.
2) Providing alternately frame and physical time to the same method can
lead to nonmonotonic time.  The scrollbar fade controller currently
suffers from this and sometimes fails to fade the scrollbar as a result
(see bug).

This patch changes CurrentFrameTimeTicks to use physical time as a
fallback when frame time is unavailable.  This is monotonic and can't be
used incorrectly.

NOTRY=true
BUG=356032
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259735

[aelias_OOO_until_Jul13, 2014-03-25 07:39:29]

PTAL.  This is an alternative to https://codereview.chromium.org/184023002/ . 
It turns out there is an M35 regression in scrollbar fade (probably indirectly
caused by another of mithro's change) related to the exact issue I brought up in
the other patch.  Given the recurring problems, here's an attempt at a cleaner
way to solve this issue.  (I'll fix the tests soon.)

[mithro-old, 2014-03-25 08:00:54]

On 2014/03/25 07:39:29, aelias wrote:
> PTAL.  This is an alternative to https://codereview.chromium.org/184023002/ . 
> It turns out there is an M35 regression in scrollbar fade (probably indirectly
> caused by another of mithro's change) related to the exact issue I brought up
in
> the other patch.  Given the recurring problems, here's an attempt at a cleaner
> way to solve this issue.  (I'll fix the tests soon.)

Hi aelias,

I would like to have a chat to you about this sometime. It'll probably be faster
than trying to figure this all out via email and bad ascii diagrams :). 


I don't quite understand this comment;

  // Try to use the current frame time to keep animations non-jittery.  But if
  // we're not in a frame (because this is during an input event or a delayed
  // task), fall back to physical time.  This should still be monotonic.

I don't understand how we could be "not in a frame"? If the tab is active, then
it should be rendering frame by frame, right?

If the tab is in the background, it is getting background ticking. How do you
get input events when you are a background tab?

The frames being shown on the screen are at a consistent interval so we want to
render the output at a consistent interval otherwise it will look janky. At the
moment we are rendering for the time of the last frame, rather than for the time
the frame will be displayed on the screen. Eventually I hope we can actually
render for display time but we aren't even getting the rendering for previous
frame time right yet.

With this in mind, with a screen a updating at 10ms,

          Rendering for
 frame1 = t+0ms       == displayed at => t+10ms
 frame2 = t+10ms      == displayed at => t+20ms
 frame3 = t+20ms      == displayed at => t+30ms
 frame4 = t+30ms      == displayed at => t+40ms

If we get the input at t+5ms, we can't render it as part of frame1 as that
rendering is already in progress. The next frame we can include the input data
in is the frame2 which will be displayed on the screen at t+20ms.

Does that make any sense?

Tim

[aelias_OOO_until_Jul13, 2014-03-25 08:07:16]

There are not always frames.  If I visit a simple text page and sit idle for 10
seconds, there are no frames for those ten seconds, and the scheduler should sit
almost completely idle.  (If Chrome spun at 60fps the whole time, this would be
easier to reason about but a gigantic waste of battery life.)  If after those
ten seconds suddenly an input event comes in, if we use the frame time it would
be ten seconds out of date.

[aelias_OOO_until_Jul13, 2014-03-25 08:10:04]

I do appreciate the importance of aligning with vsync, by the way.  I agree
everything that's positioned on an animation curve should defiinitely use frame
time for that.  We just also need a story for these out-of-band random tasks.

[mithro-old, 2014-03-25 08:31:41]

On 2014/03/25 08:10:04, aelias wrote:
> I do appreciate the importance of aligning with vsync, by the way.  I agree
> everything that's positioned on an animation curve should defiinitely use
frame
> time for that.  We just also need a story for these out-of-band random tasks.

Wouldn't the following occur;

 * Input comes in, updates system state that input started at t=X and marks the
system as needing to redraw.
 * Scheduler notices we need to redraw, asks the system to "generate new
content" with the frame time information.
 * Scheduler notices nothing more needs drawing and goes back to sleep.

As well, the t=X shouldn't be Now() but instead the time the input was generated
at?

Tim

[aelias_OOO_until_Jul13, 2014-03-25 08:48:51]

Not every input triggers an immediate redraw.  The one at issue here is a
ScrollEnd event that notes the current time, then posts a scrollbar fade
animation task for 300ms in the future.  That task wakes up, checks the time (to
determine whether there have been any further scrolls in the intervening 300ms
in which case it should repost another task and go back to sleep), then if
needed starts a fade animation with a SetNeedsRedraw.

> As well, the t=X shouldn't be Now() but instead the time the input was
generated at?

I'd be fine with making that sort of change in the future.  In this case though
we also have a task completely decoupled from input.

[mithro-old, 2014-03-25 09:02:56]

On 2014/03/25 08:48:51, aelias wrote:
> Not every input triggers an immediate redraw.  The one at issue here is a
> ScrollEnd event that notes the current time, then posts a scrollbar fade
> animation task for 300ms in the future.  That task wakes up, checks the time
(to
> determine whether there have been any further scrolls in the intervening 300ms
> in which case it should repost another task and go back to sleep), then if
> needed starts a fade animation with a SetNeedsRedraw.

There is a definite trigger of ScrollEnd right (even if it is just Xms relative
to another event). 

Shouldn't every ScrollEnd cancel and re-schedule the fade task? Otherwise the
fade is going to happen at 300ms + random amount after the last scroll time.

Tim

[aelias_OOO_until_Jul13, 2014-03-25 09:14:06]

On 2014/03/25 09:02:56, mithro wrote:
> There is a definite trigger of ScrollEnd right (even if it is just Xms
relative
> to another event). 

Well, it's a bit implicit (via SetCurrentlyScrollingLayer(NULL) ), and can also
happen via a fling animation ending but more or less.

> 
> Shouldn't every ScrollEnd cancel and re-schedule the fade task? Otherwise the
> fade is going to happen at 300ms + random amount after the last scroll time.

Solving that issue is the reason this needs physical timestamps.  The task
checks the delta from the last ScrollEnd and then reposts with a wait that ends
up 300ms later.

When I wrote this initially I wanted to avoid task cancellation since that can
get messy.  I'm starting to think all this timestamp wrangling is messier,
though.  Thanks for highlighting that, I'll see tomorrow if I can come up with a
simpler solution that uses task cancellation.

> 
> Tim

[aelias_OOO_until_Jul13, 2014-03-26 04:37:37]

I'm looking into switching to callbacks, but it's likely to be a fairly large
patch that may be too scary to merge after branch point, and we need a quick fix
for the M35 regression that the scrollbar doesn't fade half the time.  I think
this patch is an incremental improvement on the status quo and fixes the bug, so
could I land this and work on the callback change in the next few weeks?

[mithro-old, 2014-03-26 05:52:10]

On 2014/03/26 04:37:37, aelias wrote:
> I'm looking into switching to callbacks, but it's likely to be a fairly large
> patch that may be too scary to merge after branch point, and we need a quick
fix
> for the M35 regression that the scrollbar doesn't fade half the time.  I think
> this patch is an incremental improvement on the status quo and fixes the bug,
so
> could I land this and work on the callback change in the next few weeks?

I don't see how this patch fixes the M35 regression?

The current code only seems to use gfx::Now() and your replacing it with using
gfx::Now or frame time? Isn't this what is causing your issue in the first
place?

[aelias_OOO_until_Jul13, 2014-03-26 06:01:30]

On 2014/03/26 05:52:10, mithro wrote:
> I don't see how this patch fixes the M35 regression?
> 
> The current code only seems to use gfx::Now() and your replacing it with using
> gfx::Now or frame time? Isn't this what is causing your issue in the first
> place?

I haven't discussed the M35 regression yet, just the more fundamental issues why
I needed physical time.

The M35 regression is caused by the scrollbar_controller->Animate() call in
StartScrollbarAnimationRecursive being called with physical time to kick off the
first frame of the animation proper, while the one in AnimateScrollbars is
called with frame time.  As of M35 the start commonly happens while in a frame
(I haven't tracked down what changed to cause that), so this makes time go
backwards from the POV of the controller and in the second call, it thinks the
animation has actually not quite started yet and doesn't schedule any more
redraws.

[mithro-old, 2014-03-26 06:58:03]

After discussion with aelias I'm happy with this change.

Can you please get a second LGTM from another reviewer before landing.

Thanks!

Tim 'mithro' Ansell

[brianderson, 2014-03-26 21:44:46]

lgtm

[aelias_OOO_until_Jul13, 2014-03-26 21:56:39]

The CQ bit was checked by aelias@chromium.org

[commit-bot: I haz the power, 2014-03-26 21:57:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/210793002/40001

[commit-bot: I haz the power, 2014-03-26 22:54:51]

Change committed as 259735