New issue
Advanced search Search tips

Issue 824782 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature

Blocking:
issue 430155


Participants' hotlists:
Hostlist-AnimationWorklet-OT


Sign in to add a comment

Animation Worklet - worklet animation does not honor delay

Project Member Reported by majidvp@chromium.org, Mar 22 2018

Issue description

See: https://jsfiddle.net/majidvp/weef02s9/26/

In this example there are two worklet animation with exact animate function
but effects that differ only in their "delay" amount. The expectation is that
the animation that has delay effect to start later but they start together.


effect.localTime should be updating the local time of the effect [1]. Delay get applied to local time. This is not happening with worklet animations currently.

[1] https://drafts.csswg.org/web-animations/#animation-effect-calculations-overview
 
Summary: Animation Worklet - worklet animation does not honor delay (was: Animation Worklet - worklet animation do not honor delay)
Owner: smcgruer@chromium.org
Status: Started (was: Available)
So what is meant to happen here (as far as I understand) is that KeyframeEffect::TickKeyframeModel should bail for the first startDelay seconds due to keyframe_model->InEffect(monotonic_time) failing.

For a normal composited animation, that is what happens; KeyframeModel::ConvertToActiveTime ticks up from -startDelay to 0, and then the animation takes effect.

But for an Animation Worklet like the one in the fiddle, it is immediately positive once a time is returned from the animation. I do need to check what time the JS code is returning though, maybe the actual worklet code is wrong.
Fun aside - *someone* is calling KeyframeModel::ConvertFromActiveTime with a 'normal' input time (i.e. not worklet driven) during execution, because interspersed with the calls from KeyframeEffect::TickKeyframeModel I see some with the expected output value:

[1:9:0410/230309.970811:INFO:keyframe_model.cc(185)] ConvertToActiveTime, returning 6772872924246 bogo-microseconds - 6772872790918 bogo-microseconds - 0 s = 0.133328 s
[1:9:0410/230309.970869:INFO:keyframe_model.cc(185)] ConvertToActiveTime, returning 6772872924246 bogo-microseconds - 6772872790918 bogo-microseconds - 0 s = 0.133328 s
[1:9:0410/230309.970920:INFO:keyframe_model.cc(185)] ConvertToActiveTime, returning 6772868802345 bogo-microseconds - 6772872790918 bogo-microseconds - 0 s = -3.98857 s // Correct value
[1:9:0410/230309.988580:INFO:keyframe_model.cc(185)] ConvertToActiveTime, returning 6772872935673 bogo-microseconds - 6772872790918 bogo-microseconds - 0 s = 0.144755 s
[1:9:0410/230309.988638:INFO:keyframe_model.cc(185)] ConvertToActiveTime, returning 6772872935673 bogo-microseconds - 6772872790918 bogo-microseconds - 0 s = 0.144755 s
[1:9:0410/230309.988688:INFO:keyframe_model.cc(185)] ConvertToActiveTime, returning 6772868819011 bogo-microseconds - 6772872790918 bogo-microseconds - 0 s = -3.97191 s // Correct value
[1:9:0410/230310.013364:INFO:keyframe_model.cc(185)] ConvertToActiveTime, returning 6772872952338 bogo-microseconds - 6772872790918 bogo-microseconds - 0 s = 0.16142 s
[1:9:0410/230310.013419:INFO:keyframe_model.cc(185)] ConvertToActiveTime, returning 6772872952338 bogo-microseconds - 6772872790918 bogo-microseconds - 0 s = 0.16142 s
[1:9:0410/230310.013460:INFO:keyframe_model.cc(185)] ConvertToActiveTime, returning 6772868844628 bogo-microseconds - 6772872790918 bogo-microseconds - 0 s = -3.94629 s // Correct value

This also shows the difference between monotonic_time and the time from tick_provider; it's different by a good number of digits so no wonder the AW is ahead enough to start instantly.

6772872924246 (tick provider) vs
6772868802345 (montonic_time)
Oh, KeyframeModel::ConvertFromActiveTime incorrectly subtracts time_offset_. Removing that subtraction makes the code work.

Tomorrow's task - proving to myself that I believe that this is the right change to make... and documenting it :D.
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d358adeeefdf6e1fa835a1738ea50ecf6806a51c

commit d358adeeefdf6e1fa835a1738ea50ecf6806a51c
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Fri Apr 13 21:11:14 2018

Animation Worklet: Use the correct time in WorkletAnimation::GetTimeForKeyframeModel

This code used to assume that the local_time_ was equivalent to the
active time, but that isn't true. Active time is local time - start
delay, so the start delay (time_offset_) shouldn't be removed when
converting local_time_ to 'real' time.

Bug:  824782 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I3cc570c4054843bf31cdba0f58917f4566ccd8b1
Reviewed-on: https://chromium-review.googlesource.com/1008062
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550766}
[modify] https://crrev.com/d358adeeefdf6e1fa835a1738ea50ecf6806a51c/cc/animation/keyframe_model.cc
[modify] https://crrev.com/d358adeeefdf6e1fa835a1738ea50ecf6806a51c/cc/animation/keyframe_model.h
[modify] https://crrev.com/d358adeeefdf6e1fa835a1738ea50ecf6806a51c/cc/animation/worklet_animation.cc
[add] https://crrev.com/d358adeeefdf6e1fa835a1738ea50ecf6806a51c/third_party/WebKit/LayoutTests/virtual/threaded/fast/animationworklet/animation-worklet-start-delay-expected.html
[add] https://crrev.com/d358adeeefdf6e1fa835a1738ea50ecf6806a51c/third_party/WebKit/LayoutTests/virtual/threaded/fast/animationworklet/animation-worklet-start-delay.html

Status: Fixed (was: Started)
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d358adeeefdf6e1fa835a1738ea50ecf6806a51c

commit d358adeeefdf6e1fa835a1738ea50ecf6806a51c
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Fri Apr 13 21:11:14 2018

Animation Worklet: Use the correct time in WorkletAnimation::GetTimeForKeyframeModel

This code used to assume that the local_time_ was equivalent to the
active time, but that isn't true. Active time is local time - start
delay, so the start delay (time_offset_) shouldn't be removed when
converting local_time_ to 'real' time.

Bug:  824782 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I3cc570c4054843bf31cdba0f58917f4566ccd8b1
Reviewed-on: https://chromium-review.googlesource.com/1008062
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550766}
[modify] https://crrev.com/d358adeeefdf6e1fa835a1738ea50ecf6806a51c/cc/animation/keyframe_model.cc
[modify] https://crrev.com/d358adeeefdf6e1fa835a1738ea50ecf6806a51c/cc/animation/keyframe_model.h
[modify] https://crrev.com/d358adeeefdf6e1fa835a1738ea50ecf6806a51c/cc/animation/worklet_animation.cc
[add] https://crrev.com/d358adeeefdf6e1fa835a1738ea50ecf6806a51c/third_party/WebKit/LayoutTests/virtual/threaded/fast/animationworklet/animation-worklet-start-delay-expected.html
[add] https://crrev.com/d358adeeefdf6e1fa835a1738ea50ecf6806a51c/third_party/WebKit/LayoutTests/virtual/threaded/fast/animationworklet/animation-worklet-start-delay.html

Sign in to add a comment