Animation: Cancel same-property animations on compositor

Animation: Cancel same-property animations on compositor

Do not support multiple animations per property on compositor.
In case of addition of a second animation we must pull back the already running
animations from compositor and rely on blink thread to visualize that
combination properly.

This is the most reasonable short-term solution for the bug.

BUG=358832
R=shans@chromium.org
R=dstockwell@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189319

[dstockwell, 2015-01-28 05:44:06]

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Animat...
File Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Animat...
Source/core/animation/Animation.cpp:303: void
Animation::cancelAffectedAnimationsOnCompositor()
I think the term 'affected' is a little strange, how about 'overlapping' or
'incompatible'?

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Animat...
File Source/core/animation/AnimationPlayer.h (right):

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Animat...
Source/core/animation/AnimationPlayer.h:150: bool affects(CSSPropertyID) const;
This should have an element for context (conceptually Players may target more
than one element):
bool affects(Element&, CSSPropertyID);

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:74: bool
alreadyActiveAnimationsBlockProperties(const Element& targetElement, const
AnimationPlayer& playerToAdd, const AnimationEffect& effectToAdd)
I think this would be better written as:

bool affectsOpacity = effectToAdd.affects(CSSPropertyOpacity);

... for ...
  if (affectsOpacity && attachedPlayer->affects(...Opacity))
    return true;

return false;


Also the name is hard, if we went with 'incompatible' it could be just
'hasIncompatatibleAnimations'

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:85: const AnimationPlayer*
attachedPlayer = entry.key;
Is it possible to skip consideration of earlier players which are filling?

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:168: bool
CompositorAnimations::isCandidateForAnimationOnCompositor(const Timing& timing,
const Element* targetElement, const AnimationPlayer* playerToAdd, const
AnimationEffect& effect, double playerPlaybackRate)
When can targetElement be null?

[loyso (OOO), 2015-01-28 06:33:32]

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Animat...
File Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Animat...
Source/core/animation/Animation.cpp:303: void
Animation::cancelAffectedAnimationsOnCompositor()
On 2015/01/28 05:44:05, dstockwell wrote:
> I think the term 'affected' is a little strange, how about 'overlapping' or
> 'incompatible'?
I'll go with 'incompatible'! See next patch set.

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Animat...
File Source/core/animation/AnimationPlayer.h (right):

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Animat...
Source/core/animation/AnimationPlayer.h:150: bool affects(CSSPropertyID) const;
On 2015/01/28 05:44:05, dstockwell wrote:
> This should have an element for context (conceptually Players may target more
> than one element):
> bool affects(Element&, CSSPropertyID);
Is that possible with current codebase? AnimationPlayer contains just one
AnimationNode* m_content. Should we simply support that concept in a class API?

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:74: bool
alreadyActiveAnimationsBlockProperties(const Element& targetElement, const
AnimationPlayer& playerToAdd, const AnimationEffect& effectToAdd)
On 2015/01/28 05:44:05, dstockwell wrote:
> I think this would be better written as:
> 
> bool affectsOpacity = effectToAdd.affects(CSSPropertyOpacity);
> 
> ... for ...
>   if (affectsOpacity && attachedPlayer->affects(...Opacity))
>     return true;
> 
> return false;
Good point. That's also will be more efficient. 
> 
> 
> Also the name is hard, if we went with 'incompatible' it could be just
> 'hasIncompatatibleAnimations'
That's more generic but good. Thanks!

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:168: bool
CompositorAnimations::isCandidateForAnimationOnCompositor(const Timing& timing,
const Element* targetElement, const AnimationPlayer* playerToAdd, const
AnimationEffect& effect, double playerPlaybackRate)
On 2015/01/28 05:44:05, dstockwell wrote:
> When can targetElement be null?
Unit tests. I could setup a fake one there if we need the reference type badly.

[loyso (OOO), 2015-01-28 23:18:34]

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:168: bool
CompositorAnimations::isCandidateForAnimationOnCompositor(const Timing& timing,
const Element* targetElement, const AnimationPlayer* playerToAdd, const
AnimationEffect& effect, double playerPlaybackRate)
On 2015/01/28 06:33:31, loyso wrote:
> On 2015/01/28 05:44:05, dstockwell wrote:
> > When can targetElement be null?
> Unit tests. I could setup a fake one there if we need the reference type
badly.
Do we need it?

