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

Status: RESOLVED FIXED

(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)]

(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)]

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.

Comment 25

[Boris Chiou [:boris]]

I just realized that my previous patches are not correct. I think the purpose of this bug is to find a way to retrieve the sampled animation value from the compositor to the main thread, so we can replace the keyframe value together with the compositor animations.

My previous patches didn't update the keyframe values of the transition on the main thread so we cannot get the correct property value via script on the main thread. (i.e. test_transitions_replacement_on_busy_frame.html was not passed with my patches.)

Comment 26

[Hiroyuki Ikezoe (:hiro)]

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

My previous patches didn't update the keyframe values of the transition on the main thread so we cannot get the correct property value via script on the main thread. (i.e. test_transitions_replacement_on_busy_frame.html was not passed with my patches.)

That's bug 1634945?

Comment 27

[Boris Chiou [:boris]]

Oh right. We should put them together; otherwise this may cause issues.

I'm checking your bug now (bug 1634945). Thanks.

Comment 28

[Boris Chiou [:boris]]

So for now, in this bug, we could just minimize the implementation:
We still compute the new start value in AnimationInfo::AddAnimationForProperty for the main thread. However, we don't use that value for the compositor. As per my previous patches, we send a flag to the compositor, so we just use the previous sampled animation value as the new start value on the compositor.

This makes the computed values on the main thread be a little bit different from the actual rendering result on the compositor thread, but the rendering result could be better (i.e. without any lag on the compositor).

Comment 29

[Boris Chiou [:boris]]

Attachment: Bug 1626165 - Part 1: Use move for aAnimationValues in StoreAnimatedValue().

We will store the sampled animations (for transforms) in AnimatedValue.
So let's change the API to move this array, and so we can just move it
into AnimationTransform in the following patch.

No behavior change.

Comment 30

[Boris Chiou [:boris]]

Attachment: Bug 1626165 - Part 2: Replace the start value and start time of the transition on the compositor.

Though we re-compute the start value of the transition when sending it
to the compositor, we still want to use the last sampled animation value
as the new start value, to avoid any possible jittery.

We don't add new test case. Instead, we just have to make sure we pass
the existing test: test_transitions_replacement_on_busy_frame.html.

No behavior change. Just a minor improvement.

Comment 31

[Boris Chiou [:boris]]

We probably have to fix Bug 1901641 first before landing these patches. I'd like to use the test case in Bug 1901641 to verify the patches in this bug.

Comment 32

[Hiroyuki Ikezoe (:hiro)]

I thought about bug 1901641 yesterday, I have an idea to solve bug 1901641 and this bug altogether.

Now we are going to use the last sampled animation value on the compositor as the start value of a new transition in this bug.

And the problem caused bug 1901641 is that the start time of the new transition is stale if there's busy state on the main-thread.

That's right?

Then, in this bug we can also use the last Vsync timestamp on the compositor as the start time of the new transition. I suppose it should solve both bugs altogether. Maybe the start time should be the current timestamp? maybe?

Comment 33

[Boris Chiou [:boris]]

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

I thought about bug 1901641 yesterday, I have an idea to solve bug 1901641 and this bug altogether.

Now we are going to use the last sampled animation value on the compositor as the start value of a new transition in this bug.

And the problem caused bug 1901641 is that the start time of the new transition is stale if there's busy state on the main-thread.

That's right?

Then, in this bug we can also use the last Vsync timestamp on the compositor as the start time of the new transition. I suppose it should solve both bugs altogether. Maybe the start time should be the current timestamp? maybe?

Thanks a lot. This makes senses. Let me try to use the last Vsync timestamp on the compositor as the start time to check both bugs.

Comment 34

[Boris Chiou [:boris]]

My local patch looks good to fix bug 1901641 (by using the previous sample time as the new start time, when replacing the transition). I'd like to see if there are any regression in try.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ff6b79a8b54
Part 1: Use move for aAnimationValues in StoreAnimatedValue(). r=layout-reviewers,firefox-animation-reviewers,hiro
https://hg.mozilla.org/integration/autoland/rev/594ad70c37c1
Part 2: Replace the start value and start time of the transition on the compositor. r=layout-reviewers,firefox-animation-reviewers,hiro

Comment 37

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/4ff6b79a8b54
https://hg.mozilla.org/mozilla-central/rev/594ad70c37c1