1623425 - transform translateX animation with requestAnimationFrame is jittery

Status: NEW

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

Description

[Ksenia Berezina [:ksenia]]

Attachment: 50046.html

This was initially reported here:
https://github.com/webcompat/web-bugs/issues/50046

To reproduce open the attached test case in Firefox Preview or Preview Nightly.

Expected:
Timer bar decreases smoothly

Actual:
Timer bar jumps forward and back

Comment 1

[Ksenia Berezina [:ksenia]]

Attachment: 50046.html

Comment 2

[Miko Mynttinen]

The testcase uses requestAnimationFrame to animate transform: translateX(p%) one percentage point per second, and relies on transition to "fill in the gaps". Due to timing inaccuracies and rounding, the RAF callback

1. does not always trigger at the right time and,
2. might skip or repeat percentage values for translate.

Brian: is this combination of transforms and transitions "supported" with animations?

Comment 3

[Brian Birtles (:birtles)]

Sorry for the massive delay here, this arrived just as I went on PTO.

I think there is something broken about the way we update transitions running on the compositor.

If we explicitly flush styles in the animate() function in the test case the problem goes away. That suggests that we're updating the transition on the compositor with old styles from the main thread (and likely we're throttling updates on the main thread too making the style older still).

We have code specifically to avoid this that we introduced for Firefox OS but I guess it's not working correctly here.

We've seen other cases where updating transitions on the compositor was not behaving as expected (there was one Xidorn filed but I can't find it right now) so this is probably a good opportunity to debug what is going on here.

Perhaps Boris or Hiro is interested in digging into this?

Comment 4

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #3)

We have code specifically to avoid this that we introduced for Firefox OS but I guess it's not working correctly here.

I checked this very quickly by some logs. It seems this function works well sometimes (i.e. it calculates the correct start value and then replaces the transition well). However, the jittery happens when its computedTiming.mProgress is null, so we don't calculate the start value. I will keep checking what happens.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

If we still want to keep using the tweak, I'd suggest that we do the tweak on the compositor not on the main thread.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)

If we still want to keep using the tweak, I'd suggest that we do the tweak on the compositor not on the main thread.

Filed bug 1626165. That should fix this issue.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Moving into CSS Transitions and Animations.

Comment 8

[Brian Birtles (:birtles)]

I'm curious why the start positions get so out of sync here. I thought we did a mini-flush before triggering transitions to bring the main thread up to date when it was throttled? Looking at the test case though it seems like the main thread value is very old.

Comment 9

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

The component has been changed since the backlog priority was decided, so we're resetting it.
For more information, please visit auto_nag documentation.

Comment 10

[Brian Birtles (:birtles)]

For what it's worth, I think this bug (or bug 1626165 if we fix it there), is fairly high priority. This has come up a number of times and I've personally had to work around it a few times in Web apps by adding extra style flushes. I think it's even more noticeable than bug 1540906 so it's probably on par with that for priority.

Comment 11

[Boris Chiou [:boris]]

Attachment: Bug 1623425 - Use end value if the replaced transition has been finished.

We set mReplacedTransition only when it has current effect (i.e. in active
phase). However, when we replace the start value with mReplacedTransition,
the current time stamp might be a little bit late and the progress of the
mReplacedTransition might be just finished, so it's in after phase.
We have to handle the case of after phase case, so we send the up-to-date
transition into the compositor.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

I could find a problem here in nsTransitionManager::ConsiderInitiatingTransition. We get the start value for the new transition from |aOldStyle|, but at the moment, the transform style has been being throttling, so the |aOldStyle| is pretty much stale. As Brian suspected this is a regression by Stylo.

When we call this ConsiderInitiatingTransition it's in a sequential task after a normal style traversal, we skipped the first animation restyling before this normal traversal. It's hard to recall why I did avoid making the element dirty when the transform style is changed...

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Dropping bug 1626165 from the dependency list since this issue is not related to the start time tweak machinery.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: A patch to fix this issue

This is a patch I wrote before for bug 1607723. I noticed this patch also fixes this issue.

An alternative way I can think of to fix this issue requires the third animation only restyling, which sounds awful. :) FWIW, the alternative way is, 1) defer creating a new transition if we are going to replace the old transition (i.e. defer discarding the old transition) 2) post restyle::layer for the target element 3) restyle the throttled animation in the second animation restyling 4) trigger a sequential task to create a new transition and discard the old transition somehow 5) restyle the new transition in the third animation restyling. I understand this sounds....

Another hacky way I can think of is to use the timeline time when we replace the old transition instead of TimeStamp::Now() if TimeStamp::Now() - the timeline time < vsync interval.

Comment 15

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)

Created attachment 9140703 [details] [diff] [review]
A patch to fix this issue

This is a patch I wrote before for bug 1607723. I noticed this patch also fixes this issue.

After applying this patch, my log looks pretty different. In most cases, the original transition is correct, so we don't have to replace the start value (i.e. computedTiming.mProgress.IsNull() is true in CSSTransition::UpdateStartValueFromReplacedTransition()). And even if we replace the start value, the original start value is pretty close to the new start value. This behavior looks like much close to what hiro mentioned in Riot: UpdateStartValueFromReplacedTransition() is for the low-spec device (or busy main thread case), not for the general case.

It seems we are close to the root cause.

Comment 16

[Firefox Bug Husbandry Bot]

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is P3 (Backlog,) indicating it has been triaged, the bug's Severity is being updated to S3 (normal.)

Comment 17

[Brian Birtles (:birtles)]

(In reply to Firefox Bug Husbandry Bot from comment #16)

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is P3 (Backlog,) indicating it has been triaged, the bug's Severity is being updated to S3 (normal.)

Is it weird that the Firefox Bug Husbandry Bot should reset the assignee?

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Oops, I should have noted about the patch in comment 14.
I know it's suboptimal, because it's flushes unrelated throttling animations, that said it should work in the wild. But there is a fundamental issue in the patch, that is that it breaks our reftest-no-flush machinery. When we use reftest-no-flush, we also use reftest-wait on the root document element, which means when the reftest-wait is removed, the document element gets dirty, which means we flush all throttled animations in the document. That's quite unfortunate.