cc: Use HighResNow as timebase if it is fast and reliable

cc: Use HighResNow as timebase if it is fast and reliable

If base::TimeTicks::HighResNow is fast and reliable, we use it
for frame times, animations, and scheduling. Otherwise, we use
base::TimeTicks::Now and use a timebase of zero to avoid an
incorrect or jittery timebase.

This adds a gfx::FrameTime::Now() function, so the same timebase
is used across the ui and cc directories.

Additionally, the OutputSurface now uses a HighRes version of the
DelayBasedTimesource if gfx::FrameTime::Now() is HighRes.

BUG=303356
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231089

[brianderson, 2013-10-17 18:41:25]

PTAL.

frame_time.h just has a couple static methods for now so that everyone that
needs access to a consistent timebase can get it.

I really want frame_time.h to be like the following:
https://codereview.chromium.org/26880010, but I would like to tackle that in a
separate patch as part of a different effort to reduce the sources of timestamps
and conversions of time to floating point.

[ajuma, 2013-10-17 20:14:55]

https://codereview.chromium.org/27710005/diff/5001/cc/trees/single_thread_pro...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/27710005/diff/5001/cc/trees/single_thread_pro...
cc/trees/single_thread_proxy.cc:63: if
(!CommitAndComposite(gfx::FrameTime::Now(),
We'll also need to fix the other call to CommitAndComposite (from
CompositeImmediately below), which means tracing back to where the time passed
in to CommitAndComposite comes from. I believe RenderWidget::DoDeferredUpdate
and ui::Compositor::Draw are two such places. Also, looking at ui::Compositor,
SchedulerDraw() calls LayerTreeHost::Composite with a TimeTicks::Now().

https://codereview.chromium.org/27710005/diff/5001/ui/compositor/layer_animat...
File ui/compositor/layer_animator.cc (right):

https://codereview.chromium.org/27710005/diff/5001/ui/compositor/layer_animat...
ui/compositor/layer_animator.cc:182: last_step_time_ = gfx::FrameTime::Now();
We'll also need to make the same change in
test/layer_animator_test_controller.cc and in layer_animator_unittest.cc

[brianderson, 2013-10-22 14:11:31]

ptal

[ajuma, 2013-10-22 16:54:35]

lgtm

[brianderson, 2013-10-22 16:59:12]

+danakj and +piman for owners review.

[danakj, 2013-10-22 21:06:15]

ui/gfx LGTM

Can you base base::TimeTicks::Now and HighResNow in the cc/ PRESUBMIT.py?

[danakj, 2013-10-22 21:06:26]

On 2013/10/22 21:06:15, danakj wrote:
> ui/gfx LGTM
> 
> Can you base base::TimeTicks::Now and HighResNow in the cc/ PRESUBMIT.py?

Can you ban*

[piman, 2013-10-23 00:03:45]

https://codereview.chromium.org/27710005/diff/204001/cc/scheduler/frame_rate_...
File cc/scheduler/frame_rate_controller.cc (right):

https://codereview.chromium.org/27710005/diff/204001/cc/scheduler/frame_rate_...
cc/scheduler/frame_rate_controller.cc:101: timebase = base::TimeTicks();
mmh, aren't we losing functionality here?
Should we make the UpdateVSyncParameter logic use FrameTime too?

[brianderson, 2013-10-23 13:04:22]

On 2013/10/22 21:06:26, danakj wrote:
> On 2013/10/22 21:06:15, danakj wrote:
> > ui/gfx LGTM
> > 
> > Can you base base::TimeTicks::Now and HighResNow in the cc/ PRESUBMIT.py?
> 
> Can you ban*

It doesn't make sense to ban it completely since some sensitive timing code
might still want to use HighResNow even if FrameTime just uses Now().

To prevent comparing different timebases and to discourage sampling the
timestamp in random parts of the code, I want to do a followup patch that
introduces two separate types:
FrameTime (A frame time that is snapped to the vsync, used to update
animations.)
FractionalFrameTime (deadlines, animation start/end times, etc.)

Only a small set of files would be allowed to create a FrameTime (via friends or
PRESUBMIT), while anyone could create a FractionalFrameTime. Sampling of all
animations would require a FrameTime.

[brianderson, 2013-10-23 13:04:42]

https://codereview.chromium.org/27710005/diff/204001/cc/scheduler/frame_rate_...
File cc/scheduler/frame_rate_controller.cc (right):

https://codereview.chromium.org/27710005/diff/204001/cc/scheduler/frame_rate_...
cc/scheduler/frame_rate_controller.cc:101: timebase = base::TimeTicks();
On 2013/10/23 00:03:45, piman wrote:
> mmh, aren't we losing functionality here?
> Should we make the UpdateVSyncParameter logic use FrameTime too?

The problem is that the DWM API's on Windows only provide the timebase as a QPC
value, which shares the same timebase as HighResNow(). Translating that to the
Now()'s timebase is going to be error prone and jittery.

I've run with a timebase of 0 in the past and it works very well since there is
no jitter. We would run into the issue of drift over long periods of non-stop
swapping, which will cause us to either 1) eventually skip producing a frame 
because we are producing too fast or 2) skip swapping a frame because we are
producing too slowly. I would prefer that infrequent periodic jank to a more
unpredictable jank every frame though.

