Animation Worklet - worklet animations local time is not guaranteed to apply in the same tick |
|||
Issue descriptionCurrently our logic for ticking worklet animations does not ensure that the updated local time is actually applied and updates the animation output. The relevant logic is in AnimationHost::SetMutationUpdate were we set the local time but don't tick the animations. At the moment the whole worklet animation update cycle is synchronous so a simple fix is to swap the order of 1) ticking animations 2) updating worklet animations. Swapping that order resolves the issue. But once worklet animation updates are async then this is no longer a solution. Alternativly we can immediately tick the worklet animations once their local time is updated. This may work but we have to check that ticking animations then is a safe thing to do.
,
Jul 13
,
Jul 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/51207b18e8b497b905207a06291c895e0a4c6125 commit 51207b18e8b497b905207a06291c895e0a4c6125 Author: Majid Valipour <majidvp@chromium.org> Date: Thu Jul 19 21:16:41 2018 [animation-worklet] apply worklet animation local time immediately Fix a bug in our logic where ticking worklet animations does not ensure that the updated local time is actually applied to update the animation output. The relevant logic is in AnimationHost::SetMutationUpdate where we set the local time but don't tick the animations. This was not noticeable for the most part because in many situations we call AnimationHost::Animate twice in the same frame which masks the issues. Though this becomes noticeable where we have main thread scrolling. See [1] for more details. The fix is to first tick mutator which updates worklet animations output state (in particular their local time) and then tick animations (which includes worklet animations). This reverses the previous order. Other minor changes: - Add a test for ensuring we don't tick main side worklet animation - Remove unnecessary call to SetNeedsPushProperties inside SetOutputState. We no longer push local_time_ from main to impl so it does not make sense to ask for pushing props when it is set. - Make WorkletAnimation::local_time_ an optional value instead of incorrectly initializing it to base::TimeDelta(). In fact this was masking a bug in worklet-animation-local-time-before-start.html [1] https://bugs.chromium.org/p/chromium/issues/detail?id=844436#c1 Bug: 844436 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Change-Id: I0273c2ee1ad473d5675ea83cc3fcfd2a7956b6cd Reviewed-on: https://chromium-review.googlesource.com/1066358 Reviewed-by: Robert Flack <flackr@chromium.org> Reviewed-by: Yi Gu <yigu@chromium.org> Commit-Queue: Majid Valipour <majidvp@chromium.org> Cr-Commit-Position: refs/heads/master@{#576632} [modify] https://crrev.com/51207b18e8b497b905207a06291c895e0a4c6125/cc/animation/animation_host.cc [modify] https://crrev.com/51207b18e8b497b905207a06291c895e0a4c6125/cc/animation/animation_host_unittest.cc [modify] https://crrev.com/51207b18e8b497b905207a06291c895e0a4c6125/cc/animation/keyframe_effect.h [modify] https://crrev.com/51207b18e8b497b905207a06291c895e0a4c6125/cc/animation/single_keyframe_effect_animation.cc [modify] https://crrev.com/51207b18e8b497b905207a06291c895e0a4c6125/cc/animation/single_keyframe_effect_animation.h [modify] https://crrev.com/51207b18e8b497b905207a06291c895e0a4c6125/cc/animation/worklet_animation.cc [modify] https://crrev.com/51207b18e8b497b905207a06291c895e0a4c6125/cc/animation/worklet_animation.h [modify] https://crrev.com/51207b18e8b497b905207a06291c895e0a4c6125/cc/animation/worklet_animation_unittest.cc [modify] https://crrev.com/51207b18e8b497b905207a06291c895e0a4c6125/third_party/WebKit/LayoutTests/virtual/threaded/fast/animationworklet/worklet-animation-local-time-before-start.html
,
Jul 27
I am still investigating other related jitters that we are seeing. Attached is a simple test page that I am using. So far I have identified to possible source of jitter: 1. Using stale scroll offset when scroll is being animated (e.g., smooth scroll). I have a patch to fix this: https://chromium-review.googlesource.com/c/chromium/src/+/1152899 2. Pixel Snapping in Transform Node: I have not fully identified how this is playing out but if I make cc::TransformTree::UpdateSnapping a no-op the remaining jittering is fixed.
,
Aug 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c06c126142307731fdad8f32c09521474d24a689 commit c06c126142307731fdad8f32c09521474d24a689 Author: Majid Valipour <majidvp@chromium.org> Date: Thu Aug 02 20:45:51 2018 [animation-worklet] ensure scroll animation output is seen by mutator WorkletAnimations which have a ScrollTimeline need to be animated and ticked every time that scroll offset is changed on compositor thread. This ensures their output is in sync with scrolling. Scroll animations on compositor (e.g., smooth wheel animations) are a source of scroll offset change on compositor. So to make sure scroll-linked worklet animations can respond to latest scroll offset we need to tick these animations **after** cc scroll animations produce their output. Previously our approach was to tick mutator (update all worklet animations) once before ticking all animations and every time a cc scroll animation change the scroll offset. However, ticking mutator updated worklet animation without actually applying their output which meant worklet animation can fall behind cc scroll animation. Further more this causes mutator to be ticked multiple times (once per each scroll animation). This patch changes our behavior as follows: - Tick regular animations which including cc scroll animations. - Tick mutator which sends update to AnimationWorklet and receive its new output. - Tick worklet animations to apply the new output. This ensures we see the scroll animation and any linked worklet animation output in the same frame. Also it avoid excessively ticking mutator since we only need to do it once after all cc scroll animations are ticked. TEST:AnimationHostTes.LayerTreeMutatorUpdateReflectsScrollAnimations Bug: 844436 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Change-Id: Ica09a4779ae3b78af9f0f93125a43846d91e8bc1 Reviewed-on: https://chromium-review.googlesource.com/1152899 Commit-Queue: Majid Valipour <majidvp@chromium.org> Reviewed-by: Robert Flack <flackr@chromium.org> Cr-Commit-Position: refs/heads/master@{#580310} [modify] https://crrev.com/c06c126142307731fdad8f32c09521474d24a689/cc/animation/animation_host.cc [modify] https://crrev.com/c06c126142307731fdad8f32c09521474d24a689/cc/animation/animation_host_unittest.cc [modify] https://crrev.com/c06c126142307731fdad8f32c09521474d24a689/cc/trees/layer_tree_host_impl.cc
,
Aug 2
The second issue in #4 is already tracked by Issue 585458 and I am working on a patch for it. |
|||
►
Sign in to add a comment |
|||
Comment 1 by majidvp@chromium.org
, Jun 27 2018