[loyso (OOO), 2015-01-29 01:08:54]

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:85: const AnimationPlayer*
attachedPlayer = entry.key;
On 2015/01/28 05:44:05, dstockwell wrote:
> Is it possible to skip consideration of earlier players which are filling?
What do you mean? Players with fill=forwards will be in
activeAnimations().players(), I suppose.

[dstockwell, 2015-01-29 01:14:07]

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Animat...
File Source/core/animation/AnimationPlayer.h (right):

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Animat...
Source/core/animation/AnimationPlayer.h:150: bool affects(CSSPropertyID) const;
On 2015/01/28 at 06:33:31, loyso wrote:
> On 2015/01/28 05:44:05, dstockwell wrote:
> > This should have an element for context (conceptually Players may target
more
> > than one element):
> > bool affects(Element&, CSSPropertyID);
> Is that possible with current codebase? AnimationPlayer contains just one
AnimationNode* m_content. Should we simply support that concept in a class API?

No it is not possible currently, but I would like to avoid having to plumb this
though later. If this function does not take Element* then the functions in
CompositorAnimations should only require ActiveAnimations* instead of Element.

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:85: const AnimationPlayer*
attachedPlayer = entry.key;
On 2015/01/29 at 01:08:54, loyso wrote:
> On 2015/01/28 05:44:05, dstockwell wrote:
> > Is it possible to skip consideration of earlier players which are filling?
> What do you mean? Players with fill=forwards will be in
activeAnimations().players(), I suppose.

Yes. Hmm, also players which are idle or finished will be in the list.

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:168: bool
CompositorAnimations::isCandidateForAnimationOnCompositor(const Timing& timing,
const Element* targetElement, const AnimationPlayer* playerToAdd, const
AnimationEffect& effect, double playerPlaybackRate)
On 2015/01/28 at 23:18:34, loyso wrote:
> On 2015/01/28 06:33:31, loyso wrote:
> > On 2015/01/28 05:44:05, dstockwell wrote:
> > > When can targetElement be null?
> > Unit tests. I could setup a fake one there if we need the reference type
badly.
> Do we need it?

Unless it's non-trivial to create an Element in the test I think we should
prefer targetElement to be non-nullable.

[loyso (OOO), 2015-01-29 04:35:59]

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:85: const AnimationPlayer*
attachedPlayer = entry.key;
On 2015/01/29 01:14:07, dstockwell wrote:
> On 2015/01/29 at 01:08:54, loyso wrote:
> > On 2015/01/28 05:44:05, dstockwell wrote:
> > > Is it possible to skip consideration of earlier players which are filling?
> > What do you mean? Players with fill=forwards will be in
> activeAnimations().players(), I suppose.
> 
> Yes. Hmm, also players which are idle or finished will be in the list.
Is that a problem? Should we address it?

[dstockwell, 2015-01-29 04:40:18]

On 2015/01/29 at 04:35:59, loyso wrote:
>
https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
> File Source/core/animation/CompositorAnimations.cpp (right):
> 
>
https://codereview.chromium.org/881183003/diff/1/Source/core/animation/Compos...
> Source/core/animation/CompositorAnimations.cpp:85: const AnimationPlayer*
attachedPlayer = entry.key;
> On 2015/01/29 01:14:07, dstockwell wrote:
> > On 2015/01/29 at 01:08:54, loyso wrote:
> > > On 2015/01/28 05:44:05, dstockwell wrote:
> > > > Is it possible to skip consideration of earlier players which are
filling?
> > > What do you mean? Players with fill=forwards will be in
> > activeAnimations().players(), I suppose.
> > 
> > Yes. Hmm, also players which are idle or finished will be in the list.
> Is that a problem? Should we address it?

It's not a problem that they are in the list. But it shouldn't be the case that
a finished player prevents subsequent animations from running on the compositor.

[loyso (OOO), 2015-01-29 05:53:25]

On 2015/01/29 04:40:18, dstockwell wrote:

