Closed Bug 1109390 Opened 5 years ago Closed 5 years ago

Make AnimationPlayer.pause wait to sync up with the compositor

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(30 files, 2 obsolete files)

694 bytes, text/html
Details
45.23 KB, patch
Details | Diff | Splinter Review
4.85 KB, patch
jwatt
: review+
birtles
: checkin+
Details | Diff | Splinter Review
1.11 KB, patch
jwatt
: review+
birtles
: checkin+
Details | Diff | Splinter Review
7.76 KB, patch
jwatt
: review+
birtles
: checkin+
Details | Diff | Splinter Review
1.78 KB, patch
pbro
: review+
birtles
: checkin+
Details | Diff | Splinter Review
10.35 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
5.15 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
7.01 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
8.96 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
1.64 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
9.25 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
1.63 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
2.22 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
3.85 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
1.83 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
1.16 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
4.19 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
2.82 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
5.84 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
6.53 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
8.89 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
5.24 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
4.01 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
4.87 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
11.88 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
3.50 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
2.17 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
2.21 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
1.80 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
In bug 927349 we make play wait until the first paint is complete before actually beginning playback. In the meantime we go to the pending state.

For pausing we should do the same thing. Currently we set the pause time based where the main thread is currently up to. However, when the main thread is busy this can be significantly behind where the compositor is up to. As a result, the animation jumps backwards to pause.

Instead, we should go to the pending state until we sync up with the compositor. Perhaps we could remove the animation from the layer then wait for EndComposite.
We need this not only for the sake of the Web Animations API which says that pause is async, but also because it represents an observable performance problem where pausing an animation that is running on the compositor can cause the animation to jump backwards if the main thread is lagging behind. This test case demonstrates that jump back (needs OMTA to be turned on).
Thinking out loud about how to fix this, I think there are number of approaches. Firstly, from an API point of view, the pausing needs to happen async whether or not the animation is running on the compositor. This is for consistency and is also what the spec requires (or should do---it could be more clear about this but this is what we agreed with Google should happen).

With that in mind, we have a few possibilities:

 (a) For main thread animations resolve the time immediately but don't set it until later. For compositor animations wait until we get DidComposite and use that as the pause time.
 (b) For all animations wait until we get DidComposite and use that.
 (c) For all animations use the same animation start time as we use for the async start of animations (bug 927349).
 (d) For all animations simply don't pause until the next frame.

(a) is no good. If you pause two animations at the same time (and that started at the same time) they should end up in lock-step regardless of whether they were running on the compositor or not.

(b) is probably ideal but introduces complexity: firstly it looks difficult to hook up to DidCompositeWindow cleanly (except for listening for MozAfterPaint) but more significantly, because it's async there's a window of time between the layer transaction where we removed paused animations and when we get DidComposite. If further animations were paused inbetween we'd have to be careful to *not* treat them as being paused, i.e. we'd have to represent two different states--animations that are paused and just waiting for the compositor to finish its job and animations that are paused but which have yet to be removed from the compositor. Perhaps we can use the existing mIsRunningOnCompositor for that, however.

(c) Is the easiest. We'd be reusing almost the exact same code as we do for async starting which has already been tested for a while now. The main drawback is that you could still get animations jumping backwards if there's a significant delay between ending the layer transaction on the content side and applying the update on the compositor side. That's still going to be a *lot* better than what we currently do, but it's still a drawback.

(d) is probably going to be too late in most cases.
  

I lean towards (c) because it's simplest but (b) might not be so bad if we can re-use mIsRunningOnCompositor to detect animations that have been paused since the last layer transaction.
Attached patch WIP patch v1Splinter Review
This should past most tests but needs a bit more work.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
In comparing our algorithm for playing an animation (player) I noticed we don't set startTime to null like the spec says to. The behavior the spec is trying to produce is that startTime is null while we're waiting to play (since, in a general since, the purpose of asynchronous playing is to resolve the startTime to a suitable time). Meanwhile, when we're waiting to pause startTime is not cleared until we actually pause.

