Thread ui opacity animations

Thread ui opacity animations

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

[ajuma, 2013-01-24 18:39:08]

I've updated the patch to ensure that sequences started together with
LayerAnimator::StartTogether or LayerAnimator::ScheduleTogether get the same
effective start time even when some are threaded and some aren't.

I still need to add some tests before this is ready for review.

[Ian Vollick, 2013-01-25 15:11:47]

I really like this approach. A few minor comments.

https://codereview.chromium.org/11896017/diff/3001/cc/animation_id_provider.h
File cc/animation_id_provider.h (right):

https://codereview.chromium.org/11896017/diff/3001/cc/animation_id_provider.h...
cc/animation_id_provider.h:19: }
nit: // namespace cc

https://codereview.chromium.org/11896017/diff/3001/ui/compositor/layer_animat...
File ui/compositor/layer_animation_element.cc (right):

https://codereview.chromium.org/11896017/diff/3001/ui/compositor/layer_animat...
ui/compositor/layer_animation_element.cc:539: need_draw =
OnProgress(Tween::CalculateValue(tween_type_, t), delegate);
I think you can just call OnProgress(t, delegate) since t == 1.0. Also, don't we
need to set first_frame_ = true in this case? We should have a test with an
accelerated animation in a looping sequence.

https://codereview.chromium.org/11896017/diff/3001/ui/compositor/layer_animat...
ui/compositor/layer_animation_element.cc:544: RequestEffectiveStart(delegate);
Just double checking -- it's not possible to call ::Progress before
::ProgressToEffective start, is it? I think you might end up calling
RequestEffectiveStart twice in that case, but maybe this is impossible.

https://codereview.chromium.org/11896017/diff/3001/ui/compositor/layer_animat...
File ui/compositor/layer_animation_sequence.cc (right):

https://codereview.chromium.org/11896017/diff/3001/ui/compositor/layer_animat...
ui/compositor/layer_animation_sequence.cc:195: return;
Can you add some DCHECKs that we do find the things we're searching for below.
Looking at the way this is called from LayerAnimator, I think it would be an
error if we found nothing.

[ajuma, 2013-01-29 18:31:23]

Patch updated to address comments, add/fix tests, and fix some bugs uncovered by
the tests.

One remaining issue: currently, visibility and opacity are allowed to animate
simultaneously. However, both opacity and visibility are handled in ui::Layer by
setting the opacity of the cc layer. This approach breaks when opacity is being
animated on the cc layer (since any attempt by the ui layer to change visibility
while opacity is animating will lose to the opacity animation). I'm inclined to
treat opacity and visibility as conflicting properties that simply aren't
allowed to animate simultaneously (much as we already prevent multiple
animations on the same property from happening simultaneously).

https://chromiumcodereview.appspot.com/11896017/diff/3001/ui/compositor/layer...
File ui/compositor/layer_animation_element.cc (right):

https://chromiumcodereview.appspot.com/11896017/diff/3001/ui/compositor/layer...
ui/compositor/layer_animation_element.cc:539: need_draw =
OnProgress(Tween::CalculateValue(tween_type_, t), delegate);
On 2013/01/25 15:11:47, vollick wrote:
> I think you can just call OnProgress(t, delegate) since t == 1.0. 

Done.

> Also, don't we
> need to set first_frame_ = true in this case?

We do need first_frame_ = true, but it's already true by the if condition.

> We should have a test with an
> accelerated animation in a looping sequence.

Added LayerAnimatorTest.ThreadedCyclicSequences.

https://chromiumcodereview.appspot.com/11896017/diff/3001/ui/compositor/layer...
ui/compositor/layer_animation_element.cc:544: RequestEffectiveStart(delegate);
On 2013/01/25 15:11:47, vollick wrote:
> Just double checking -- it's not possible to call ::Progress before
> ::ProgressToEffective start, is it? I think you might end up calling
> RequestEffectiveStart twice in that case, but maybe this is impossible.

Right, calling Progress before ProgressToEffectiveStart shouldn't happen. The
DCHECK(first_frame_) in ProgressToEffectiveStart will catch this.

