cc: Removing base::TimeTicks argument to DrawLayers as it isn't used.

cc: Removing base::TimeTicks argument to DrawLayers as it isn't used.

R=skyostil,brianderson
BUG=346230
Committed: https://crrev.com/248d172e6a28652b0159dfc5c33425c7507f8034
Cr-Commit-Position: refs/heads/master@{#328283}

[mithro-old, 2014-06-18 07:21:39]

Hi!

A CL which removes base::TimeTicks from DrawLayers as it doesn't appear to
actually be needed as the only user is the fps_counter_ which can just use the
local CurrentTimeTicks method?

Can you see any issues here?

Tim 'mithro' Ansell

[danakj, 2014-06-18 15:23:19]

In general I prefer pushing needed data to methods rather than having them pull
the data from elsewhere, but since this is a member var on the LTHI, I think it
makes sense.

Looks like some unrelated changes in this CL though?

https://codereview.chromium.org/340743002/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (left):

https://codereview.chromium.org/340743002/diff/1/cc/trees/thread_proxy.cc#old...
cc/trees/thread_proxy.cc:1063:
impl().layer_tree_host_impl->Animate(impl().animation_time);
This change is not part of the CL description.

[mithro-old, 2014-07-02 05:39:51]

Hi Dana,

PTAL.

Thanks!

Tim

https://codereview.chromium.org/340743002/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (left):

https://codereview.chromium.org/340743002/diff/1/cc/trees/thread_proxy.cc#old...
cc/trees/thread_proxy.cc:1063:
impl().layer_tree_host_impl->Animate(impl().animation_time);
On 2014/06/18 15:23:18, danakj wrote:
> This change is not part of the CL description.

This was from the CL this one was based on. It should be removed in the latest
patchset.

[danakj, 2014-07-02 18:51:38]

https://codereview.chromium.org/340743002/diff/20001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (left):

https://codereview.chromium.org/340743002/diff/20001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:432: layer_tree_host_impl_->DrawLayers(frame,
frame_begin_time);
How does the frame_begin_time get to the LTHI for DrawLayers in this case? Would
this break the FPS counter in the UI compositor? --ui-show-fps-counter to see
it. If not, can you explain why not?

[mithro-old, 2014-07-07 05:27:32]

https://codereview.chromium.org/340743002/diff/20001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (left):

https://codereview.chromium.org/340743002/diff/20001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:432: layer_tree_host_impl_->DrawLayers(frame,
frame_begin_time);
On 2014/07/02 18:51:38, danakj wrote:
> How does the frame_begin_time get to the LTHI for DrawLayers in this case?
Would
> this break the FPS counter in the UI compositor? --ui-show-fps-counter to see
> it. If not, can you explain why not?

(I fixed this code to use the CurrentFrameTimeTicks() method.)

The internal current_time_ticks_ value is updated in the above Animate call if
it hasn't already been set.

BTW Would it make more sense that the fps_counter_ is updated in Animate? It is
the only thing which talks about time in this area, everything else is about
doing the actual drawing.

[brianderson, 2014-07-07 20:22:48]

nit: I don't think there is a hard rule, but consider keeping the patch title
shorter by using simple+present+active voice and moving the 'why' to the patch
description.

Otherwise, lgtm.

[danakj, 2014-07-07 20:56:45]

https://codereview.chromium.org/340743002/diff/20001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (left):

https://codereview.chromium.org/340743002/diff/20001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:432: layer_tree_host_impl_->DrawLayers(frame,
frame_begin_time);
On 2014/07/07 05:27:32, mithro wrote:
> On 2014/07/02 18:51:38, danakj wrote:
> > How does the frame_begin_time get to the LTHI for DrawLayers in this case?
> Would
> > this break the FPS counter in the UI compositor? --ui-show-fps-counter to
see
> > it. If not, can you explain why not?
> 
> (I fixed this code to use the CurrentFrameTimeTicks() method.)
> 
> The internal current_time_ticks_ value is updated in the above Animate call if
> it hasn't already been set.
> 
> BTW Would it make more sense that the fps_counter_ is updated in Animate? It
is
> the only thing which talks about time in this area, everything else is about
> doing the actual drawing.

The problem with doing it in animate is that we don't necessarily draw after
animate, and we want the fps meter to track only frames that we draw.

