1181392 - Try to replace IsFinishedTransition with simply recognizing irrelevant transitions

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

Transitions have an extra "finished transition" state which we use to keep transitions around but ignore them for most purposes. I find this confusing. It's unique to transitions yet we have to check this flag all over the codebase and it has led to bugs where we've failed to update this state.

I wonder, now that animations have an idle state, if we can use this idels state represent finished transitions.

I'm not certain that this is possible but, if it is, I think it could simplify our code considerably and help bring animations and transitions a little closer together.

Comment 1

[Brian Birtles (:birtles)]

Attachment: WIP part 1 - Try to drop IsFinishedTransition

Very much WIP, doesn't even compile. I started working on this but I realized
I need bug 1180125 to land first.

At first I tried cancelling finished transitions but I think from an API and
DevTools point of view that doesn't make sense. I think you'd expect the
transitions to be there but just be finished such that you can rewind them.

What that suggests, then, is that we need to make all the update the call sites
of IsFinishedTransition to check one or more of the following conditions:

 anim->HasCurrentEffect() -- means the animation has an effect and hasn't
                             finished yet
 anim->IsInEffect() -- means the animation is actually affecting style

For a finished transition, neither of those should be true but in some cases we
want to test for one or the other or both.

By making this change, we should actually fix correctness in the general case
(not just transitions).

The part where I got stuck is dispatching transitionend events where we need
a flag to recognize a newly finished transition. Once bug 1180125 lands that
flag will be readily available.

Comment 2

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #1)
> The part where I got stuck is dispatching transitionend events where we need
> a flag to recognize a newly finished transition. Once bug 1180125 lands that
> flag will be readily available.

It turns out this doesn't quite give us what we need.

* mPreviousFinishedState lets us recognize changes to the finished state in between calls to Animation::UpdateTiming.
* mFinishedAtLastComposeStyle lets us recognize changes to the finished state in between calls to Animation::ComposeStyle.

For dispatching events, however, we need to recognize changes to the finished state between calls to Animation::Tick. Furthermore, in order to use this in Animation::CanThrottle (in order to do away with mFinishedAtLastComposeStyle and be able to detect newly idle animations etc.) we'd currently need to store *two* flags--one for the latest play state during the previous tick, and one for the current tick. That seems really clumsy.

Longer-term we might end up doing something quite different such as:

* Requesting restyle from within a tick--that would allow us to get rid of CanThrottle altogether and explicitly request an unthrottled sample when we detect a change requiring it. This might happen fairly soon as part of the work on bug 1151731.

* Store the current tick's timeline time on the Animation. This would actually simplify things the most since at the start of a call to Tick() we could simply recalculate the play state with the old time and store it making it easy to detect changes in state throughout the lifetime of the tick.

I'm not quite sure what to do in the interim, however.

The good news is the patches I have in progress for this might fix a number of perf and correctness issues.

Comment 3

[Brian Birtles (:birtles)]

Attachment: part 1 - Remove use of IsFinishedTransition from Animation::CanThrottle

Previously we used IsFinishedTransition so that if the only animations present
are finished transitions we could throttle the tick. It turns out we can
generalize this further and throttle ticks for all finished animations
that are not newly finished, regardless of whether they are running on the
compositor or not (although this method won't be called unless the animation
property could be run on the compositor anyway).

This method is unfortunately quite confusing. For one, it is not strictly
limited to animations that are running on the compositor. It purports to only
return true when the animation is running on the compositor but the
mIsRunningOnCompositor flag doesn't get cleared when the animation finishes
(bug 1151694). As a result this method also deals with animations that are now
running on the main thread. I'm not sure why it ends up optimizing such
animations (by making sure we throttle finished animations running on the main
thread) but so long as we're doing that, we should do it properly.

This patch reworks this method so that it's hopefully a little easier to follow
and a little more consistent since I spent several hours trying to understand
the different combinations of inputs this method could take and what question
it was trying to answer.

Comment 4

[Brian Birtles (:birtles)]

Attachment: part 2 - Remove use of IsFinishedTransition from Animation::ComposeStyle

Animation::ComposeStyle uses IsFinishedTransition to skip doing work for
transitions that have run their course. We can, however, generalize this to
cover all animations that are not currently contributing to the animated
style--that is animations that are not "in effect".