> > > Yes. Hmm, also players which are idle or finished will be in the list.
> > Is that a problem? Should we address it?
> 
> It's not a problem that they are in the list. But it shouldn't be the case
that
> a finished player prevents subsequent animations from running on the
compositor.
We can skip finished players easily. I don't think that we need to track idle or
paused players.
What do you think?

[dstockwell, 2015-01-29 06:00:47]

On 2015/01/29 at 05:53:25, loyso wrote:
> On 2015/01/29 04:40:18, dstockwell wrote:
> 
> > > > Yes. Hmm, also players which are idle or finished will be in the list.
> > > Is that a problem? Should we address it?
> > 
> > It's not a problem that they are in the list. But it shouldn't be the case
that
> > a finished player prevents subsequent animations from running on the
compositor.
> We can skip finished players easily. I don't think that we need to track idle
or paused players.
> What do you think?

I think we can skip anything which is not playState == Running | Pending --
these are the only states where we can have compositor animations.

Although.. finished with a filling effect may be special, another player which
is filling is incompatible if it should apply on top.

[dstockwell, 2015-01-30 02:39:23]

lgtm, some minor nits and I think we should add some more manual tests to cover
these cases.

https://codereview.chromium.org/881183003/diff/80001/Source/core/animation/Co...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/881183003/diff/80001/Source/core/animation/Co...
Source/core/animation/CompositorAnimations.cpp:96: bool
hasIncompatatibleAnimations(const Element& targetElement, const AnimationPlayer&
playerToAdd, const AnimationEffect& effectToAdd)
Incompatatible -> Incompatible

https://codereview.chromium.org/881183003/diff/80001/Source/core/animation/Co...
Source/core/animation/CompositorAnimations.cpp:185: if (playerToAdd &&
hasIncompatatibleAnimations(targetElement, *playerToAdd, effect))
Perhaps we should move this check to the end -- around line 175. That would
avoid having to visit the other players in the event that the animated
properties are not compositable.

https://codereview.chromium.org/881183003/diff/80001/Source/core/animation/Co...
File Source/core/animation/CompositorAnimationsTestHelper.h (right):

https://codereview.chromium.org/881183003/diff/80001/Source/core/animation/Co...
Source/core/animation/CompositorAnimationsTestHelper.h:148: const unsigned char*
getTraceCategoryEnabledFlag(const char* categoryName) override
This seems unrelated?

[loyso (OOO), 2015-01-30 02:53:11]

https://codereview.chromium.org/881183003/diff/80001/Source/core/animation/Co...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/881183003/diff/80001/Source/core/animation/Co...
Source/core/animation/CompositorAnimations.cpp:96: bool
hasIncompatatibleAnimations(const Element& targetElement, const AnimationPlayer&
playerToAdd, const AnimationEffect& effectToAdd)
On 2015/01/30 02:39:23, dstockwell wrote:
> Incompatatible -> Incompatible

Done.

https://codereview.chromium.org/881183003/diff/80001/Source/core/animation/Co...
Source/core/animation/CompositorAnimations.cpp:185: if (playerToAdd &&
hasIncompatatibleAnimations(targetElement, *playerToAdd, effect))
On 2015/01/30 02:39:23, dstockwell wrote:
> Perhaps we should move this check to the end -- around line 175. That would
> avoid having to visit the other players in the event that the animated
> properties are not compositable.

Done.

https://codereview.chromium.org/881183003/diff/80001/Source/core/animation/Co...
File Source/core/animation/CompositorAnimationsTestHelper.h (right):

https://codereview.chromium.org/881183003/diff/80001/Source/core/animation/Co...
Source/core/animation/CompositorAnimationsTestHelper.h:148: const unsigned char*
getTraceCategoryEnabledFlag(const char* categoryName) override
On 2015/01/30 02:39:23, dstockwell wrote:
> This seems unrelated?
Trace macroses just crash with our previous PlatformProxy.

[loyso (OOO), 2015-02-02 06:17:18]

The CQ bit was checked by loyso@chromium.org

[commit-bot: I haz the power, 2015-02-02 06:18:23]

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

[commit-bot: I haz the power, 2015-02-02 06:21:32]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189319