There are many ways to shave this yak, like we could save the time in the
FrameData instead or something.

So this frame_begin_time is unused in the impl-side of a single-thread
draw/composite. Can we stop passing it?

[mithro-old, 2014-07-08 04:47:07]

On 2014/07/07 20:56:45, danakj wrote:
>
https://codereview.chromium.org/340743002/diff/20001/cc/trees/single_thread_p...
> File cc/trees/single_thread_proxy.cc (left):
> 
>
https://codereview.chromium.org/340743002/diff/20001/cc/trees/single_thread_p...
> cc/trees/single_thread_proxy.cc:432: layer_tree_host_impl_->DrawLayers(frame,
> frame_begin_time);
> On 2014/07/07 05:27:32, mithro wrote:
> > On 2014/07/02 18:51:38, danakj wrote:
> > > How does the frame_begin_time get to the LTHI for DrawLayers in this case?
> > Would
> > > this break the FPS counter in the UI compositor? --ui-show-fps-counter to
> see
> > > it. If not, can you explain why not?
> > 
> > (I fixed this code to use the CurrentFrameTimeTicks() method.)
> > 
> > The internal current_time_ticks_ value is updated in the above Animate call
if
> > it hasn't already been set.
> > 
> > BTW Would it make more sense that the fps_counter_ is updated in Animate? It
> is
> > the only thing which talks about time in this area, everything else is about
> > doing the actual drawing.
> 
> The problem with doing it in animate is that we don't necessarily draw after
> animate, and we want the fps meter to track only frames that we draw.
> 
> There are many ways to shave this yak, like we could save the time in the
> FrameData instead or something.
> 
> So this frame_begin_time is unused in the impl-side of a single-thread
> draw/composite. Can we stop passing it?

Okay if I land this patch as is and look at shaving the yak below in another
one?

Actually tagging the frame data with the time we used to generate it sounds like
a generally good idea to me. (It would also be nice to tag tiles with frame time
data too, as that would fix the nasty priority hack.) If we did implement the
tagging I'm unsure if we should be passing the tagged time to the FPS meter or
the current time? In theory we should never be drawing for something which is
way behind the current time, but there might be cases where such a thing occurs?

Tim

[danakj, 2014-07-08 16:11:52]

On Tue, Jul 8, 2014 at 12:47 AM, <mithro@mithis.com> wrote:

> On 2014/07/07 20:56:45, danakj wrote:
>
> https://codereview.chromium.org/340743002/diff/20001/cc/
> trees/single_thread_proxy.cc
>
>> File cc/trees/single_thread_proxy.cc (left):
>>
>
>
> https://codereview.chromium.org/340743002/diff/20001/cc/
> trees/single_thread_proxy.cc#oldcode432
>
>> cc/trees/single_thread_proxy.cc:432: layer_tree_host_impl_->
>> DrawLayers(frame,
>> frame_begin_time);
>> On 2014/07/07 05:27:32, mithro wrote:
>> > On 2014/07/02 18:51:38, danakj wrote:
>> > > How does the frame_begin_time get to the LTHI for DrawLayers in this
>> case?
>> > Would
>> > > this break the FPS counter in the UI compositor?
>> --ui-show-fps-counter to
>> see
>> > > it. If not, can you explain why not?
>> >
>> > (I fixed this code to use the CurrentFrameTimeTicks() method.)
>> >
>> > The internal current_time_ticks_ value is updated in the above Animate
>> call
>>
> if
>
>> > it hasn't already been set.
>> >
>> > BTW Would it make more sense that the fps_counter_ is updated in
>> Animate? It
>> is
>> > the only thing which talks about time in this area, everything else is
>> about
>> > doing the actual drawing.
>>
>
>  The problem with doing it in animate is that we don't necessarily draw
>> after
>> animate, and we want the fps meter to track only frames that we draw.
>>
>
>  There are many ways to shave this yak, like we could save the time in the
>> FrameData instead or something.
>>
>
>  So this frame_begin_time is unused in the impl-side of a single-thread
>> draw/composite. Can we stop passing it?
>>
>
> Okay if I land this patch as is and look at shaving the yak below in
> another
> one?
>

