Ensure LayerTreeHostImpl's current frame time is updated every frame

Ensure LayerTreeHostImpl's current frame time is updated every frame

LayerTreeHostImpl's current frame time is currently updated after every
draw. This means that if, in some frame, we try (and fail) to activate
the pending tree but we don't draw, then when we eventually do draw we'll
be using an old current frame time. As a result, animations started during
that frame will have an old start time, and hence will appear to jump during
the following frame.

This CL makes us update the current frame time after every frame, regardless
of whether drawing occurred during the frame.

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

[ajuma, 2013-05-16 15:21:56]

PTAL.

I think a better long-term approach is to switch from lazily setting the frame
time the first time that CurrentFrameTimeTicks is called (and clearing the time
at the end of each frame) to explicitly setting the frame time at the start of
each frame. Since this would involve scheduler changes (since ThreadProxy would
have to be explicitly informed about the start of each frame), it makes sense to
do this after the refactor from http://crbug.com/240945 is done.

[danakj, 2013-05-16 15:28:43]

On 2013/05/16 15:21:56, ajuma wrote:
> PTAL.
> 
> I think a better long-term approach is to switch from lazily setting the frame
> time the first time that CurrentFrameTimeTicks is called (and clearing the
time
> at the end of each frame) to explicitly setting the frame time at the start of
> each frame. Since this would involve scheduler changes (since ThreadProxy
would
> have to be explicitly informed about the start of each frame), it makes sense
to
> do this after the refactor from http://crbug.com/240945 is done.

Can you file and/or block a bug on 240945 regarding changing this as you
suggest?

[danakj, 2013-05-16 15:37:18]

https://codereview.chromium.org/15139007/diff/1/cc/trees/layer_tree_host_unit...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/15139007/diff/1/cc/trees/layer_tree_host_unit...
cc/trees/layer_tree_host_unittest.cc:651: frame_ = 1;
I think you can just EndTest() here, as this test is really meant for testing
activation.

Instead, could you add a second test that verifies that when we do draw a frame,
the frame time changes as well? That should cover both impl and non-impl paint.

[ajuma, 2013-05-16 17:24:34]

PTAL.

On 2013/05/16 15:28:43, danakj wrote:
> On 2013/05/16 15:21:56, ajuma wrote:
> > PTAL.
> > 
> > I think a better long-term approach is to switch from lazily setting the
frame
> > time the first time that CurrentFrameTimeTicks is called (and clearing the
> time
> > at the end of each frame) to explicitly setting the frame time at the start
of
> > each frame. Since this would involve scheduler changes (since ThreadProxy
> would
> > have to be explicitly informed about the start of each frame), it makes
sense
> to
> > do this after the refactor from http://crbug.com/240945 is done.
> 
> Can you file and/or block a bug on 240945 regarding changing this as you
> suggest?

Filed http://crbug.com/241438, blocked on 240945.

https://codereview.chromium.org/15139007/diff/1/cc/trees/layer_tree_host_unit...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/15139007/diff/1/cc/trees/layer_tree_host_unit...
cc/trees/layer_tree_host_unittest.cc:651: frame_ = 1;
On 2013/05/16 15:37:18, danakj wrote:
> I think you can just EndTest() here, as this test is really meant for testing
> activation.

Done.

> Instead, could you add a second test that verifies that when we do draw a
frame,
> the frame time changes as well? That should cover both impl and non-impl
paint.

Done.

[danakj, 2013-05-16 17:42:38]

Thanks! LGTM with a small request for the test

https://codereview.chromium.org/15139007/diff/11001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/15139007/diff/11001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest.cc:692: impl->SetNeedsRedraw();
Can you add a CommitCompleteOnThread override and demonstrate that there isn't a
commit happening between these two frames, so we can be sure the impl drawing is
causing it to update?

[brianderson, 2013-05-16 18:16:00]

https://codereview.chromium.org/15139007/diff/11001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/15139007/diff/11001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:1035: layer_tree_host_impl_->BeginNextFrame();
I know this is tangential to what you are changing, but can we rename
BeginNextFrame to UpdateNextFrameTime? BeginNextFrame implies more than what it
is actually doing.

[danakj, 2013-05-16 18:18:09]

https://codereview.chromium.org/15139007/diff/11001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/15139007/diff/11001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:1035: layer_tree_host_impl_->BeginNextFrame();
On 2013/05/16 18:16:00, Brian Anderson wrote:
> I know this is tangential to what you are changing, but can we rename
> BeginNextFrame to UpdateNextFrameTime? BeginNextFrame implies more than what
it
> is actually doing.

It's not quite update either, it's setting things to 0 at the moment so they
will be updated later.

ResetCurrentFrameTime()?

[danakj, 2013-05-16 18:24:07]

https://codereview.chromium.org/15139007/diff/11001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/15139007/diff/11001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:1035: layer_tree_host_impl_->BeginNextFrame();
On 2013/05/16 18:18:09, danakj wrote:
> On 2013/05/16 18:16:00, Brian Anderson wrote:
> > I know this is tangential to what you are changing, but can we rename
> > BeginNextFrame to UpdateNextFrameTime? BeginNextFrame implies more than what
> it
> > is actually doing.
> 
> It's not quite update either, it's setting things to 0 at the moment so they
> will be updated later.
> 
> ResetCurrentFrameTime()?

Or ResetCurrentFrameTimeForNextFrame(), to say this is being reset cuz we're
moving into the next frame?

[ajuma, 2013-05-16 18:33:33]

