cc: Make animations tick regardless of drawing.

cc: Make animations tick regardless of drawing.

The compositor should tick and finish animations even if it is not
drawing anything. It can not draw for various reasons:
1) The tab is backgrounded.
2) CanDraw() is false for any of its reasons.
3) PrepareToDraw() fails due to checkerboarding.
4) There is no damage on the screen to draw.

Currently the problems are:
- When backgrounded, the animations are ticked but their states are not updated.
- When CanDraw() is false, we don't draw, and animations just stop ticking.
- If we stop drawing when we damage the screen, we tick animations, but don't
  update animation state since this happens in DrawLayers().

To solve these problems, I've moved the animation control more explicitly out
of LayerTreeHostImpl. The proxy already calls Animate(). Now it will also call
UpdateAnimationState(). It always does this after calling Animate() regardless
if drawing or not.

Secondly, the missing UpdateAnimationState() call is added to the OnTimerTick
for background animation ticking.

We enable background ticking only when we change visibility, currently. But
when CanDraw() is false, we don't draw and thus don't tick animations. So
instead we add to LayerTreeHostImpl a UpdateBackgroundAnimateTicking() method.
We call this always after calling Animate() since that can remove animations -
it's something Animate() used to do. And we also call this:
a) After a commit - this could add new animations, or change visibility.
b) After changing CanDraw()'s state.

However, when PrepareToDraw() is false, we do not want to start new animations
so we let animations finish without starting new ones. This is verified by 
the LayerTreeHostAnimationTestCheckerboardDoesntStartAnimations test.

This is covered by adding single-thread mode to all the animation unit tests
(except those that call SetNeedsAnimate() which is not legal in single thread
mode). Also by new tests:
LayerTreeHostAnimationTestRunAnimationWhenNotCanDraw.RunSingleThread
LayerTreeHostAnimationTestRunAnimationWhenNotCanDraw.RunMultiThread
LayerTreeHostAnimationTestRunAnimationWhenNotVisible.RunSingleThread
LayerTreeHostAnimationTestRunAnimationWhenNotVisible.RunMultiThread
LayerTreeHostAnimationTestCheckerboardDoesntStartAnimations.RunMultiThread

Added scheduler tests:
SchedulerStateMachineTest.ReportIfNotDrawing
SchedulerStateMachineTest.ReportIfNotDrawingFromAcquiredTextures

R=ajuma@chromium.org
BUG=222915
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192671

[danakj, 2013-04-04 03:46:42]

There's a bunch of common boilerplate in the tests for adding/removing
animations, which I can abstract into the test baseclass in a follow-up CL.

[danakj, 2013-04-04 03:47:40]

This is a pre-req for https://codereview.chromium.org/12662021/

[ajuma, 2013-04-04 15:28:47]

I like the general direction here, but I'd like to make sure we still avoid
starting animations when we're skipping a frame because of checkerboarding.

https://chromiumcodereview.appspot.com/13613003/diff/1/cc/animation/layer_ani...
File cc/animation/layer_animation_controller.cc (right):

https://chromiumcodereview.appspot.com/13613003/diff/1/cc/animation/layer_ani...
cc/animation/layer_animation_controller.cc:119:
FinishAnimationsCompletedOnMainThread(controller_impl);
This will also change the way animations that are aborted or removed on the main
thread are handled by the impl thread -- instead of removing them, we'll mark
them as Finished and later send a Finished event. Sending a Finished event for
an aborted animation seems ok (since aborting an animation is a decision made by
LayerAnimationController), but sending a Finished event for an animation that
was explicitly removed by a call to RemoveAnimation seems wrong, particularly
since the animation might not even have started yet.

