cc::AnimationPlayer should not expose Remove|Add for individual animations |
||||
Issue descriptionThe current interface for removing/pausing/aborting Animations to AnimationPlayer is very awkward. It requires the client to provide an animation id: SingleTickerAnimationPlayer::RemoveAnimation(int animation_id); SingleTickerAnimationPlayer::AbortAnimation(int animation_id); SingleTickerAnimationPlayer::PauseAnimation(int animation_id, In reality we never remove/abort individual animations of a single player but do them all at the same time. The current interface requires the client code to keep track of all animation ids just so that it can use them with these methods. I suggests the following we replace these with RemoveAnimations(), AbortAnimations(), PauseAnimations() that operate on all animations on a single player. This means the clients no longer need to keep track of animation id(s) and we can keep animation id as an internal detail of cc animation machinery. We can get rid of these and simplify the client code as well: - blink::KeyframeEffectReadOnly::compositor_animation_ids_ - blink::ScrollAnimatorCompositorCoordinator::compositor_animation_id_ - ui::ThreadedLayerAnimationElement::animaiton_id Additional refactor: Perhaps we should also add SingleTickerAnimationPlayer::AddAnimations to take in an array of animations to be consistent here and get rid of the old one.
,
Feb 7 2018
Another advantage is that a lots of book keeping work would be done once e.g., AnimationTicker::RemoveAnimation() needs to do |UpdateTickingState| after each removal but after the change we remove all animation and then update ticking state once!
,
Feb 15 2018
issue 807667 is to rename the cc side terminology so that this refactoring issue could go more smoothly (fewer naming conflict with blink).
,
Jun 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e73a8251869a9d8982c2a92940b1491ce4af6ed7 commit e73a8251869a9d8982c2a92940b1491ce4af6ed7 Author: Yi Gu <yigu@chromium.org> Date: Fri Jun 29 21:57:11 2018 Remove KeyframeModel ids from animation clients Currently removing/pausing/aborting keyframe models to cc::Animation requires the client to provide the keyframe model id. e.g. Animation::RemoveKeyframeModel(id). Actually in reality we never remove/abort individual keyframe model of an animation but do them all together. This patch removes the ids from the clients so that they don't need to track the keyframe models. This patch is to clear barriers of introducing Group Effects. The non trivial changes on ui animation: Current logic: LayerAnimationSequence owns n LayerAnimationElement which could be either threaded (opacity, transform) or non threaded. Each of the threaded LayerAnimationElements has a corresponding cc::KeyframeModel which is added / removed on the LayerAnimationElement level. e.g. element->AddKeyframeModel(), element->RemoveKeyframeModel(). New logic: Given that threaded keyframe models can only be removed all together, we move the removal logic to the LayerAnimationSequence level. e.g. sequence->RemoveKeyframeModels(). The *add* logic is still on the element level because adding individual keyframe model for each element is allowed. Note that a sequence can contain both threaded and non threaded elements simultaneously. e.g. [threaded, non threaded, threaded]. To make sure that we don't remove the keyframe model from the second threaded element upon finishing the first threaded element, we need to finish the whole sequence to proceed. Also note that the sequence may be repeated infinitely and every time a threaded element is started we create a cc::KeyframeModel for it. That being said, we need to make sure that the existing keyframe models are removed before the sequence gets repeated. Bug: 810003 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_layout_tests_slimming_paint_v2;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:linux_vr;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Change-Id: Ia7dd8dab07a374da82f1619d011a23c31421295b Reviewed-on: https://chromium-review.googlesource.com/1102831 Commit-Queue: Yi Gu <yigu@chromium.org> Reviewed-by: Majid Valipour <majidvp@chromium.org> Reviewed-by: Ali Juma <ajuma@chromium.org> Reviewed-by: Ian Vollick <vollick@chromium.org> Reviewed-by: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#571658} [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/cc/animation/animation.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/cc/animation/animation.h [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/cc/animation/animation_unittest.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/cc/animation/element_animations_unittest.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/cc/animation/keyframe_effect.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/cc/animation/keyframe_effect.h [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/cc/animation/scroll_offset_animations_impl.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/cc/animation/single_keyframe_effect_animation.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/cc/animation/single_keyframe_effect_animation.h [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/cc/animation/worklet_animation.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/cc/animation/worklet_animation.h [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/cc/animation/worklet_animation_unittest.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/cc/test/animation_test_common.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/cc/test/animation_test_common.h [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/cc/trees/layer_tree_host_common_unittest.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/cc/trees/layer_tree_host_unittest_animation.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/chrome/browser/vr/animation.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/chrome/browser/vr/animation.h [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/chrome/browser/vr/animation_unittest.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/chrome/browser/vr/elements/paged_scroll_view.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/chrome/browser/vr/elements/throbber.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/chrome/browser/vr/elements/ui_element.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/chrome/browser/vr/elements/ui_element.h [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/chrome/browser/vr/ui_scene_creator.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/third_party/blink/renderer/core/animation/compositor_animations.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/third_party/blink/renderer/core/animation/compositor_animations.h [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/third_party/blink/renderer/core/animation/compositor_animations_test.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/third_party/blink/renderer/core/animation/keyframe_effect.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/third_party/blink/renderer/core/animation/keyframe_effect.h [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/third_party/blink/renderer/platform/animation/compositor_animation.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/third_party/blink/renderer/platform/animation/compositor_animation.h [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/third_party/blink/renderer/platform/graphics/graphics_layer_test.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/third_party/blink/renderer/platform/scroll/scroll_animator_compositor_coordinator.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/ui/compositor/layer_animation_element.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/ui/compositor/layer_animation_sequence.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/ui/compositor/layer_animation_sequence.h [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/ui/compositor/layer_animator.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/ui/compositor/layer_animator.h [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/ui/compositor/layer_animator_unittest.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/ui/compositor/layer_threaded_animation_delegate.h [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/ui/compositor/test/test_layer_animation_delegate.cc [modify] https://crrev.com/e73a8251869a9d8982c2a92940b1491ce4af6ed7/ui/compositor/test/test_layer_animation_delegate.h
,
Jul 3
We still need to support adding animation (new name KeyframeModel) individually as requested by the clients. No further work is needed for this bug ATM.
,
Jul 4
The assumption that we never remove individual keyframe model of an animation is false. Given that a SingleKeyframeEffectAnimation may not own all the effects for a given target, we may have an opacity animation and a transform animation running on the same target simultaneously. Therefore removing only one of them at a specific time is legit. To avoid clients tracking the ids, we could remove all keyframe models with the same property. Currently it's legal to have two opacity animations on the same target codewise, e.g. 1 running 1 waiting for target availability. But maybe we should forbid this behavior in cc.
,
Jul 4
To be clear, the opacity animation and transform animation running on the same target may belong to the same keyframe effect and removing only one of them is legit.
,
Jul 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4e1dce3d95e3c4d761dee0544ecbbf666a663944 commit 4e1dce3d95e3c4d761dee0544ecbbf666a663944 Author: Yi Gu <yigu@chromium.org> Date: Wed Jul 04 20:52:15 2018 Revert "Remove KeyframeModel ids from animation clients" This reverts commit e73a8251869a9d8982c2a92940b1491ce4af6ed7. Reason for revert: Reverting this patch as it regressed ui animation on ChromeOS. Problem has been located. Fix it in a follow up patch. Original change's description: > Remove KeyframeModel ids from animation clients > > Currently removing/pausing/aborting keyframe models to cc::Animation > requires the client to provide the keyframe model id. e.g. > Animation::RemoveKeyframeModel(id). > > Actually in reality we never remove/abort individual keyframe model of > an animation but do them all together. This patch removes the ids from > the clients so that they don't need to track the keyframe models. > > This patch is to clear barriers of introducing Group Effects. > > The non trivial changes on ui animation: > > Current logic: LayerAnimationSequence owns n LayerAnimationElement which > could be either threaded (opacity, transform) or non threaded. Each of > the threaded LayerAnimationElements has a corresponding > cc::KeyframeModel which is added / removed on the LayerAnimationElement > level. e.g. element->AddKeyframeModel(), element->RemoveKeyframeModel(). > > New logic: Given that threaded keyframe models can only be removed all > together, we move the removal logic to the LayerAnimationSequence level. > e.g. sequence->RemoveKeyframeModels(). The *add* logic is still on the > element level because adding individual keyframe model for each element > is allowed. > Note that a sequence can contain both threaded and non threaded elements > simultaneously. e.g. [threaded, non threaded, threaded]. To make sure > that we don't remove the keyframe model from the second threaded element > upon finishing the first threaded element, we need to finish the whole > sequence to proceed. Also note that the sequence may be repeated > infinitely and every time a threaded element is started we create a > cc::KeyframeModel for it. That being said, we need to make sure that the > existing keyframe models are removed before the sequence gets repeated. > > Bug: 810003 > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_layout_tests_slimming_paint_v2;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:linux_vr;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel > Change-Id: Ia7dd8dab07a374da82f1619d011a23c31421295b > Reviewed-on: https://chromium-review.googlesource.com/1102831 > Commit-Queue: Yi Gu <yigu@chromium.org> > Reviewed-by: Majid Valipour <majidvp@chromium.org> > Reviewed-by: Ali Juma <ajuma@chromium.org> > Reviewed-by: Ian Vollick <vollick@chromium.org> > Reviewed-by: Stephen McGruer <smcgruer@chromium.org> > Cr-Commit-Position: refs/heads/master@{#571658} TBR=vollick@chromium.org,ajuma@chromium.org,majidvp@chromium.org,yigu@chromium.org,smcgruer@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 810003 Change-Id: I4e7bdc75ad5ad06a6f0079d19a2b867d207a518a Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_layout_tests_slimming_paint_v2;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:linux_vr;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Reviewed-on: https://chromium-review.googlesource.com/1126361 Commit-Queue: Yi Gu <yigu@chromium.org> Reviewed-by: Yi Gu <yigu@chromium.org> Cr-Commit-Position: refs/heads/master@{#572652} [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/cc/animation/animation.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/cc/animation/animation.h [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/cc/animation/animation_unittest.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/cc/animation/element_animations_unittest.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/cc/animation/keyframe_effect.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/cc/animation/keyframe_effect.h [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/cc/animation/scroll_offset_animations_impl.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/cc/animation/single_keyframe_effect_animation.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/cc/animation/single_keyframe_effect_animation.h [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/cc/animation/worklet_animation.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/cc/animation/worklet_animation.h [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/cc/animation/worklet_animation_unittest.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/cc/test/animation_test_common.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/cc/test/animation_test_common.h [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/cc/trees/layer_tree_host_common_unittest.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/cc/trees/layer_tree_host_unittest_animation.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/chrome/browser/vr/animation.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/chrome/browser/vr/animation.h [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/chrome/browser/vr/animation_unittest.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/chrome/browser/vr/elements/paged_scroll_view.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/chrome/browser/vr/elements/throbber.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/chrome/browser/vr/elements/ui_element.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/chrome/browser/vr/elements/ui_element.h [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/chrome/browser/vr/ui_scene_creator.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/third_party/blink/renderer/core/animation/compositor_animations.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/third_party/blink/renderer/core/animation/compositor_animations.h [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/third_party/blink/renderer/core/animation/compositor_animations_test.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/third_party/blink/renderer/core/animation/keyframe_effect.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/third_party/blink/renderer/core/animation/keyframe_effect.h [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/third_party/blink/renderer/platform/animation/compositor_animation.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/third_party/blink/renderer/platform/animation/compositor_animation.h [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/third_party/blink/renderer/platform/graphics/graphics_layer_test.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/third_party/blink/renderer/platform/scroll/scroll_animator_compositor_coordinator.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/ui/compositor/layer_animation_element.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/ui/compositor/layer_animation_sequence.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/ui/compositor/layer_animation_sequence.h [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/ui/compositor/layer_animator.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/ui/compositor/layer_animator.h [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/ui/compositor/layer_animator_unittest.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/ui/compositor/layer_threaded_animation_delegate.h [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/ui/compositor/test/test_layer_animation_delegate.cc [modify] https://crrev.com/4e1dce3d95e3c4d761dee0544ecbbf666a663944/ui/compositor/test/test_layer_animation_delegate.h
,
Jul 9
Actually we may remove individual keyframe model as per #6. In addition, we may have different keyframe models of the same target property within the same keyframe effect as long as they don't start together, e.g. scroll offset animation. Therefore when a property is done animating we cannot remove the unstarted keyframe models with the same property. Close this bug as won't fix as grouping keyframe models to remove is not always correct. |
||||
►
Sign in to add a comment |
||||
Comment 1 by yigu@chromium.org
, Feb 7 2018Owner: yigu@chromium.org