1127380 - Implement AnimationPlayer.playbackRate

Status: RESOLVED FIXED

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

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

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

Comment on attachment 8572669 [details] [diff] [review]
patch

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

This looks good but I'd like to check DoPlay again after you've updated it.

Also, ResumeAt needs to be updated to check for a zero playback rate (see http://w3c.github.io/web-animations/#play-a-player).

And this needs tests.

::: dom/animation/AnimationPlayer.cpp
@@ +168,5 @@
> +  if (!previousTime.IsNull()) {
> +    SilentlySetCurrentTime(previousTime);
> +  }
> +}
> +

I don't think SilentSetPlaybackRate is used anywhere yet so can we drop it for now?

@@ +415,5 @@
>    // animation-play-state we *don't* trigger finishing behavior.
>  
> +  // XXX why does this not seem to match very well to
> +  // http://w3c.github.io/web-animations/#playing-a-player-section ???
> +  // Go back over that section and apply the 'rate' changes.

I don't understand this comment.

@@ +421,5 @@
>    Nullable<TimeDuration> currentTime = GetCurrentTime();
> +  if (mPlaybackRate > 0 &&
> +      (currentTime.IsNull() ||
> +       currentTime < 0 ||
> +       currentTime ≥ SourceContentEnd())) {

This is partially introducing finishing behavior (bug 1074630). I'm not sure we want to partially do it here since we'll probably end up with odd behavior where it partially works. In particular, as per the comment at the start of this method, we should *not* do finishing behavior when called due to a change in animation-play-state.

It's probably better to do this all properly in bug 1074630.

@@ +426,5 @@
>      mHoldTime.SetValue(TimeDuration(0));
> +  } else if (mPlaybackRate < 0 &&
> +             (mHoldTime.IsNull() ||
> +              currentTime <= 0 ||
> +              currentTime > SourceContentEnd())) {

Likewise, here too.

@@ +431,1 @@
>      // If the hold time is null, we are already playing normally

I'm not sure this comment still makes sense if we're changing the condition above. We should probably just change this logic to match the pseudocode in the spec.

@@ +438,5 @@
> +
> +  if (mHoldTime.IsNull()) {
> +    if (mReady) {
> +      mReady->MaybeResolve(this);
> +    }

As far as I can tell, this isn't necessary until we implement deferred pausing (bug 1109390) so we should just drop this for now.

::: dom/animation/AnimationPlayer.h
@@ +52,5 @@
>  
>  public:
>    explicit AnimationPlayer(AnimationTimeline* aTimeline)
>      : mTimeline(aTimeline)
> +    , mPlaybackRate(1)

This might be more clear as 1.0.

::: dom/webidl/AnimationPlayer.webidl
@@ +23,5 @@
>    attribute double? startTime;
>    [SetterThrows, BinaryName="currentTimeAsDouble"]
>    attribute double? currentTime;
>  
> +           attribute double             playbackRate;

This will need review from a DOM peer.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: part 1 - code changes

Comment 4

[Jonathan Watt [:jwatt]]

Comment on attachment 8574985 [details] [diff] [review]
part 1 - code changes

Also tag smaug for review of the minor DOM changes.

Comment 5

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

Comment on attachment 8574985 [details] [diff] [review]
part 1 - code changes

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

r=me with comments addressed. Particularly, the one about setting mHoldTime to zero instead of null in DoPlay.

::: dom/animation/AnimationPlayer.cpp
@@ +53,5 @@
>  #endif
>    Nullable<TimeDuration> previousCurrentTime = GetCurrentTime();
>    mStartTime = aNewStartTime;
>    if (!aNewStartTime.IsNull()) {
> +    if (mPlaybackRate != 0) {

Here, and a half a dozen other places in this file I think it would be better to compare against 0.0 so it's clear we're doing floating-point comparison.

@@ +150,5 @@
>    // http://w3c.github.io/web-animations/#update-a-players-finished-state
>  }
>  
> +void
> +AnimationPlayer::SetPlaybackRate(double aPlaybackRate)

The duplication between SetPlaybackRate and SilentlySetPlaybackRate isn't great. With the simplifications to SetCurrentTime proposed in bug 1072037 comment 15 this will become a little more simple.

What do you think about adding an enum parameter to toggle between the two modes? I don't mind if you want to keep them separate however.

@@ +417,5 @@
>    // animation-play-state we *don't* trigger finishing behavior.
>  
>    Nullable<TimeDuration> currentTime = GetCurrentTime();
> +  if (mPlaybackRate > 0 &&
> +      (currentTime.IsNull())) {

Drop the extra parentheses here around currentTime.IsNull() and just put it all on one line? We can re-add them when we need to.

@@ +422,3 @@
>      mHoldTime.SetValue(TimeDuration(0));
> +  } else if (mPlaybackRate < 0 &&
> +             (currentTime.IsNull())) {

Likewise here.

@@ +423,5 @@
> +  } else if (mPlaybackRate < 0 &&
> +             (currentTime.IsNull())) {
> +    mHoldTime.SetValue(TimeDuration(SourceContentEnd()));
> +  } else if (mPlaybackRate == 0 && currentTime.IsNull()) {
> +    mHoldTime.SetNull();

The spec says to set the hold time to zero in this case. Is there a reason we make it null instead?

@@ +480,5 @@
>               "Expected to resume a pending player");
>    MOZ_ASSERT(!mHoldTime.IsNull(),
>               "A player in the pending state should have a resolved hold time");
>  
> +  mStartTime.SetValue(aResumeTime - (mHoldTime.Value() / mPlaybackRate));

I think we need to handle mPlaybackRate == 0.0 here. For this part, the spec says:

> If the player playback rate is zero, let new start time be simply ready time.

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: extra tweak

Comment 7

[Jonathan Watt [:jwatt]]

As requested by Brian, pushed to unblock some of his work:

https://hg.mozilla.org/integration/mozilla-inbound/rev/78195e7115a2

I'll finish the tests and put them up for review soon.

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/78195e7115a2

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: tests

There are now several playbackRate tests in:

dom/animation/test/css-animations/test_animation-currenttime.html
dom/animation/test/css-animations/test_animation-finished.html

but we should still add at least these tests.

Comment 10

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

Comment on attachment 8595708 [details] [diff] [review]
tests

We really should go and add tests to every algorithm that factors in the playbackRate (e.g. when setting the startTime we need to check of playbackRate == 0).

I understand you might not have bandwidth for that now but could we at least add a test that checks that playbackRate actually affects the currentTime (the current test would pass even if we never implemented playbackRate). We could return the times of the document timeline between subsequent frames and then check that the corresponding difference in currentTime is proportional to the playbackRate?

Comment 11

[Jonathan Watt [:jwatt]]

timeline.currentTime isn't a sampled time that remains unchanged during a frame. It's live, so:

  assert_equals(timeline.currentTime, timeline.currentTime, 'blah');

may fail. To work around that there will need to be some sort of error tolerance, but I'm not sure that we can do that and guarantee avoiding orange. Maybe if I make the playbackRate something very large?

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: tests

Perhaps something like this?

Comment 13

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

(In reply to Jonathan Watt [:jwatt] from comment #11)
> timeline.currentTime isn't a sampled time that remains unchanged during a
> frame.

It should remain unchanged during a frame.

> It's live, so:
> 
>   assert_equals(timeline.currentTime, timeline.currentTime, 'blah');
> 
> may fail.

It shouldn't.

Comment 14

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

(In reply to Jonathan Watt [:jwatt] from comment #12)
> Created attachment 8595850 [details] [diff] [review]
> tests
> 
> Perhaps something like this?

It looks mostly fine but I'm not sure why you say animation.timeline.currentTime is not frozen within a frame?

Comment 15

[Jonathan Watt [:jwatt]]

(In reply to Brian Birtles (:birtles) from comment #14)
> It looks mostly fine but I'm not sure why you say
> animation.timeline.currentTime is not frozen within a frame?

I'm not sure what I'm recalling, but at some point there was code that got a TimeStamp::Now() call rather than fetching the time from the refresh driver. Anyway, that seems not to be relevant here. That said, I do still seem to need around a 2% error tolerance when running this test locally. I'm not sure where that's coming from.

Comment 16

[Jonathan Watt [:jwatt]]

Attachment: tests

Comment 17

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

Comment on attachment 8598994 [details] [diff] [review]
tests

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

I think this test is assuming that when the ready promise resolves the animation's currentTime is zero but the startTime of animations can actually fall between frames.

Let's assume the we create the animation 500ms after navigationStart and we get one animation every 16ms (i.e. 60fps).

t=500ms:

Animation is played
timeline.currentTime = 500;

t=510ms:

We finish painting the first frame of the animation and record the ready time as '510' (or actually the equivalent TimeStamp)
We store the pending ready time on the animation.

t=516ms: (the next frame)

We apply the ready time to the animation as its startTime:
animation.startTime = 510;
animation.currentTime = 6;

We resolve its ready promise:
initialTimelineTime = animation.timeline.currentTime - animation.currentTime;
i.e. initialTimelineTime = 516 - 6 = 510;

Then we set the playbackRate to 100.
The animation will automatically seek so that its currentTime remains 6. It does this by adjusting the startTime.
animation.startTime = timeline time - (seek time / playback rate) = 516 - (6 / 100) = 515.94

Wait 10 frames (16ms each):

t=676ms:

timelineTimeDelta = animation.timeline.currentTime - initialTimelineTime
                  = 676 - 510
                  = 166

timeAdvanceRatio = animation.currentTime / timelineTimeDelta
                 = [(timeline time - start time) * playback rate] / timelineTimeDelta
                 = [(676 - 515.94) * 100] / timelineTimeDelta
                 = 16006 / 166
                 = 96.422...

Hence why the comparison to 100 has a large margin of error.

A few suggestions:

* We should just record the timeline current times and animation current times between subsequent frames and compare the ratio. If we do that, we can set the playbackRate before waiting for the ready promise which simplifies the test somewhat.
* I think a playbackRate of 10 would be better since if the test machine is really lagged, waiting 10 frames with a playbackRate of 100 could put us near to the end of the active interval. In the example above where we run at an ideal 60fps, we still get 16s into the 1000s interval. If a machine is really lagged and takes 1s per frame we would end up outside the active interval and run into finishing behavior which would cause the test to fail.
* Also, I wonder if we need to wait 10 frames? If we do the measurement correctly there should be no error (except floating-point error) so 2 or 3 frames should be plenty.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

jwatt, if you can not take time to finish the the test, may I do it?

Comment 19

[Jonathan Watt [:jwatt]]

Certainly, that would be very helpful. I think there are probably some bugs that will be found by the tests though.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Attachment: tests v2

Updated jwatt's tests.

* Checking that animation.currentTime is surely advanced as expected in each frame.
* Checking that animation element position as well.
* Changing playbackRate to 10, 3, 21 in each frame.
* 100ms busy loop in a frame to simulate lag.

For reference here is a try results on android and b2g.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ed526c31256

The patch in the try is slightly different from this patch:
 * does not check animation element position yet
 * does factor out currentTime check as new function

Though Android 4.3 debug tests are not finished yet now, other results are perfect.

One thing that is not clear to me is accuracy for checking element position. It's now 0.1 in this patch because it's pixel but I am not sure it's a proper value there.

Comment 21

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

Comment on attachment 8629188 [details]
tests v2

Looks good but I think it could be simplified further.

>+async_test(function(t) {
>+  var div = addDiv(t, {'class': 'animated-div'});
>+
>+  div.style.animation = 'anim 10s';

This fine but I think it could be a little neater to just change the stylesheet to:

  div {
    /* Make it easier to calculate expected values: */
    animation-timing-function: linear ! important;
  }

And then make this:

  var div = addDiv(t, { 'style' : 'animation: anim 10s' });

>+function assert_playbackrate(target,
>+                             previousAnimationCurrentTime,
>+                             previousTimelineCurrentTime,
>+                             description) {
>+  var accuracy = 0.001; /* accuracy of DOMHighResTimeStamp */
>+
>+  var animation = target.getAnimations()[0];

This should just take animation as a parameter (instead of target).

>+function assert_animation_position(target,
>+                                   previousAnimationCurrentTime,
>+                                   previousMarginLeft,
>+                                   description) {
>+  var accuracy = 0.1; /* what is a proper value here? */

I don't think we need this method. We should just test the result of playbackRate on the currentTime (not on margin-left). 

>+async_test(function(t) {
>+  var div = addDiv(t, {'class': 'animated-div'});
>+
>+  div.style.animation = 'anim 10s';
>+
>+  var animation = div.getAnimations()[0];
>+  animation.playbackRate = 10;
>+
>+  var previousTimelineCurrentTime;
>+  var previousAnimationCurrentTime;
>+  var previousMarginLeft;

I don't think we need previousMarginLeft.

>+  animation.ready.then(function() {
>+    previousAnimationCurrentTime = animation.currentTime;
>+    previousTimelineCurrentTime = animation.timeline.currentTime;
>+    previousMarginLeft = parseFloat(getComputedStyle(div).marginLeft);
>+    return waitForFrame();
>+  }).then(t.step_func(function() {
>+    assert_playbackrate(div,
>+                        previousAnimationCurrentTime,
>+                        previousTimelineCurrentTime,
>+                        "animation.currentTime should be 10 times faster than timeline.");

This, I think, is enough? I don't think we need the rest of the test?

>+    // Psuedo busyness
>+    var timeAtStart = window.performance.now();
>+    while (window.performance.now() - timeAtStart < 100) {
>+    }
>+    return waitForFrame();
>+  })).then(t.step_func(function() {
>+    assert_playbackrate(div,
>+                        previousAnimationCurrentTime,
>+                        previousTimelineCurrentTime,
>+                        "animation.currentTime should be 10 times faster than timeline even if main thread is busy.");

Why would the result differ if the main thread was busy?

>+    assert_animation_position(div,
>+                              previousAnimationCurrentTime,
>+                              previousMarginLeft);
>+
>+    animation.playbackRate = 3;

Why would a playbackRate of 10 work, but a playbackRate of 3 fail?

If this is meant to test a dynamic update of playbackRate while moving, then it should be a separate test. In general we are trying to keep each test as small and isolated as possible.

>+    previousAnimationCurrentTime = animation.currentTime;
>+    previousTimelineCurrentTime = animation.timeline.currentTime;
>+    previousMarginLeft = parseFloat(getComputedStyle(div).marginLeft);
>+    return waitForFrame();
>+  })).then(t.step_func(function() {
>+    assert_playbackrate(div,
>+                        previousAnimationCurrentTime,
>+                        previousTimelineCurrentTime,
>+                        "animation.currentTime should be 3 times faster than timeline now.");
>+    assert_animation_position(div,
>+                              previousAnimationCurrentTime,
>+                              previousMarginLeft);
>+
>+    animation.playbackRate = 21;

Again, why would 21 be any different?

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Thanks quick reply!

(In reply to Brian Birtles (:birtles) from comment #21)
> >+async_test(function(t) {
> >+  var div = addDiv(t, {'class': 'animated-div'});
> >+
> >+  div.style.animation = 'anim 10s';
> 
> This fine but I think it could be a little neater to just change the
> stylesheet to:
> 
>   div {
>     /* Make it easier to calculate expected values: */
>     animation-timing-function: linear ! important;
>   }
> 
> And then make this:
> 
>   var div = addDiv(t, { 'style' : 'animation: anim 10s' });

Will do.

> >+function assert_playbackrate(target,
> >+                             previousAnimationCurrentTime,
> >+                             previousTimelineCurrentTime,
> >+                             description) {
> >+  var accuracy = 0.001; /* accuracy of DOMHighResTimeStamp */
> >+
> >+  var animation = target.getAnimations()[0];
> 
> This should just take animation as a parameter (instead of target).

Will do.
 
> >+function assert_animation_position(target,
> >+                                   previousAnimationCurrentTime,
> >+                                   previousMarginLeft,
> >+                                   description) {
> >+  var accuracy = 0.1; /* what is a proper value here? */
> 
> I don't think we need this method. We should just test the result of
> playbackRate on the currentTime (not on margin-left). 

I think this sort of tests are necessary to avoid bugs like bug 1175751.
Actually animation.currentTime is correctly advanced in case of CSS transform(the case of bug 1175751).

But I will remove this for now and reconsider it in bug 1175751.

> >+async_test(function(t) {
> >+  var div = addDiv(t, {'class': 'animated-div'});
> >+
> >+  div.style.animation = 'anim 10s';
> >+
> >+  var animation = div.getAnimations()[0];
> >+  animation.playbackRate = 10;
> >+
> >+  var previousTimelineCurrentTime;
> >+  var previousAnimationCurrentTime;
> >+  var previousMarginLeft;
> 
> I don't think we need previousMarginLeft.

Will do.

> >+  animation.ready.then(function() {
> >+    previousAnimationCurrentTime = animation.currentTime;
> >+    previousTimelineCurrentTime = animation.timeline.currentTime;
> >+    previousMarginLeft = parseFloat(getComputedStyle(div).marginLeft);
> >+    return waitForFrame();
> >+  }).then(t.step_func(function() {
> >+    assert_playbackrate(div,
> >+                        previousAnimationCurrentTime,
> >+                        previousTimelineCurrentTime,
> >+                        "animation.currentTime should be 10 times faster than timeline.");
> 
> This, I think, is enough? I don't think we need the rest of the test?
> 
> >+    // Psuedo busyness
> >+    var timeAtStart = window.performance.now();
> >+    while (window.performance.now() - timeAtStart < 100) {
> >+    }
> >+    return waitForFrame();
> >+  })).then(t.step_func(function() {
> >+    assert_playbackrate(div,
> >+                        previousAnimationCurrentTime,
> >+                        previousTimelineCurrentTime,
> >+                        "animation.currentTime should be 10 times faster than timeline even if main thread is busy.");
> 
> Why would the result differ if the main thread was busy?

Actually I do not know.
My intention here is that this is some kind of black-box test.
I imagined that animation.currentTime should be advanced correctly even if some frames are skipped because of heavy load in main thread. But OK I will remove this.

> >+    assert_animation_position(div,
> >+                              previousAnimationCurrentTime,
> >+                              previousMarginLeft);
> >+
> >+    animation.playbackRate = 3;
> 
> Why would a playbackRate of 10 work, but a playbackRate of 3 fail?
> 
> If this is meant to test a dynamic update of playbackRate while moving, then
> it should be a separate test. In general we are trying to keep each test as
> small and isolated as possible.

I will create the separate test.
 
> >+    previousAnimationCurrentTime = animation.currentTime;
> >+    previousTimelineCurrentTime = animation.timeline.currentTime;
> >+    previousMarginLeft = parseFloat(getComputedStyle(div).marginLeft);
> >+    return waitForFrame();
> >+  })).then(t.step_func(function() {
> >+    assert_playbackrate(div,
> >+                        previousAnimationCurrentTime,
> >+                        previousTimelineCurrentTime,
> >+                        "animation.currentTime should be 3 times faster than timeline now.");
> >+    assert_animation_position(div,
> >+                              previousAnimationCurrentTime,
> >+                              previousMarginLeft);
> >+
> >+    animation.playbackRate = 21;
> 
> Again, why would 21 be any different?

Will remove.

Comment 23

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #22)
> > >+function assert_animation_position(target,
> > >+                                   previousAnimationCurrentTime,
> > >+                                   previousMarginLeft,
> > >+                                   description) {
> > >+  var accuracy = 0.1; /* what is a proper value here? */
> > 
> > I don't think we need this method. We should just test the result of
> > playbackRate on the currentTime (not on margin-left). 
> 
> I think this sort of tests are necessary to avoid bugs like bug 1175751.
> Actually animation.currentTime is correctly advanced in case of CSS
> transform(the case of bug 1175751).
> 
> But I will remove this for now and reconsider it in bug 1175751.

Thanks. We should work out what the cause is for bug 1175751 and write a specific test for that. If it's something that could affect other implementations then we can add it here, otherwise we can put it in the 'mozilla' directory.

> > >+    // Psuedo busyness
> > >+    var timeAtStart = window.performance.now();
> > >+    while (window.performance.now() - timeAtStart < 100) {
> > >+    }
> > >+    return waitForFrame();
> > >+  })).then(t.step_func(function() {
> > >+    assert_playbackrate(div,
> > >+                        previousAnimationCurrentTime,
> > >+                        previousTimelineCurrentTime,
> > >+                        "animation.currentTime should be 10 times faster than timeline even if main thread is busy.");
> > 
> > Why would the result differ if the main thread was busy?
> 
> Actually I do not know.
> My intention here is that this is some kind of black-box test.
> I imagined that animation.currentTime should be advanced correctly even if
> some frames are skipped because of heavy load in main thread. But OK I will
> remove this.

I'm afraid I don't really understand how this might fail. If the main thread is busy the next frame will be delayed but not skipped. If somehow the currentTime and computed style are getting out of sync then it's probably not related to just playbackRate?

Comment 24

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #23)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #22)
> > > >+function assert_animation_position(target,
> > > >+                                   previousAnimationCurrentTime,
> > > >+                                   previousMarginLeft,
> > > >+                                   description) {
> > > >+  var accuracy = 0.1; /* what is a proper value here? */
> > > 
> > > I don't think we need this method. We should just test the result of
> > > playbackRate on the currentTime (not on margin-left). 
> > 
> > I think this sort of tests are necessary to avoid bugs like bug 1175751.
> > Actually animation.currentTime is correctly advanced in case of CSS
> > transform(the case of bug 1175751).
> > 
> > But I will remove this for now and reconsider it in bug 1175751.
> 
> Thanks. We should work out what the cause is for bug 1175751 and write a
> specific test for that. If it's something that could affect other
> implementations then we can add it here, otherwise we can put it in the
> 'mozilla' directory.

Thanks for the explanation. I will do it next time when I face such situations.

> > > Why would the result differ if the main thread was busy?
> > 
> > Actually I do not know.
> > My intention here is that this is some kind of black-box test.
> > I imagined that animation.currentTime should be advanced correctly even if
> > some frames are skipped because of heavy load in main thread. But OK I will
> > remove this.
> 
> I'm afraid I don't really understand how this might fail. If the main thread
> is busy the next frame will be delayed but not skipped. If somehow the
> currentTime and computed style are getting out of sync then it's probably
> not related to just playbackRate?

Ah, right. The next frame is not skipped. I imagined the frame number I had been trying to implement in VSync drivers (bug 1167487).  In that case the frame is skipped but from the perspective of main thread it's just delayed. Sorry for the confusion.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Attachment: tests v3

Addressed all comments.

* some style fix (indentation, use single quote instead of double one).
* factor out create_animation function.

Comment 26

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

Comment on attachment 8629242 [details] [diff] [review]
tests v3

>+function create_animation(test) {
>+  var div = addDiv(test, {'style': 'animation: anim 10s'});
>+  var animation = div.getAnimations()[0];
>+
>+  animation.playbackRate = 10;
>+
>+  return animation;
>+}

I don't think we should factor this out as a separate function. I think it makes the tests harder to read (since you have to remember that 'create_animation' *also* updates the playbackRate to 10 and creates a new div).

If you want to keep this function then please remove the line where we set the playbackRate and rename 'createAnimation' (we've been using camelCase elsewhere in these tests--it's only the testharness.js methods that use underscores_to_separate_words).

>+async_test(function(t) {
>+  var animation = create_animation(t);
>+
>+  var previousTimelineCurrentTime;
>+  var previousAnimationCurrentTime;
>+
>+  animation.ready.then(function() {
>+    previousAnimationCurrentTime = animation.currentTime;
>+    previousTimelineCurrentTime = animation.timeline.currentTime;
>+
>+    animation.playbackRate = 1;
>+
>+    return waitForFrame();
>+  }).then(t.step_func_done(function() {
>+    assert_equals(animation.playbackRate, 1,
>+      'double check that animation.playbackRate should surely be 1.');

I'm not sure we need this but if we do, perhaps 'sanity check: animation.playbackRate is still 1' is a more clear description?

The rest looks great. Thanks for doing this.

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Attachment: tests v4

Updated.

Pushed a try now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd9b3fc5280f

I will add checkin-needed once the try results are good.

(In reply to Brian Birtles (:birtles) from comment #26)
> rename 'createAnimation' (we've been using camelCase
> elsewhere in these tests--it's only the testharness.js methods that use
> underscores_to_separate_words).

Wow! I did not notice it. I was wondering why there are camelCase and underscore_case.
Anyway the function was removed in this patch.

Thanks!

Comment 28

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #27)
> Pushed a try now:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd9b3fc5280f
> 
> I will add checkin-needed once the try results are good.

Thanks! You can drop the leave-open too when you do.

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Attachment: Disable tests on android

Unfortunately the tests failed on android emulator.

An example:

155 INFO TEST-UNEXPECTED-FAIL | dom/animation/test/css-animations/test_animation-playbackrate.html | Test the effect of setting playbackRate on currentTime - Test the effect of setting playbackRate on currentTime: assert_approx_equals: animation.currentTime should be 10 times faster than timeline. expected 8999.999639999996 +/- 0.001 but got 1786.0272100000002

I am suspecting this failure is specific to android. (Re-trying 10 times on b2g was no failures of this test, one failure of another test.) So I'd propose to disable this test on android for now. And I will investigate the failure reason in a new bug.

Comment 30

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

Comment on attachment 8629716 [details] [diff] [review]
Disable tests on android

>diff --git a/dom/animation/test/mochitest.ini b/dom/animation/test/mochitest.ini
>--- a/dom/animation/test/mochitest.ini
>+++ b/dom/animation/test/mochitest.ini
>@@ -14,16 +14,17 @@ support-files = css-animations/file_anim
> support-files = css-animations/file_animation-finished.html
> [css-animations/test_animation-pausing.html]
> support-files = css-animations/file_animation-pausing.html
> [css-animations/test_animation-play.html]
> support-files = css-animations/file_animation-play.html
> [css-animations/test_animation-playbackrate.html]
> support-files = css-animations/file_animation-playbackrate.html
> [css-animations/test_animation-playstate.html]
>+skip-if = os == 'android'
> support-files = css-animations/file_animation-playstate.html

I think you meant to put this under 'playbackrate', not 'playstate'.


(In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
...
> 155 INFO TEST-UNEXPECTED-FAIL |
> dom/animation/test/css-animations/test_animation-playbackrate.html | Test
> the effect of setting playbackRate on currentTime - Test the effect of
> setting playbackRate on currentTime: assert_approx_equals:
> animation.currentTime should be 10 times faster than timeline. expected
> 8999.999639999996 +/- 0.001 but got 1786.0272100000002
> 
> I am suspecting this failure is specific to android. (Re-trying 10 times on
> b2g was no failures of this test, one failure of another test.) So I'd
> propose to disable this test on android for now. And I will investigate the
> failure reason in a new bug.

Seeing as this code has already landed without tests, I think it's ok to land these tests with Android turned off. However, it's a pretty significant failure so we should investigate it soon. Please leave a comment here with the bug number for investigating this.

r=me with the 'skip-if' in the right place

Thanks again for looking into this.

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Attachment: Disable tests on android

Fix the horrible mistake.

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8629251 [details]
tests v4

I am sorry for wrong flag here.

Comment 33

[Hiroyuki Ikezoe (:hiro)]

> (In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> ...
> > 155 INFO TEST-UNEXPECTED-FAIL |
> > dom/animation/test/css-animations/test_animation-playbackrate.html | Test
> > the effect of setting playbackRate on currentTime - Test the effect of
> > setting playbackRate on currentTime: assert_approx_equals:
> > animation.currentTime should be 10 times faster than timeline. expected
> > 8999.999639999996 +/- 0.001 but got 1786.0272100000002
> > 
> > I am suspecting this failure is specific to android. (Re-trying 10 times on
> > b2g was no failures of this test, one failure of another test.) So I'd
> > propose to disable this test on android for now. And I will investigate the
> > failure reason in a new bug.
> 
> Seeing as this code has already landed without tests, I think it's ok to
> land these tests with Android turned off. However, it's a pretty significant
> failure so we should investigate it soon. Please leave a comment here with
> the bug number for investigating this.

Filed bug 1180559.

Comment 34

[Carsten Book [:Tomcat]]

Hi, this failed to apply:
patching file dom/animation/test/mochitest.ini
Hunk #1 FAILED at 11
1 out of 1 hunks FAILED -- saving rejects to file dom/animation/test/mochitest.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh skip_playbackrate_on_android.patch

could you take a look ? thanks!

Comment 35

[Hiroyuki Ikezoe (:hiro)]

Attachment: Tests v5 with disabling the tests on android

That's my fault. I meant both of files are needed to be checked-in.

Here is a unified patch. Please check this in. I am sorry for confusion.

Comment 36

[Carsten Book [:Tomcat]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #35)
> Created attachment 8629817 [details] [diff] [review]
> Tests v5 with disabling the tests on android
> 
> That's my fault. I meant both of files are needed to be checked-in.
> 
> Here is a unified patch. Please check this in. I am sorry for confusion.

np :)

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04420ed3c036

Comment 38

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=11384529&repo=mozilla-inbound

Comment 39

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1017a7caaa3

Comment 40

[Hiroyuki Ikezoe (:hiro)]

(In reply to Carsten Book [:Tomcat] from comment #38)
> sorry had to back this out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=11384529&repo=mozilla-
> inbound

This failure is the same as the one on android emulator. I will investigate it.
Thanks!

Comment 41

[Hiroyuki Ikezoe (:hiro)]

Attachment: Test v6 longer duration and smaller plabackRate

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

As I wrote in bug 1180559 comment #6, 100s and 2x playbackRate bring good results on the try.

Comment 42

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8630273 [details] [diff] [review]
Test v6 longer duration and smaller plabackRate

I posted Old patch. sorry.

Comment 43

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

Comment on attachment 8630273 [details] [diff] [review]
Test v6 longer duration and smaller plabackRate

>diff --git a/dom/animation/test/mochitest.ini b/dom/animation/test/mochitest.ini
>--- a/dom/animation/test/mochitest.ini
>+++ b/dom/animation/test/mochitest.ini
>@@ -11,16 +11,19 @@ support-files = css-animations/file_anim
> [css-animations/test_animation-finish.html]
> support-files = css-animations/file_animation-finish.html
> [css-animations/test_animation-finished.html]
> support-files = css-animations/file_animation-finished.html
> [css-animations/test_animation-pausing.html]
> support-files = css-animations/file_animation-pausing.html
> [css-animations/test_animation-play.html]
> support-files = css-animations/file_animation-play.html
>+[css-animations/test_animation-playbackrate.html]
>+skip-if = os == 'android'
>+support-files = css-animations/file_animation-playbackrate.html

Can we drop the "skip-if = os == 'android'" part now?

Comment 44

[Hiroyuki Ikezoe (:hiro)]

Attachment: Test v7 longer duration and smaller plabackRate

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

This is the correct one.

Comment 45

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3dfa2011601

Comment 46

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e3dfa2011601

Comment 47

[Jonathan Watt [:jwatt]]

Thanks, hiro!