https://chromiumcodereview.appspot.com/11896017/diff/3001/ui/compositor/layer...
File ui/compositor/layer_animation_sequence.cc (right):

https://chromiumcodereview.appspot.com/11896017/diff/3001/ui/compositor/layer...
ui/compositor/layer_animation_sequence.cc:195: return;
On 2013/01/25 15:11:47, vollick wrote:
> Can you add some DCHECKs that we do find the things we're searching for below.
> Looking at the way this is called from LayerAnimator, I think it would be an
> error if we found nothing.

DCHECK added.

[Ian Vollick, 2013-02-01 15:21:36]

Can we get rid of ui::Layer::opacity_ and return the cc layer's opacity instead
so that asking for the ui::Layer's opacity in the middle of an accelerated
animation returns something roughly correct?

[ajuma, 2013-02-04 16:06:57]

On 2013/02/01 15:21:36, vollick wrote:
> Can we get rid of ui::Layer::opacity_ and return the cc layer's opacity
instead
> so that asking for the ui::Layer's opacity in the middle of an accelerated
> animation returns something roughly correct?

Done.

Also, with https://chromiumcodereview.appspot.com/12093067 (which has now
landed), we no longer change the cc layer's opacity in order to change the ui
layer's visibility. This means that visibility and opacity can continue to
animate together even when opacity animations are threaded.

[Ian Vollick, 2013-02-13 03:59:38]

https://codereview.chromium.org/11896017/diff/21001/ui/compositor/layer_anima...
File ui/compositor/layer_animation_element.h (right):

https://codereview.chromium.org/11896017/diff/21001/ui/compositor/layer_anima...
ui/compositor/layer_animation_element.h:137: void
ProgressToEffectiveStart(LayerAnimationDelegate* delegate);
There's a big disconnect between this comment and the method name. It would be
nice to explain how dispatching to the compositor thread will affect the
effective start time.

https://codereview.chromium.org/11896017/diff/21001/ui/compositor/layer_anima...
File ui/compositor/layer_animation_sequence.cc (right):