I went to write a test to prove that we have a bug but we don't yet support enough of the API to test this. This is because if we're paused or idle then startTime is already null. If we're running then we'll ignore a redundant call to play(). The only other states we can transition to play-pending from are finished (bug 1074630) and pause-pending (this bug).

I'm going to put up some patches which add tests--or will once we support the necessary features.

There are going to be quite a few patches in this series so I'll just put them up for review as they're ready and land them in chunks.
This brings us into line with the algorithm in:

  https://w3c.github.io/web-animations/#play-an-animation

which makes the other patches in this series easier to compare with
the specification.
Attachment #8580536 - Flags: review?(jwatt)
Although pause() is not yet asynchronous, any time we finish calling it the
ready promise should be resolved so we can safely wait on the ready promise
after calling pause already. This way, once pause() becomes async later in this
bug, code relying on this actor will continue to work.
Attachment #8580539 - Flags: review?(pbrosset)
Now that we have separate tests for checking the initial state of startTime we
can remove these checks from tests for setting the startTime.

Also, while we're at it, we needn't check the playState and animationPlayState
since these should be covered by other tests.
Attachment #8580542 - Flags: review?(jwatt)
Comment on attachment 8580539 [details] [diff] [review]
part 4 - Make DevTools animation actor wait for asynchronous pause

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

Thanks Brian. I think you also need to change AnimationsActor.pauseAll:

  pauseAll: method(function() {
    for (let player of this.getAllAnimationPlayers()) {
      player.pause();
    }
    this.allAnimationsPaused = true;
  }, {
    request: {},
    response: {}
  }),

I think it should now do like AnimationsActor.playAll and promise.all all ready promises of the players that are being paused.:

  playAll: method(function() {
    let readyPromises = [];
    for (let player of this.getAllAnimationPlayers()) {
      player.play();
      readyPromises.push(player.ready);
    }
    this.allAnimationsPaused = false;
    return promise.all(readyPromises);
  }, {
    request: {},
    response: {}
  }),
Attachment #8580539 - Flags: review?(pbrosset)
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #9)
> Thanks Brian. I think you also need to change AnimationsActor.pauseAll:

Did you mean to r+ this? For future reference, if not, I've found it useful to change r? to feedback+ if a patch is along the right lines but you want another pass at review once comments are addressed.
Attachment #8580534 - Flags: review?(jwatt) → review+
Comment on attachment 8580542 [details] [diff] [review]
part 2 - Remove some unneeded startTime tests

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

You might want to look at checkStateOnSettingCurrentTimeToZero too at some point.
Attachment #8580542 - Flags: review?(jwatt) → review+
Attachment #8580536 - Flags: review?(jwatt) → review+
Address review feedback from comment 9.
Attachment #8581441 - Flags: review?(pbrosset)
Attachment #8580539 - Attachment is obsolete: true
A number of animation tests assume that pausing happens instantaneously. This
patch adjust many of those tests so that they will continue to work when
pausing happens asynchronously. In many cases this is possible because we
know the ready promise on AnimationPlayer (soon to be Animation) objects will
be resolved so we can wait on it and it will resolve immediately now, but when
asynchronous pausing is introduced the test the promise won't resolve until
after the pause operation is complete.

There are some tests that can't be so easily adjusted and we will have to fix
these at the same time as we turn on async pausing. However, taking care of
this set of tests now should reduce the size of subsequent patches in this
series.
Attachment #8581465 - Flags: review?(jwatt)
This patch extends the PendingPlayerTracker which is currently used to record
which animations are waiting to play, such that it can also handle animations
which are waiting to complete a pause operation.

It doesn't yet do anything with the pause-pending animations, that will come
in another patch.
Attachment #8581477 - Flags: review?(jwatt)
Attachment #8581465 - Flags: review?(jwatt) → review+
Comment on attachment 8581477 [details] [diff] [review]
part 6 - Generalize PendingPlayerTracker to support pausing as well

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