It's a small change and you're leaving an unused variable behind, i'd
rather you did it now.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mithro-old, 2014-07-10 06:28:25]

> 
> It's a small change and you're leaving an unused variable behind, i'd
> rather you did it now.

Oh, sorry I misunderstood what you where asking me to do! DoComposite no longer
takes the unused timeticks argument now.

Will check the try bots and then see if it will land.

Tim

[danakj, 2014-07-10 17:23:26]

THanks, LGTM

BTW please try avoid uploading a patchset that contains both a rebase and your
own changes in it.

[mithro-old, 2014-07-10 22:35:18]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-07-10 22:37:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mithro@mithis.com/340743002/100001

[commit-bot: I haz the power, 2014-07-10 23:16:50]

Change committed as 282456

[mithro-old, 2015-04-24 04:40:04]

On 2014/07/10 23:16:50, I haz the power (commit-bot) wrote:
> Change committed as 282456

This patch can be relanded after https://codereview.chromium.org/1099703004/ has
landed.

[danakj, 2015-04-24 17:24:23]

https://codereview.chromium.org/340743002/diff/160001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/340743002/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1463: base::TimeTicks frame_begin_time =
CurrentBeginFrameArgs().frame_time;
I think I prefer passing things to a function rather than pulling them out of
some state on the class when it's not troublesome to do so.

[danakj, 2015-04-24 17:24:47]

https://codereview.chromium.org/340743002/diff/160001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/340743002/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1463: base::TimeTicks frame_begin_time =
CurrentBeginFrameArgs().frame_time;
On 2015/04/24 17:24:22, danakj wrote:
> I think I prefer passing things to a function rather than pulling them out of
> some state on the class when it's not troublesome to do so.

(Esp for such a complicated/huge class as this)

[mithro-old, 2015-04-27 02:17:45]

On 2015/04/24 17:24:47, danakj wrote:
>
https://codereview.chromium.org/340743002/diff/160001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/340743002/diff/160001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_impl.cc:1463: base::TimeTicks frame_begin_time =
> CurrentBeginFrameArgs().frame_time;
> On 2015/04/24 17:24:22, danakj wrote:
> > I think I prefer passing things to a function rather than pulling them out
of
> > some state on the class when it's not troublesome to do so.
> 
> (Esp for such a complicated/huge class as this)

I agree that passing the value in would be a better solution normally. However,
at the moment we have the issue of having both internal state and passed in
state which *must match* for the system to work correctly, when these two states
are out of sync you get subtle errors. An example of this problem is that inside
DoComposite, DrawLayers is using a different time to DoAnimate() because it
never updates the internal LTHI value! This patch is making the function calls
consistent, so something like this can't happen in the future (as the only way
to update DrawLayers's time is to update the LTHI state).

Maybe in the future can work on eliminating the internal state of LTHI and
changing everything to passing the value in the future. 

Does that make sense?

Tim 'mithro' Ansell

[danakj, 2015-04-27 17:27:07]

On Sun, Apr 26, 2015 at 7:17 PM, <mithro@mithis.com> wrote:

> On 2015/04/24 17:24:47, danakj wrote:
>
>
>
https://codereview.chromium.org/340743002/diff/160001/cc/trees/layer_tree_hos...
>
>> File cc/trees/layer_tree_host_impl.cc (right):
>>
>
>
>
>
https://codereview.chromium.org/340743002/diff/160001/cc/trees/layer_tree_hos...
>
>> cc/trees/layer_tree_host_impl.cc:1463: base::TimeTicks frame_begin_time =
>> CurrentBeginFrameArgs().frame_time;
>> On 2015/04/24 17:24:22, danakj wrote:
>> > I think I prefer passing things to a function rather than pulling them
>> out
>>
> of
>
>> > some state on the class when it's not troublesome to do so.
>>
>
>  (Esp for such a complicated/huge class as this)
>>
>
> I agree that passing the value in would be a better solution normally.
> However,
> at the moment we have the issue of having both internal state and passed in
> state which *must match* for the system to work correctly, when these two
> states
> are out of sync you get subtle errors. An example of this problem is that
> inside
> DoComposite, DrawLayers is using a different time to DoAnimate() because it
> never updates the internal LTHI value! This patch is making the function
> calls
> consistent, so something like this can't happen in the future (as the only
> way
> to update DrawLayers's time is to update the LTHI state).
>

