Moving background animation ticking from LayerTreeHostImpl into the Scheduler.

Moving background animation ticking from LayerTreeHostImpl into the Scheduler.

Background ticking currently happens in the LayerTreeHostImpl, this makes it very hard to preserve the monotonicity guarantee needed by frame times. http://crrev.com/267783004 set up the scheduler to become the source of background ticks and this CL makes use of that functionality.

BUG=345459
Committed: https://crrev.com/4df3c4366015739a7c6b6c1539a8d7c9198e38ef
Cr-Commit-Position: refs/heads/master@{#302757}

Committed: https://crrev.com/185438b3e5e3f43ce2996da0022f510fbe193a6e
Cr-Commit-Position: refs/heads/master@{#303423}

Committed: https://crrev.com/719bf67948d010b5462993ee1b2697f437053c9c
Cr-Commit-Position: refs/heads/master@{#303467}

[mithro-old, 2014-09-30 12:16:29]

mithro@mithis.com changed reviewers:
+ brianderson@chromium.org, danakj@chromium.org

[mithro-old, 2014-10-01 07:25:22]

mithro@mithis.com changed reviewers:
+ skyostil@chromium.org

[mithro-old, 2014-10-01 15:41:04]

Hi everyone,

Now the scheduler is in a good place regarding BeginFrameSources I'm trying to
remove background ticking from the LayerTreeHost. This is going mostly okay, but
I need some help figuring out the history / reason behind some current code.

My two major questions are;

 * Why is LTHI->UpdateAnimationState occurring in DoSwapInternal? 
   This seems to tie animation state change to drawing occurring?


 * What is the point of the "start_ready_animations" argument to
   LTHI->UpdateAnimationState? 

   It seems to exist to make sure that animations only marked started
   when they successfully draw. Is this some type of "animations are
   only started when they are displayed on screen" requirement?

   start_ready_animations seems to be unrelated to checkerboarding,
   as that is controlled by "animations_frozen_until_next_draw"
   parameter.

   Background ticking currently always sets this value to true.


Can anyone give me more insight to what is going on here? 

It you have a good understanding of the LayerTreeHost tests, some suggestions on
the correct places to look to figure out why they are really failing would also
be helpful (I seem to have made a couple flaky). I've believe I've gotten the
failures down to just tests which are actually indicating issues in this change.


Below is also my current understanding of things;

The "background flow", we previous did the following;
---------------------------------------------------------
 LTHI->Animate(frame_time)
 LTHI->UpdateAnimationState(true)
 if (LTHI->pending_tree()) {
   LTHI->pending_tree()->UpdateDrawProperties();
   LTHI->ManageTiles();
 }
---------------------------------------------------------

In the "foreground" flow, we currently do;
---------------------------------------------------------
 ThreadProxy->ScheduledActionAnimate() {
   LTHI->Animate(frame_time)
 }

 ThreadProxy->ScheduledBeginMainFrame() then
ThreadProxy->ScheduledActionCommit() {
   // Animations could be updated here...
 }

 ThreadProxy->ScheduledActionDrawAndSwapIfPossible() ||
ThreadProxy->ScheduledActionDrawAndSwapForced()
 (via ThreadProxy->DoSwapInternal) {
   ...
   (maybe) LTHI->Animate(frame_time)
   ...
   LTHI->UpdateAnimationState(forced_draw || result == DRAW_SUCCESS)
   ...
 }

 ThreadProxy->ScheduledActionManageTiles() {
   LTHI->ManageTiles()
 }
---------------------------------------------------------

There are also two logic flags which are important to animating;
 * animations_frozen_until_next_draw
   * The mode is toggled on/off in ScheduledActionDrawAndSwap* by getting a draw
result == DRAW_ABORTED_CHECKERBOARD_ANIMATIONS
   * When enabled, animation time is updated in ScheduledActionCommit (using
time in the main thread) rather than ScheduledActionAnimate (using time in the
impl thread).

 * did_commit_after_animating - This should go away with
https://codereview.chromium.org/621823003/

Notable, both are ignored by the current background ticking implementation. 

Thanks for your help!

Tim 'mithro' Ansell

[danakj, 2014-10-01 15:44:24]

danakj@chromium.org changed reviewers:
+ ajuma@chromium.org

[danakj, 2014-10-01 15:44:24]

+ajuma who can help answer these questions

[ajuma, 2014-10-01 17:03:59]

On 2014/10/01 15:41:04, mithro wrote:
> My two major questions are;
> 
>  * Why is LTHI->UpdateAnimationState occurring in DoSwapInternal? 
>    This seems to tie animation state change to drawing occurring?
> 
> 
>  * What is the point of the "start_ready_animations" argument to
>    LTHI->UpdateAnimationState? 
> 
>    It seems to exist to make sure that animations only marked started
>    when they successfully draw. Is this some type of "animations are
>    only started when they are displayed on screen" requirement?

Both of these are because we don't want to start animations when we'll
immediately skip frames due to checkerboarding (since otherwise, the animation
will suddenly start part way through, rather than being visible from beginning
to end). The logic to accomplish this is to hold animations in a "Starting"
state until we have a successful draw. We communicate this to
UpdateAnimationState using start_ready_animations.


>    start_ready_animations seems to be unrelated to checkerboarding,
>    as that is controlled by "animations_frozen_until_next_draw"
>    parameter.

animations_frozen_until_next_draw deals with a different kind of checkerboarding
-- checkerboarding in the middle of an animation rather than at the start. This
is only used with non-impl-side painting (see crbug.com/260979) so I'm planning
to delete this.

>    Background ticking currently always sets this value to true.

Right, there's no point holding animations till draw when background ticking,
since we're not drawing in that state.

[mithro-old, 2014-11-04 14:48:26]

Can everyone please take another look? This CL now passes all the cc_unittests
locally on my machine (will fix issues found on the try bots).

Once I figured out the issue with the tests, I was able to reduce the complexity
meaning this CL removes 110 lines from Chrome! Of which a majority are from
LayerTreeHostImpl, which I think is a big win. It is also the last requirement
before I can go about removing Now() from LayerTreeHostImpl and finally make it
all use the passed in BeginFrameTime!!!!

Things should get even simpler when we remove the non-scheduler-client case of
single_thread_proxy.

Tim 'mithro' Ansell

[brianderson, 2014-11-04 18:54:39]

https://codereview.chromium.org/595973002/diff/460001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_settings.cc (right):

https://codereview.chromium.org/595973002/diff/460001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_settings.cc:39: 1.0 /
settings.background_animation_rate)) {
Should we rename the LT setting?

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

https://codereview.chromium.org/595973002/diff/460001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3097: SetNeedsAnimate();
How is this SetNeedsAnimate related to background ticking?

https://codereview.chromium.org/595973002/diff/460001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/595973002/diff/460001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:366:
scheduler_on_impl_thread_->SetNeedsAnimate();
Is this just a cleanup, or is it also needed for the new background ticking to
work?

https://codereview.chromium.org/595973002/diff/460001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/595973002/diff/460001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:939: if
(!impl().layer_tree_host_impl->AnimationsAreVisible()) {
Should we make this part of the aborted draw and swap?

[mithro-old, 2014-11-04 22:16:51]

Thank you for your speedy review, specially while BlinkOn is happening, it's
super appreciated.

Tim 'mithro' Ansell

https://codereview.chromium.org/595973002/diff/460001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_settings.cc (right):

https://codereview.chromium.org/595973002/diff/460001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_settings.cc:39: 1.0 /
settings.background_animation_rate)) {
On 2014/11/04 18:54:38, brianderson wrote:
> Should we rename the LT setting?

I think the "background_frame_interval" and "background_animation_rate" are
actually slightly different concepts which are related at the moment but might
not be in the future.

If you think they are the same, then I'm happy to change to one name or the
other.

(Also rate verse interval?)

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

https://codereview.chromium.org/595973002/diff/460001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3097: SetNeedsAnimate();
On 2014/11/04 18:54:38, brianderson wrote:
> How is this SetNeedsAnimate related to background ticking?

This is needed otherwise SetNeedsAnimate is not always set when background
ticking. The previous background code didn't depend on SetNeedsAnimate to be set
for things to still animate.

https://codereview.chromium.org/595973002/diff/460001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/595973002/diff/460001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:366:
scheduler_on_impl_thread_->SetNeedsAnimate();
On 2014/11/04 18:54:38, brianderson wrote:
> Is this just a cleanup, or is it also needed for the new background ticking to
> work?

Both.

Previously, the SetNeedsAnimateOnImplThread was not doing the right thing (IE
not calling SetNeedsAnimate). However, as animate only happened when we redrew,
this code was almost equivalent. Now that a redraw and animate are not longer
equivalent this change is needed.

As well, when background ticking previously, SetNeedsAnimate was never called
because it was all handled internally in the LayerTreeHostImpl.

Does that make sense?

https://codereview.chromium.org/595973002/diff/460001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/595973002/diff/460001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:939: if
(!impl().layer_tree_host_impl->AnimationsAreVisible()) {
On 2014/11/04 18:54:38, brianderson wrote:
> Should we make this part of the aborted draw and swap?

As far as I can tell, ScheduledActionDrawAndSwapIfPossible is never even called
when AnimationsAreVisible is false at the moment.

I think it's probably worth landing this change and then looking if this could
be moved into ScheduledActionDrawAndSwapIfPossbile in another CL?

[brianderson, 2014-11-04 23:32:20]

Thanks for the explanations, just one more question.

https://codereview.chromium.org/595973002/diff/460001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_settings.cc (right):

https://codereview.chromium.org/595973002/diff/460001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_settings.cc:39: 1.0 /
settings.background_animation_rate)) {
On 2014/11/04 22:16:50, mithro wrote:
> On 2014/11/04 18:54:38, brianderson wrote:
> > Should we rename the LT setting?
> 
> I think the "background_frame_interval" and "background_animation_rate" are
> actually slightly different concepts which are related at the moment but might
> not be in the future.
> 
> If you think they are the same, then I'm happy to change to one name or the
> other.
> 
> (Also rate verse interval?)

Don't care too much. It also guess it's nice that background_animation_rate
matches it's neighbor refresh_rate in the list of settings it belongs to.

https://codereview.chromium.org/595973002/diff/460001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/595973002/diff/460001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:364: client_->ScheduleComposite();
Also, why is ScheduleComposite needed instead of SetNeedsRedrawOnImplThread now?

https://codereview.chromium.org/595973002/diff/460001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/595973002/diff/460001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:939: if
(!impl().layer_tree_host_impl->AnimationsAreVisible()) {
On 2014/11/04 22:16:50, mithro wrote:
> On 2014/11/04 18:54:38, brianderson wrote:
> > Should we make this part of the aborted draw and swap?
> 
> As far as I can tell, ScheduledActionDrawAndSwapIfPossible is never even
called
> when AnimationsAreVisible is false at the moment.
> 
> I think it's probably worth landing this change and then looking if this could
> be moved into ScheduledActionDrawAndSwapIfPossbile in another CL?

Sure, can you open a bug?

[mithro-old, 2014-11-05 00:02:35]

mithro@mithis.com changed required reviewers:
+ brianderson@chromium.org

[mithro-old, 2014-11-05 00:02:36]

Thanks once again Brian.

Dana / Ali do you have any comments on this or are you happy with Brian's
judgement?

Tim

https://codereview.chromium.org/595973002/diff/460001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_settings.cc (right):

https://codereview.chromium.org/595973002/diff/460001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_settings.cc:39: 1.0 /
settings.background_animation_rate)) {
On 2014/11/04 23:32:19, brianderson wrote:
> On 2014/11/04 22:16:50, mithro wrote:
> > On 2014/11/04 18:54:38, brianderson wrote:
> > > Should we rename the LT setting?
> > 
> > I think the "background_frame_interval" and "background_animation_rate" are
> > actually slightly different concepts which are related at the moment but
might
> > not be in the future.
> > 
> > If you think they are the same, then I'm happy to change to one name or the
> > other.
> > 
> > (Also rate verse interval?)
> 
> Don't care too much. It also guess it's nice that background_animation_rate
> matches it's neighbor refresh_rate in the list of settings it belongs to.

Yeah, that is why I chose that name. 

This change (which I did to make the testing work) actually sets up the
possibility of having the background ticking length more configurable, which is
nice.

https://codereview.chromium.org/595973002/diff/460001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/595973002/diff/460001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:364: client_->ScheduleComposite();
On 2014/11/04 23:32:19, brianderson wrote:
> Also, why is ScheduleComposite needed instead of SetNeedsRedrawOnImplThread
now?

The old code use to just delegate to SetNeedsRedrawOnImplThread.
SetNeedsRedrawOnImplThread calls ScheduleComposite() and then SetNeedsRedraw()
on the scheduler (see lines 357 to 361).

Since we want to change the code to call SetNeedsAnimate rather than
SetNeedsRedraw we need to reimplement the calls in SetNeedsRedrawOnImplThread.

https://codereview.chromium.org/595973002/diff/460001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/595973002/diff/460001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:939: if
(!impl().layer_tree_host_impl->AnimationsAreVisible()) {
On 2014/11/04 23:32:19, brianderson wrote:
> On 2014/11/04 22:16:50, mithro wrote:
> > On 2014/11/04 18:54:38, brianderson wrote:
> > > Should we make this part of the aborted draw and swap?
> > 
> > As far as I can tell, ScheduledActionDrawAndSwapIfPossible is never even
> called
> > when AnimationsAreVisible is false at the moment.
> > 
> > I think it's probably worth landing this change and then looking if this
could
> > be moved into ScheduledActionDrawAndSwapIfPossbile in another CL?
> 
> Sure, can you open a bug?

Will do when this code lands (incase this changes).

[brianderson, 2014-11-05 00:41:40]

lgtm

[ajuma, 2014-11-05 01:10:29]

lgtm too

[mithro-old, 2014-11-05 04:45:50]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-11-05 04:46:41]

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

[commit-bot: I haz the power, 2014-11-05 05:30:55]

Committed patchset #9 (id:460001)

[commit-bot: I haz the power, 2014-11-05 05:31:34]

Patchset 9 (id:??) landed as
https://crrev.com/4df3c4366015739a7c6b6c1539a8d7c9198e38ef
Cr-Commit-Position: refs/heads/master@{#302757}

[mithro-old, 2014-11-05 07:35:58]

A revert of this CL (patchset #9 id:460001) has been created in
https://codereview.chromium.org/698413002/ by mithro@mithis.com.

The reason for reverting is: Reverting because of failure here
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

Will investigate tomorrow.


.

[mithro-old, 2014-11-10 04:00:58]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-11-10 04:01:39]

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

[commit-bot: I haz the power, 2014-11-10 04:50:40]

Committed patchset #10 (id:480001)

[commit-bot: I haz the power, 2014-11-10 04:51:18]

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

[mithro-old, 2014-11-10 06:20:16]

A revert of this CL (patchset #10 id:480001) has been created in
https://codereview.chromium.org/713783002/ by mithro@mithis.com.

The reason for reverting is: Reverting because it breaks the following
interactive_ui_tests;
 * InterruptedAutoStartEnrollment
 * LoginUIVisible
 * PRE_InterruptedAutoStartEnrollment
 * PRE_LoginUIVisible

The failure log is;

[27898:27924:1109/213628:FATAL:layer_animation_controller.cc(215)] Check failed:
last_tick_time_ != base::TimeTicks().
#0 0x7f92b9beb75e base::debug::StackTrace::StackTrace()
#1 0x7f92b9c7ec82 logging::LogMessage::~LogMessage()
#2 0x7f92b8cdafc4 cc::LayerAnimationController::UpdateState()
#3 0x7f92b8fe9474 cc::LayerTreeHostImpl::UpdateAnimationState()
#4 0x7f92b902ecf4 cc::ThreadProxy::DrawSwapInternal()
#5 0x7f92b902f542 cc::ThreadProxy::ScheduledActionDrawAndSwapIfPossible()
#6 0x7f92b902f59c cc::ThreadProxy::ScheduledActionDrawAndSwapIfPossible()
#7 0x7f92b8f83324 cc::Scheduler::DrawAndSwapIfPossible()
#8 0x7f92b8f80541 cc::Scheduler::ProcessScheduledActions()
#9 0x7f92b8f7f54d cc::Scheduler::OnBeginImplFrameDeadline().

[mithro-old, 2014-11-10 14:43:22]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-11-10 14:44:17]

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

[commit-bot: I haz the power, 2014-11-10 15:37:01]

Committed patchset #12 (id:520001)

[commit-bot: I haz the power, 2014-11-10 15:37:51]

Patchset 12 (id:??) landed as
https://crrev.com/719bf67948d010b5462993ee1b2697f437053c9c
Cr-Commit-Position: refs/heads/master@{#303467}