::: dom/animation/PendingPlayerTracker.cpp
@@ +27,2 @@
>  {
> +  aSet.PutEntry(&aPlayer);

I think it makes the code more difficult to understand when you pass in a member. It's unexpected that that's what a caller would be doing. Besides that, I personally don't think the amount of code duplication you're avoiding warrants the complication of the indirection to another function.
Attachment #8581477 - Flags: review?(jwatt) → review+
Attachment #8581441 - Flags: review?(pbrosset) → review+
(In reply to Jonathan Watt [:jwatt] from comment #15)
> Comment on attachment 8581477 [details] [diff] [review]
> part 6 - Generalize PendingPlayerTracker to support pausing as well
> 
> Review of attachment 8581477 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/PendingPlayerTracker.cpp
> @@ +27,2 @@
> >  {
> > +  aSet.PutEntry(&aPlayer);
> 
> I think it makes the code more difficult to understand when you pass in a
> member. It's unexpected that that's what a caller would be doing. Besides
> that, I personally don't think the amount of code duplication you're
> avoiding warrants the complication of the indirection to another function.

Why is it unexpected to pass a member?

I think that not duplicating three different functions (and risking bugs from them getting out of sync) is worthwhile. I don't think it's complicated but of course I would say that!
Attachment #8580534 - Flags: checkin+
Attachment #8580542 - Flags: checkin+
Attachment #8580536 - Flags: checkin+
Attachment #8581441 - Flags: checkin+
These methods will soon be used to start animations that are waiting to start
and also to finish pausing animations that are waiting to pause. As a result
we rename them to TriggerXXX since that's a bit more generic.

There are still references to StartXXX within PendingPlayerTracker. These will
be updated in a subsequent patch once we have the appropriate methods available
on AnimationPlayer to call.
Attachment #8582280 - Flags: review?(jwatt)
This won't actually do anything yet because:
(a) We don't yet add pause-pending players to the PendingPlayerTracker
(b) We never mark pausing players as pending so
    AnimationPlayer::TriggerOnNextTick will just ignore them.
Attachment #8582282 - Flags: review?(jwatt)
IsPaused is used in nsAnimationManager to detect if a newly created animation
should be paused. It is also used inside AnimationPlayer::IsRunning which is
used to determine what animations to send to the compositor (we don't send
paused animations to the compositor). In all these cases we want to treat paused
animations and pause-pending animations alike.

We should possibly rename IsPaused to IsPausedOrPausing but that seems a bit
cumbersome.

This patch also adjusts a few nearby one-line functions to put the opening brace
on a new line since apparently this is what the coding style says to do.
Attachment #8582285 - Flags: review?(jwatt)
This patch simply updates the method that cancels pending plays to also cancel
pending pauses. As it stands, for some of places where this is called it might
not be appropriate to cancel pending pauses but we will adjust each of these
call sites one-by-one in subsequent patches in this series.
Attachment #8582286 - Flags: review?(jwatt)
There are still a couple more code patches plus some test patches to come, but I'd like to test them a bit more before putting them up for review.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Target Milestone: mozilla39 → ---
Attachment #8582280 - Flags: review?(jwatt) → review+
Attachment #8582281 - Flags: review?(jwatt) → review+
Attachment #8582282 - Flags: review?(jwatt) → review+
Attachment #8582283 - Flags: review?(jwatt) → review+
Attachment #8582284 - Flags: review?(jwatt) → review+
Comment on attachment 8582285 [details] [diff] [review]
part 12 - Update IsPaused to handle pause-pending players as well

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

I would be for the IsPausedOrPausing rename. I much prefer cumbersome names that prevent me drawing incorrect conclusions about code to actually drawing incorrect conclusions about code. ;)
Attachment #8582285 - Flags: review?(jwatt) → review+
(In reply to Jonathan Watt [:jwatt] from comment #29)
> Comment on attachment 8582285 [details] [diff] [review]
> part 12 - Update IsPaused to handle pause-pending players as well
> 
> Review of attachment 8582285 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would be for the IsPausedOrPausing rename. I much prefer cumbersome names
> that prevent me drawing incorrect conclusions about code to actually drawing
> incorrect conclusions about code. ;)

Yes, on further thought, I agree. Will do.
Comment on attachment 8582286 [details] [diff] [review]
part 13 - Cancel pending pauses as well as pending plays

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

::: dom/animation/AnimationPlayer.h
@@ +244,5 @@
>    void PostUpdate();
>    // Remove this player from the pending player tracker and reset
>    // mPendingState as necessary. The caller is responsible for resolving or
>    // aborting the mReady promise as necessary.
> +  void CancelPendingTasks();

While you're here can you update this comment to mention pausing, and change the comment to a proper Doxygen style documenting comment? I.e.

/**
 * ...
 */
Attachment #8582286 - Flags: review?(jwatt) → review+
Attachment #8582287 - Flags: review?(jwatt) → review+
Attachment #8582288 - Flags: review?(jwatt) → review+
In preparation for introducing IsInPlay (where "in play" is a term in the Web
Animations spec), this patch aligns the existing IsCurrent with the definition
in the spec that says an animation effect is only current if it is attached
to an animation (player in our current naming) that is not finished. In order
to ensure that we need to pass the animation/player into the method.

This actually changes the behavior of IsCurrent since now we will return false
for animations that are finished. As far as I can tell, all the call sites that
are requesting current animations should only be concerned with animations that
are actually running. If not, they need to be adjusted to look for animations
that are either current or in effect.
Attachment #8584365 - Flags: review?(jwatt)
This patch adds a method for testing if an animation is "in play" which is
a term defined in the Web Animations spec. This is in preparation for removing
some slightly redundant code in IsRunning and aligning better with the spec.
Attachment #8584366 - Flags: review?(jwatt)
This patch renames the confusing IsRunning method since IsRunning() is *not*
the same as (PlayState() == AnimationPlayState::Running). It also removes
the old definition to make better re-use of PlayState() and IsInPlay().
Attachment #8584368 - Flags: review?(jwatt)
Attachment #8584365 - Flags: review?(jwatt) → review+
Attachment #8584366 - Flags: review?(jwatt) → review+
Attachment #8584368 - Flags: review?(jwatt) → review+
Attachment #8584400 - Flags: review?(jwatt)
This patch adds an options flag to GetAnimationsForCompositor for three reasons.

a) We want to reuse this functionality in nsLayoutUtils.cpp rather than
   duplicating the same logic. To do that and maintain the existing behavior,
   however, we need to *not* update the active layer tracker when calling this
   from nsLayoutUtils.cpp.

b) It's surprising that GetAnimationsForCompositor also has this side effect of
   updating the active layer tracker. Adding this as an option makes it clear at
   the call site that this is what will happen.

