1928801 - bounce when reversing compositor-accelerated transition

Status: NEW

(Core :: CSS Transitions and Animations, defect, P3)

Description

[Kevin Doughty]

Attachment: firefoxTransitionBounceNotFasterReversalSlow.mp4

Steps to reproduce:

Visual bounce in a toggle animation of translateX with or without 3.1 Faster Reversal. Suspected for all composited properties. Not present when transitioning left.

Reproduction:
https://jsbin.com/japinovocu/edit?output

Actual results:

Slight bounce on transition retarget of translateX but not left. Ubuntu, both Firefox 131.0.3 and Nightly 133.0a1.

printf and println logs using opacity instead of translateX:

console.log: ">>>>>>>>>>>>>>>>>>>>"
console.log: ">>>>>>>>>>>>>>>>>>>>" // before the transition
console.log: ">>>>>>>>>>>>>>>>>>>>"
nsStyleStruct.cpp opacity old:0.000000; new:1.000000;
glue.rs current (calculated) value:None;
glue.rs old value:Opacity(0.0);
glue.rs default current_value or old_value:Opacity(0.0);
glue.rs result start value:Opacity(0.0);
glue.rs result end value:Opacity(1.0);
nsTransitionManager.cpp ConsiderInitiatingTransition FASTER REVERSAL?
nsTransitionManager.cpp DoUpdateTransitions startedAny:1; index:0;
nsTransitionManager.cpp DoUpdateTransitions length:1;
nsStyleStruct.cpp opacity old:1.000000; new:0.000000;
nsStyleStruct.cpp opacity old:0.000000; new:0.001188;
console.log: ">>>>>>>>>>>>>>>>>>>>"
console.log: ">>>>>>>>>>>>>>>>>>>>" // during the transition before the retarget
console.log: ">>>>>>>>>>>>>>>>>>>>"
nsStyleStruct.cpp opacity old:0.001188; new:0.478928;
nsStyleStruct.cpp opacity old:0.478928; new:0.000000;
nsTransitionManager.cpp ConsiderInitiatingTransition length:1;
nsTranstiionManager ConsiderInitiatingTransition replace at index:0;
nsTransitionManager.cpp GetReplacedTransitionProperties
glue.rs current start value:Opacity(0.0);
glue.rs current end value:Opacity(1.0);
glue.rs progress:0.48598567528063485;
glue.rs current (calculated) value:Some(Opacity(0.48598567));
glue.rs old value:Opacity(0.47892752);
glue.rs default current_value or old_value:Opacity(0.48598567);
glue.rs result start value:Opacity(0.48598567);
glue.rs result end value:Opacity(0.0);
nsTransitionManager.cpp ConsiderInitiatingTransition FASTER REVERSAL?
nsTransitionManager.cpp ConsiderInitiatingTransition FASTER REVERSAL!
nsTransitionManager.cpp DoUpdateTransitions startedAny:1; index:0;
nsTransitionManager.cpp DoUpdateTransitions length:1;
nsStyleStruct.cpp opacity old:0.000000; new:0.485986;
nsStyleStruct.cpp opacity old:0.485986; new:0.497298;
console.log: ">>>>>>>>>>>>>>>>>>>>"
console.log: ">>>>>>>>>>>>>>>>>>>>" // during the second transition after the retarget
console.log: ">>>>>>>>>>>>>>>>>>>>"
nsStyleStruct.cpp opacity old:0.497298; new:0.000000;
console.log: ">>>>>>>>>>>>>>>>>>>>"
console.log: ">>>>>>>>>>>>>>>>>>>>" // transition finished
console.log: ">>>>>>>>>>>>>>>>>>>>"

Expected results:

The visual bounce should not be evident.

Since I last experimented with Nightly, code has changed.

Formerly nsTransitionManager.cpp called glue.rs Servo_ComputedValues_ExtractAnimationValue.

Currently nsTransitionManager.cpp calls glue.rs Servo_ComputedValues_ShouldTransition.

I believe this code is for more accurate values of composited properties, and may not be the source of the bug, but I don't know.

The JSBin animates translateX from 0px to 1000px and back. Logging uses opacity because it's easier. There is a transition from 0 to 1, then a retarget midway from 1 to 0.

After the retarget, logs from nsStyleStruct.cpp show opacity changing from an animation-only restyle value of 0.001188 to 0.478928. It then gets changed to the discrete retarget value of 0.000000.

Logs from the glue.rs Servo_ComputedValues_ShouldTransition show an old_value of 0.47892752 (same as the above animation-only restyle but not rounded) and the (newly calculated) current_value of 0.48598567.

Then, more logs from nsStyleStruct.cpp show setting opacity to the (rounded from the above) current_value of 0.485986. Then finally, I think it's an animation-only restyle when nsStyleStruct.cpp logs a change to (a third, new value of) 0.497298.

My understanding of throttled animations is poor but I believe an animation-only restyle results in a rendered value that does not match the composited value, or maybe the value from Servo_ComputedValues_ShouldTransition does not match.

All I know is off the three values, 0.478928, 0.485986, and 0.497298, one of them is probably evidence of the bounce.

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Core::CSS Transitions and Animations' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

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

The severity field is not set for this bug.
:emilio, could you have a look please?

For more information, please visit BugBot documentation.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Test-case from comment 0

