1112480 - For pending animations, store their start time but don't trigger them until the next refresh driver tick

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Bug 927349 introduces a means of deferring the start of animations until they have painted their first frame. When the first frame is painted the start time of the pending animations is recorded. Then we fast-forward the animation timeline to that time and set the start time of pending animations to the current timeline time.

That has a lot of problems including:
* In between the time where we fast-forward the timeline and the next refresh driver tick it is possible to observe an inconsistency between the timing of some animations and their computed style
* I'm pretty sure it means the difference between the code we use when testing differs from the code used when playing normally is greater than it needs to be

I think we could fix this by storing the start time on each pending player but not actually applying it until the next refresh driver tick. That way we only ever update timing once per refresh driver tick (with the exception of script-driven changes to timing properties, of course).

The difficulties with this are:

* We don't have a clear sense of when ticks occur inside dom/animation since layout/style can call AnimationPlayer::Tick multiple times per refresh driver tick. Long-term we should record all animation players observing a given AnimationTimeline against that timeline and make the timeline observe the refresh driver. Then we can make sure the timeline only ticks its children only once per refresh driver tick. In the short term we can just not apply the start time until it is >= the timeline time.

* We introduce an extra state "no longer pending but not started either" which we need to test. I was originally concerned that during this phase the compositor may have started playing the animation and if we then pause/cancel it we will get surprising results. However, since pause will also become async (bug 1109390) perhaps the results won't be so surprising after all.

Also, if we go ahead with storing animation players on their timeline and ticking them from there we could cause their ready promises to be resolved in a more predictable manner since presumably we would tick the animation players in their priority order.

Note that attachment 8533571 [details] [diff] [review] implements something similar to this. The difference is that it uses the timeline time at the next refresh tick which proved to be too late (based on some tests of UI animations on a Flame device).

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

The other nice thing about this is that when we don't have a document for the source content (and hence no pending player tracker) we can record the current timeline time as the start time to use and when the timeline ticks us we can apply that start time. That makes play() *always* asynchronous where as the patches in bug 927349 mean play() is actually synchronous in this (admittedly rare) case.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch

Here's a first pass at this. I'll tidy it up next week.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch v2

This version should pass more tests.

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

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch v3

This should mostly work. I just need to tidy it up and split it into pieces.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Adjust tests to accommodate changed animation start behavior

In this patch series we adjust the behavior of animation starting so that the
animation does not actually start until the following refresh driver tick. This
requires some tweaks to tests to ensure they continue to pass.

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 2 - Factor out ResumeAt

In addition to AnimationPlayer::StartNow, this patch series also makes
AnimationPlayer::Tick start animations.

Since these methods will share a lot of code we first factor out a common
ResumeAt method to encapsulate the common code.

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3 - Add AnimationPlayer::StartOnNextTick

Adds a method that schedules an animation player to begin on the next tick using
the supplied time as the start time.

We don't call this yet, however, but simply add the method and the
mPendingReadyTime member it sets.

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 4 - Use mPendingReadyTime in AnimationPlayer::Tick

This patch makes AnimationPlayer act on requests to StartOnNextTick by checking
for mPendingReadyTime on each tick.

We also check that the ready time is not in the future since currently it might
be possible that we get multiple calls to AnimationPlayer::Tick within a single
refresh driver tick.

Note that this patch shouldn't actually produce any observable change yet,
however, since we don't call StartOnNextTick anywhere.

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 5 - Add AnimationTimeline::IsUnderTestControl

Earlier in this patch series we added AnimationPlayer::StartOnNextTick which
takes a ready time parameter expressed in timeline time. In order to call this
method when painting finishes we need to convert the TimeStamp recorded when
painting finished to a timeline time. However, when the timeline is driven by
a refresh driver under test control we can no longer meaningfully do this
conversion since there is no correspondence between the notion of time used to
record the time when painting finished (wallclock time) and the notion of time
used by the timeline (which has been arbitrarily adjusted by test code).

We need a way to detect this situation so that we know not to call
ToTimelineTime in that case.

Alternatively, we could make ToTimelineTime automatically return a null value
when its refresh driver is under test control. However, in this situation
ToTimelineTime can still actually be used to convert a TimeStamp to a timeline
time as long as the TimeStamp is from the same refresh driver. Indeed,
GetCurrentTime does exactly that. So if we were to go down that route we would
have to provide a way for GetCurrentTime to work around that restriction.

For now, this patch puts the onus on the caller of ToTimelineTime to check if
the timeline is under test control first (unless they are passing a TimeStamp
from the same refresh driver, in which case there is no need to check).

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 6 - Make PendingPlayerTracker call StartOnNextTick

This patch switches on the new, "actually start the player in the next refresh
driver tick" behavior. It updates PendingPlayerTracker, adding
a StartPendingPlayersOnNextTick method which calls the appropriate method on
AnimationPlayer. The existing StartPendingPlayers is renamed to
StartPendingPlayersNow and is used for testing only.

Furthermore, since we now expect AnimationPlayer::StartOnNextTick to be
functional, AnimationPlayer::DoPlay is updated to use it when there is no
document available. This should make playing an animation player always
asynchronous, that is, always transition to the pending state temporarily
(unless we are already playing).

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 7 - Add checking for orphaned players

When a player is made pending, we rely on it being added to a pending player
tracker that will eventually start the player. However, there are a few
situations where this might not happen. For example, we can't find a pending
player tracker (e.g. there's no source content or the source content isn't
attached to a document), or the pending player tracker disappeared.

