1634945 - Replace the start value of the new transition with the last value on the compositor

Status: NEW

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

Description

[Hiroyuki Ikezoe (:hiro)]

Once after bug 1626165 landed, we use the last composited value on the compositor for newly created transitions as the start value if the transition replaced an old transition, but still we keep using a stale start value on the main-thread, it results jumpy animations when the throttled transitions are flushed on the main-thead.

The jumpy animations will be mitigated by bug 1632957 if the flush is caused by event handling stuff. But bug 1632957 doesn't help cases where the flush is caused by other reasons. So ideally we should also update the start value on the main-thread as well as on the compositor.

I wrote a hacky patch to do that. As of now it just works only for transform property with WebRender. On my environment, it's quite hard to notice the original jittery transition so i't hard to tell whether the patch will fix the jumpy animations or not. But hopefully it will solve it without bug 1632957 (theoretically, the patch should fix the issue without bug 1632957).

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1634945 - Undef CurrentTime.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1634945 - Update start value.

Depends on D73575

Comment 3

[Hiroyuki Ikezoe (:hiro)]

During thinking about bug 1633442, I noticed that we need to update the KeyframeEffect directly instead of indirectly using the CSSTransition since the CSSTransition's KeyframeEffect might be replaced by Web APIs. An obvious annoying point is that if the KeyframeEffect was replaced before we get the start value from the compositor, users will see visual glitches.

Comment 4

[Brian Birtles (:birtles)]

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

Once after bug 1626165 landed, we use the last composited value on the compositor for newly created transitions as the start value if the transition replaced an old transition, but still we keep using a stale start value on the main-thread, it results jumpy animations when the throttled transitions are flushed on the main-thead.

I believe that was deliberate since otherwise the replacing behavior would be observable to Web content. That is, we deliberately decided to allow the values on main thread and the compositor to drift. But I guess we didn't think about how that would affect the updating behavior.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

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

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

Once after bug 1626165 landed, we use the last composited value on the compositor for newly created transitions as the start value if the transition replaced an old transition, but still we keep using a stale start value on the main-thread, it results jumpy animations when the throttled transitions are flushed on the main-thead.

I believe that was deliberate since otherwise the replacing behavior would be observable to Web content. That is, we deliberately decided to allow the values on main thread and the compositor to drift. But I guess we didn't think about how that would affect the updating behavior.

I think that was the case before Stylo. Since stylo, we have no mini flush when the new transition is going to replace the old one, which means the start value of the new transition is the start value of the old transition. That's really really a big problem, and not the case we deliberated.

So what we should do I have in my mind is;

1. Flush throttled animations (the patch I posted in bug 1623425)
2. Use the latest value on the compositor (bug 1634945) if the difference between TimeStamp::now() and the refresh driver's time is greater than Vsync span (no bug yet)
3. Replace the start value with the latest one (this bug)

Without 1), there remain a bunch of annoying cases the transition jumps back.

Comment 6

[Brian Birtles (:birtles)]

Yes, I completely agree that (1) is the first thing we want to fix. I was concerned about (3), this bug, because I was afraid it would be Web observable, i.e. that the result of getKeyframes() would suddenly change. However, maybe that's not necessary if we just update mStartForReversingTest and leave the keyframes on the main-thread untouched?