Based on bug 1158928 (particularly comments 19~22) we sometimes encounter a situation where we fire a new transition while an existing transition is already running on the compositor. If the compositor is running ahead of the content thread/process and we make the new transition start from the current animated values on the content thread/process we'll see a jump in the animation. When we create a transition, we should annotate the segment to indicate that the initial value is based on the current animated value so the compositor can overwrite it with the animated value it has calculated. I'll try to make up a test case to demonstrate the problem next week.

(In reply to Brian Birtles (:birtles) from comment #0) > already running on the compositor. If the compositor is running ahead of the > content thread/process and we make the new transition start from the current > animated values on the content thread/process we'll see a jump in the > animation. It shouldn't be significantly ahead, since the entire reason for the animation-only style flush (i.e., miniflush) was to make the main thread up-to-date before processing such style changes.

(In reply to David Baron [:dbaron] ⏰UTC-4 from comment #1) > It shouldn't be significantly ahead, since the entire reason for the > animation-only style flush (i.e., miniflush) was to make the main thread > up-to-date before processing such style changes. If anything holds up the main thread, it can get significantly ahead as observed in bug 1158928. The test case also demonstrates this by simulating some expensive javascript running on the main thread. The trouble is that even if the main thread updates before processing style changes, it will update animations using the latest refresh driver time which will be the time before running script such as requestAnimationFrame callbacks. Also, even if we update the animation style using TimeStamp::Now(), for example, we'll still see the same effect when we have an expensive paint or texture upload so ideally we should calculate the starting position on the compositor. (That could mean that the intermediate values reported on the main thread momentarily differ from those on the compositor but that's always the case to some degree anyway and they will converge on the final value.)

[Blocking Requested - why for this release]: This is affecting Gaia apps including both Music (OGA) and Music NGA. The transition when showing/hiding the star-rating bar on the "Player" view is completely broken and appears to be doing the exact same thing that the attached test case in this bug is doing.

(In reply to Justin D'Arcangelo [:justindarc] from comment #4) > This is affecting Gaia apps including both Music (OGA) and Music NGA. The > transition when showing/hiding the star-rating bar on the "Player" view is > completely broken and appears to be doing the exact same thing that the > attached test case in this bug is doing. Do you have a pointer to the relevant bits of code for this? I'd like to be sure this is the same problem before prioritizing this.

(In reply to Brian Birtles (:birtles) from comment #5) > (In reply to Justin D'Arcangelo [:justindarc] from comment #4) > > This is affecting Gaia apps including both Music (OGA) and Music NGA. The > > transition when showing/hiding the star-rating bar on the "Player" view is > > completely broken and appears to be doing the exact same thing that the > > attached test case in this bug is doing. > > Do you have a pointer to the relevant bits of code for this? > > I'd like to be sure this is the same problem before prioritizing this. This is in a web component (custom element) in the Music NGA app: https://github.com/mozilla-b2g/gaia/blob/329f67f0964a7d6f2137fcb7adc61a7d831a9a03/dev_apps/music-nga/elements/music-artwork.js#L74 Tapping in the element toggles the '.show-overlay' class which changes the `transform` CSS property on two elements. Each time the transition starts, it is in the wrong place. This was not an issue until sometime last week.

(In reply to Justin D'Arcangelo [:justindarc] from comment #6) > (In reply to Brian Birtles (:birtles) from comment #5) > > (In reply to Justin D'Arcangelo [:justindarc] from comment #4) > > > This is affecting Gaia apps including both Music (OGA) and Music NGA. The > > > transition when showing/hiding the star-rating bar on the "Player" view is > > > completely broken and appears to be doing the exact same thing that the > > > attached test case in this bug is doing. > > > > Do you have a pointer to the relevant bits of code for this? > > > > I'd like to be sure this is the same problem before prioritizing this. > > This is in a web component (custom element) in the Music NGA app: > > https://github.com/mozilla-b2g/gaia/blob/ > 329f67f0964a7d6f2137fcb7adc61a7d831a9a03/dev_apps/music-nga/elements/music- > artwork.js#L74 > > Tapping in the element toggles the '.show-overlay' class which changes the > `transform` CSS property on two elements. Each time the transition starts, > it is in the wrong place. This was not an issue until sometime last week. If it only started last week, it's not likely to be this bug. This bug has been around since we introduced compositor animation which has been in Firefox OS since 1.0 I believe. It hasn't been in desktop until recently however. Also, this bug is only about starting a transition which interrupts another running transition. It doesn't concern starting transitions from rest. Does that still sound like what you're seeing?

Sorry, my bad. I thought this sounded like the same problem. What I'm seeing is problems starting from rest.

I'm in two minds about this bug. I think it could make a significant difference to perceived performance by eliminating jumps when triggering animations and Hiro tells me that Chrome seems to do this for transitions (but not animations). However, I'm a bit concerned that for a case where transitions are continually interrupted we could end up with significant drift between main thread and compositor thread. One approach we could try is to actually update the "from" value on the main thread. Although I mentioned in comment 3 that using TimeStamp::Now() to calculate the from value wouldn't fix the problem if painting was expensive, I think it probably shouldn't be expensive since we should have layerized the contents already--in fact in most cases there shouldn't be any painting to do if we are just interrupting an existing transition on the same compositor-animatable property. Then we'd be able to ensure that the main thread and compositor thread are using the same animation endpoints.

Thanks Brian. But I am now suspecting this issue is caused by another factor since this issue is solved if we use getComputedStyle(div) instead of getComputedStyle(div).transform. More interestingly, if we call getComputedStyle(div).transform after calling getComputedStyle(div), the problem still persists. getComputedStyle(div).transform breaks something? Or I am missing something.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #10) > Thanks Brian. But I am now suspecting this issue is caused by another > factor since this issue is solved if we use getComputedStyle(div) instead of > getComputedStyle(div).transform. More interestingly, if we call > getComputedStyle(div).transform after calling getComputedStyle(div), the > problem still persists. getComputedStyle(div).transform breaks something? > Or I am missing something. No, it's just that getComputedStyle(div) doesn't flush the transform style so we don't start the transform until the next tick (when there's no longer a noticeable lag present). We actually need to call getComputedStyle(div).transform in order to trigger the transition within the same frame.

(In reply to Brian Birtles (:birtles) from comment #11) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #10) > > Thanks Brian. But I am now suspecting this issue is caused by another > > factor since this issue is solved if we use getComputedStyle(div) instead of > > getComputedStyle(div).transform. More interestingly, if we call > > getComputedStyle(div).transform after calling getComputedStyle(div), the > > problem still persists. getComputedStyle(div).transform breaks something? > > Or I am missing something. > > No, it's just that getComputedStyle(div) doesn't flush the transform style > so we don't start the transform until the next tick (when there's no longer > a noticeable lag present). We actually need to call > getComputedStyle(div).transform in order to trigger the transition within > the same frame. Ah, thanks for the correction. Then I did a same mistake when I checked the behavior on Chrome. Chrome does not trigger the new transition at all when the second calling of getComputedStyle(div).transform.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12) > (In reply to Brian Birtles (:birtles) from comment #11) > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #10) > > > Thanks Brian. But I am now suspecting this issue is caused by another > > > factor since this issue is solved if we use getComputedStyle(div) instead of > > > getComputedStyle(div).transform. More interestingly, if we call > > > getComputedStyle(div).transform after calling getComputedStyle(div), the > > > problem still persists. getComputedStyle(div).transform breaks something? > > > Or I am missing something. > > > > No, it's just that getComputedStyle(div) doesn't flush the transform style > > so we don't start the transform until the next tick (when there's no longer > > a noticeable lag present). We actually need to call > > getComputedStyle(div).transform in order to trigger the transition within > > the same frame. > > Ah, thanks for the correction. Then I did a same mistake when I checked the > behavior on Chrome. Chrome does not trigger the new transition at all when > the second calling of getComputedStyle(div).transform. Oops. I was wrong. Chrome does trigger the new transition actually.. Sorry for the confusion.

Pass TimeStamp::Now() to ComposeStyle() and use it to resolve style if it gets called from EffectCompositor::AddStyleUpdatesTo().

I was wondering if we could limit this to nsTransitionManager::ConsiderStartingTransition so that we just calculate the starting point by calling Interpolate() directly (with the 'progress' value from the existing transition after passing TimeStamp::Now() for the time) when haveCurrentTransition is true. Would that work? As opposed to doing this more generally in EffectCompositor::AddStyleUpdatesTo()?

(In reply to Brian Birtles (:birtles) from comment #15) > I was wondering if we could limit this to > nsTransitionManager::ConsiderStartingTransition so that we just calculate > the starting point by calling Interpolate() directly (with the 'progress' > value from the existing transition after passing TimeStamp::Now() for the > time) when haveCurrentTransition is true. > > Would that work? As opposed to doing this more generally in > EffectCompositor::AddStyleUpdatesTo()? Sure, that will work well. I was again thinking about CSS animations triggered by mouse events or something. But yes, I can punt the case later.

We need Animation::GetCurrentTime at TimeStamp::Now() anyway. Brian, is this what you have in mind in comment 15?

Comment on attachment 8754100 [details] [diff] [review] A patch implements the approach in comment 15 Review of attachment 8754100 [details] [diff] [review]: ----------------------------------------------------------------- This seems fine to me. Although I've just realized a disadvantage of applying this tweak when setting up the transition. If we have two requestAnimationFrame callbacks, and we set up the transition in the first callback, but the *second* callback takes a long time, I think we'll still have the same problem. Is that right? I suppose solving this more generally could be quite difficult. ::: dom/animation/Animation.cpp @@ +209,2 @@ > Nullable<TimeDuration> > +Animation::GetCurrentTime(GetTimelineTimeFunc aGetTimelineTimeFunc) const This seems more complicated than it needs to be. It seems like we could just pass in a Nullable<TimeDuration>? ::: layout/style/nsTransitionManager.cpp @@ +573,5 @@ > + // calculate plausible value from TimeStamp::Now() because transtions on > + // the compositor sometimes go further ahead of the main-thread. > + if (haveCurrentTransition && > + (aProperty == eCSSProperty_opacity || > + aProperty == eCSSProperty_transform)) { (Presumably when we go to implement this properly we could actually look up the current transition and see if it is running on the compositor.) @@ +613,5 @@ > + // Check that we can interpolate between these values > + // (If this is ever a performance problem, we could add a > + // CanInterpolate method, but it seems fine for now.) > + StyleAnimationValue::Interpolate(aProperty, startValue, endValue, > + 0.5, dummyValue); (I suspect we won't need this check in the case where we've successfully done an interpolation above but maybe it doesn't matter.)

(In reply to Brian Birtles (:birtles) from comment #18) > Comment on attachment 8754100 [details] [diff] [review] > A patch implements the approach in comment 15 > > Review of attachment 8754100 [details] [diff] [review]: > ----------------------------------------------------------------- > > This seems fine to me. Although I've just realized a disadvantage of > applying this tweak when setting up the transition. > > If we have two requestAnimationFrame callbacks, and we set up the transition > in the first callback, but the *second* callback takes a long time, I think > we'll still have the same problem. Is that right? I suppose solving this > more generally could be quite difficult. Ah, yes, that's right. To solve this generally we have no choice but to calculate the last transform position on the compositor. I was halfway to do the calculation on the compositor. What the rest work there was to report the state that the position of new transition has been calculated on the compositor back to the counter-part of the transition on the main-thread. This rest work is the most important part of the approach, because if we don't do so, the calculation will be done on every transaction. I will try it someday. > ::: dom/animation/Animation.cpp > @@ +209,2 @@ > > Nullable<TimeDuration> > > +Animation::GetCurrentTime(GetTimelineTimeFunc aGetTimelineTimeFunc) const > > This seems more complicated than it needs to be. It seems like we could just > pass in a Nullable<TimeDuration>? Sure. > ::: layout/style/nsTransitionManager.cpp > @@ +573,5 @@ > > + // calculate plausible value from TimeStamp::Now() because transtions on > > + // the compositor sometimes go further ahead of the main-thread. > > + if (haveCurrentTransition && > > + (aProperty == eCSSProperty_opacity || > > + aProperty == eCSSProperty_transform)) { > > (Presumably when we go to implement this properly we could actually look up > the current transition and see if it is running on the compositor.) Will do. > @@ +613,5 @@ > > + // Check that we can interpolate between these values > > + // (If this is ever a performance problem, we could add a > > + // CanInterpolate method, but it seems fine for now.) > > + StyleAnimationValue::Interpolate(aProperty, startValue, endValue, > > + 0.5, dummyValue); > > (I suspect we won't need this check in the case where we've successfully > done an interpolation above but maybe it doesn't matter.) Maybe, but I guess it will get more complicated. I will leave it as it is for now.

I've written a reftest for this bug, but it fails intermittently. I am guessing it's a reftest bug. Sometimes reftest does not take a snapshot after removing "reftest-wait" class. An artificial busy frame might have affected it. I will upload a patch without test for now.

Transitions on the compositor sometimes go further ahead while the main-thread is busy. When the transition on the compositor is replaced by a new one, until now we calculate the current position of the old one with the most recent refresh time. But if the replace is done on a busy frame, the calculated position will be far from the real position on the compositor. As a result, we can see jumping transitions after busy frames. To mitigate this issue, we should calculate a plausible current position of the old one with the current time. Review commit: https://reviewboard.mozilla.org/r/53818/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53818/

I think this fix is mostly good, but I also think it's a bit unfortunate to introduce a fix that only works sometimes -- i.e. whether or not you get a smooth transition start depends on the order in which requestAnimationFrame callbacks are registered. I'm wonder if we can do a more general fix. I think we probably could do something entirely on the main thread. Thinking aloud: * We could set up the transition as normal * If we are interrupting a transition running on the compositor, store, somewhere (on the ElementPropertyTransition object? And then discard when we've finished with it?) the specified timing of the running transition and the two StyleAnimationValue objects that represent the endpoints of the new transition * At some later point after running all animation callbacks (but supposedly just before painting) replace the transition start value by reevaluating the computed timing using TimeStamp::Now() and the store specified timing, and re-interpolating the interval. The "At some later point..." is the hard part. This could be quite messy if there's no convenient place to do that. Do you think that's possible? Or do you think it's worth fixing this even if it only works sometimes?

(In reply to Brian Birtles (:birtles) from comment #22) > * If we are interrupting a transition running on the compositor, store, > somewhere (on the ElementPropertyTransition object? And then discard when > we've finished with it?) the specified timing of the running transition and > the two StyleAnimationValue objects that represent the endpoints of the new > transition Err, that should be the old transition, I think.

(In reply to Brian Birtles (:birtles) from comment #22) > I think this fix is mostly good, but I also think it's a bit unfortunate to > introduce a fix that only works sometimes -- i.e. whether or not you get a > smooth transition start depends on the order in which requestAnimationFrame > callbacks are registered. I'm wonder if we can do a more general fix. > > I think we probably could do something entirely on the main thread. Thinking > aloud: > > * We could set up the transition as normal > * If we are interrupting a transition running on the compositor, store, > somewhere (on the ElementPropertyTransition object? And then discard when > we've finished with it?) the specified timing of the running transition and > the two StyleAnimationValue objects that represent the endpoints of the new > transition > * At some later point after running all animation callbacks (but supposedly > just before painting) replace the transition start value by reevaluating the > computed timing using TimeStamp::Now() and the store specified timing, and > re-interpolating the interval. > > The "At some later point..." is the hard part. This could be quite messy if > there's no convenient place to do that. Yeah, it really sounds an interesting idea. A place to inject such process is hopefully AddAnimationsAndTransitionsToLayer(). Anyway it's worth trying.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #24) > Yeah, it really sounds an interesting idea. A place to inject such process > is hopefully AddAnimationsAndTransitionsToLayer(). Anyway it's worth trying. I hope that works. I wonder if it might be too late. If we've already resolved style for the current frame on the main thread before updating the transition 'from' value, is it possible we might see a frame with the wrong/delayed transition start point used. Maybe not. I'm not really familiar with how that interaction works.

Comment on attachment 8754199 [details] MozReview Request: Bug 1167519 - Part 1: Calculate plausible starting value on compositor with TimeStamp::Now() when replacing an old transtion. r?birtles Clearing review request since it seems like we might try another approach that would cover all cases of this bug.

I tried the calculation in AddAnimationForProperty, it needs mStartTime and mTimingFunction additionally, but it is not a big problem. Yes, it worked well on E10S, but does not work on non-E10S.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #27) > I tried the calculation in AddAnimationForProperty, it needs mStartTime and > mTimingFunction additionally, but it is not a big problem. Yes, it worked > well on E10S, but does not work on non-E10S. It seems a X11-forwarding bug. Yes, it works well with non-E10S on pure X-Window.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #20) > Created attachment 8754198 [details] [diff] [review] > A reftest causes intermittent failure > > I've written a reftest for this bug, but it fails intermittently. I am > guessing it's a reftest bug. Sometimes reftest does not take a snapshot > after removing "reftest-wait" class. An artificial busy frame might have > affected it. I will upload a patch without test for now. I should note about the reftest issue. It's not related to the artificial busy, it's related to compositor animations. I hope bug 1248828 solves the issue.

For the record, I am attaching a modification test case of attachment 8610350 [details]. This test case has two requestAnimationFrame callbacks just Brian mentioned in comment 18.

Comment on attachment 8754199 [details] MozReview Request: Bug 1167519 - Part 1: Calculate plausible starting value on compositor with TimeStamp::Now() when replacing an old transtion. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53818/diff/1-2/

Comment on attachment 8754199 [details] MozReview Request: Bug 1167519 - Part 1: Calculate plausible starting value on compositor with TimeStamp::Now() when replacing an old transtion. r?birtles https://reviewboard.mozilla.org/r/53818/#review50832 This is looking really good! ::: dom/animation/Animation.h:319 (Diff revision 2) > + // Return animation current time based on a given time instead of timeline > + // time with a given start time. This is useful when we estimate the > + // current animated value running on the compositor because the animation > + // on the compositor is often running ahead while main-thread is busy. > + Nullable<TimeDuration> GetCurrentTimeAt( > + const TimeStamp& aBaseTime, const TimeDuration& aStartTime) const; I really like this patch. However, I think this part could be a bit different. This method doesn't take into account the playbackRate so if the author changes the playbackRate and then triggers a new transition I think the animation will jump. Also, this doesn't check that mTimeline != nullptr, that mTimeline->GetCurrentTime != null, that mTimeline->TracksWallclockTime() is true etc. I thought it might be possible to store a pointer to the replaced CSSTransition but that isn't safe since script might run in-between triggering the transition and updating the 'from' value, and that script could change, for example, the playback rate. So maybe we just need to store the playback rate, and check that the replaced transition uses the document timeline as its timeline? So long as we're passing in a different start time and a different playback rate, this feels like it should be a static method? Perhaps defined on CSSTransition? What do you think? ::: layout/base/nsDisplayList.cpp:394 (Diff revision 2) > + // If we are replacing the previous transition is running on the compositor and still > + // active, store the old transition effect and its start time > + // to calculate a plausible value from TimeStamp::Now() later. > + // That's because transtions on the compositor sometimes go further ahead > + // of the main-thread. > + if (aAnimation->AsCSSTransition() && > + aAnimation->GetEffect()->AsTransition()->mOldTransition) { > + aAnimation->GetEffect()->AsTransition()-> > + ReplaceStartValueWithPlausibleValueOnCompositor(); > + } Instead of "old transition", I think "replaced transition" might be more clear. So, perhaps: // If we are starting a new transition that replaces an existing transition // running on the compositor, it is possible that the animation on the // compositor will have advanced ahead of the main thread. If we use as // the starting point of the new transition, the current value of the // replaced transition as calculated on the main thread using the refresh // driver time, the new transition will jump when it starts. Instead, we // re-calculate the starting point of the new transition by applying the // current TimeStamp to the parameters of the replaced transition. // // We need to do this here, rather than when we generate the new transition, // since after generating the new transition other requestAnimationFrame // callbacks may run that introduce further lag between the main thread and // the compositor. ::: layout/base/nsDisplayList.cpp:399 (Diff revision 2) > + if (aAnimation->AsCSSTransition() && > + aAnimation->GetEffect()->AsTransition()->mOldTransition) { In future, it will not be guaranteed that because aAnimation->AsCSSTransition() != nullptr that aAnimation->GetEffect()->AsTransition() != nullptr. It won't even be guaranteed that GetEffect() != nullptr. Instead, we could do something like: if (aAnimation->AsCSSTransition()) { ElementPropertyTransition* transition = aAnimation->GetEffect() ? aAnimation->GetEffect()->AsTransition() : nullptr; if (transition) { ... } } Also, see my comments later--I think we possibly don't need to check for mOldTransition--we can just make the function we call check for it and return early if it is not set. ::: layout/style/nsTransitionManager.h:99 (Diff revision 2) > + // Replace the start value with a plausible value on the compositor. > + // This function should be called only when we replace a transition which is > + // running on the compositor with a new transition. > + void ReplaceStartValueWithPlausibleValueOnCompositor(); Perhaps we could call this UpdateStartValueFromReplacedTransition() ? I'm not sure if we need the sentence that begins, "This function should be called only..." ::: layout/style/nsTransitionManager.h:110 (Diff revision 2) > + TimeDuration mStartTime; > + TimingParams mTiming; > + Maybe<ComputedTimingFunction> mTimingFunction; > + StyleAnimationValue mFromValue, mToValue; > + }; > + Maybe<OldTransitionProperties> mOldTransition; I think mReplacedTransition might be more clear here. ::: layout/style/nsTransitionManager.cpp:76 (Diff revision 2) > + MOZ_ASSERT(mOldTransition, > + "We should have a valid old transition properties"); Could we just make this return early if mReplacedTransition is empty? ::: layout/style/nsTransitionManager.cpp:78 (Diff revision 2) > + MOZ_ASSERT(mProperties[0].mProperty == eCSSProperty_opacity || > + mProperties[0].mProperty == eCSSProperty_transform, > + "The transition property should be opacty or transform"); We should use nsCSSProps::PropHasFlags(TransitionProperty(), CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR) ::: layout/style/nsTransitionManager.cpp:83 (Diff revision 2) > + // Ideally this should be called against the old animation, > + // but it doesn't matter since the animation must live in the same > + // document as the old one does. I think if we check the timeline is the document timeline before filling in mReplacedTransition, and we use a static method for this, then we won't need this comment. ::: layout/style/nsTransitionManager.cpp:782 (Diff revision 2) > + // If the previous transition is running on the compositor and still > + // active, store the old transition effect and its start time > + // to calculate a plausible value from TimeStamp::Now() later. > + // That's because transtions on the compositor sometimes go further ahead > + // of the main-thread. // If this new transition is replacing an existing transition that is running // on the compositor, we store select parameters from the replaced transition // so that later, once all scripts have run, we can update the start value // of the transition using TimeStamp::Now(). This allows us to avoid a // large jump when starting a new transition when the main thread lags behind // the compositor. ::: layout/style/nsTransitionManager.cpp:787 (Diff revision 2) > + if (oldPT->IsCurrent() && > + oldPT->IsRunningOnCompositor()) { I think we'll also need to check that the timeline hasn't been changed. ::: layout/style/nsTransitionManager.cpp:789 (Diff revision 2) > + // to calculate a plausible value from TimeStamp::Now() later. > + // That's because transtions on the compositor sometimes go further ahead > + // of the main-thread. > + if (oldPT->IsCurrent() && > + oldPT->IsRunningOnCompositor()) { > + // Hold the old effect. Do we need this comment?

(In reply to Brian Birtles (:birtles) from comment #32) > Comment on attachment 8754199 [details] > MozReview Request: Bug 1167519 - Calculate plausible starting value on > compositor with TimeStamp::Now() when replacing an old transtion. r?birtles > > https://reviewboard.mozilla.org/r/53818/#review50832 > > This is looking really good! > > ::: dom/animation/Animation.h:319 > (Diff revision 2) > > + // Return animation current time based on a given time instead of timeline > > + // time with a given start time. This is useful when we estimate the > > + // current animated value running on the compositor because the animation > > + // on the compositor is often running ahead while main-thread is busy. > > + Nullable<TimeDuration> GetCurrentTimeAt( > > + const TimeStamp& aBaseTime, const TimeDuration& aStartTime) const; > > I really like this patch. However, I think this part could be a bit > different. > > This method doesn't take into account the playbackRate so if the author > changes the playbackRate and then triggers a new transition I think the > animation will jump. > > Also, this doesn't check that mTimeline != nullptr, that > mTimeline->GetCurrentTime != null, that mTimeline->TracksWallclockTime() is > true etc. > > I thought it might be possible to store a pointer to the replaced > CSSTransition but that isn't safe since script might run in-between > triggering the transition and updating the 'from' value, and that script > could change, for example, the playback rate. > > So maybe we just need to store the playback rate, and check that the > replaced transition uses the document timeline as its timeline? > > So long as we're passing in a different start time and a different playback > rate, this feels like it should be a static method? Yes, actually I made the function a static and stored the CSSTransition yesterday, but it resulted another unwanted jump. It might be caused because I did not update mProperties at that time (just passed the update value to the compositor). Anyway I will revise it. > CSSTransition? What do you think? Good idea! I've never thought it. Thanks! > ::: layout/base/nsDisplayList.cpp:394 > (Diff revision 2) > > + // If we are replacing the previous transition is running on the compositor and still > > + // active, store the old transition effect and its start time > > + // to calculate a plausible value from TimeStamp::Now() later. > > + // That's because transtions on the compositor sometimes go further ahead > > + // of the main-thread. > > + if (aAnimation->AsCSSTransition() && > > + aAnimation->GetEffect()->AsTransition()->mOldTransition) { > > + aAnimation->GetEffect()->AsTransition()-> > > + ReplaceStartValueWithPlausibleValueOnCompositor(); > > + } > > Instead of "old transition", I think "replaced transition" might be more > clear. Nice! > So, perhaps: > > // If we are starting a new transition that replaces an existing transition > // running on the compositor, it is possible that the animation on the > // compositor will have advanced ahead of the main thread. If we use as > // the starting point of the new transition, the current value of the > // replaced transition as calculated on the main thread using the refresh > // driver time, the new transition will jump when it starts. Instead, we > // re-calculate the starting point of the new transition by applying the > // current TimeStamp to the parameters of the replaced transition. > // > // We need to do this here, rather than when we generate the new > transition, > // since after generating the new transition other requestAnimationFrame > // callbacks may run that introduce further lag between the main thread and > // the compositor. > > ::: layout/base/nsDisplayList.cpp:399 > (Diff revision 2) > > + if (aAnimation->AsCSSTransition() && > > + aAnimation->GetEffect()->AsTransition()->mOldTransition) { > > In future, it will not be guaranteed that because > aAnimation->AsCSSTransition() != nullptr that > aAnimation->GetEffect()->AsTransition() != nullptr. It won't even be > guaranteed that GetEffect() != nullptr. Ah, exactly. > Also, see my comments later--I think we possibly don't need to check for > mOldTransition--we can just make the function we call check for it and > return early if it is not set. Thanks. I will do it. > ::: layout/style/nsTransitionManager.h:99 > (Diff revision 2) > > + // Replace the start value with a plausible value on the compositor. > > + // This function should be called only when we replace a transition which is > > + // running on the compositor with a new transition. > > + void ReplaceStartValueWithPlausibleValueOnCompositor(); > > Perhaps we could call this UpdateStartValueFromReplacedTransition() ? > > I'm not sure if we need the sentence that begins, "This function should be > called only..." > > ::: layout/style/nsTransitionManager.h:110 > (Diff revision 2) > > + TimeDuration mStartTime; > > + TimingParams mTiming; > > + Maybe<ComputedTimingFunction> mTimingFunction; > > + StyleAnimationValue mFromValue, mToValue; > > + }; > > + Maybe<OldTransitionProperties> mOldTransition; > > I think mReplacedTransition might be more clear here. > > ::: layout/style/nsTransitionManager.cpp:76 > (Diff revision 2) > > + MOZ_ASSERT(mOldTransition, > > + "We should have a valid old transition properties"); > > Could we just make this return early if mReplacedTransition is empty? > > ::: layout/style/nsTransitionManager.cpp:78 > (Diff revision 2) > > + MOZ_ASSERT(mProperties[0].mProperty == eCSSProperty_opacity || > > + mProperties[0].mProperty == eCSSProperty_transform, > > + "The transition property should be opacty or transform"); > > We should use nsCSSProps::PropHasFlags(TransitionProperty(), > CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR) > > ::: layout/style/nsTransitionManager.cpp:83 > (Diff revision 2) > > + // Ideally this should be called against the old animation, > > + // but it doesn't matter since the animation must live in the same > > + // document as the old one does. > > I think if we check the timeline is the document timeline before filling in > mReplacedTransition, and we use a static method for this, then we won't need > this comment. > > ::: layout/style/nsTransitionManager.cpp:782 > (Diff revision 2) > > + // If the previous transition is running on the compositor and still > > + // active, store the old transition effect and its start time > > + // to calculate a plausible value from TimeStamp::Now() later. > > + // That's because transtions on the compositor sometimes go further ahead > > + // of the main-thread. > > // If this new transition is replacing an existing transition that is running > // on the compositor, we store select parameters from the replaced > transition > // so that later, once all scripts have run, we can update the start value > // of the transition using TimeStamp::Now(). This allows us to avoid a > // large jump when starting a new transition when the main thread lags > behind > // the compositor. > > ::: layout/style/nsTransitionManager.cpp:787 > (Diff revision 2) > > + if (oldPT->IsCurrent() && > > + oldPT->IsRunningOnCompositor()) { > > I think we'll also need to check that the timeline hasn't been changed. Though I don't follow up that timeline (bug 1178662?), haveCurentTransition will not check timeline changing? > ::: layout/style/nsTransitionManager.cpp:789 > (Diff revision 2) > > + // to calculate a plausible value from TimeStamp::Now() later. > > + // That's because transtions on the compositor sometimes go further ahead > > + // of the main-thread. > > + if (oldPT->IsCurrent() && > > + oldPT->IsRunningOnCompositor()) { > > + // Hold the old effect. > > Do we need this comment? Thanks, this was copied from other place.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #33) > > ::: layout/style/nsTransitionManager.cpp:787 > > (Diff revision 2) > > > + if (oldPT->IsCurrent() && > > > + oldPT->IsRunningOnCompositor()) { > > > > I think we'll also need to check that the timeline hasn't been changed. > > Though I don't follow up that timeline (bug 1178662?), haveCurentTransition > will not check timeline changing? Oh, I don't know. Will it matter? Why would it need to?

(In reply to Brian Birtles (:birtles) from comment #34) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #33) > > > ::: layout/style/nsTransitionManager.cpp:787 > > > (Diff revision 2) > > > > + if (oldPT->IsCurrent() && > > > > + oldPT->IsRunningOnCompositor()) { > > > > > > I think we'll also need to check that the timeline hasn't been changed. > > > > Though I don't follow up that timeline (bug 1178662?), haveCurentTransition > > will not check timeline changing? > > Oh, I don't know. Will it matter? Why would it need to? I don't know either. ;-) I've never thought what happens on changing timeline.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=367e628c0e29&selectedJob=21162755 A try told us there is a case that mStartTime of the old transition is null. I just confirmed it locally that it's in playpending state. I will add a null check for mStartTime before setting mReplacedTransition values.

Comment on attachment 8754199 [details] MozReview Request: Bug 1167519 - Part 1: Calculate plausible starting value on compositor with TimeStamp::Now() when replacing an old transtion. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53818/diff/2-3/

https://reviewboard.mozilla.org/r/53818/#review50832 > I think we'll also need to check that the timeline hasn't been changed. mStartTime check is also added here.

Comment on attachment 8754199 [details] MozReview Request: Bug 1167519 - Part 1: Calculate plausible starting value on compositor with TimeStamp::Now() when replacing an old transtion. r?birtles https://reviewboard.mozilla.org/r/53818/#review50870 Thank you for doing this! This looks good. The main changes I think we need are: * Using the document timeline when calculating the time for the replaced transition * Checking the result of Interpolate ::: layout/base/nsDisplayList.cpp:409 (Diff revision 3) > + MOZ_ASSERT(aAnimation->GetEffect()->AsTransition(), > + "CSSTransition' effect should be an ElementPropertyTransition"); This won't be true in future but I guess we can fix that in bug 1049975. Would you mind adding a comment referring to bug 1049975 so that when we implement that we know to change this? ::: layout/style/nsTransitionManager.h:99 (Diff revision 3) > // Compute the portion of the *value* space that we should be through > // at the current time. (The input to the transition timing function > // has time units, the output has value units.) > double CurrentValuePortion() const; > + > + // Replace the start value with a plausible value on the compositor. I don't think 'plausible' is quite the right word here. Perhaps just something like: // For a new transition interrupting an existing transition on the // compositor, update the start value to match the value of the replaced // transitions at the current time. ? ::: layout/style/nsTransitionManager.h:199 (Diff revision 3) > } > // True for transitions that are generated from CSS markup and continue to > // reflect changes to that markup. > bool IsTiedToMarkup() const { return mOwningElement.IsSet(); } > > + // Return animation current time based on a given time, a given start time s/Return animation current time/Return the animation current time/ ? s/a given time,/a given TimeStamp,/ ? ::: layout/style/nsTransitionManager.h:199 (Diff revision 3) > + // Return animation current time based on a given time, a given start time > + // and a given playbackRate on a given timeline. This is useful when we 'Return the animation current time based on a given TimeStamp, start time, playbackRate and timeline.' ::: layout/style/nsTransitionManager.h:202 (Diff revision 3) > bool IsTiedToMarkup() const { return mOwningElement.IsSet(); } > > + // Return animation current time based on a given time, a given start time > + // and a given playbackRate on a given timeline. This is useful when we > + // estimate the current animated value running on the compositor because > + // the animation on the compositor is often running ahead while main-thread Nit: s/'often'/'may be'/ ? ::: layout/style/nsTransitionManager.cpp:85 (Diff revision 3) > + CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR), > + "The transition property should be able to be run on the " > + "compositor"); > + > + ComputedTiming computedTiming = GetComputedTimingAt( > + dom::CSSTransition::GetCurrentTimeAt(GetAnimation()->GetTimeline(), I think it would make more sense to fetch the document timeline here, right? That's what we check for in ConsiderStartingTransition. If script changes the new transition's timeline to something else before we update the start value then I think we will get the wrong result here? (Also, if we use the document timeline, maybe the aTimeline parameter could be a reference instead of a pointer?) ::: layout/style/nsTransitionManager.cpp:97 (Diff revision 3) > + StyleAnimationValue::Interpolate(mProperties[0].mProperty, > + mReplacedTransition->mFromValue, > + mReplacedTransition->mToValue, > + valuePosition, startValue); Can we check the result of Interpolate here? I think it *might* be possible that we could reach here with a value of from/to that is not interpolable. So perhaps we should just skip setting the from value in that case. ::: layout/style/nsTransitionManager.cpp:813 (Diff revision 3) > + const AnimationPropertySegment& segment = > + oldPT->Properties()[0].mSegments[0]; This should be safe because we can't modify a transitions keyframes and still have an ElementPropertyTransition (which is what oldPT is) but I think it would be good to add an assertion that we have at least one property and one segment.

(In reply to Brian Birtles (:birtles) from comment #39) > ::: layout/base/nsDisplayList.cpp:409 > (Diff revision 3) > > + MOZ_ASSERT(aAnimation->GetEffect()->AsTransition(), > > + "CSSTransition' effect should be an ElementPropertyTransition"); > > This won't be true in future but I guess we can fix that in bug 1049975. > Would you mind adding a comment referring to bug 1049975 so that when we > implement that we know to change this? Sure. > ::: layout/style/nsTransitionManager.cpp:85 > (Diff revision 3) > > + CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR), > > + "The transition property should be able to be run on the " > > + "compositor"); > > + > > + ComputedTiming computedTiming = GetComputedTimingAt( > > + dom::CSSTransition::GetCurrentTimeAt(GetAnimation()->GetTimeline(), > > I think it would make more sense to fetch the document timeline here, right? > That's what we check for in ConsiderStartingTransition. > > If script changes the new transition's timeline to something else before we > update the start value then I think we will get the wrong result here? Thanks for the explanation. I'm now getting to understand about changing timeline. If I understand it correctly, we should use the original transition's timeline here when it's possible. That's because the original timeline might be also replaced. So I think we should store the original timeline too, right? If so, I'd like to leave this issue here and fix it later with some test cases after bug 1178662 is fixed.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #29) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #20) > > Created attachment 8754198 [details] [diff] [review] > > A reftest causes intermittent failure > > > > I've written a reftest for this bug, but it fails intermittently. I am > > guessing it's a reftest bug. Sometimes reftest does not take a snapshot > > after removing "reftest-wait" class. An artificial busy frame might have > > affected it. I will upload a patch without test for now. > > I should note about the reftest issue. It's not related to the artificial > busy, it's related to compositor animations. I hope bug 1248828 solves the > issue. Now I realized we can use pause() just before removing "reftest-wait".

(In reply to Hiroyuki Ikezoe (:hiro) from comment #40) > (In reply to Brian Birtles (:birtles) from comment #39) > > ::: layout/base/nsDisplayList.cpp:409 > > (Diff revision 3) > > > + MOZ_ASSERT(aAnimation->GetEffect()->AsTransition(), > > > + "CSSTransition' effect should be an ElementPropertyTransition"); > > > > This won't be true in future but I guess we can fix that in bug 1049975. > > Would you mind adding a comment referring to bug 1049975 so that when we > > implement that we know to change this? > > Sure. > > > ::: layout/style/nsTransitionManager.cpp:85 > > (Diff revision 3) > > > + CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR), > > > + "The transition property should be able to be run on the " > > > + "compositor"); > > > + > > > + ComputedTiming computedTiming = GetComputedTimingAt( > > > + dom::CSSTransition::GetCurrentTimeAt(GetAnimation()->GetTimeline(), > > > > I think it would make more sense to fetch the document timeline here, right? > > That's what we check for in ConsiderStartingTransition. > > > > If script changes the new transition's timeline to something else before we > > update the start value then I think we will get the wrong result here? > > Thanks for the explanation. I'm now getting to understand about changing > timeline. > If I understand it correctly, we should use the original transition's > timeline here when it's possible. That's because the original timeline > might be also replaced. So I think we should store the original timeline > too, right? If so, I'd like to leave this issue here and fix it later with > some test cases after bug 1178662 is fixed. Hmm, after some more thought, I realized we can't provide the test cases until we have customizable timeline.

Changing timeline is really annoying me. We will meet a case that timeline has been changed but not reached to the compositor on busy frame. So now I concur we should use the document timeline for now.

Comment on attachment 8754199 [details] MozReview Request: Bug 1167519 - Part 1: Calculate plausible starting value on compositor with TimeStamp::Now() when replacing an old transtion. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53818/diff/3-4/

https://reviewboard.mozilla.org/r/53818/#review50870 > I think it would make more sense to fetch the document timeline here, right? That's what we check for in ConsiderStartingTransition. > > If script changes the new transition's timeline to something else before we update the start value then I think we will get the wrong result here? > > (Also, if we use the document timeline, maybe the aTimeline parameter could be a reference instead of a pointer?) I did use GetRenderedDocument() to obtain the document here. If it's not suitable here, please leave comments. I will upload a reftest and request a review on next Monday. Thank you, https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ec8c50a325d

Without part 1, a green box is painted nearby start point of the first transition, i.e., 0px. If this bug is properly fixed, the green box will be painted far from the start point of the first transition. So, this test uses 'fuzzy(255,10000) !=' to ensure that there is no intersection area between test image and reference image. Review commit: https://reviewboard.mozilla.org/r/54448/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/54448/

Comment on attachment 8754199 [details] MozReview Request: Bug 1167519 - Part 1: Calculate plausible starting value on compositor with TimeStamp::Now() when replacing an old transtion. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53818/diff/4-5/

(In reply to Hiroyuki Ikezoe (:hiro) from comment #40) > (In reply to Brian Birtles (:birtles) from comment #39) > > ::: layout/style/nsTransitionManager.cpp:85 > > (Diff revision 3) > > > + CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR), > > > + "The transition property should be able to be run on the " > > > + "compositor"); > > > + > > > + ComputedTiming computedTiming = GetComputedTimingAt( > > > + dom::CSSTransition::GetCurrentTimeAt(GetAnimation()->GetTimeline(), > > > > I think it would make more sense to fetch the document timeline here, right? > > That's what we check for in ConsiderStartingTransition. > > > > If script changes the new transition's timeline to something else before we > > update the start value then I think we will get the wrong result here? > > Thanks for the explanation. I'm now getting to understand about changing > timeline. > If I understand it correctly, we should use the original transition's > timeline here when it's possible. That's because the original timeline > might be also replaced. So I think we should store the original timeline > too, right? If so, I'd like to leave this issue here and fix it later with > some test cases after bug 1178662 is fixed. I'm suggesting we only apply this optimization if the transition is running on the default document timeline. Then we can just fetch the default document timeline later when we go to calculate the time. If people start using non-default document timelines and we want to apply this optimization in that scenario too, we can just store the zero time. That saves storing an extra timeline (and all the lifetime management that involves).

https://reviewboard.mozilla.org/r/53818/#review51088 ::: layout/style/nsTransitionManager.cpp:86 (Diff revision 5) > + "The transition property should be able to be run on the " > + "compositor"); > + MOZ_ASSERT(GetRenderedDocument(), > + "We should have a valid document at this moment"); > + > + dom::DocumentTimeline* timeline = GetRenderedDocument()->Timeline(); This should probably be mTarget->mElement->GetOwnerDoc() (with appropriate assertions for mTarget and mElement, I guess) since that's what we use when setting up the transition and what we compare against when storing the replaced transition parameters.

Comment on attachment 8755226 [details] MozReview Request: Bug 1167519 - Part 2: A mochitest to check a new transform position replaced by old transform transition is close to the old one. r?birtles https://reviewboard.mozilla.org/r/54448/#review51090 I think this test assumes the animation runs on the compositor. If that's the case, I guess it will fail if we run on a platform with OMTA turned off? Perhaps we could rewrite this as a mochitest so we can check that OMTA is enabled? I think the test format could be basically the same--so long as we wait until the next animation frame, the from value should have been updated and we should be able to use getComputedStyle, right? ::: layout/reftests/css-transitions/1167519-1.html:20 (Diff revision 1) > + var div = document.querySelector("div"); > + // Start first transition > + div.style.transform = "translateX(300px)"; > + getComputedStyle(div); > + > + setTimeout(function() { ::: layout/reftests/css-transitions/1167519-1.html:27 (Diff revision 1) > + // Tie up main thread for 1s > + var startTime = performance.now(); > + while (performance.now() - startTime < 1000);

(In reply to Brian Birtles (:birtles) from comment #50) > Comment on attachment 8755226 [details] > MozReview Request: Bug 1167519 - Part 2. A reftest checks a new transform > position replaced by old transform transition is close to the old one. > r?birtles > > https://reviewboard.mozilla.org/r/54448/#review51090 > > I think this test assumes the animation runs on the compositor. If that's > the case, I guess it will fail if we run on a platform with OMTA turned off? > > Perhaps we could rewrite this as a mochitest so we can check that OMTA is > enabled? I think the test format could be basically the same--so long as we > wait until the next animation frame, the from value should have been updated > and we should be able to use getComputedStyle, right? Thanks. Actually the test was fragile on Android (because of short duration?). https://treeherder.mozilla.org/#/jobs?repo=try&revision=5543e367997f&selectedJob=21254575 Now a try with rewriting it as a mochitest is running: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5fddd6eb204 Once the result is confirmed, I will request a review.

The try failed. https://treeherder.mozilla.org/logviewer.html#?job_id=21265349&repo=try#L1991 I guess we need to wait for a paint after calling pause(). https://treeherder.mozilla.org/#/jobs?repo=try&revision=6843989ea92bd86fafed08605d4948512fe18851

Comment on attachment 8754199 [details] MozReview Request: Bug 1167519 - Part 1: Calculate plausible starting value on compositor with TimeStamp::Now() when replacing an old transtion. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53818/diff/5-6/

Comment on attachment 8755226 [details] MozReview Request: Bug 1167519 - Part 2: A mochitest to check a new transform position replaced by old transform transition is close to the old one. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54448/diff/1-2/

https://reviewboard.mozilla.org/r/53818/#review51088 > This should probably be mTarget->mElement->GetOwnerDoc() (with appropriate assertions for mTarget and mElement, I guess) since that's what we use when setting up the transition and what we compare against when storing the replaced transition parameters. Thanks! Fixed it.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #52) > The try failed. > https://treeherder.mozilla.org/logviewer.html#?job_id=21265349&repo=try#L1991 > I guess we need to wait for a paint after calling pause(). > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=6843989ea92bd86fafed08605d4948512fe18851 This second try results looks good. The reason why we need to wait for a frame there is that the animating style is not yet updated just right after pause(), we need to wait for a request of layer update caused by pause() is surely processed before checking getComputedStyle() result.

Comment on attachment 8755226 [details] MozReview Request: Bug 1167519 - Part 2: A mochitest to check a new transform position replaced by old transform transition is close to the old one. r?birtles https://reviewboard.mozilla.org/r/54448/#review51400 I think we need to check that OMTA is enabled? I thought that was the purpose of using mochitests? Also, we need to make sure the Web Animations API is enabled before using it (and I think that will mean splitting into separate test_* and file_* files). ::: layout/style/test/test_transitions_replacement_on_busy_frame.html:43 (Diff revision 2) > + requestAnimationFrame(function() { > + // Start second transition > + div.style.transform = "translateX(0px)"; > + previousMatrix = > + convertTo3dMatrix(getComputedStyle(div).transform); > + }); > + requestAnimationFrame(function() { > + // Tie up main thread for 1s > + var startTime = performance.now(); > + while (performance.now() - startTime < 1000); > + resolve(); > + }); Just curious, is it guaranteed that rAF callbacks are called in the order they are requested? ::: layout/style/test/test_transitions_replacement_on_busy_frame.html:50 (Diff revision 2) > + // Tie up main thread for 1s > + var startTime = performance.now(); > + while (performance.now() - startTime < 1000); Is 300ms enough to cause this to fail most of the time if the optimization is regressed? If possible, I'd like to avoid adding 1s to nearly every future automation run if we can help it. ::: layout/style/test/test_transitions_replacement_on_busy_frame.html:64 (Diff revision 2) > + // Wait for a paint to ensure that animation.pause() affects its styles. > + yield waitForPaints(); Is waiting on animation.ready enough here? I think that's what we're waiting for, right?

(In reply to Brian Birtles (:birtles) from comment #57) > Comment on attachment 8755226 [details] > MozReview Request: Bug 1167519 - Part 2: A mochitest to check a new > transform position replaced by old transform transition is close to the old > one. r?birtles > > https://reviewboard.mozilla.org/r/54448/#review51400 > > I think we need to check that OMTA is enabled? I thought that was the > purpose of using mochitests? Ah, right. I totally forgot about that pref. I'd personally think the pref finished its role. ;-) > Also, we need to make sure the Web Animations API is enabled before using it > (and I think that will mean splitting into separate test_* and file_* files). Sure. > ::: layout/style/test/test_transitions_replacement_on_busy_frame.html:43 > (Diff revision 2) > > + requestAnimationFrame(function() { > > + // Start second transition > > + div.style.transform = "translateX(0px)"; > > + previousMatrix = > > + convertTo3dMatrix(getComputedStyle(div).transform); > > + }); > > + requestAnimationFrame(function() { > > + // Tie up main thread for 1s > > + var startTime = performance.now(); > > + while (performance.now() - startTime < 1000); > > + resolve(); > > + }); > > Just curious, is it guaranteed that rAF callbacks are called in the order > they are requested? The spec looks to me that the order is guaranteed. > user agent MUST schedule a script-based animation resampling by appending > to the end of the animation frame request callback list > ::: layout/style/test/test_transitions_replacement_on_busy_frame.html:50 > (Diff revision 2) > > + // Tie up main thread for 1s > > + var startTime = performance.now(); > > + while (performance.now() - startTime < 1000); > > Is 300ms enough to cause this to fail most of the time if the optimization > is regressed? I am not sure, I will try several runs with 300ms. > If possible, I'd like to avoid adding 1s to nearly every future automation > run if we can help it. > > ::: layout/style/test/test_transitions_replacement_on_busy_frame.html:64 > (Diff revision 2) > > + // Wait for a paint to ensure that animation.pause() affects its styles. > > + yield waitForPaints(); > > Is waiting on animation.ready enough here? I think that's what we're waiting > for, right? I guess so, need try runs.

A try with 300ms looks good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4d12ed39fa1&selectedJob=21344531 I forgot to check the OMTA pref there again, though.

Comment on attachment 8754199 [details] MozReview Request: Bug 1167519 - Part 1: Calculate plausible starting value on compositor with TimeStamp::Now() when replacing an old transtion. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53818/diff/6-7/

Comment on attachment 8755226 [details] MozReview Request: Bug 1167519 - Part 2: A mochitest to check a new transform position replaced by old transform transition is close to the old one. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54448/diff/2-3/

Comment on attachment 8755226 [details] MozReview Request: Bug 1167519 - Part 2: A mochitest to check a new transform position replaced by old transform transition is close to the old one. r?birtles https://reviewboard.mozilla.org/r/54448/#review51646 ::: layout/style/test/file_transitions_replacement_on_busy_frame.html:34 (Diff revision 3) > +const OMTAPrefKey = "layers.offmainthreadcomposition.async-animations"; > +var omtaEnabled = SpecialPowers.DOMWindowUtils.layerManagerRemote && > + opener.SpecialPowers.getBoolPref(OMTAPrefKey); (A very very minor point, so long as we're using const once, we may as well use it everywhere? const simply says you can't re-assign the value.) ::: layout/style/test/file_transitions_replacement_on_busy_frame.html:60 (Diff revision 3) > + previousMatrix = > + convertTo3dMatrix(getComputedStyle(div).transform); > + }); > + > + requestAnimationFrame(function() { > + // Tie up main thread for 300ms I think this test would be easier to understand if we add a comment like: // Tie up main thread for 300ms. In the meantime, the first transition // will continue running on the compositor. If we don't update the start // point of the second transition, it will appear to jump when it starts. ::: layout/style/test/file_transitions_replacement_on_busy_frame.html:65 (Diff revision 3) > + // Note that requestAnimationFrame is not suitable here since on Android > + // there is a case that paint process has not been done even though > + // the callback of requestAnimationFrame is called. ... is a case where the paint process has not completed even when the requestAnimationFrame callback is run (and it is during the paint process that we update the transition start point). ::: layout/style/test/file_transitions_replacement_on_busy_frame.html:78 (Diff revision 3) > + ok(currentMatrix[3][0] - previousMatrix[3][0] >= difference, > + currentMatrix + " should be advanced ahead of " + previousMatrix); Does this print something useful in the failure case?

Comment on attachment 8755226 [details] MozReview Request: Bug 1167519 - Part 2: A mochitest to check a new transform position replaced by old transform transition is close to the old one. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54448/diff/3-4/

https://reviewboard.mozilla.org/r/54448/#review51646 > Does this print something useful in the failure case? Actually it was while debugging.

https://hg.mozilla.org/integration/mozilla-inbound/rev/72c16a3d0b0a https://hg.mozilla.org/integration/mozilla-inbound/rev/05ebd1f677ed

https://hg.mozilla.org/mozilla-central/rev/72c16a3d0b0a https://hg.mozilla.org/mozilla-central/rev/05ebd1f677ed