c) We will later reuse this flag to specify if we are interested in only getting
   the compositor animations that should be *added* (i.e. where
   AnimationPlayer::IsPlaying is true) or also the ones that might already be
   running but shouldn't be added in the next transaction (i.e. where
   AnimationPlayer::IsPlayingOrPausing is true).
Attachment #8584401 - Flags: review?(jwatt)
There are still a few more patches to come but I'll add them next week. Feel free to wait until then to review these as it might hard to tell what they're trying to do without the rest of the picture.
Attachment #8584400 - Flags: review?(jwatt) → review+
Attachment #8584401 - Flags: review?(jwatt) → review+
Attachment #8584402 - Flags: review?(jwatt) → review+
This patch adds the method that is called when an asynchronous pause operation
has completed. It is not used yet, however, since we don't yet put
AnimationPlayer objects in the pause-pending map.
Attachment #8585297 - Flags: review?(jwatt)
When a pending pause operation is interrupted by a play operation we should
preserve the original start time of the animation so that it appears to continue
moving uninterrupted. At the same time, however, for consistency with other
calls to play(), the operation should complete asynchronously.
Attachment #8585298 - Flags: review?(jwatt)
This patch (finally) puts pausing animations in the pending player map so that
they are resolved asynchronously.

Since this changes the pausing behavior this patch updates a number of tests so
that they continue to pass.
Attachment #8585302 - Flags: review?(jwatt)
Comment on attachment 8584400 [details] [diff] [review]
part 19 - Add AnimationPlayer::IsPlayingOrPausing

