1195180 - Tick animations from their timeline

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

This corresponds to the following parts from bug 1151731 comment 4:

  * Make the DocumentTimeline observe the refresh driver and call tick on its
    animations. There's complexity here but I think the WIP patch is probably
    not too bad in this area.
  * Remove all refresh driver watching from the managers.

Comment 1

[Brian Birtles (:birtles)]

In bug 1183461 and bug 1194037 I've been making sure that even if we tick animations in a random order, all side-effects (mutation events, animation events etc.) still happen in a deterministic way.

However, I forgot to consider:

* Animation finish promise
* Animation finish events

Although these are dispatched/run in separate tasks/microtasks, the order in which those tasks/microtasks are queued will depend on the order in which the corresponding Animations are ticked.

I think I need to revisit the approach to ticking animations. Originally I wanted to store the Animations in a hashtable on their timeline so that teardown of a document subtree does not become O(n^2).

However, I think we could avoid this by simply storing the animations in an array. They would be appended in the order in which they were associated with the timeline which is deterministic. Then, we wouldn't eagerly remove the Animations when they stopped being associated with a timeline. Instead, on each tick as we iterate through the animations to tick them, we'd just check Animation's reported timeline with ourselves and, if it doesn't match, drop it from the array.

We'd have to do something similar in GetAnimations too to avoid returning animations no longer associated with us.

When an Animation is disassociated with a timeline, it should report "needs ticks" to the disassociated timeline so that the timeline can remove it on the next tick.


The work in bug 1183461 and bug 1194037 has still been useful as it has fixed a bunch of other issues and made the dispatch of animation events much more consistent (i.e. it is no longer dependent on the frame rate).

A recap on *why* we are doing this:

* So that we don't need to introduce another manager just to tick script-generated animations
* So we can support animations that don't have any effect on style (time-only animations)
* Because the current refresh-driver watching code in nsAnimationManager/nsTransitionManager is very complex and error-prone
* To create a better separation of timing from animation

