The animation will finish when setting small negative playbackRate.

NEW
Assigned to

Status

()

P3
normal
3 years ago
2 years ago

People

(Reporter: mantaroh, Assigned: mantaroh)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8779649 [details] [diff] [review]
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.
(Assignee)

Updated

3 years ago
Component: CSS Parsing and Computation → DOM: Animation
It seems that the playback rate in the test case is positive.
(Assignee)

Comment 2

3 years ago
Created attachment 8779656 [details] [diff] [review]
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
(Assignee)

Updated

3 years ago
Attachment #8779649 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Summary: The animation will finish when setting negative small playbackRate. → The animation will finish when setting small negative playbackRate.
Priority: -- → P3
(Assignee)

Comment 3

2 years ago
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?
Flags: needinfo?(hiikezoe)
Flags: needinfo?(bbirtles)
(Assignee)

Comment 4

2 years ago
Created attachment 8782730 [details] [diff] [review]
Add_PlaybackRate_Range_Check

This patch is an example for comment #3.
(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.
Flags: needinfo?(hiikezoe)
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
Flags: needinfo?(bbirtles)
(Assignee)

Comment 7

2 years ago
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?
Flags: needinfo?(bbirtles)
(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.
Flags: needinfo?(bbirtles)
Comment hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
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.
Attachment #8783817 - Flags: review?(bbirtles)
(Assignee)

Comment 11

2 years ago
Created attachment 8788353 [details] [diff] [review]
(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.
Assignee: nobody → mantaroh
Attachment #8782730 - Attachment is obsolete: true
(Assignee)

Comment 12

2 years ago
Created attachment 8789248 [details] [diff] [review]
(WIP)Change_the_playbackrate_when_exceed_the_limit.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7af1380bb6d
Attachment #8788353 - Attachment is obsolete: true
(Assignee)

Comment 13

2 years ago
Created attachment 8789630 [details] [diff] [review]
(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
Attachment #8789248 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.