1626165 - Consider using the last animation value on the compositor when a new transition replaces the old one

Status: ASSIGNED

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

Description

[Hiroyuki Ikezoe (:hiro)]

In bug 1167519, we did introduce a tweak that using a plausible start value for the new transition which replaces the old transition. But if we can get the current value on the compositor and use the value as the start value, it would be a better result, I believe.

Anyways, I don't think it's worth spending time on this right now. This should be a future work for us.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

A wrong copy-and-paste. :/

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Anyways, I don't think it's worth spending time on this right now. This should be a future work for us.

I don't mind how we fix bug 1167519 but I actually think it is pretty high priority to fix it one way or another (which might not be this bug necessarily). It comes up a lot in my experience and I am constantly having to work around it.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

I was thinking that we will fix this bug as a part of bug 1625029 (run more CSS properties on the compositor) because once we support animations of more CSS properties on the compositor, users might see the issue more often.

I am adding this in the dependency list of bug 1625029.

Comment 4

[Boris Chiou [:boris]]

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

I don't mind how we fix bug 1167519 but I actually think it is pretty high priority to fix it one way or another (which might not be this bug necessarily). It comes up a lot in my experience and I am constantly having to work around it.

OK. I may spend more time on this or Bug 1623425 first. Perhaps check why bug 1167519 doesn't fix Bug 1623425, as my first step. (Sorry, debugging other layout bugs in these two days.)

Comment 5

[Hiroyuki Ikezoe (:hiro)]

What we need to do here is

1. Get the previous animation's id by calling AnimationInfo::GetCompositorAnimationsId and send the id to the compositor if we replaced the previous transition by a new transition
   2a) For non WebRender, we can get the last value in AnimationInfo::SetCompositorAnimations by calling CompositorAnimationStorage::GetOMTAValue and replace the start value with the last value. (Probably we can do it in AnimationHelper::ExtractAnimations)
   2b) For WebRender, we can get the last value with the same way but I don't know of the right place to do it, we need to ask someone in gfx team. Anyway an important thing is we need to do this stuff before the previous animation data is decarded

Hmm that's all we have to do? I think I am definitely missing something, but basically this should work.

Also I should note that during writing this comment, I started suspecting the animation id seems to be conflict in some cases, e.g. an animation object has multiple css propery animations such as transform and opacity.

I also realized a side effect of this approach is that the start value of the transition becomes a wrong value if user see the value in the devtools animation panel.

Comment 6

[Boris Chiou [:boris]]

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

2a) For non WebRender, we can get the last value in AnimationInfo::SetCompositorAnimations by calling CompositorAnimationStorage::GetOMTAValue

The OMTA value (or AnimatedValue) works well for properties except for transform/translate/rotate/scale/offset. We only store a final matrix for transform-like properties, so perhaps we have to store the last animation values for each transform-like property (if this property is a transition property).

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris] from comment #6)

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

2a) For non WebRender, we can get the last value in AnimationInfo::SetCompositorAnimations by calling CompositorAnimationStorage::GetOMTAValue

The OMTA value (or AnimatedValue) works well for properties except for transform/translate/rotate/scale/offset. We only store a final matrix for transform-like properties, so perhaps we have to store the last animation values for each transform-like property (if this property is a transition property).

I see. Given that individual transform properties are not commonly used in the wild, you can do it in a followup bug.

Comment 8

[Boris Chiou [:boris]]

Just rechecked the issue hiro mentioned: the start value on the devtools, and perhaps other places (e.g. sync with mouse event?):

We replace the startValue on the compositor, but the keyframe doesn't know the new start value on the main thread. This may cause some issues on the main thread. So perhaps we still have to do the replacement on the main thread, and use IPC to retrieve the last AnimationValue if possible.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris] from comment #8)

Just rechecked the issue hiro mentioned: the start value on the devtools, and perhaps other places (e.g. sync with mouse event?):

We replace the startValue on the compositor, but the keyframe doesn't know the new start value on the main thread.

Yes, I've noticed it.

This may cause some issues on the main thread.

Do you ever notice actual issues? The only one issue I was thinking is that the start value of the transition in devtools animation inspector might be wrong (but I haven't checked).

So perhaps we still have to do the replacement on the main thread, and use IPC to retrieve the last AnimationValue if possible.

I had a plan to add an IPC call from the compositor to the main-thread to tell the fact that an animation caused a janky situation on the compositor for bug 1324591. But after some chats with botond, I decided I am not going to add the IPC call initially.

Anyways, my question is what the actual issues are? I think the devtools' one is not a big problem at all.

Comment 10

[Boris Chiou [:boris]]

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

Anyways, my question is what the actual issues are? I think the devtools' one is not a big problem at all.

I'm still checking this. I still saw some back and forth issues when I have some mouse events. The log looks like if we have a main thread compose during the sampling, the start value would be rolled back to the original incorrect one. (i.e. not the one I just update by the last AnimationValue). Perhaps I miss something.

Comment 11

[Boris Chiou [:boris]]

Attachment: Bug 1626165 - Support replacing start value on transform, opacity, and background-color on the compositor.

Comment 12

[Boris Chiou [:boris]]

Attachment: Bug 1626165 - Support replacing start value on all transform-like properties.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Boris, can you please rebase patches here? I am going to finish writing up patches for bug 1634945. There is another bug report (bug 1638444), in that case it's background transitions. Anyways we should at least fix the issue visually even if the transition's keyframe vale is odd (that's been odd regardless of those fixes).

Comment 14

[Boris Chiou [:boris]]

OK. I'm rebasing these.

Comment 15

[Boris Chiou [:boris]]

I pushed the rebased patches. Please let me know if you see any build errors. :)

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Thanks! Yep, there are a bunch of build errors due to recent nsTArray changes.

Comment 17

[Boris Chiou [:boris]]

OK. I should run local build to fix them. :) Thanks.

Comment 18

[Boris Chiou [:boris]]

Working on the compilation errors now. Sorry for the inconvenience.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

No worries. Don't need to rush at all.

Comment 20

[Boris Chiou [:boris]]

OK. We should always use "move" for AnimatedValue and AnimationTransform. These patches are updated.

Comment 22

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Boris, do you still intend to land this? Just asking because I notice there is a new dupe of it.

Comment 23

[Boris Chiou [:boris]]

I may have to revisit these patches. Keep the ni for myself.

Comment 24

[Boris Chiou [:boris]]

This is on my current goals, so drop the ni.