(For the most part, it's more about ticking the animations from something *other* than the managers, rather than the timeline. We happen to use the timelines since they have a getAnimations() method so they need to track their associated animations anyway.)

Comment 2

[Brian Birtles (:birtles)]

Attachment: part 1 - Add Animation::NeedsTicks

Adds a method to determine if an animation requires refresh driver ticks.
We will use this function later to determine when it is safe to stop
observing the refresh driver.

Comment 3

[Brian Birtles (:birtles)]

Attachment: part 2 - Make DocumentTimeline inherit from nsARefreshObserver

Comment 4

[Brian Birtles (:birtles)]

Attachment: part 3 - Add flag to record if DocumentTimeline is observing the refresh driver

Comment 5

[Brian Birtles (:birtles)]

Attachment: part 4 - Unregister from refresh observer when there are no animations needing ticks

Comment 6

[Brian Birtles (:birtles)]

Attachment: part 5 - Synchronize refresh driver observing with presshell creation/teardown

Comment 7

[Brian Birtles (:birtles)]

Attachment: part 6 - Lazily remove animations from timelines

Now that DocumentTimeline observes the refresh driver we can use regular
ticks to remove unnecessary animations.

We do this because in a subsequent patch, in order to provide deterministic
enumeration order when ticking animations, we will store animations in an array.
Removing an arbitrary element from an nsTArray is O(n) since we have to search
for the array index first, or O(log n) if we keep the array sorted. If we
destroy a subtree containing n animations, the operation effectively becomes
O(n^2), or, if we keep the array sorted, O(n log n). By destroying during a
tick when we are already iterating over the array, however, we will be able
to do this much more efficiently.

Whether an animation is newly associated with a timeline, or is disassociated
from a timeline, or if it merely has its timing updated, the behavior
implemented in this patch is to simply make sure we are observing the refresh
driver and deal with the animation on the next tick.

It might seem that we could be a lot more clever about this and, for example, if
an animation reports NeedsTicks() == false, not start observing the refresh
driver. There are various edge cases however that need to be taken into account.
For example, if a CSS animation is finished (IsRelevant() == false so that
animation will have been removed from the timeline), and paused
(NeedsTicks() == false), and we seek it back to the point where it is relevant
again, we actually need to observe the refresh driver so that it can dispatch an
animationstart event on the next tick. A test case in a subsequent patch tests
this specific situation.

We could possibly add logic to detect if we need to fire events on the next tick
but the complexity does not seem warranted given that even if we unnecessarily
start observing the refresh driver, we will stop watching it on the next tick.

This patch removes some rather lengthy comments from
AnimationTiming::UpdateTiming. This is, in part, because of the behavior
described above that makes these comments no longer relevant. Other parts are
removed because the Web Animations specification has been updated such that a
timeline becoming inactive now pauses the animation[1] so that the issue
regarding detecting timelines becoming active/inactive no longer applies
since animations attached to an inactive timeline remain "relevant".

[1] https://w3c.github.io/web-animations/#responding-to-a-newly-inactive-timeline

Comment 8

[Brian Birtles (:birtles)]

Attachment: part 7 - Store animations in an array

Currently AnimationTimeline stores animations in a hashmap which means that
when we go to iterate over those animations to tick them we will visit them
in an order that is non-deterministic.

Although many of the observable effects of ticking an animation (e.g. CSS
animation/transition events, mutation observer events) are later sorted so that
the result does not depend on the order in which animations are ticked, this is
not true for in all cases. In particular, the order in which Animation.finished
promises are resolved will vary depending on the order in which animations are
ticked. Likewise, for Animation finish events.

Furthermore, it seems generally desirable to have a deterministic order for
visiting animations in order to aid reproducing bugs.

To achieve this, this patch switches the storage of animations in
AnimationTimeline to use an array instead. However, when adding animations
we need to determine if the animation to add already exists. To this end we
also maintain a hashmap of the animations so we can quickly determine if
the animation to add is a duplicate or not.

REVIEW: We never actually dereference the pointers in mAnimationSet. Should we
store raw pointers instead to save some addref/release traffic?

Comment 9

[Brian Birtles (:birtles)]

Attachment: part 8 - Tick animations from their timeline

Comment 10

[Brian Birtles (:birtles)]

Attachment: part 9 - Add test for seeking finished -> paused

This patch adds a test that even when we seek from being irrelevant to another
state where we no longer need ticks that we still spin the refresh driver
in order to queue and dispatch an animationstart event.

Comment 11

[Brian Birtles (:birtles)]

Attachment: part 10 - Remove refresh driver observing machinery from CommonAnimationManager

This patch removes a lot of code involved with observing the refresh driver from
nsAnimationManager and nsTransitionManager now that we no longer need to do
this.

The one piece it does not remove, however, is
AnimationCollection::mNeedsRefreshes since this flag actually serves a secondary
purpose in telling us when the animation style has not changed and so does
not need to be updated. A subsequent patch in this series will rename this
and update the code that makes use of it.

Comment 12

[Brian Birtles (:birtles)]

Attachment: part 11 - Rename AnimationCollection::mNeedsRefreshes to mStyleChanging

This patch is mostly a renaming to indicate that
AnimationCollection::mNeedsRefreshes no longer has any relationship to whether
or not we observe the refresh driver.

One functional change, however, is that we no longer query
Animation::HasEndEventToQueue when setting this member since we only needed to
do that when this member was used to determine if we need to keep observing
the refresh driver or not. The arrangement for observing the refresh driver
introduced earlier in this patch series should take care of this situation
already (and we have tests that cover this, see bug 1194037 comment 8). We will
remove this method altogether in a subsequent patch.

Comment 13

[Brian Birtles (:birtles)]

Attachment: part 12 - Remove Animation::HasEndEventToQueue

Comment 14

[Brian Birtles (:birtles)]

Attachment: part 13 - Update pending finishing handling

Animation::Tick contains special handling to cope with the fact that it could
be called multiple times per refresh-driver tick. As of this patch series,
however, that should no longer be the case so we can simplify this handling
somewhat.

Comment 15

[Brian Birtles (:birtles)]

Attachment: part 13 - Update pending finishing handling

Animation::Tick contains special handling to cope with pending ready times
that are in the future. This was originally introduced to cope with the
situation where we are called multiple times per refresh-driver tick.

As of this patch series, Animation::Tick should no longer be called multiple
times per refresh driver tick. It would seem, therefore, that we no longer
need to check for a future time. However, since introducing this check, the
vsync refresh driver timer has been added which means that we can still have
a recorded time from TimeStamp::Now that is ahead of the vsync time used to
update the refresh driver. In that case, however, rather than waiting for the
next tick, we should simply clamp that pending ready time to the refresh driver
time and finish pending immediately.

Comment 16

[Brian Birtles (:birtles)]

Not putting this up for review yet because I'm seeing two classes of test failures on try.

(1) The first class are reftests that wait on transitionend events:

  /layout/reftests/bugs/983084-3.html failing on Linux opt, OSX opt
  /layout/reftests/bugs/1111753-1.html failing on Linux opt/debug e10s, B2G ICS Emulator debug
  /layout/reftests/bugs/598726-1.html failing on Linux opt e10s
  
  Also, sometimes:
  /layout/reftests/webcomponents/input-transition-1.html failing on Android 4.3 opt/debug 

Look at the failures, it appears we are getting a render roughly one frame before the end of the animation. That could be because:

* We are dispatching the transitionend too soon
* We are failing to update style on the last frame
* We are unregistering from the refresh driver too soon

This seems to happen more often on e10s. Unfortunately, however, I can't reproduce it locally.

(2) The second class is the following failure:

  /dom/animation/test/css-animations/test_animation-reverse.html | reverse() inverts playbackRate - reverse() inverts playbackRate: An attempt was made to use an object that is not, or is no longer, usable

This is only failing on Android and only with part 13 applied. I can't imagine what in part 13 could cause that, however, it happened on both debug and opt.

If I can't work out part (2) soonish I'll spin it off into a separate bug.

Comment 17

[Brian Birtles (:birtles)]

Bisection on try, with only parts 1~8 applied, the reftests in (1) from comment 16 appear to pass. My hunch is part 11 is the culprit. If that's the case, then I wonder if we have something like the following

1. Part 11 removes the call to HasEndEventToQueue
2. As a result, during some call to ComposeStyle (which *doesn't* always happen as part of a tick), we end up *not* setting mStyleChanging to true
3. On the next (and final) tick where we dispatch the transitionend event, we fail to update style because we figure it's already up-to-date

I can't understand why we would fail to update mStyleChanging if we weren't finished though.

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8662213 [details] [diff] [review]
part 2 - Make DocumentTimeline inherit from nsARefreshObserver

Review of attachment 8662213 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/DocumentTimeline.h
@@ +52,5 @@
>                                                                       override;
>    TimeStamp ToTimeStamp(const TimeDuration& aTimelineTime) const override;
>  
> +  // nsARefreshObserver methods
> +  void WillRefresh(mozilla::TimeStamp aTime) override;

Nit: Drop "mozilla::" since we're in the namespace already (and other declarations on DocumentTimeline above have dropped it).

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 8662215 [details] [diff] [review]
part 4 - Unregister from refresh observer when there are no animations needing ticks

Review of attachment 8662215 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/DocumentTimeline.cpp
@@ +99,5 @@
> +  bool needsTicks = false;
> +
> +  for (auto iter = mAnimations.Iter(); !iter.Done(); iter.Next()) {
> +    Animation* animation = iter.Get()->GetKey();
> +    needsTicks |= animation->NeedsTicks();

Might be worth breaking out of the loop early if we set needTicks to true?

@@ +104,5 @@
> +  }
> +
> +  if (!needsTicks) {
> +    nsRefreshDriver* refreshDriver = GetRefreshDriver();
> +    if (refreshDriver) {

Not objecting to the null check, but I wonder if it ever can be null?  Can other refresh observers cause the pres shell to go away?

Comment 20

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #19)
> ::: dom/animation/DocumentTimeline.cpp
> @@ +99,5 @@
> > +  bool needsTicks = false;
> > +
> > +  for (auto iter = mAnimations.Iter(); !iter.Done(); iter.Next()) {
> > +    Animation* animation = iter.Get()->GetKey();
> > +    needsTicks |= animation->NeedsTicks();
> 
> Might be worth breaking out of the loop early if we set needTicks to true?

I see in a later patch you need to keep iterating through the rest of the loop to remove animations, so don't worry about this.

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8662217 [details] [diff] [review]
part 6 - Lazily remove animations from timelines

Review of attachment 8662217 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/AnimationTimeline.cpp
@@ +37,5 @@
> +    // Skip animations which are no longer relevant or which have been
> +    // associated with another timeline. These animations will be removed
> +    // on the next tick.
> +    if (!animation->IsRelevant() || animation->GetTimeline() != this) {
> +      continue;

Maybe assert in here that NeedsTick() return true?

Comment 22

[Cameron McCormack (:heycam)]

Comment on attachment 8662218 [details] [diff] [review]
part 7 - Store animations in an array

Review of attachment 8662218 [details] [diff] [review]:
-----------------------------------------------------------------

> REVIEW: We never actually dereference the pointers in mAnimationSet.
> Should we store raw pointers instead to save some addref/release
> traffic?

Would it be better to make mAnimationsSet be the one that holds the strong reference to the Animations?  Then you wouldn't get addref/release traffic when building up animationsToKeep.  (This would mean that you could remove an Animation from the set, have it be destroyed, while it still exists in mAnimationsArray until we swap it with animationsToKeep.  But it's pretty localized, so should be OK with a comment.)

If you do this please extend the comment above their declaration to point out that they're kept in sync and that mAnimationsSet is the one that holds the strong reference.

Nit: I'm not a big fan of the data structure name being used in the variable name.  Maybe leave mAnimations as named, and call mAnimationsArray mAnimationOrder?

Comment 23

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #19)
> @@ +104,5 @@
> > +  }
> > +
> > +  if (!needsTicks) {
> > +    nsRefreshDriver* refreshDriver = GetRefreshDriver();
> > +    if (refreshDriver) {
> 
> Not objecting to the null check, but I wonder if it ever can be null?  Can
> other refresh observers cause the pres shell to go away?

Yes, they can, but nsRefreshDriver itself checks this before calling WillRefresh so we shouldn't need this check. I'll add a MOZ_ASSERT instead.

Comment 24

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #21)
> Comment on attachment 8662217 [details] [diff] [review]
> part 6 - Lazily remove animations from timelines
> 
> Review of attachment 8662217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/AnimationTimeline.cpp
> @@ +37,5 @@
> > +    // Skip animations which are no longer relevant or which have been
> > +    // associated with another timeline. These animations will be removed
> > +    // on the next tick.
> > +    if (!animation->IsRelevant() || animation->GetTimeline() != this) {
> > +      continue;
> 
> Maybe assert in here that NeedsTick() return true?

I'm not sure I understand. This is returning all animations that are current or in effect which includes paused and finished (but filling) animations. These animations don't need ticks, however.

Maybe, asserting that mIsObservingRefreshDriver is true?

Comment 25

[Brian Birtles (:birtles)]

Attachment: part 7 - Store animations in an array

(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #22)
> Comment on attachment 8662218 [details] [diff] [review]
> part 7 - Store animations in an array
> 
> Review of attachment 8662218 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > REVIEW: We never actually dereference the pointers in mAnimationSet.
> > Should we store raw pointers instead to save some addref/release
> > traffic?
> 
> Would it be better to make mAnimationsSet be the one that holds the strong
> reference to the Animations?  Then you wouldn't get addref/release traffic
> when building up animationsToKeep.  (This would mean that you could remove
> an Animation from the set, have it be destroyed, while it still exists in
> mAnimationsArray until we swap it with animationsToKeep.  But it's pretty
> localized, so should be OK with a comment.)

I went to do this but I realised that in part 8 we will call out to
Animation::Tick within this method so it's a little less localized. I'd rather
not call out to another class (which will, in turn, call back to this class)
while we're in a state of having an array with dangling pointers.

> Nit: I'm not a big fan of the data structure name being used in the variable
> name.  Maybe leave mAnimations as named, and call mAnimationsArray
> mAnimationOrder?

Fixed.

Comment 26

[Brian Birtles (:birtles)]

I'm going to split parts 11-13 off into a separate bug after I've fixed bug 1208385 which is what is causing the test failures in part 11.

Comment 27

[Cameron McCormack (:heycam)]

(In reply to Brian Birtles (:birtles) from comment #24)
> (In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #21)
> > Comment on attachment 8662217 [details] [diff] [review]
> > part 6 - Lazily remove animations from timelines
> > 
> > Review of attachment 8662217 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/animation/AnimationTimeline.cpp
> > @@ +37,5 @@
> > > +    // Skip animations which are no longer relevant or which have been
> > > +    // associated with another timeline. These animations will be removed
> > > +    // on the next tick.
> > > +    if (!animation->IsRelevant() || animation->GetTimeline() != this) {
> > > +      continue;
> > 
> > Maybe assert in here that NeedsTick() return true?
> 
> I'm not sure I understand. This is returning all animations that are current
> or in effect which includes paused and finished (but filling) animations.
> These animations don't need ticks, however.
> 
> Maybe, asserting that mIsObservingRefreshDriver is true?

I see, yes asserting mIsObservingRefreshDriver sounds right then (I just wanted to assert that we will remove these animations, per the comment).

Comment 28

[Cameron McCormack (:heycam)]

Comment on attachment 8665825 [details] [diff] [review]
part 7 - Store animations in an array

Review of attachment 8665825 [details] [diff] [review]:
-----------------------------------------------------------------

> REVIEW: We never actually dereference the pointers in mAnimationSet. Should we
> store raw pointers instead to save some addref/release traffic?

Don't forget to remove this from the commit message.

Comment 29

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #27)
> (In reply to Brian Birtles (:birtles) from comment #24)
> > (In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #21)
> > > Comment on attachment 8662217 [details] [diff] [review]
> > > part 6 - Lazily remove animations from timelines
> > > 
> > > Review of attachment 8662217 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: dom/animation/AnimationTimeline.cpp
> > > @@ +37,5 @@
> > > > +    // Skip animations which are no longer relevant or which have been
> > > > +    // associated with another timeline. These animations will be removed
> > > > +    // on the next tick.
> > > > +    if (!animation->IsRelevant() || animation->GetTimeline() != this) {
> > > > +      continue;
> > > 
> > > Maybe assert in here that NeedsTick() return true?
> > 
> > I'm not sure I understand. This is returning all animations that are current
> > or in effect which includes paused and finished (but filling) animations.
> > These animations don't need ticks, however.
> > 
> > Maybe, asserting that mIsObservingRefreshDriver is true?
> 
> I see, yes asserting mIsObservingRefreshDriver sounds right then (I just
> wanted to assert that we will remove these animations, per the comment).

Unfortunately that flag is only available on the subclass, DocumentTimeline.

I think we'll end up reworking this arrangement based on this discussion at [1] so hopefully we can tighten this up then.

[1] https://lists.w3.org/Archives/Public/public-fx/2015JulSep/0073.html

Comment 30

[Brian Birtles (:birtles)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ca1f983de00

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2854a40e71
https://hg.mozilla.org/integration/mozilla-inbound/rev/33cbe1ef4956
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e72573ef43f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d53140de0b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/193b5f39f806
https://hg.mozilla.org/integration/mozilla-inbound/rev/8406c5300ab7
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef6210305ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/05dfb6716db3
https://hg.mozilla.org/integration/mozilla-inbound/rev/3aa2e3b0f240
https://hg.mozilla.org/integration/mozilla-inbound/rev/19e81f18c957

Comment 32

[Brian Birtles (:birtles)]

I will land parts 11-13 in bug 1208938 once bug 1208385 is fixed.

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb2854a40e71
https://hg.mozilla.org/mozilla-central/rev/33cbe1ef4956
https://hg.mozilla.org/mozilla-central/rev/9e72573ef43f
https://hg.mozilla.org/mozilla-central/rev/6d53140de0b0
https://hg.mozilla.org/mozilla-central/rev/193b5f39f806
https://hg.mozilla.org/mozilla-central/rev/8406c5300ab7
https://hg.mozilla.org/mozilla-central/rev/3ef6210305ae
https://hg.mozilla.org/mozilla-central/rev/05dfb6716db3
https://hg.mozilla.org/mozilla-central/rev/3aa2e3b0f240
https://hg.mozilla.org/mozilla-central/rev/19e81f18c957