https://codereview.chromium.org/11896017/diff/21001/ui/compositor/layer_anima...
ui/compositor/layer_animation_sequence.cc:48: LayerAnimationDelegate* delegate)
{
If we're waiting for group start, do we need to progress at all?

https://codereview.chromium.org/11896017/diff/21001/ui/compositor/layer_anima...
ui/compositor/layer_animation_sequence.cc:168:
elements_.push_back(make_linked_ptr(element));
Since we early out in LayerAnimation::OnThreadedAnimationStarted if we're not
waiting for group start, should we maybe set waiting_for_group_start_ to true
here if the first element is threaded?

[ajuma, 2013-02-13 23:28:18]

https://chromiumcodereview.appspot.com/11896017/diff/21001/ui/compositor/laye...
File ui/compositor/layer_animation_element.h (right):

https://chromiumcodereview.appspot.com/11896017/diff/21001/ui/compositor/laye...
ui/compositor/layer_animation_element.h:137: void
ProgressToEffectiveStart(LayerAnimationDelegate* delegate);
On 2013/02/13 03:59:38, vollick wrote:
> There's a big disconnect between this comment and the method name. It would be
> nice to explain how dispatching to the compositor thread will affect the
> effective start time.

Done.

https://chromiumcodereview.appspot.com/11896017/diff/21001/ui/compositor/laye...
File ui/compositor/layer_animation_sequence.cc (right):

https://chromiumcodereview.appspot.com/11896017/diff/21001/ui/compositor/laye...
ui/compositor/layer_animation_sequence.cc:48: LayerAnimationDelegate* delegate)
{
On 2013/02/13 03:59:38, vollick wrote:
> If we're waiting for group start, do we need to progress at all?

No, in this case, we shouldn't even call Progress since having a start time set
is a prerequisite for calling Progress (and, indeed, LayerAnimator doesn't call
Progress on sequences waiting for a group start).

I've added a DCHECK here that we do have a start time.

https://chromiumcodereview.appspot.com/11896017/diff/21001/ui/compositor/laye...
ui/compositor/layer_animation_sequence.cc:168:
elements_.push_back(make_linked_ptr(element));
On 2013/02/13 03:59:38, vollick wrote:
> Since we early out in LayerAnimation::OnThreadedAnimationStarted if we're not
> waiting for group start, should we maybe set waiting_for_group_start_ to true
> here if the first element is threaded?

Only sequences started by LayerAnimator::ScheduleTogether or
LayerAnimator::StartTogether need to wait for a group start (whenever the group
contains at least one sequence whose first element is threaded). The early out
in OnThreadedAnimationStarted just prevents notifying the other sequences in the
same group, which makes sense when there aren't actually any other sequences in
the group since the group has size 1.

[Ian Vollick, 2013-02-14 17:53:07]

On 2013/02/13 23:28:18, ajuma wrote:
>
https://chromiumcodereview.appspot.com/11896017/diff/21001/ui/compositor/laye...
> File ui/compositor/layer_animation_element.h (right):
> 
>
https://chromiumcodereview.appspot.com/11896017/diff/21001/ui/compositor/laye...
> ui/compositor/layer_animation_element.h:137: void
> ProgressToEffectiveStart(LayerAnimationDelegate* delegate);
> On 2013/02/13 03:59:38, vollick wrote:
> > There's a big disconnect between this comment and the method name. It would
be
> > nice to explain how dispatching to the compositor thread will affect the
> > effective start time.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/11896017/diff/21001/ui/compositor/laye...
> File ui/compositor/layer_animation_sequence.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11896017/diff/21001/ui/compositor/laye...
> ui/compositor/layer_animation_sequence.cc:48: LayerAnimationDelegate*
delegate)
> {
> On 2013/02/13 03:59:38, vollick wrote:
> > If we're waiting for group start, do we need to progress at all?
> 
> No, in this case, we shouldn't even call Progress since having a start time
set
> is a prerequisite for calling Progress (and, indeed, LayerAnimator doesn't
call
> Progress on sequences waiting for a group start).
> 
> I've added a DCHECK here that we do have a start time.
> 
>
https://chromiumcodereview.appspot.com/11896017/diff/21001/ui/compositor/laye...
> ui/compositor/layer_animation_sequence.cc:168:
> elements_.push_back(make_linked_ptr(element));
> On 2013/02/13 03:59:38, vollick wrote:
> > Since we early out in LayerAnimation::OnThreadedAnimationStarted if we're
not
> > waiting for group start, should we maybe set waiting_for_group_start_ to
true
> > here if the first element is threaded?
> 
> Only sequences started by LayerAnimator::ScheduleTogether or
> LayerAnimator::StartTogether need to wait for a group start (whenever the
group
> contains at least one sequence whose first element is threaded). The early out
> in OnThreadedAnimationStarted just prevents notifying the other sequences in
the
> same group, which makes sense when there aren't actually any other sequences
in
> the group since the group has size 1.

Thanks! This lgtm.

Sky, could you please give a 2nd opinion on this approach?

[sky, 2013-02-14 20:18:15]

Ian,

Are you asking me to review this as well, or just my thoughts on the approach
here?

[Ian Vollick, 2013-02-16 01:51:27]

I was just hoping to get your blessing on the general approach, not a detailed
review, although you're more than welcome to do one.

[sky, 2013-02-19 17:06:56]

This seems like a reasonable approach. Mostly just a bunch of random
questions/comments.

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer.h#new...
ui/compositor/layer.h:341: virtual void RemoveThreadedAnimation(int
animation_id) OVERRIDE;
Seems weird that Add and Remove take different things.

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
File ui/compositor/layer_animation_element.cc (right):

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_element.cc:383: bool IsThreaded() const {
virtual OVERRIDE

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_element.cc:384: return (duration() !=
base::TimeDelta());
Should whether a LayerAnimationElement is threaded be pushed up to
LayerAnimationElement? By that I mean this check could easily be moved there.

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_element.cc:424: 
nit: remove newline.

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
File ui/compositor/layer_animation_element.h (right):

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_element.h:139: void
ProgressToEffectiveStart(LayerAnimationDelegate* delegate);
Can this be called Start() and always invoked? Internally it could do whatever
is appropriate based on if threaded or not.

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_element.h:170: // Used to identify animations.
These need better descriptions. In particular how the two differ and what
they're used for. 

If possible, can animation_id_ be a const and set in the constructor?

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
File ui/compositor/layer_animation_sequence.cc (right):

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_sequence.cc:25:
LayerAnimationSequence::LayerAnimationSequence(LayerAnimationElement* element)
Can we get rid of this and only have one constructor?

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_sequence.cc:45:
elements_[0]->set_animation_group_id(animation_group_id_);
How come this is only done for the first element?

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_sequence.cc:51: DCHECK(start_time_ !=
base::TimeTicks());
DCHECK_NE

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_sequence.cc:73: last_progressed_fraction_ =
Should this be reset early on to make sure it it set to 0?

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
File ui/compositor/layer_animator.cc (right):

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animator.cc:353: : animator_(animator) {
too many spaces here.

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
File ui/compositor/layer_animator.h (right):

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animator.h:211: class TestController {
If this is only for tests move it into its own file so that its compiled only
for tests.

[ajuma, 2013-02-20 16:09:10]

Thanks for the review!

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer.h#new...
ui/compositor/layer.h:341: virtual void RemoveThreadedAnimation(int
animation_id) OVERRIDE;
On 2013/02/19 17:06:56, sky wrote:
> Seems weird that Add and Remove take different things.

This is consistent with cc::Layer::addAnimation and cc::Layer::removeAnimation.
Since AddThreadedAnimation takes ownership of the animation (and passes
ownership on to cc::Layer::addAnimation), it's not possible to make
RemoveThreadedAnimation take an animation rather than an id, since the caller
doesn't actually have the animation (but it can easily keep a copy of the id).

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
File ui/compositor/layer_animation_element.cc (right):

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_element.cc:383: bool IsThreaded() const {
On 2013/02/19 17:06:56, sky wrote:
> virtual OVERRIDE

Done.

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_element.cc:384: return (duration() !=
base::TimeDelta());
On 2013/02/19 17:06:56, sky wrote:
> Should whether a LayerAnimationElement is threaded be pushed up to
> LayerAnimationElement? By that I mean this check could easily be moved there.

No. Simply checking for non-zero duration in LayerAnimationElement isn't
sufficient, since there will continue to exist non-threaded
LayerAnimationElements (everything other than opacity and transform). Only
descendants of ThreadedLayerAnimationElement that have non-zero duration are
threaded, so the check needs to go here.

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_element.cc:424: 
On 2013/02/19 17:06:56, sky wrote:
> nit: remove newline.

Done.

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
File ui/compositor/layer_animation_element.h (right):

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_element.h:139: void
ProgressToEffectiveStart(LayerAnimationDelegate* delegate);
On 2013/02/19 17:06:56, sky wrote:
> Can this be called Start() and always invoked? Internally it could do whatever
> is appropriate based on if threaded or not.

Done. I've similarly renamed LayerAnimationSequence::ProgressToEffectiveStart to
Start() and required that it is always invoked.

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_element.h:170: // Used to identify animations.
On 2013/02/19 17:06:56, sky wrote:
> These need better descriptions. In particular how the two differ and what
> they're used for.

Done. 

> If possible, can animation_id_ be a const and set in the constructor?

Done.

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
File ui/compositor/layer_animation_sequence.cc (right):

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_sequence.cc:25:
LayerAnimationSequence::LayerAnimationSequence(LayerAnimationElement* element)
On 2013/02/19 17:06:56, sky wrote:
> Can we get rid of this and only have one constructor?

Constructing a one-element sequence seems common enough that I think it's worth
keeping this constructor (there are several existing callers of this constructor
scattered across ui/ and ash/). If you feel strongly about removing this
constructor, how about I do this in a separate CL (either a follow-up or one
that lands before this one, whichever you prefer)?

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_sequence.cc:45:
elements_[0]->set_animation_group_id(animation_group_id_);
On 2013/02/19 17:06:56, sky wrote:
> How come this is only done for the first element?

To avoid making an extra pass over the sequence, this is set when starting each
element. To make this more clear, this is now being passed as an argument to
LayerAnimationElement::Start().

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_sequence.cc:51: DCHECK(start_time_ !=
base::TimeTicks());
On 2013/02/19 17:06:56, sky wrote:
> DCHECK_NE

This won't compile, since values passed to DCHECK_NE need to have
operator<<(ostream, ...) defined (this is used when logging failures), but
base::TimeTicks doesn't define this.

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animation_sequence.cc:73: last_progressed_fraction_ =
On 2013/02/19 17:06:56, sky wrote:
> Should this be reset early on to make sure it it set to 0?

Done. This is now reset in ::Start.

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
File ui/compositor/layer_animator.cc (right):

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animator.cc:353: : animator_(animator) {
On 2013/02/19 17:06:56, sky wrote:
> too many spaces here.

Done and moved to ui/compositor/test/layer_animator_test_controller.cc.

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
File ui/compositor/layer_animator.h (right):

https://codereview.chromium.org/11896017/diff/37001/ui/compositor/layer_anima...
ui/compositor/layer_animator.h:211: class TestController {
On 2013/02/19 17:06:56, sky wrote:
> If this is only for tests move it into its own file so that its compiled only
> for tests.

Moved to ui/compositor/test/layer_animator_test_controller.h.

[sky, 2013-02-20 17:20:03]

Thanks for integrating my comments. Since Ian did the bulk of the review I'm not
going to go through looking at the nitty gritty again, I just wanted to
understand the flow. The ash changes LGTM

[ajuma, 2013-02-20 18:05:57]

+enne for compositor_bindings OWNERS

Note that the compositor_bindings part of this patch is actually pretty small --
just moving WebAnimationIdProvider to cc::AnimationIdProvider.

[jamesr, 2013-02-20 18:42:07]

webkit/compositor_bindings lgtm

[ajuma, 2013-02-21 21:19:13]

Patch updated to fix a couple problems uncovered by ash_unittests.

1) Zero-duration ThreadedLayerAnimationElements weren't getting an effective
start time. This is now fixed in
ThreadedLayerAnimationElement::RequestEffectiveStart.

2) Two DesktopBackgroundControllerTests were hanging since they try to force
running animations to finish just by repeatedly calling LayerAnimator::Step. But
this isn't sufficient for threaded animations, since these expect to be notified
about start times. I've added a method to LayerAnimatorTestController that
issues these start notifications, and this method is now used by
RunAnimationForWidget in desktop_background_controller_unittest.cc.

vollick@, PTAL.

[Ian Vollick, 2013-02-22 18:44:29]

On 2013/02/21 21:19:13, ajuma wrote:
> Patch updated to fix a couple problems uncovered by ash_unittests.
> 
> 1) Zero-duration ThreadedLayerAnimationElements weren't getting an effective
> start time. This is now fixed in
> ThreadedLayerAnimationElement::RequestEffectiveStart.
> 
> 2) Two DesktopBackgroundControllerTests were hanging since they try to force
> running animations to finish just by repeatedly calling LayerAnimator::Step.
But
> this isn't sufficient for threaded animations, since these expect to be
notified
> about start times. I've added a method to LayerAnimatorTestController that
> issues these start notifications, and this method is now used by
> RunAnimationForWidget in desktop_background_controller_unittest.cc.
> 
> vollick@, PTAL.

The updates lgtm.

[ajuma, 2013-02-22 18:51:10]

sky@, PTAL at the additional change to 	
ash/desktop_background/desktop_background_controller_unittest.cc

[sky, 2013-02-22 20:38:11]

LGTM

[commit-bot: I haz the power, 2013-02-22 20:40:00]

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

[commit-bot: I haz the power, 2013-02-23 01:54:18]

Change committed as 184254