We need to add this check *after* we update aNeedsRefreshes since an animation
that is not "in effect" because it has a delay and no backwards fill (in this
case it will have a play state of "running") still needs refreshes.

Comment 5

[Brian Birtles (:birtles)]

Attachment: part 3 - Remove use of IsFinishedTransition in KeyframeEffectReadOnly

KeyframeEffectReadOnly uses IsFinishedTransition to exclude finished transitions
from certain tests. In the case of IsInPlay and IsCurrent, this call is
redundant since finished transitions should already be in the finished state.
For KeyframeEffectReadOnly::IsInEffect(), this check is also redundant since
finished transitions have no forwards fill and hence the calculated progress
value will be null.

Comment 6

[Brian Birtles (:birtles)]

Attachment: part 4 - Remove use of IsFinishedTransition from nsLayoutUtils

GetMinAndMaxScaleForAnimationProperty in nsLayoutUtils uses IsFinishedTransition
to ignore finished transitions since they should not have any effect on current
or future scale values. We can generalize this, however, and say we are only
interested in animations that are *either*:

a) running or scheduled to run in the future, i.e. "current", OR
b) applying a value, including a finished animation with a forwards fill,
   i.e. "in effect"

Elsewhere, animations that fulfil *either* of this conditions are referred to as
"relevant animations" so we can simply test for relevance in this function.

Comment 7

[Brian Birtles (:birtles)]

Attachment: part 5 - Remove use of IsFinishedTransition from AnimationCollection::HasAnimationOfProperty

AnimationCollection::HasAnimationOfProperty uses IfFinishedTransition to filter
out transitions that should otherwise be ignored. This is used in the following
places:

1. nsLayoutUtils::HasAnimations

   The is only used by nsIFrame::BuildDisplayListForStackingContext to see if
   there are any opacity animations

   For this case, simply returning *current* animations would be sufficient
   (since finished but filling animations should have already filled in the
   display opacity)

2. CommonAnimationManager::GetAnimationsForCompositor

   This should really only return *current* animations--that is, animations that
   are running or scheduled to run. Finished animations never run on the
   compositor. Indeed, only *playing* animations run on the compositor but, as
   we will see in some of the cases below, it is sometimes useful to know that
   an animation *will* run on the compositor in the near future (e.g. so we can
   pre-render content).

   The places where GetAnimationsForCompositor is used are:

   - When building layers to add animations to layers in nsDisplayList--in this
     case we skip any animations that aren't playing so if
     GetAnimationsForCompositor only returned current animations that would be
     more than sufficient.

   - In nsLayoutUtils::HasAnimationsForCompositor. This in turn is used:

     - In ChooseScaleAndSetTransform to see if the transform is being animated
       on the compositor. If so, it calls
       nsLayoutUtils::ComputeSuitableScaleForAnimation (which also calls
       GetAnimationsForCompositor) and passes the result to
       GetMinAndMaxScaleForAnimationProperty which we have already adjusted in
       part 4 of this patch series to only deal with *relevant* animations

       Relevant animations include both current animations and in effect
       animations but we don't run forwards-filling animations on the compositor
       so GetAnimationsForCompositor should NOT return them. Current should be
       enough. In fact, playing should be enough but we might want to pre-render
       layers at a suitable size during their delay phase so current is probably
       ok.

     - In nsDisplayListBuilder::MarkOutOfFlowFrameForDisplay to add a fuzz
       factor to the overflow rect for frames undergoing a transform animation
       on the compositor. In this case too current should be sufficient.

     - In nsDisplayOpacity::NeedsActiveLayer to say "yes" if we are animating
       opacity on the compositor. Presumably in this case it would be good to
       say "yes" if the animation is in the delay phase too. After the animation
       is finished, we should drop the layer, i.e. current should be sufficient.

     - In nsDisplayTransform::ShouldPrerenderTransformedContent. As with
       nsDisplayOpacity::NeedsActiveLayer, we only need to pre-render
       transformed content for animations that are current.

     - In nsDisplayTransform::GetLayerState. As with
       nsDisplayOpacity::NeedsActiveLayer, we only need to return active here
       for current animations.

     - In nsIFrame::IsTransformed. Here we test the display style to see if
       there is a transform and also check if transform is being animated on the
       compositor. As a result, we really only need HasAnimationsForCompositor
       to return true for animations that are playing--otherwise the display
       style will tell us if we're transformed or not. Returning true for all
       current compositor animations (which is a superset of playing), however,
       should not cause problems (we already return true for even more than
       that).

     - In nsIFrame::HasOpacityInternal which is much the same as
       nsIFrame::IsTransformed and hence current should be fine.

