1223255 - Compositor animations with positive delay and playbackRate != 1 jump

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case

Animations on compositor which has positive delay and is set playbackRate jumps to unusual position after it started.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I found the reason of this problem.

For compositor animations elapsed duration is calculated at [1].
 (aPoint - animation.startTime()).MultDouble(animation.playbackRate())

The animation.startTime() comes from [2].
 timeline->ToTimeStamp(startTime.Value() + timing.mDelay)

We need here to divide the mDelay by playback rate just like Animation::AnimationTimeToTimeStamp does[3]. Or use AnimationTimeToTimeStamp here too.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#561
[2] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#380
[3] https://dxr.mozilla.org/mozilla-central/source/dom/animation/Animation.cpp#1162

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Use Animation::AnimationTimeToTimeStamp to calculate timestamp for the compositor

Use Animation::AnimationTimeToTimeStamp instead of timeline->ToTimeStamp otherwise delay value is multiplied by playbackRate twice.

Comment 3

[Brian Birtles (:birtles)]

Comment on attachment 8688244 [details] [diff] [review]
Use Animation::AnimationTimeToTimeStamp to calculate timestamp for the compositor

>--- a/dom/animation/Animation.h
>+++ b/dom/animation/Animation.h
>@@ -294,16 +294,18 @@ public:
>   void ComposeStyle(RefPtr<AnimValuesStyleRule>& aStyleRule,
>                     nsCSSPropertySet& aSetProperties,
>                     bool& aStyleChanging);
> 
>   void NotifyEffectTimingUpdated();
> 
>   AnimationCollection* GetCollection() const;
> 
>+  TimeStamp AnimationTimeToTimeStamp(const StickyTimeDuration& aTime) const;
>+

* Nit: This might make more sense just after GetCurrentOrPendingStartTime since we seems to have most of the time-related methods first, then the effect-related methods.

* Please add a comment here:

"Converts a time in the timescale of this Animation's currentTime, to a TimeStamp. Returns a null TimeStamp if the conversion cannot be performed because of the current state of this Animation (e.g. it has no timeline, a zero playbackRate, an unresolved start time etc.) or the value of the time passed-in (e.g. an infinite time)."

(We should probably expose GetAnimationTimeAsTimelineTime instead (or as well), and then convert to a TimeStamp at the call-site, but I think this is fine for now.)

* Perhaps we should move the order of AnimationTimeToTimeStamp within the .cpp file to match the .h file order? What do you think?

* The following comment in AnimationTimeToTimeStamp is no longer true:

  // Since we never compare the result of this method with TimeStamp::Now()
  // it is ok to return values even if mTimeline->TracksWallclockTime() is
  // false.
  https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/dom/animation/Animation.cpp#1144

  We should just drop that whole paragraph.

>+addAsyncAnimTest(function *() {
>+  var [ div, cs ] = new_div("animation: anim 10s 1s");
>+  var animation = div.getAnimations()[0];
>+  animation.playbackRate = 0.5;
>+
>+  advance_clock(2000); // 1s * (1 / playbackRate)
>+
>+  yield waitForPaints();
>+  omta_is(div, "transform", { tx: 0 }, RunningOn.Compositor,
>+          "animation with positive delay and playbackRate > 1 should " +
>+          "start from the initial position at the beginning of the " +
>+          "active duration");
>+  done_div();
>+});

Does this test fail without the changes to nsDisplayList.cpp?

r=birtles assuming the tests fail without the changes applied.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #3)
> Comment on attachment 8688244 [details] [diff] [review]
> Use Animation::AnimationTimeToTimeStamp to calculate timestamp for the
> compositor
> 
> >--- a/dom/animation/Animation.h
> >+++ b/dom/animation/Animation.h
> >@@ -294,16 +294,18 @@ public:
> >   void ComposeStyle(RefPtr<AnimValuesStyleRule>& aStyleRule,
> >                     nsCSSPropertySet& aSetProperties,
> >                     bool& aStyleChanging);
> > 
> >   void NotifyEffectTimingUpdated();
> > 
> >   AnimationCollection* GetCollection() const;
> > 
> >+  TimeStamp AnimationTimeToTimeStamp(const StickyTimeDuration& aTime) const;
> >+
> 
> * Nit: This might make more sense just after GetCurrentOrPendingStartTime
> since we seems to have most of the time-related methods first, then the
> effect-related methods.

OK. I will do it.

> * Please add a comment here:
> 
> "Converts a time in the timescale of this Animation's currentTime, to a
> TimeStamp. Returns a null TimeStamp if the conversion cannot be performed
> because of the current state of this Animation (e.g. it has no timeline, a
> zero playbackRate, an unresolved start time etc.) or the value of the time
> passed-in (e.g. an infinite time)."

Thanks always for comments. I will try my best.

> (We should probably expose GetAnimationTimeAsTimelineTime instead (or as
> well), and then convert to a TimeStamp at the call-site, but I think this is
> fine for now.)

Ah, that will make much sense!

> * Perhaps we should move the order of AnimationTimeToTimeStamp within the
> .cpp file to match the .h file order? What do you think?

It sounds good. I will change the order.

> * The following comment in AnimationTimeToTimeStamp is no longer true:
> 
>   // Since we never compare the result of this method with TimeStamp::Now()
>   // it is ok to return values even if mTimeline->TracksWallclockTime() is
>   // false.
>  
> https://dxr.mozilla.org/mozilla-central/rev/
> a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/dom/animation/Animation.cpp#1144
> 
>   We should just drop that whole paragraph.

Thanks. I did not notice the comment. I will remove the paragraph.

> >+addAsyncAnimTest(function *() {
> >+  var [ div, cs ] = new_div("animation: anim 10s 1s");
> >+  var animation = div.getAnimations()[0];
> >+  animation.playbackRate = 0.5;
> >+
> >+  advance_clock(2000); // 1s * (1 / playbackRate)
> >+
> >+  yield waitForPaints();
> >+  omta_is(div, "transform", { tx: 0 }, RunningOn.Compositor,
> >+          "animation with positive delay and playbackRate > 1 should " +
> >+          "start from the initial position at the beginning of the " +
> >+          "active duration");
> >+  done_div();
> >+});
> 
> Does this test fail without the changes to nsDisplayList.cpp?

Yes, sure. Without changes the animation lives still on main-thread (does not start) at the beginning.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Use Animation::AnimationTimeToTimeStamp to calculate timestamp for the compositor v2

Addressed comment #3.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

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

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/272af1e271a4

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/272af1e271a4