cc:KeyframeModel::Pause does not correctly take into account start-delay and multiple pause/unpause |
||
Issue descriptionThe current logic to convert the pause time offset to monotonic time is incorrect. The resulting pause time and monotonic time should have have the same base clock. This is important since we compute total_paused_duration_ as difference between the two Also Blink provides its animation's current time as pause offset which maps to the concept of local time in cc. So to convert it to monotonic time we don't need to include time_offset_. Instead to convert the client pause time to monotonic time, we should not add the |time_offset_| but add |total_pause_duration_| . Without this correction animations with start-delay or multiple pause/unpause cycles would lead to incorrect pause time calculation. I suspect these issues flew under the radar mainly because pausing is only ever used for simple test cases not involving either of these condition.
,
Jan 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f7bf7332194a4a475e570a02afe5625aaf48c30c commit f7bf7332194a4a475e570a02afe5625aaf48c30c Author: Majid Valipour <majidvp@chromium.org> Date: Wed Jan 02 18:34:17 2019 [cc] Pausing Keyframes now correctly handles delay and multiple pauses Existing pause time application in cc::KeyframeModel is incorrect. To convert the client pause time to monotonic time, we used to add the |time_offset_| (1) but not add |total_pause_duration_| (2). The new logic now excludes (1) and adds (2). This ensures pause time and monotonic time have the same base clock. The original conversion was incorrect because: a. Pause time should have the same base clock as monotonic time since we compute total_paused_duration_ as difference between the two. b. Blink provides its animation's current time as pause offset which maps to the concept of local time in cc. So to convert it to monotonic time we don't need to include time_offset_. Without this correction animations with start-delay or multiple pause/unpause cycles would lead to incorrect pause time calculation. I suspect these issues flew under the radar mainly because pausing is only ever used for simple test cases not involving either of these conditions. Bug: 840364 TEST: cc/animation/keyframe_model_unittest.cc Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel Change-Id: Id52ca7a3f3d63899864b7153470c7ef64c582d29 Reviewed-on: https://chromium-review.googlesource.com/c/1045126 Commit-Queue: Majid Valipour <majidvp@chromium.org> Reviewed-by: Stephen McGruer <smcgruer@chromium.org> Reviewed-by: Yi Gu <yigu@chromium.org> Cr-Commit-Position: refs/heads/master@{#619437} [modify] https://crrev.com/f7bf7332194a4a475e570a02afe5625aaf48c30c/cc/animation/keyframe_model.cc [modify] https://crrev.com/f7bf7332194a4a475e570a02afe5625aaf48c30c/cc/animation/keyframe_model_unittest.cc
,
Jan 7
|
||
►
Sign in to add a comment |
||
Comment 1 by flackr@chromium.org
, May 8 2018