In these cases we still want to ensure that such a player does actually get
started. This patch attempts to detect such situations and start players
accordingly.

There are, unfortunately, currently no tests for this. I have been unsuccessful
in recreating any of the situations these tests are supposed to cover.

Review comment: I'll have another go at writing some tests for this otherwise it
might be ok to just leave this out?

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 8 - Remove AnimationTimeline::FastForward

Now that we don't actually start pending animations until the following refresh
driver tick we no longer need to be able to fast-forward the AnimationTimeline
between ticks.

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 9 - Remove call to PostUpdate from AnimationPlayer::ResumeAt

Since pending animations are no longer started outside of a style update, we no
longer need to call PostUpdate from ResumeAt.

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 10 - Mark css-animations/test_animation-pausing.html as no longer failing

With this updated approach to starting pending animations, this test should no
longer fail on Mac 10.8.

Update: In bug 1117955 I also disabled this test for Windows. Once that
changeset hits m-c I'll need to update this patch to re-enable the test for
Windows too.

Comment 15

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Currently running through try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bacddd7c687b

Comment 16

[Jonathan Watt [:jwatt]]

Comment on attachment 8544381 [details] [diff] [review]
part 3 - Add AnimationPlayer::StartOnNextTick

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

::: dom/animation/AnimationPlayer.cpp
@@ +146,5 @@
> +  // Normally we expect the play state to be pending but it's possible that,
> +  // due to the handling of possibly orphaned players in Tick() [coming
> +  // in a later patch in this series], this player
> +  // got started whilst still being in another document's pending player
> +  // map.

Maybe reformat this block so the third line doesn't finish short (or maybe that happens in a subsequent patch).

::: dom/animation/AnimationPlayer.h
@@ +99,4 @@
>     * Typically, when a player is played, it does not start immediately but is
>     * added to a table of pending players on the document of its source content.
>     * In the meantime it sets its hold time to the time from which should
>     * begin playback.

You could fix this while you're here (to "from which playback should begin").

@@ +103,5 @@
>     *
>     * When the document finishes painting, any pending players in its table
> +   * are marked as being ready to start by calling StartOnNextTick.
> +   * The moment when the paint completed is also recorded, converted to a
> +   * timeline time, and passed to StartOnTick.

I think it might be clearer to mention right here that it is so that the animations can be timed from this passed time.

@@ +108,5 @@
> +   *
> +   * After calling StartOnNextTick, players remain in the pending state until
> +   * the next refresh driver tick. At that time they transition out of the
> +   * pending state using the time passed to StartOnNextTick as the effective
> +   * at which they resumed.

"as the effective..."? Seems like text is missing.

@@ +121,5 @@
> +   *
> +   * - Starting the animation immediately when painting finishes is problematic
> +   *   because, in order to achieve a consistent state between all animations
> +   *   we would need to update the timeline, all attached animations and
> +   *   all their animated style twice for that refresh driver tick.

I don't think its immediately obvious why this is, but I'm not sure how to expand on this text without a long addition to the comment. If you can think of something feel free to add it.

@@ +134,5 @@
> +  void StartOnNextTick(const Nullable<TimeDuration>& aReadyTime);
> +
> +  // Testing only: Start a pending player using the current timeline time.
> +  // This is used to support existing tests that expect animations to begin
> +  // immediately.

Maybe append "Ideally we would rewrite the those tests and get rid of this method, but there are a lot of them."

Comment 17

[Jonathan Watt [:jwatt]]

Comment on attachment 8544382 [details] [diff] [review]
part 4 - Use mPendingReadyTime in AnimationPlayer::Tick

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

::: dom/animation/AnimationPlayer.cpp
@@ +134,5 @@
>  AnimationPlayer::Tick()
>  {
> +  if (mIsPending &&
> +      !mPendingReadyTime.IsNull() &&
> +      mPendingReadyTime.Value() <= mTimeline->GetCurrentTime().Value()) {

You might add a comment here noting why mPendingReadyTime may be a time in the future.

Comment 18

[Jonathan Watt [:jwatt]]

Comment on attachment 8544388 [details] [diff] [review]
part 8 - Remove AnimationTimeline::FastForward

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

\o/

Comment 19

[Brian Birtles (:birtles, Feb 26~ parental leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15f5148fe589
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfba5219541f
https://hg.mozilla.org/integration/mozilla-inbound/rev/73091940bedf
https://hg.mozilla.org/integration/mozilla-inbound/rev/69a084bd65e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/003c367a510f
https://hg.mozilla.org/integration/mozilla-inbound/rev/d99b6fd9ca56
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d3d4759f4ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/632d54874ed2
https://hg.mozilla.org/integration/mozilla-inbound/rev/2081656d2497
https://hg.mozilla.org/integration/mozilla-inbound/rev/95267af607c3

Try run of the same:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=20b043cd4fca

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/15f5148fe589
https://hg.mozilla.org/mozilla-central/rev/dfba5219541f
https://hg.mozilla.org/mozilla-central/rev/73091940bedf
https://hg.mozilla.org/mozilla-central/rev/69a084bd65e4
https://hg.mozilla.org/mozilla-central/rev/003c367a510f
https://hg.mozilla.org/mozilla-central/rev/d99b6fd9ca56
https://hg.mozilla.org/mozilla-central/rev/1d3d4759f4ed
https://hg.mozilla.org/mozilla-central/rev/632d54874ed2
https://hg.mozilla.org/mozilla-central/rev/2081656d2497
https://hg.mozilla.org/mozilla-central/rev/95267af607c3