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.

This should past most tests but needs a bit more work.

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.

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.

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.

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: {} }),

(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.

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.

Address review feedback from comment 9.

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.

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.

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.

(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!

Parts 1-4 https://hg.mozilla.org/integration/mozilla-inbound/rev/03dd3dff57b3 https://hg.mozilla.org/integration/mozilla-inbound/rev/35b6ee1472e9 https://hg.mozilla.org/integration/mozilla-inbound/rev/cb71bf27300c https://hg.mozilla.org/integration/mozilla-inbound/rev/c12acbaf33d8

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.

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.

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.

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.

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.

https://hg.mozilla.org/mozilla-central/rev/03dd3dff57b3 https://hg.mozilla.org/mozilla-central/rev/35b6ee1472e9 https://hg.mozilla.org/mozilla-central/rev/cb71bf27300c https://hg.mozilla.org/mozilla-central/rev/c12acbaf33d8

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. ;)

(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. /** * ... */

Parts 5-15 (more to come): https://hg.mozilla.org/integration/mozilla-inbound/rev/71e33f9c871e https://hg.mozilla.org/integration/mozilla-inbound/rev/ff6b730f3a89 https://hg.mozilla.org/integration/mozilla-inbound/rev/92fd678cb47c https://hg.mozilla.org/integration/mozilla-inbound/rev/38f3018ddf85 https://hg.mozilla.org/integration/mozilla-inbound/rev/de1b1fbecf37 https://hg.mozilla.org/integration/mozilla-inbound/rev/e443000da039 https://hg.mozilla.org/integration/mozilla-inbound/rev/7d0f03b20dbe https://hg.mozilla.org/integration/mozilla-inbound/rev/bd35c507997e https://hg.mozilla.org/integration/mozilla-inbound/rev/7d8a20857b50 https://hg.mozilla.org/integration/mozilla-inbound/rev/a9ae40ed6d0c https://hg.mozilla.org/integration/mozilla-inbound/rev/ecac23a4d713

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.

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.

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().

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).

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.

https://hg.mozilla.org/mozilla-central/rev/71e33f9c871e https://hg.mozilla.org/mozilla-central/rev/ff6b730f3a89 https://hg.mozilla.org/mozilla-central/rev/92fd678cb47c https://hg.mozilla.org/mozilla-central/rev/38f3018ddf85 https://hg.mozilla.org/mozilla-central/rev/de1b1fbecf37 https://hg.mozilla.org/mozilla-central/rev/e443000da039 https://hg.mozilla.org/mozilla-central/rev/7d0f03b20dbe https://hg.mozilla.org/mozilla-central/rev/bd35c507997e https://hg.mozilla.org/mozilla-central/rev/7d8a20857b50 https://hg.mozilla.org/mozilla-central/rev/a9ae40ed6d0c https://hg.mozilla.org/mozilla-central/rev/ecac23a4d713

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1a7fa0fe2f8 https://hg.mozilla.org/integration/mozilla-inbound/rev/ccc27d049cd1 https://hg.mozilla.org/integration/mozilla-inbound/rev/d4306ea579d9 Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0a1dcefed10

https://hg.mozilla.org/mozilla-central/rev/d1a7fa0fe2f8 https://hg.mozilla.org/mozilla-central/rev/ccc27d049cd1 https://hg.mozilla.org/mozilla-central/rev/d4306ea579d9

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.

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.

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.

Comment on attachment 8584400 [details] [diff] [review] part 19 - Add AnimationPlayer::IsPlayingOrPausing It turns out I don't need this after all.

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.

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.

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.

(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.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab074c8ddb56 https://hg.mozilla.org/integration/mozilla-inbound/rev/6bec9324b555 https://hg.mozilla.org/integration/mozilla-inbound/rev/ded0fc853a7b https://hg.mozilla.org/integration/mozilla-inbound/rev/2273676edfc2 https://hg.mozilla.org/integration/mozilla-inbound/rev/6df80592242c https://hg.mozilla.org/integration/mozilla-inbound/rev/a3d94537f677 https://hg.mozilla.org/integration/mozilla-inbound/rev/99b338f0c780 https://hg.mozilla.org/integration/mozilla-inbound/rev/e78d118cfc91 https://hg.mozilla.org/integration/mozilla-inbound/rev/948c0b8b82c2 Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2d6363d3250

https://hg.mozilla.org/mozilla-central/rev/ab074c8ddb56 https://hg.mozilla.org/mozilla-central/rev/6bec9324b555 https://hg.mozilla.org/mozilla-central/rev/ded0fc853a7b https://hg.mozilla.org/mozilla-central/rev/2273676edfc2 https://hg.mozilla.org/mozilla-central/rev/6df80592242c https://hg.mozilla.org/mozilla-central/rev/a3d94537f677 https://hg.mozilla.org/mozilla-central/rev/99b338f0c780 https://hg.mozilla.org/mozilla-central/rev/e78d118cfc91 https://hg.mozilla.org/mozilla-central/rev/948c0b8b82c2