1293945 - The animation will finish when setting small negative playbackRate.

Status: NEW

(Core :: DOM: Animation, defect, P3)

Description

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: test-case.patch

When setting small negative playbackRate, The animation will finish with currentTime=0.

We used following formula when getting current time.[1][2]
'(timeline time - start time) × playback rate'

[1] https://dxr.mozilla.org/mozilla-central/rev/6cf0089510fad8deb866136f5b92bbced9498447/dom/animation/Animation.cpp#237
[2] https://w3c.github.io/web-animations/#current-time

If playback rate is too small negative value, current time will be zero in Gecko.
The Gecko using TimeDuration when calculating time. The type of this TimeDuration's internal holding value is int64_t. So current time will be zero when calculated result is less than 1.

According to Web Animation spec, we will finish the animation when current time is zero and playbackRate < 0. [3]

[3] https://w3c.github.io/web-animations/#play-state


As the same point, start time will overflow.
In the |SilentlySetCurrentTime()|, we use following fomula.
'timeline time - (seek time / playback rate)'

[4] http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/dom/animation/Animation.cpp#653
[5] https://w3c.github.io/web-animations/#silently-set-the-current-time

If playback rate is too small negative value, start time will be overflow.
So start time will be big negative value. (We expect positive start time value).

The attachment file is testcase of web-platform tests.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

It seems that the playback rate in the test case is positive.

Comment 2

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: test-case.patch

Hi hiro,
Sorry I attached the wrong file.

The example page is as follow:
http://people.mozilla.org/~mantaroh/bug1293945/test-case.html

Comment 3

[Mantaroh Yoshinaga[:mantaroh]]

This phenomenon reason is the start time underflow.

When setting the playbackRate, we will calculate start time from previous current time and playbackRate.[1] 
But if playbackRate is too small negative value, the start time will become a big negative value.
In gecko, we hold this value as int64_t(TimeDuration), so it will underflow.

This underflow will effect several problems.
e.g. 
The Calculated current time will become zero always. So we judged animation is finished like this bug phenomenon.
(Because playbackRate is less than zero and current time is zero[2].)

[1] https://dxr.mozilla.org/mozilla-central/rev/97a52326b06a07930216ebefa5af333271578904/dom/animation/Animation.cpp#653
[2] https://dxr.mozilla.org/mozilla-central/rev/97a52326b06a07930216ebefa5af333271578904/dom/animation/Animation.cpp#328

I think that we will need to prevent the too small negative playbackRate which calculated current time become overflowed.
e.g.
If the calculated start time is overflowed, We will throw the TypeError.
(like 'the playbackRate is outside the supported range'.)

Brian, Hiro,
What do you think about this range of playbackRate check?

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add_PlaybackRate_Range_Check

This patch is an example for comment #3.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #3)
> This phenomenon reason is the start time underflow.
> 
> When setting the playbackRate, we will calculate start time from previous
> current time and playbackRate.[1] 
> But if playbackRate is too small negative value, the start time will become
> a big negative value.
> In gecko, we hold this value as int64_t(TimeDuration), so it will underflow.

If the overflow did not happen, can we obtain correct current time?
I've never considered about this spec, but I guess we can prevent this arithmetic error.
Given that the start time is evaluated lazily... I mean playbackRate * (1 / playbackRate) == 1.

Or should we set +Infinity to the current time in such case?  I think it's a reasonable way because we already set +Infinity on setCurrentTime if the input value large enough.

Comment 6

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

Comment on attachment 8782730 [details] [diff] [review]
Add_PlaybackRate_Range_Check

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

::: dom/animation/Animation.cpp
@@ +235,5 @@
>      Nullable<TimeDuration> timelineTime = mTimeline->GetCurrentTime();
>      if (!timelineTime.IsNull()) {
>        result.SetValue((timelineTime.Value() - mStartTime.Value())
>                          .MultDouble(mPlaybackRate));
> +      }

Is this supposed to be here?

@@ +291,5 @@
> +  TimeDuration determine = previousTime.Value().MultDouble(1/aPlaybackRate);
> +  if (aPlaybackRate < 0 && (determine == TimeDuration::NegativeForever())) {
> +    aRv.ThrowTypeError<dom::MSG_PLAYBACK_RATE_VALUE_OUT_OF_RANGE>();
> +    return;
> +  }

Rather than throwing an exception, why not just clamp aPlaybackRate to 0 which should be well-defined.