One thing I could do (that this patch doesn't do yet) is use the timebase when
there is a parent compositor and deadline scheduling is disabled, since we
control the timebase in that case.

[piman, 2013-10-23 23:45:07]

https://codereview.chromium.org/27710005/diff/204001/cc/scheduler/frame_rate_...
File cc/scheduler/frame_rate_controller.cc (right):

https://codereview.chromium.org/27710005/diff/204001/cc/scheduler/frame_rate_...
cc/scheduler/frame_rate_controller.cc:101: timebase = base::TimeTicks();
On 2013/10/23 13:04:43, Brian Anderson wrote:
> On 2013/10/23 00:03:45, piman wrote:
> > mmh, aren't we losing functionality here?
> > Should we make the UpdateVSyncParameter logic use FrameTime too?
> 
> The problem is that the DWM API's on Windows only provide the timebase as a
QPC
> value, which shares the same timebase as HighResNow(). Translating that to the
> Now()'s timebase is going to be error prone and jittery.

If 0 is ok, then can we make the producer of the timebase make the decision?
Contract is, we only use FrameTime everywhere, it's a lot simpler to reason
about and less error prone.

> 
> I've run with a timebase of 0 in the past and it works very well since there
is
> no jitter. We would run into the issue of drift over long periods of non-stop
> swapping, which will cause us to either 1) eventually skip producing a frame 
> because we are producing too fast or 2) skip swapping a frame because we are
> producing too slowly. I would prefer that infrequent periodic jank to a more
> unpredictable jank every frame though.
> 
> One thing I could do (that this patch doesn't do yet) is use the timebase when
> there is a parent compositor and deadline scheduling is disabled, since we
> control the timebase in that case.
> 
>

[brianderson, 2013-10-24 11:26:51]

https://codereview.chromium.org/27710005/diff/204001/cc/scheduler/frame_rate_...
File cc/scheduler/frame_rate_controller.cc (right):

https://codereview.chromium.org/27710005/diff/204001/cc/scheduler/frame_rate_...
cc/scheduler/frame_rate_controller.cc:101: timebase = base::TimeTicks();
On 2013/10/23 23:45:08, piman wrote:
> On 2013/10/23 13:04:43, Brian Anderson wrote:
> > On 2013/10/23 00:03:45, piman wrote:
> > > mmh, aren't we losing functionality here?
> > > Should we make the UpdateVSyncParameter logic use FrameTime too?
> > 
> > The problem is that the DWM API's on Windows only provide the timebase as a
> QPC
> > value, which shares the same timebase as HighResNow(). Translating that to
the
> > Now()'s timebase is going to be error prone and jittery.
> 
> If 0 is ok, then can we make the producer of the timebase make the decision?
> Contract is, we only use FrameTime everywhere, it's a lot simpler to reason
> about and less error prone.
> 
> > 
> > I've run with a timebase of 0 in the past and it works very well since there
> is
> > no jitter. We would run into the issue of drift over long periods of
non-stop
> > swapping, which will cause us to either 1) eventually skip producing a frame

> > because we are producing too fast or 2) skip swapping a frame because we are
> > producing too slowly. I would prefer that infrequent periodic jank to a more
> > unpredictable jank every frame though.
> > 
> > One thing I could do (that this patch doesn't do yet) is use the timebase
when
> > there is a parent compositor and deadline scheduling is disabled, since we
> > control the timebase in that case.
> > 
> > 
> 

Sounds good. I will change the patch.

[brianderson, 2013-10-24 15:28:10]

ptal

[piman, 2013-10-24 19:37:18]

lgtm

[jbauman, 2013-10-25 00:57:17]

lgtm

[commit-bot: I haz the power, 2013-10-25 11:14:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/27710005/864001

[commit-bot: I haz the power, 2013-10-25 11:41:05]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[brianderson, 2013-10-25 12:50:44]

+kbr for owners review of ui/surface

[Ken Russell (switch to Gerrit), 2013-10-25 17:53:19]

ui/surface lgtm

[commit-bot: I haz the power, 2013-10-25 17:55:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/27710005/864001

[commit-bot: I haz the power, 2013-10-25 19:46:53]

Change committed as 231089