Also, note that there's an in-progress CL
(https://chromiumcodereview.appspot.com/13465014) to fix the issue with Finished
events using a different approach: not deleting finished animations on the main
thread side until the Finished event is received.

https://chromiumcodereview.appspot.com/13613003/diff/1/cc/animation/layer_ani...
cc/animation/layer_animation_controller.cc:190:
PurgeAnimationsMarkedForDeletion();
This creates a risk of pushing an animation from the main thread to the impl
thread more than once:
1) We push the animation to the impl thread.
2) The animation starts and finishes on the impl thread, sending events back to
the main thread.
3) Before the main thread has a chance to process these events, a commit occurs.
In this case, PushNewAnimationsToImplThread will push the animation to the impl
thread again, and it will run again.

https://chromiumcodereview.appspot.com/13613003/diff/1/cc/animation/layer_ani...
cc/animation/layer_animation_controller.cc:322: };
With the change below, this struct is now unused. Please remove.

https://chromiumcodereview.appspot.com/13613003/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://chromiumcodereview.appspot.com/13613003/diff/1/cc/trees/thread_proxy....
cc/trees/thread_proxy.cc:907: layer_tree_host_impl_->UpdateAnimationState();
If the call to PrepareToDraw (above) returned false, causing us to skip the
frame, we don't want to start animations (to avoid regressing
http://crbug.com/123593), but it makes sense to continue ticking/finishing
in-progress animations. We could add a |DoNotPromoteAnimations| argument to
UpdateAnimationState, passing it on to LayerAnimationController::UpdateState,
preventing this from calling PromoteStartedAnimations.

[danakj, 2013-04-04 18:28:25]

PTAL 

I've reverted the LayerAnimationController changes as we discussed, and stuck a
TODO(ajuma) in the failing test saying to make it test that animations finish.

https://chromiumcodereview.appspot.com/13613003/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://chromiumcodereview.appspot.com/13613003/diff/1/cc/trees/thread_proxy....
cc/trees/thread_proxy.cc:907: layer_tree_host_impl_->UpdateAnimationState();
On 2013/04/04 15:28:47, ajuma wrote:
> If the call to PrepareToDraw (above) returned false, causing us to skip the
> frame, we don't want to start animations (to avoid regressing
> http://crbug.com/123593), but it makes sense to continue ticking/finishing
> in-progress animations. We could add a |DoNotPromoteAnimations| argument to
> UpdateAnimationState, passing it on to LayerAnimationController::UpdateState,
> preventing this from calling PromoteStartedAnimations.

Ok, I've done this and added
LayerTreeHostAnimationTestCheckerboardDoesntStartAnimations.RunMultiThread.

This test does not run in single thread mode as it does not  skip drawing due to
checkerboard.

[danakj, 2013-04-04 19:02:18]

Confirmed this still makes the unit tests work for
https://codereview.chromium.org/12662021/ as well.

[ajuma, 2013-04-04 19:07:04]

LGTM. Thanks for the tests!

[enne (OOO), 2013-04-04 23:36:41]

I absolutely agree that animations should tick if there's no damage, as a
further animation could cause damage.

However, why do you need to update animations if CanDraw() or PrepareToDraw()
fails?  What would be different than if you just updated them before the next
time you drew at the right time?

[ajuma, 2013-04-05 00:00:53]

On 2013/04/04 23:36:41, enne wrote:
> I absolutely agree that animations should tick if there's no damage, as a
> further animation could cause damage.
> 
> However, why do you need to update animations if CanDraw() or PrepareToDraw()
> fails?  What would be different than if you just updated them before the next
> time you drew at the right time?

By updating animations in this situation, we can still send Finished events
right away for animations that happened to finish, rather than unnecessarily
delaying these events until the next draw.

[danakj, 2013-04-05 01:17:19]

On 2013/04/05 00:00:53, ajuma wrote:
> On 2013/04/04 23:36:41, enne wrote:
> > I absolutely agree that animations should tick if there's no damage, as a
> > further animation could cause damage.
> > 
> > However, why do you need to update animations if CanDraw() or
PrepareToDraw()
> > fails?  What would be different than if you just updated them before the
next
> > time you drew at the right time?
> 
> By updating animations in this situation, we can still send Finished events
> right away for animations that happened to finish, rather than unnecessarily
> delaying these events until the next draw.