Comment 4

[Kevin Doughty]

Attachment: verbose loging

```

Comment 5

[Kevin Doughty]

Attachment: verbose-logging

Comment 6

[Kevin Doughty]

Apologies if this isn't helpful. Attachment showed an error message so they're accidental duplicates.

Faster reversal was disabled by commenting out code in nsTransitionManager. Video from original post disabled faster reversal in javascript by transitioning from 0 to 400 to 1. Bug happens with or without faster reversal.

Verbose logging shows Servo_ComputedValues_ShouldTransition calculate return transition start value [D] (at arrow #1) and discrete end value [A] (at arrow #2).

An animation-only restyle shows changing the discrete former destination value [Z] to the calculated start value [D] (at arrow #3).

A second animation-only restyle (at arrow #4) shows changing the calculated start value [D] to a new value [E]. Value [E] is greater than value [D] when it should be less. It's going in the wrong direction. It's as if the restyle somehow used the previous transition (pure speculation).

Logs for transitioning line-height at the equivalent to arrow #4 go in the correct direction.

Comment 7

[Kevin Doughty]

Bug is possibly fixed after bypassing code in AnimationInfo::AddAnimationForProperty.

  bool needReplaceTransition = false;
  // if (dom::CSSTransition* cssTransition = aAnimation->AsCSSTransition()) {
  //   needReplaceTransition =
  //       cssTransition->UpdateStartValueFromReplacedTransition();
  // }

It follows a long comment explaining its purpose, which perhaps new changes to nsTransitionManager have made obsolete.

Logging produces expected output but I am not sure if there are any unintended consequences.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

This is the relevant counter-part in nsTransitionManager: https://searchfox.org/mozilla-central/rev/6050bf4eca89956c9d91bfd89fa59294ae32a689/layout/style/nsTransitionManager.cpp#329,459

That code recently came into play as well in bug 1930907.

Comment 9

[Kevin Doughty]

Attachment: WIP: Bug 1928801 - removes code so as to not replace transition start value

Comment 10

[Kevin Doughty]

Attachment: verbose logs showing result of patch

Comment 11

[Kevin Doughty]

Compositor value is calculated here:
https://searchfox.org/mozilla-central/source/servo/ports/geckolib/glue.rs#1239

The following calls from the aboved attached file are therefore redundant and cause the jump:

// CSSTransition.cpp UpdateStartValueFromReplacedTransition
// CSSTransition.cpp ComputeTransformedProgress
// glue.rs Servo_AnimationValues_Interpolate from:Opacity(0.0); to:Opacity(1.0); result:Opacity(0.3952261);
// KeyframeEffect.cpp ReplaceTransitionStartValue

I haven't run any local tests because I don't know which ones to run. I've only verified translateX visually and opacity from logging.

Comment 12

[Kevin Doughty]

There were some xpctest failures but they don’t appear to be related. Mostly networking. Running mochitests on dom/base/test/mochitest.ini right now but it's going to take a while

Comment 13

[Kevin Doughty]

Attachment: WIP: Bug 1928801 - missed unused variable

Depends on D229556

Comment 14

[Kevin Doughty]

dom/animation/test/chrome/test_animation_performance_warning.html
FAIL animations on compositor - animations on compositor: assert_equals: runningOnCompositor property should match expected true but got false

Comment 15

[Kevin Doughty]

Same test failure occurs regardless if patch is applied or not. I get a lot of other unrelated test failures. Suspect limitations of my machine and dev environment

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Boris, Hiro, any feedback on this? See bug 1626165.

Kevin, just ni? me when you need a try run, I can send it to automation rather than making you run the test-suite locally. I'd also be happy to vouch for you to get L1 (try) access if there are failures you want / need to investigate.

Just sent your stack here: https://treeherder.mozilla.org/jobs?repo=try&revision=d0061110ddde2d64b8ecd4251b39db3966652144

Comment 17

[Boris Chiou [:boris]]

Could you please check the test:
https://searchfox.org/mozilla-central/source/layout/style/test/test_transitions_replacement_on_busy_frame.html
and
https://searchfox.org/mozilla-central/source/layout/style/test/test_transitions_replacement_with_setKeyframes.html
?

Brian added this implementation to fix the issue if there is a time gap between the creation of new transition and the timing of sending the transition to the compositor, e.g. busy main thread.

If we drop them, we may regress these tests and their original bugs I think.

(I just saw Emilio's link, and those patches make these tests failed indeed)

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Yeah, the change looks to me that it's just a backed out change of bug 1626165. So, we have concluded bug 1626165 is a regressor? Even if it is, just simply backing out isn't reasonable to me. We need to investigate how the bounce happens in details and figure out a reasonable way.

If you want me to dive into the problem, NI to me again. Thanks!

Comment 19

[Kevin Doughty]

Patch fails on test_transitions_replacement_on_busy_frame.html.

I'm also finding other problems without my patch after inserting lag (while (performance.now() - startTime < lag);) and will put a demo together

Comment 20

[Kevin Doughty]

Opened a new bug only because I didn't realize there was an attachment button at the top of the page.
https://bugzilla.mozilla.org/show_bug.cgi?id=1932917
It's related to the same implementation of UpdateStartValueFromReplacedTransition/ReplaceTransitionStartValue/replacedTransitionId