How about passing the time to DoAnimate then?


>
> Maybe in the future can work on eliminating the internal state of LTHI and
> changing everything to passing the value in the future.
>
> Does that make sense?
>
> Tim 'mithro' Ansell
>
> https://codereview.chromium.org/340743002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mithro-old, 2015-04-28 01:28:05]

On 2015/04/27 17:27:07, danakj wrote:
> On Sun, Apr 26, 2015 at 7:17 PM, <mailto:mithro@mithis.com> wrote:
> 
> > On 2015/04/24 17:24:47, danakj wrote:
> >
> >
> >
>
https://codereview.chromium.org/340743002/diff/160001/cc/trees/layer_tree_hos...
> >
> >> File cc/trees/layer_tree_host_impl.cc (right):
> >>
> >
> >
> >
> >
>
https://codereview.chromium.org/340743002/diff/160001/cc/trees/layer_tree_hos...
> >
> >> cc/trees/layer_tree_host_impl.cc:1463: base::TimeTicks frame_begin_time =
> >> CurrentBeginFrameArgs().frame_time;
> >> On 2015/04/24 17:24:22, danakj wrote:
> >> > I think I prefer passing things to a function rather than pulling them
> >> out
> >>
> > of
> >
> >> > some state on the class when it's not troublesome to do so.
> >>
> >
> >  (Esp for such a complicated/huge class as this)
> >>
> >
> > I agree that passing the value in would be a better solution normally.
> > However,
> > at the moment we have the issue of having both internal state and passed in
> > state which *must match* for the system to work correctly, when these two
> > states
> > are out of sync you get subtle errors. An example of this problem is that
> > inside
> > DoComposite, DrawLayers is using a different time to DoAnimate() because it
> > never updates the internal LTHI value! This patch is making the function
> > calls
> > consistent, so something like this can't happen in the future (as the only
> > way
> > to update DrawLayers's time is to update the LTHI state).
> >
> 
> How about passing the time to DoAnimate then?

It's not just DoAnimate, any function which uses time would need to be modified
and there are a couple of locations where passing the time around needs
significant modifications to the current code. It also would need modifying
thousands of tests. I actually attempted this a while back and the change is
about ~3000-4000 LOC before I gave up. The always using the internal state
option is easier as the public API only has a couple functions which currently
do time passing.

[brianderson, 2015-05-01 01:18:43]

I do see value in having it passed in as an argument if we ever support a queue
of frames where the main/raster/compositor stages could potentially all be
operating on different frames+timestamps. Having each of those timestamps stored
in different places could get confusing. Right now though, I don't have a strong
preference either way as long as we are consistent.

lgtm, pending Dana's review.

[brianderson, 2015-05-01 01:21:58]

Forgot to mention: a lot of tests were using Now() for the frame time and are
now using some default value instead that doesn't necessarily seem to change
between draw calls. Is that going to be okay?

[mithro-old, 2015-05-01 04:20:04]

On 2015/05/01 01:21:58, brianderson wrote:
> Forgot to mention: a lot of tests were using Now() for the frame time and are
> now using some default value instead that doesn't necessarily seem to change
> between draw calls. Is that going to be okay?

It seems to be.

On 2015/05/01 01:18:43, brianderson wrote:
> I do see value in having it passed in as an argument if we ever support a
queue
> of frames where the main/raster/compositor stages could potentially all be
> operating on different frames+timestamps. Having each of those timestamps
stored
> in different places could get confusing. Right now though, I don't have a
strong
> preference either way as long as we are consistent.
> 
> lgtm, pending Dana's review.

My goal is to get us into a state where we are consistent and safe. Then we can
look at how we can improve the interface to allow multiple stages with the
knowledge that we are working from a solid base.

[danakj, 2015-05-04 22:41:00]

LGTM

[mithro-old, 2015-05-05 04:20:02]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2015-05-05 04:21:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/340743002/200001

[commit-bot: I haz the power, 2015-05-05 05:23:55]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2015-05-05 05:25:01]

Patchset 10 (id:??) landed as
https://crrev.com/248d172e6a28652b0159dfc5c33425c7507f8034
Cr-Commit-Position: refs/heads/master@{#328283}