That. Basically the compositor's state shouldn't block javascript (with
exception of starting new animations so they appear correctly).

[enne (OOO), 2013-04-05 16:01:12]

Ok.  I think I am convinced by danakj's offline comments that we were already
doing all this ticking, just not when we failed PrepareToDraw or !CanDraw().

I have one comment, though:

https://codereview.chromium.org/13613003/diff/10002/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/13613003/diff/10002/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:682: bool able_to_draw = visible_ && CanDraw();
These two lines of code bake in a bunch assumptions from code elsewhere about
when animations are actually updated and when they won't.

I think it'd be a little bit more elegant if it were possible to just call the
original SetBackgroundTickingEnabled(true) from the same bit of code
(ScheduledActionDrawAndSwapInternal or DoComposite) when it doesn't
UpdateAnimationState.

[danakj, 2013-04-05 16:38:58]

https://codereview.chromium.org/13613003/diff/10002/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/13613003/diff/10002/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:682: bool able_to_draw = visible_ && CanDraw();
On 2013/04/05 16:01:12, enne wrote:
> These two lines of code bake in a bunch assumptions from code elsewhere about
> when animations are actually updated and when they won't.
> 
> I think it'd be a little bit more elegant if it were possible to just call the
> original SetBackgroundTickingEnabled(true) from the same bit of code
> (ScheduledActionDrawAndSwapInternal or DoComposite) when it doesn't
> UpdateAnimationState.

Ok, PTAL.

[enne (OOO), 2013-04-05 17:06:53]

https://codereview.chromium.org/13613003/diff/21005/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/13613003/diff/21005/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:352: !layer_tree_host_impl_->visible() || !can_draw);
I think this is halfway to what I was trying to ask for.  There are parts of the
code that call UpdateAnimationState and then sometimes you don't get to those
parts.  Is it possible to call UpdateBackgroundAnimateTicking(true) in such
cases, rather than checking states here?

[danakj, 2013-04-05 17:54:49]

https://codereview.chromium.org/13613003/diff/21005/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/13613003/diff/21005/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:352: !layer_tree_host_impl_->visible() || !can_draw);
On 2013/04/05 17:06:53, enne wrote:
> I think this is halfway to what I was trying to ask for.  There are parts of
the
> code that call UpdateAnimationState and then sometimes you don't get to those
> parts.  Is it possible to call UpdateBackgroundAnimateTicking(true) in such
> cases, rather than checking states here?

To be clear, background animate ticking causes us to run
Animate()+UpdateAnimateState() on a timer. It's for when we won't be drawing, so
animations will stop progressing otherwise.

So it's about more than UpadteAnimateState(). Any time we entire a state where
a) we're not drawing and b) we have an animation, then we want to enable
background animation ticking.

is this what you were thinking of when you asked for this?

[danakj, 2013-04-05 17:55:37]

On 2013/04/05 17:54:49, danakj wrote:
> So it's about more than UpadteAnimateState(). Any time we entire a 

Any time we entire => Any time we enter

[danakj, 2013-04-05 18:28:22]

Instead of requiring logic in multiple places, and having the proxy guess if we
will draw or not based on visibility etc, I added a helper WillDrawAsNeeded() on
the scheduler that gives a real answer if we are currently able to draw or not,
and use that.

Added a similar ShouldComposite() on the single thread proxy/

[danakj, 2013-04-05 18:52:12]

New tests:
SchedulerStateMachineTest.ReportIfNotDrawing 
SchedulerStateMachineTest.ReportIfNotDrawingFromAcquiredTextures

[enne (OOO), 2013-04-05 21:08:32]

Thanks, that helps a lot.  lgtm if this still looks good to ajuma.

[ajuma, 2013-04-05 21:29:59]

On 2013/04/05 21:08:32, enne wrote:
> Thanks, that helps a lot.  lgtm if this still looks good to ajuma.

Still lgtm.

[commit-bot: I haz the power, 2013-04-05 21:32:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/13613003/27005

[commit-bot: I haz the power, 2013-04-06 00:35:22]

Change committed as 192671