::: mozglue/misc/TimeStamp.h
@@ +142,5 @@
>    }
>  
> +  static BaseTimeDuration NegativeForever()
> +  {
> +    return FromTicks(INT64_MIN);

We don't need this. TimeDuration already defines a unary minus operator for this.[1] You can just use -TimeDuration::Forever() when you need to.

[1] http://searchfox.org/mozilla-central/rev/ae78ab94fadabc89fc6258d03c4a1a70f763f43a/mozglue/misc/TimeStamp.h#162

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

Thanks Hiro, Brian,

(In reply to Brian Birtles (:birtles) from comment #6)
> @@ +291,5 @@
> > +  TimeDuration determine = previousTime.Value().MultDouble(1/aPlaybackRate);
> > +  if (aPlaybackRate < 0 && (determine == TimeDuration::NegativeForever())) {
> > +    aRv.ThrowTypeError<dom::MSG_PLAYBACK_RATE_VALUE_OUT_OF_RANGE>();
> > +    return;
> > +  }
> 
> Rather than throwing an exception, why not just clamp aPlaybackRate to 0
> which should be well-defined.
I have a question about this. I think that this behavior is reasonable, because specified playback rate is just about nil.
But this behavior doesn't defined specification. Can we implement those undefined behavior?

Comment 8

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

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #7)
> (In reply to Brian Birtles (:birtles) from comment #6)
> > Rather than throwing an exception, why not just clamp aPlaybackRate to 0
> > which should be well-defined.
> I have a question about this. I think that this behavior is reasonable,
> because specified playback rate is just about nil.
> But this behavior doesn't defined specification. Can we implement those
> undefined behavior?

Yes, there is no requirement around the precision of playback rate so we will simply behaving in a manner consistent with the playback rate being represented with limited precision.

Comment 9

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1293945 - Set zero playback rate when setting incomputable playbackRate.

The calculated current time will be overflow when setting too small negative playback rate, because the gecko have current time as TimeDuration.
We set zero to the playback rate when setting too small negative playback rate in order to prevent this overflow.

Review commit: https://reviewboard.mozilla.org/r/73206/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73206/

Comment 10

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

Comment on attachment 8783817 [details]
Bug 1293945 - Set zero playback rate when setting incomputable playbackRate.

https://reviewboard.mozilla.org/r/73206/#review71326

::: dom/animation/Animation.cpp:290
(Diff revision 1)
> +  TimeDuration experimental = previousTime.Value().MultDouble(1/aPlaybackRate);
> +  if (aPlaybackRate < 0 && (experimental == -TimeDuration::Forever())) {
> +    aPlaybackRate = 0.0;
> +  }

Why do we only need to test when aPlaybackRate < 0? Can't we have a similar situation for a very small positive aPlaybackRate?

At very least, since we're only making this change in SetPlaybackRate (and not SilentlySetPlaybackRate) we'll fail to detect the following case:

  anim.playbackRate = 0.000000000000000000000000001;
  anim.reverse();

We probably should avoid dividing by aPlaybackRate when it's zero, but more to the point I think we need to make this test independent of the previousTime. Furthermore, I'm not sure this current test will work on all compilers.

So, as I understand it, it's the following calculation we're concerned about:

  mStartTime.SetValue(mTimeline->GetCurrentTime().Value() -
                      (aSeekTime.MultDouble(1 / mPlaybackRate)));

In particular, this part:

  aSeekTime.MultDouble(1.0 / mPlaybackRate);

Which is here:

  BaseTimeDuration MultDouble(double aMultiplier) const
  {
    return FromTicks(ValueCalculator::Multiply(mValue, aMultiplier));
  }

In this case, we have the following arguments:

  mValue = ~0.01
  aMultipler = 1.0 / -0.000000000000000000000000001
             = -1e+27

Which we pass here:

  inline int64_t
  TimeDurationValueCalculator::Multiply<double>(int64_t aA, double aB)
  {
    return static_cast<int64_t>(aA * aB);
  }

  aA = > ~0.01
  aB = -1e+27

We we will go to calculate as:

  static_cast<int64_t>(-1e+25);

Now, the minimum value for a signed 64-bit integer is -9,223,372,036,854,775,808
so we exceed that. Most compilers will truncate the result to
-9,223,372,036,854,775,808 which will happen to line up with the value we use
for -TimeDuration::Forever(), however, as far as I can tell, this behavior is
not guaranteed.


So, I think we have a couple of options:

a) Find a threshold value we can use to test for small playbackRate values that will allow us to avoid this situation for any value of current time.
b) Simply detect this situation in GetCurrentTime / SilentlySetCurrentTime and apply the appropriate behavior as needed.

I suspect (b) will be easier.

Comment 11

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: (WIP)Change_the_playbackrate_when_exceed_the_limit.patch

Thanks brian.

(In reply to Brian Birtles (:birtles) from comment #10)
> Comment on attachment 8783817 [details]
> Bug 1293945 - Set zero playback rate when setting incomputable playbackRate.
> 
> https://reviewboard.mozilla.org/r/73206/#review71326
> 
> ::: dom/animation/Animation.cpp:290
> (Diff revision 1)
> > +  TimeDuration experimental = previousTime.Value().MultDouble(1/aPlaybackRate);
> > +  if (aPlaybackRate < 0 && (experimental == -TimeDuration::Forever())) {
> > +    aPlaybackRate = 0.0;
> > +  }
> 
> Why do we only need to test when aPlaybackRate < 0? Can't we have a similar
> situation for a very small positive aPlaybackRate?
We should consider that using too small positiove playbackRate.

> So, I think we have a couple of options:
> 
> a) Find a threshold value we can use to test for small playbackRate values
> that will allow us to avoid this situation for any value of current time.
> b) Simply detect this situation in GetCurrentTime / SilentlySetCurrentTime
> and apply the appropriate behavior as needed.
> 
> I suspect (b) will be easier.
I think that we can prevent this problem when setting the playbackrate.

Probably we can calculate the playback rate as follow calculating formula:
 (computable playback rate) = (previous current time) / TimeDuration::Forever.

And I think that we should notify to the web developer via console log service.

Comment 12

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: (WIP)Change_the_playbackrate_when_exceed_the_limit.patch

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

Comment 13

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: (WIP)Change_the_playbackrate_when_exceed_the_limit.patch

The previous patch will fail to 'reverse() tests' due to set the zero playback rate.
So I fixed cause functions.

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

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.