3. AnimationCollection::CanThrottleAnimation

   Here, HasAnimationOfProperty is used when looking for animations that would
   disqualify us from throttling the animation by having an out-of-date layer
   generation or being a transform animation that affects scroll and so requires
   that we do the occasional main thread sample to update scrollbars.

   It would seem like current animations are enough here too. One interesting
   case is where we *had* a compositor animation but it has finished or been
   cancelled. In that case, the animation won't be current and we should not
   throttle the animation since we need to take it off its layer.

   It turns out checking for current animations is still ok in this case too.
   The reasoning is as follows:

   - If the animation is newly finished, we'll pick that up in
     Animation::CanThrottle and return false then.

   - If the animation is newly idle then there are two cases:

     If the cancelled animation was the only compositor animation then
     AnimationCollection::CanPerformOnCompositorThread will notice that there
     are no playing compositor animations and return false and
     AnimationCollection::CanThrottleAnimation will never be called.

     If there are other compositor animations running, then
     AnimationCollection::CanThrottleAnimation will still return false because
     whatever cancelled the animation will update the animation generation and
     we'll notice the mismatch between the layer animation generation and the
     animation generation on the collection.

Comment 8

[Brian Birtles (:birtles)]

Attachment: part 6 - Remove use of IsFinishedTransition from queueing transition end events

This patch introduces a separate flag to CSSTransition for tracking if a
transition is newly-finished so we can correctly dispatch the transitionend
event after removing the IsFinished method and corresponding flag.

Note that Animation already has mIsPreviousStateFinished and
mFinishedAtLastComposeStyle which would appear to be similar however,

- mIsPreviousStateFinished will be removed in bug 1178665 and is updated more
  often than we queue events so it is not useful here.
- mFinishedAtLastComposeStyle is used to determine if we can throttle a style
  update and is also updated more frequently than we queue events and hence
  can't be used here.

Once we guarantee one call to Tick() per frame we may be able to simplify this
by tracking "state on last tick" but for now we need this additional flag on
CSSTransition. CSSAnimation has a similar flag for this
(mPreviousPhaseOrItertaion) which we may be able to unify at the same point.

Comment 9

[Brian Birtles (:birtles)]

Attachment: part 7 - Remove use of IsFinishedTransition from nsTransitionManager::StyleContextChanged

Comment 10

[Brian Birtles (:birtles)]

Attachment: part 8 - Remove use of IsFinishedTransition from nsTransitionManager::ConsiderStartingTransition

Of particular note, this patch removes the check for finished transitions in
the case when we can't animate the current change. This case occurs when either
we have a non-transition style change that happens to coincide with the current
transition value, or a style change to something we can't interpolate to. In
either case it's not clear why we would cancel the existing transition only if
it is still in motion and not if it is finished. It seems like it should be
cancelled in either case and hence this patch removes this check.

The other change relates to detecting reversing transitions. In this case we do
need to distinguish between transitions that have finished and those that have
not. For this purpose, checking if the animation is current should be
sufficient. (Note that comparing for PlayState() == Finished would not be enough
since we want to also exclude *idle*, i.e. cancelled, animations too.)

Comment 11

[Brian Birtles (:birtles)]

Attachment: part 9 - Remove use of IsFinishedTransition from nsTransitionManager::PruneCompletedTransitions

Comment 12

[Brian Birtles (:birtles)]

Attachment: part 10 - Remove use of IsFinishedTransition from nsTransitionManager::FlushTransitions

Comment 13

[Brian Birtles (:birtles)]

Attachment: part 11 - Remove KeyframeEffect::IsFinished

Comment 14

[Brian Birtles (:birtles)]

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

Comment 15

[Brian Birtles (:birtles)]

Attachment: part 1 - Remove use of IsFinishedTransition from Animation::CanThrottle

Comment 16

[Brian Birtles (:birtles)]

Attachment: part 2 - Remove use of IsFinishedTransition from Animation::ComposeStyle

