New issue
Advanced search Search tips

Issue 844436 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 27
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 430155



Sign in to add a comment

Animation Worklet - worklet animations local time is not guaranteed to apply in the same tick

Project Member Reported by majidvp@chromium.org, May 18 2018

Issue description

Currently 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.


 
This is in fact the cause for noticeable delay when scrolling twitter header demo using scrollbar.

To reproduce (using Canary with flags enabled)L 
- Go to https://googlechromelabs.github.io/houdini-samples/animation-worklet/twitter-header/
- Scroll via scrollbar


Here is what how this issue combined with the difference between fast path and slow path scrolling can 
cause the delay to be noticeable in the latter but not former.

When we have regular commits from main we will call animate twice: 1) once after begin impl frame and 2) once after commit

## Fast path case i.e., compositor scrolling

On fast path:
In (1) Tick AW with active tree offset which produces a new local time. 
In (2) Actually tick animation with the local time from (1). Commit does not change scroll offset so no AW update.

As long as this happen in a single frame the application delay is not noticed thanks to twice animation.

## Slow path case i.e., main thread scrolling

On slow path calling animate twice has a different result: 
In (1) Tick AW with active tree offset which produces a new local time. Note that the scroll offset gets replaced by main after commit and activation.
In (2) Actually tick animation with the local time from (1) which is incorrect. Then Tick AW with new pending tree offset to compute a
new local time which is correct but it is not applied in this frame. 

Note that if the pending tree is activated in the same frame, the scroll offset and the animated transform are out of sync which produces 
the visual "lag" artifact.





Status: Started (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
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.



simple-aw-scroll-test.html
1.5 KB View Download
Project Member

Comment 5 by bugdroid1@chromium.org, 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

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