It turns out I don't need this after all.
Attachment #8584400 - Attachment is obsolete: true
That should be everything!

I had a very complicated approach that worked by forcing layers to appear to be active while they were pause-pending but after I removed all the debugging statements I noticed some flicker could still appear even with that approach. Instead, I've gone with something simpler (part 24) which seems to work for all tests I tried. There's still occasionally flicker when aborting a pause but that will be fixed when bug 1117603 lands.
Attachment #8585297 - Flags: review?(jwatt) → review+
Attachment #8585298 - Flags: review?(jwatt) → review+
Attachment #8585300 - Flags: review?(jwatt) → review+
Attachment #8585302 - Flags: review?(jwatt) → review+
Attachment #8585303 - Flags: review?(jwatt) → review+
Comment on attachment 8585305 [details] [diff] [review]
part 27 - Add further test to test_animations-pausing.html for cancelling a pause by setting the current time

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

::: dom/animation/test/css-animations/test_animation-pausing.html
@@ +197,5 @@
> +    // has executed. That might give false positives in some cases but it's
> +    // a useful minimum bar.
> +    player.ready.then(function() { readyPromiseRun = true; });
> +    return waitForFrame();
> +  })).then(t.step_func(function() {

I'm not sure what this buys us over just returning |player.ready|, since that will time out if the ready promise doesn't resolve. Besides that it's unclear to me if this is a bit racy, given that both the resolve callback and next frame are async.
Attachment #8585305 - Flags: review?(jwatt) → review+
Comment on attachment 8585307 [details] [diff] [review]
part 28 - Add tests for the AnimationPlayer.startTime when pausing asynchronously

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

::: dom/animation/test/css-animations/test_animation-player-starttime.html
@@ +352,5 @@
> +  assert_equals(player.startTime, null, 'The initial startTime is null');
> +
> +  player.ready.then(t.step_func(function() {
> +    assert_true(player.startTime > 0,
> +                'After the player has started, startTime is > 0');

Seems better to keep a record of the timeline time when the animation is created, and check that startTime is greater than that.

@@ +362,5 @@
> +    // by causing startTime to appear to not change).
> +    getComputedStyle(div).animationPlayState;
> +
> +    assert_equals(player.startTime, startTimeBeforePausing,
> +                  'The startTime does not change when pausing-pending');

Reading player.startTime involves reading from a changing time source, right? (There's no caching of startTime.) So I think this may be a bit racy.
Attachment #8585307 - Flags: review?(jwatt) → review+
Attachment #8585309 - Flags: review?(jwatt) → review+
(In reply to Jonathan Watt [:jwatt] from comment #54)
> Comment on attachment 8585307 [details] [diff] [review]
> part 28 - Add tests for the AnimationPlayer.startTime when pausing
> asynchronously
> 
> Review of attachment 8585307 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/test/css-animations/test_animation-player-starttime.html
> @@ +352,5 @@
> > +  assert_equals(player.startTime, null, 'The initial startTime is null');
> > +
> > +  player.ready.then(t.step_func(function() {
> > +    assert_true(player.startTime > 0,
> > +                'After the player has started, startTime is > 0');
> 
> Seems better to keep a record of the timeline time when the animation is
> created, and check that startTime is greater than that.

Yes, thanks. Fixed.

> @@ +362,5 @@
> > +    // by causing startTime to appear to not change).
> > +    getComputedStyle(div).animationPlayState;
> > +
> > +    assert_equals(player.startTime, startTimeBeforePausing,
> > +                  'The startTime does not change when pausing-pending');
> 
> Reading player.startTime involves reading from a changing time source,
> right? (There's no caching of startTime.) So I think this may be a bit racy.

It shouldn't be changing. It should be fixed from when the player enters the running state until it enters the paused state.

(The one time where we have times that aren't locked to frame times or anything is when we let the currentTime be set from TimeStamp::Now() while pause-pending. The spec actually says that when we're pause-pending we should report currentTime as null so that shouldn't be detectable but I haven't implemented that behaviour yet--partly because it seems wrong to me.)
Depends on: 1149906
Depends on: 1207951
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.