Comment 17

[Brian Birtles (:birtles)]

Attachment: part 3 - Remove use of IsFinishedTransition in KeyframeEffectReadOnly

Comment 18

[Brian Birtles (:birtles)]

Attachment: part 4 - Remove use of IsFinishedTransition from nsLayoutUtils

Comment 19

[Brian Birtles (:birtles)]

Attachment: part 5 - Remove use of IsFinishedTransition from AnimationCollection::HasAnimationOfProperty

Comment 20

[Brian Birtles (:birtles)]

Attachment: part 6 - Remove use of IsFinishedTransition from nsTransitionManager::StyleContextChanged

Comment 21

[Brian Birtles (:birtles)]

Attachment: part 7 - Remove use of IsFinishedTransition from nsTransitionManager::ConsiderStartingTransition

Comment 22

[Brian Birtles (:birtles)]

Attachment: part 8 - Remove use of IsFinishedTransition from nsTransitionManager::PruneCompletedTransitions

Comment 23

[Brian Birtles (:birtles)]

Attachment: part 9 - Remove use of IsFinishedTransition from nsTransitionManager::FlushTransitions

Comment 24

[Brian Birtles (:birtles)]

Attachment: part 10 - Remove KeyframeEffect::IsFinished

Comment 25

[Brian Birtles (:birtles)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=779b540e2ccb

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8639145 [details] [diff] [review]
part 1 - Remove use of IsFinishedTransition from Animation::CanThrottle

>Previously we used IsFinishedTransition so that if the only animations present
>are finished transitions we could throttle the tick. It turns out we can
>generalize this further and throttle ticks for all finished animations
>that are not newly finished, regardless of whether they are running on the
>compositor or not (although this method won't be called unless the animation
>property could be run on the compositor anyway).

In general, I feel like we shouldn't even be calling CanThrottle if mNeedsRefreshes is false, which should cover some of these cases.  That said, it doesn't seem to be harmful for it to say that we can throttle when mNeedsRefreshes is false.

>+  // Finished animations can be throttled unless this is the first
>+  // sample since finishing. In that case we need an unthrottled sample
>+  // so we can apply the correct end-of-animation behavior on the main
>+  // thread (either removing the animation style or applying the fill mode).
>+  if (PlayState() == AnimationPlayState::Finished) {
>+    return mFinishedAtLastComposeStyle;
>   }
> 
>+  // We should also ignore animations which are not "in effect"--i.e. not
>+  // producing an output. This will include animations that have finished
>+  // and don't apply a forwards fill as well as animations that are idle
>+  // or have yet to start.

This comment seems misleading given the previous test of PlayState() == Finished; those cases won't get to here.

>+  //
>+  // Note that unlike newly-finished animations, we don't need to worry about
>+  // special handling for newly-idle animations or animations that are newly
>+  // yet-to-start since any operation that would cause that change (e.g. a call
>+  // to cancel() on the animation, or seeking its current time) will trigger an
>+  // unthrottled sample.
>+  if (!IsInEffect()) {
>     return true;
>   }


r=dbaron with that fixed

(although I suppose I should think about how we can extend this to doing bug 1105509)

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8639146 [details] [diff] [review]
part 2 - Remove use of IsFinishedTransition from Animation::ComposeStyle

I assume something guarantees that finished transitions always return a PlayState() of Finished?  It certainly makes sense that they should.  Is the code in Animation::PlayState good enough to guarantee that?

r=dbaron

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8639147 [details] [diff] [review]
part 3 - Remove use of IsFinishedTransition in KeyframeEffectReadOnly

I guess this commit message answers my questions from the previous patch.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8639149 [details] [diff] [review]
part 5 - Remove use of IsFinishedTransition from AnimationCollection::HasAnimationOfProperty

At some point I'd like to have fewer of these has*animations methods, but this definitely seems like an improvement.

I think bullet (2) in the commit message could have been shorter, but no point trying to shorten it now.

r=dbaron

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8639151 [details] [diff] [review]
part 7 - Remove use of IsFinishedTransition from nsTransitionManager::ConsiderStartingTransition

Yeah, I probably should have made the first change in this patch when I made retention of finished transitions permanent.

I'm not sure I follow the parenthetical comment in the second paragraph of the commit message, though.

r=dbaron

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8639152 [details] [diff] [review]
part 8 - Remove use of IsFinishedTransition from nsTransitionManager::PruneCompletedTransitions

>This patch generalizes the logic in
>nsTransitionManager::PruneCompletedTransitions to consider all transitions that
>are no longer current (i.e. not running or scheduled to run) rather than
>those marked as finished.

That's a state they can get into via the Web animations API, but not via transitions, right?

Comment 32

[Brian Birtles (:birtles)]

(In reply to David Baron [:dbaron] ⏰UTC-7 (busy Aug. 8-Aug. 30) from comment #30)
> Comment on attachment 8639151 [details] [diff] [review]
> part 7 - Remove use of IsFinishedTransition from
> nsTransitionManager::ConsiderStartingTransition
> 
> Yeah, I probably should have made the first change in this patch when I made
> retention of finished transitions permanent.
> 
> I'm not sure I follow the parenthetical comment in the second paragraph of
> the commit message, though.

This is basically saying that when considering if an a new transition reverses an existing one, we want to ignore the existing transition if it is *either* finished (i.e. it ran its course and completed normally), or idle (i.e. script called cancel() at some point disabling the animation). The patch adds a check for HasCurrentEffect() since that will cause us to ignore both cases, whereas simply testing the PlayState() against 'Finished' would only detect the first case.

Comment 33

[Brian Birtles (:birtles)]

(In reply to David Baron [:dbaron] ⏰UTC-7 (busy Aug. 8-Aug. 30) from comment #31)
> Comment on attachment 8639152 [details] [diff] [review]
> part 8 - Remove use of IsFinishedTransition from
> nsTransitionManager::PruneCompletedTransitions
> 
> >This patch generalizes the logic in
> >nsTransitionManager::PruneCompletedTransitions to consider all transitions that
> >are no longer current (i.e. not running or scheduled to run) rather than
> >those marked as finished.
> 
> That's a state they can get into via the Web animations API, but not via
> transitions, right?

Yes, that's right. They will only enter the idle state via the Web animations API.

More precisely, they *can* enter the idle state if a style update cancels the animation (e.g. elem.style.transitionProperty = '') but only script using the Web animations API to hold on to the animation object would ever see it in such a state; it will be disassociated from the nsTransitionManager by that point.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8639153 [details] [diff] [review]
part 9 - Remove use of IsFinishedTransition from nsTransitionManager::FlushTransitions

Took me a little time to convince myself that this was ok, but I'm happy now.

I wonder if it would be clearer to write this loop differently:
>+      // Look for any transitions that can't be throttled because they
>+      // may be starting or stopping
>+      for (auto iter = collection->mAnimations.cbegin();
>+           canThrottleTick && iter != collection->mAnimations.cend();
>+           ++iter) {
>+        canThrottleTick &= (*iter)->CanThrottle();
>+      }

to make it more noticeable that it breaks out in the middle when canThrottleTick is false.

Maybe it's ok as-is, though.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8639154 [details] [diff] [review]
part 10 - Remove KeyframeEffect::IsFinished

>Bug 1181392 part 10 - Remove KeyframeEffect::IsFinished

IsFinishedTransition

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa8a6d1f67f
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e8a8884e36f
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf2c0c393cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8b8e1084b40
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9b2670f5c1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/5448e9a763c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/766cbaaa7879
https://hg.mozilla.org/integration/mozilla-inbound/rev/c105cadd52e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/76626b00fac9
https://hg.mozilla.org/integration/mozilla-inbound/rev/9060b380602b

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7fa8a6d1f67f
https://hg.mozilla.org/mozilla-central/rev/4e8a8884e36f
https://hg.mozilla.org/mozilla-central/rev/bcf2c0c393cf
https://hg.mozilla.org/mozilla-central/rev/f8b8e1084b40
https://hg.mozilla.org/mozilla-central/rev/c9b2670f5c1a
https://hg.mozilla.org/mozilla-central/rev/5448e9a763c4
https://hg.mozilla.org/mozilla-central/rev/766cbaaa7879
https://hg.mozilla.org/mozilla-central/rev/c105cadd52e2
https://hg.mozilla.org/mozilla-central/rev/76626b00fac9
https://hg.mozilla.org/mozilla-central/rev/9060b380602b