On 2013/05/16 17:42:38, danakj wrote:
> Thanks! LGTM with a small request for the test
> 
>
https://codereview.chromium.org/15139007/diff/11001/cc/trees/layer_tree_host_...
> File cc/trees/layer_tree_host_unittest.cc (right):
> 
>
https://codereview.chromium.org/15139007/diff/11001/cc/trees/layer_tree_host_...
> cc/trees/layer_tree_host_unittest.cc:692: impl->SetNeedsRedraw();
> Can you add a CommitCompleteOnThread override and demonstrate that there isn't
a
> commit happening between these two frames, so we can be sure the impl drawing
is
> causing it to update?

This won't work for SingleThreadProxy, where each draw is preceded by a commit.
But we could make it a multi-thread test only. Thoughts?

https://codereview.chromium.org/15139007/diff/11001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/15139007/diff/11001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:1035: layer_tree_host_impl_->BeginNextFrame();
On 2013/05/16 18:24:07, danakj wrote:
> On 2013/05/16 18:18:09, danakj wrote:
> > On 2013/05/16 18:16:00, Brian Anderson wrote:
> > > I know this is tangential to what you are changing, but can we rename
> > > BeginNextFrame to UpdateNextFrameTime? BeginNextFrame implies more than
what
> > it
> > > is actually doing.
> > 
> > It's not quite update either, it's setting things to 0 at the moment so they
> > will be updated later.
> > 
> > ResetCurrentFrameTime()?
> 
> Or ResetCurrentFrameTimeForNextFrame(), to say this is being reset cuz we're
> moving into the next frame?

I like ResetCurrentFrameTimeForNextFrame().

[danakj, 2013-05-16 18:50:42]

On Thu, May 16, 2013 at 2:33 PM, <ajuma@chromium.org> wrote:

> This won't work for SingleThreadProxy, where each draw is preceded by a
> commit.
>  But we could make it a multi-thread test only. Thoughts?
>

Oh urg, yeh. You can guard it on ImplThread() being true in the test.

[ajuma, 2013-05-16 19:50:01]

https://codereview.chromium.org/15139007/diff/11001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/15139007/diff/11001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest.cc:692: impl->SetNeedsRedraw();
On 2013/05/16 17:42:38, danakj wrote:
> Can you add a CommitCompleteOnThread override and demonstrate that there isn't
a
> commit happening between these two frames, so we can be sure the impl drawing
is
> causing it to update?

Done.

https://codereview.chromium.org/15139007/diff/11001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/15139007/diff/11001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:1035: layer_tree_host_impl_->BeginNextFrame();
On 2013/05/16 18:33:33, ajuma wrote:
> On 2013/05/16 18:24:07, danakj wrote:
> > On 2013/05/16 18:18:09, danakj wrote:
> > > On 2013/05/16 18:16:00, Brian Anderson wrote:
> > > > I know this is tangential to what you are changing, but can we rename
> > > > BeginNextFrame to UpdateNextFrameTime? BeginNextFrame implies more than
> what
> > > it
> > > > is actually doing.
> > > 
> > > It's not quite update either, it's setting things to 0 at the moment so
they
> > > will be updated later.
> > > 
> > > ResetCurrentFrameTime()?
> > 
> > Or ResetCurrentFrameTimeForNextFrame(), to say this is being reset cuz we're
> > moving into the next frame?
> 
> I like ResetCurrentFrameTimeForNextFrame().

Renamed to ResetCurrentFrameTimeForNextFrame().

[danakj, 2013-05-16 19:51:49]

LGTM if brian's happy!

[brianderson, 2013-05-16 19:54:23]

lgtm too

[ajuma, 2013-05-17 13:36:45]

PTAL.

https://chromiumcodereview.appspot.com/15139007/diff/48001/cc/trees/layer_tre...
File cc/trees/layer_tree_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/15139007/diff/48001/cc/trees/layer_tre...
cc/trees/layer_tree_host_unittest.cc:703: while (first_frame_time_ ==
base::TimeTicks::Now());
This test (and the next one) was flaking on Windows since sometimes there isn't
enough time between frames for the value of TimeTicks::Now() to change (since it
comes from a low-resolution clock on Windows). So now we wait till the value
changes.

[danakj, 2013-05-17 13:39:36]

LGTM

https://chromiumcodereview.appspot.com/15139007/diff/48001/cc/trees/layer_tre...
File cc/trees/layer_tree_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/15139007/diff/48001/cc/trees/layer_tre...
cc/trees/layer_tree_host_unittest.cc:663: while (first_frame_time_ ==
base::TimeTicks::Now());
nit: use {} instead of ; to end the line

https://chromiumcodereview.appspot.com/15139007/diff/48001/cc/trees/layer_tre...
cc/trees/layer_tree_host_unittest.cc:703: while (first_frame_time_ ==
base::TimeTicks::Now());
and here

[ajuma, 2013-05-17 13:46:49]

https://codereview.chromium.org/15139007/diff/48001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/15139007/diff/48001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest.cc:663: while (first_frame_time_ ==
base::TimeTicks::Now());
On 2013/05/17 13:39:36, danakj wrote:
> nit: use {} instead of ; to end the line

Done.

https://codereview.chromium.org/15139007/diff/48001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest.cc:703: while (first_frame_time_ ==
base::TimeTicks::Now());
On 2013/05/17 13:39:36, danakj wrote:
> and here

Done.

[commit-bot: I haz the power, 2013-05-17 17:26:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/15139007/54001

[commit-bot: I haz the power, 2013-05-17 23:22:45]

Change committed as 200920