Open
Bug 1293945
Opened 8 years ago
Updated 2 years ago
The animation will finish when setting small negative playbackRate.
Categories
(Core :: DOM: Animation, defect, P3)
Core
DOM: Animation
Tracking
()
NEW
People
(Reporter: mantaroh, Unassigned)
Details
Attachments
(3 files, 4 obsolete files)
1.19 KB,
patch
|
Details | Diff | Splinter Review | |
58 bytes,
text/x-review-board-request
|
Details | |
10.87 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•8 years ago
|
Component: CSS Parsing and Computation → DOM: Animation
Comment 1•8 years ago
|
||
It seems that the playback rate in the test case is positive.
Reporter | ||
Comment 2•8 years ago
|
||
Hi hiro, Sorry I attached the wrong file. The example page is as follow: http://people.mozilla.org/~mantaroh/bug1293945/test-case.html
Reporter | ||
Updated•8 years ago
|
Attachment #8779649 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Summary: The animation will finish when setting negative small playbackRate. → The animation will finish when setting small negative playbackRate.
Updated•8 years ago
|
Priority: -- → P3
Reporter | ||
Comment 3•8 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)
Reporter | ||
Comment 4•8 years ago
|
||
This patch is an example for comment #3.
Comment 5•8 years ago
|
||
(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 6•8 years ago
|
||
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
Updated•8 years ago
|
Flags: needinfo?(bbirtles)
Reporter | ||
Comment 7•8 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)
Comment 8•8 years ago
|
||
(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•8 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)
Reporter | ||
Comment 11•8 years ago
|
||
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
Reporter | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7af1380bb6d
Attachment #8788353 -
Attachment is obsolete: true
Reporter | ||
Comment 13•8 years ago
|
||
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
Comment 14•2 years ago
|
||
The bug assignee is inactive on Bugzilla, so the assignee is being reset.
Assignee: mantaroh → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•