1181905 - Jump to initial position when setting playbackRate to 0 on compositor animation

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Do not multiply playback rate in SampleAnimations if the playback rate equals to zero.

That's my fault!

Comment 1

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> Do not multiply playback rate in SampleAnimations if the playback rate
> equals to zero.

Hmm, multipling zero seems to be correct...

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Possible fix

Skip animations if its playbackRate == 0.0.

I wondered that we should check calculated duration is less than or equals to 0 here[1]. But the above comment of the check says it's mainly for test purpose. So I add an if statement just before calculating duration.

[1] http://hg.mozilla.org/mozilla-central/file/f34a7120f46b/gfx/layers/composite/AsyncCompositionManager.cpp#l455

Comment 3

[Brian Birtles (:birtles)]

We probably shouldn't send animations to the compositor if the playbackRate is 0. We don't send animations to the compositor if they are paused.

Comment 4

[Brian Birtles (:birtles)]

Comment on attachment 8631395 [details] [diff] [review]
Possible fix

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

Normally we handle playback rate == 0 by adjusting the hold time (this happens in the set current time procedure[1] which is called when we update the playback rate). In order to support this on the compositor we'd have to be careful to pass the hold time to the compositor. I'm not sure if this works already but in any case, we should treat playback rate == 0 as equivalent to pausing, i.e. don't pass it to the compositor. We would only waste resources by putting it on the compositor and attempting to animate it there.

I imagine we should probably change Animation::IsPlaying to check for mPlaybackRate != 0.0. Then we should make the test check that the animation is running on the main thread.

[1] http://w3c.github.io/web-animations/#setting-the-current-time-of-an-animation

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Check in Animation::IsPlaying()

> I imagine we should probably change Animation::IsPlaying to check for
> mPlaybackRate != 0.0. Then we should make the test check that the animation
> is running on the main thread.

Thank you, it works!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29336244ecff
There were a couple of unknown oranges on Android 4.0 API11, but those fail on another tries. Those are not related to this patch.
Filed respectively bug 1182285, bug 1182294 and bug 1182292.

Comment 6

[Brian Birtles (:birtles)]

Comment on attachment 8631823 [details] [diff] [review]
Check in Animation::IsPlaying()

># HG changeset patch
># User Hiroyuki Ikezoe <[e-mail]>
># Parent  e419d48fb1aae81039e286daae8128e1ee3c02cc
>Bug 1181905 - Animation::IsPlaying should check playbackRate != 0 to stop playing on compositor animation.
>
>diff --git a/dom/animation/Animation.h b/dom/animation/Animation.h
>--- a/dom/animation/Animation.h
>+++ b/dom/animation/Animation.h
>@@ -245,16 +245,17 @@ public:
>    * candidates for running on the compositor (since we don't ship animations
>    * to the compositor when they are in their delay phase or paused).

Please update this comment to say something like

    * candidates for running on the compositor (since we don't ship animations
    * to the compositor when they are in their delay phase or paused including
    * being effectively paused due to having a zero playback rate).

>    */
>   bool IsPlaying() const
>   {
>     // We need to have an effect in its active interval, and
>     // be either running or waiting to run.
>     return HasInPlayEffect() &&
>+           mPlaybackRate != 0.0 &&
>            (PlayState() == AnimationPlayState::Running ||
>             mPendingState == PendingState::PlayPending);

Please update the comment as well to say something like:

     // We need to have an effect in its active interval, and
     // be either running or waiting to run with a non-zero playback
     // rate.

>diff --git a/layout/style/test/file_animations_playbackrate.html b/layout/style/test/file_animations_playbackrate.html
>--- a/layout/style/test/file_animations_playbackrate.html
>+++ b/layout/style/test/file_animations_playbackrate.html
...
>+addAsyncAnimTest(function *() {
>+  var [ div, cs ] = new_div("animation: anim 10s 1 linear forwards");
>+  var animation = div.getAnimations()[0];
>+  advance_clock(300);
>+  yield waitForPaints();
>+
>+  animation.playbackRate = 0;
>+
>+  yield waitForPaintsFlushed();
>+
>+  omta_is(div, "transform", { tx: 3 }, RunningOn.MainThread,
>+          "animation should stay the same position");

"animation with zero playback rate should stay in the same position and be running on the main thread"

The rest looks great. Thank you!

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Check in Animation::IsPlaying() v2

Fix comments. Thanks!

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/685f873fec78

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/685f873fec78