Group effects support in cc::AnimationPlayer [chromium/src : master]
Yi Gu (Gerrit)
Yi Gu would like Stephen McGruer and Robert Flack to review this change.
Group effects support in cc::AnimationPlayer
The current cc/animations logic assumes a single animation has a single
keyframe effect and can only affect a single layer. To enable animations
with multiple keyframe effects, cc::AnimationPlayer need to support
multiple AnimationTickers which owns the keyframe effects.
Currently there is a 1:1 relationship between AnimationPlayer and
AnimationTicker. This patch is to extend it to 1:n. Here is a summary of
changes:
- Introduce a sub-class of AnimationPlayer, a.k.a SingleAnimationPlayer,
to handle the existing logic (single effect). SingleAnimationPlayer owns
only one AnimationTicker as the AnimationPlayer does today.
- Currently a AnimationTicker is created upon creating AnimationPlayer.
In this patch, tickers are created separately and added to the player
afterwards. Tickers that the player owns may belong to different targets
therefore the player needs to coordinate with AnimationHost regarding
this situation.
- Adjust existing unit tests to according to the changes above.
Bug:
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Ibd2ed2f396effec9fbc206ff52c7b94474342d39
---
M cc/animation/BUILD.gn
M cc/animation/animation_host.cc
M cc/animation/animation_host.h
M cc/animation/animation_host_perftest.cc
M cc/animation/animation_id_provider.cc
M cc/animation/animation_id_provider.h
M cc/animation/animation_player.cc
M cc/animation/animation_player.h
M cc/animation/animation_player_unittest.cc
M cc/animation/animation_ticker.cc
M cc/animation/animation_ticker.h
M cc/animation/animation_timeline.cc
M cc/animation/animation_timeline_unittest.cc
M cc/animation/element_animations_unittest.cc
M cc/animation/scroll_offset_animations_impl.cc
M cc/animation/scroll_offset_animations_impl.h
A cc/animation/single_animation_player.cc
A cc/animation/single_animation_player.h
M cc/animation/worklet_animation_player.cc
M cc/animation/worklet_animation_player.h
M cc/test/animation_test_common.cc
M cc/test/animation_test_common.h
M cc/test/animation_timelines_test_common.cc
M cc/test/animation_timelines_test_common.h
M cc/test/layer_tree_test.cc
M cc/trees/layer_tree_host_common_unittest.cc
M cc/trees/layer_tree_host_unittest_animation.cc
M third_party/WebKit/Source/platform/animation/CompositorAnimationPlayer.cpp
M third_party/WebKit/Source/platform/animation/CompositorAnimationPlayer.h
M third_party/WebKit/Source/platform/animation/CompositorAnimationPlayerTest.cpp
M ui/compositor/layer_animator.cc
M ui/compositor/layer_animator.h
M ui/compositor/layer_owner_unittest.cc
M ui/compositor/layer_unittest.cc
34 files changed, 1,766 insertions(+), 863 deletions(-)
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Yi Gu (Gerrit)
Thanks Majid for the review! Add Rob && Stephen for more feedback.
PTAL at the updated patch.
17 comments:
File cc/animation/animation_player.h:
class CC_ANIMATION_EXPORT Animationnit: Not sure why this is relevant here.
Done
Patch Set #10, Line 49:
bool IsElementAttached(ElementId id)I don't think this method make sense here. There is no special […]
Done
ditto. […]
Done
why is this event need to be exposed?
It's currently used in AnimationPlayerTest. Two elements may have two tickers each and all the 4 tickers are stored in one AnimationPlayer. Therefore when we attach an element we need to specify which ticker.
We are using 4 different preposition With/To/Of/From to refer to tickers. […]
Done
Patch Set #10, Line 95:
imationDelegate routwhy this needs to be exposed? Tests should just call UpdateState.
Done
File cc/animation/animation_player.cc:
return !element_to_ticker_id_map_.empty();
}
scoped_refptr<ElementAnimations> AnimationPlayer::element_animations(
int ticker_iThis should be a method called RegisterTickers. It is used several places.
Done
Patch Set #10, Line 134:
UnregisterTickers/UnregisterTicker/UnregisterTickers/
Done
ayer_impl->AddTicker(std::move(to_add));
}
}
void AnimationPlayer::RemoveDetachedTickersFromImplThread(
AnimationPlayer* player_impl) const {
IdToTickerMap& tickers_impl = player_impl->id_to_ticker_map_;This seems to be the "detach" equivalent of "AttachElementWithTicker". […]
Done
Patch Set #10, Line 300:
time, event.target_property, eThis method seem unnecessary.
Done
This seems unnecessary. […]
Done
should we DCHECK that this is a valid ID?
Done
File cc/animation/animation_player_unittest.cc:
Patch Set #10, Line 24:
player_ = SingleAnimationPlayer::Create(player_id_);So this tests is creating a SingleAnimationPlayer but then it uses AnimationPlayer methods to […]
As discussed, player_ now has type SingleAnimationPlayer instead of AnimationPlayer.
File cc/animation/animation_ticker.h:
Patch Set #10, Line 49:
:unique_ptr<AnimationTicker> CreaIt seems that we always call SetAnimationPlayer even when we have a player passed in […]
Done
File cc/animation/animation_ticker.cc:
Patch Set #10, Line 41:
id AnimationTicker::SetNeedsPushProperties() {This is odd. Why is the impl side ticker is created with main thread player and then […]
Done
nit: s/size/side/
Done
Does not this detach all elements for the player?
Done
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Majid Valipour (Gerrit)
8 comments:
File cc/animation/animation_player.h:
ElementToTickerIdMap element_to_ticker_id_map_;
IdToTickerMap id_to_ticker_map_;I think it will be useful to add some documentation on when a ticker is added to either map.
and how they relate to each other.My understanding based on looking at the code is that
1) a ticker is added/removed to the
id_to_ticker_map_ when AddTicker/RemoveTicker is called.
2) a ticker is added/removed to element_to_ticker_id_map_ when AttachElementFrom/DetachElementFrom are called.From above two and how add, attach works I understand that it is possible for
a ticker to be in id_to_ticker_map_ but not in element_to_ticker_id_map_
but not possible otherwise?Is the above accurate?
File cc/animation/animation_player.cc:
nit: please use a more readable name than x, y.
Patch Set #12, Line 147:
x.firstI think it will be more readable if we create local variable aliases for
x.first and x.second that have better names and types. This is also encouraged
by style guide [1]e.g.,
int element_id = x.first;
std::unordered_set<int>& ticker_ids = x.second;
File cc/animation/animation_player_unittest.cc:
Patch Set #10, Line 24:
player_ = SingleAnimationPlayer::Create(player_id_);
As discussed, player_ now has type SingleAnimationPlayer instead of AnimationPlayer.
I agree that it makes sense to use SingleAnimationPlayer in other tests in this
patch to avoid unnecessary churn in unrelated test code. But in this particular
instance, I don't think it is the right decision.
This unit test is meant to test the AnimationPlayer logic. Most of existing tests
here assume a single ticker instance. This is why on surface it is tempting to use "SingleAnimationPlayer" and not change the tests. But this makes the tests more
indirect. I feel in this unit test we should opt for directly testing animation player
at the cost of updating the tests to use its new API.
Having said that, operationally I think the pattern of:
- create a ticker
- add ticker to player
- test as before but use local ticker instance instead of player_->animation_ticker()should pretty much cover most of the changes you need to do. It is a bit of
work but I think it is reasonable.
File cc/animation/animation_ticker.cc:
Patch Set #12, Line 26:
animation_player_hmmm, shouldn't this DCHECK always fail?
File cc/animation/single_animation_player.h:
Patch Set #12, Line 59:
GetUniqueTickernit: I don't think "Unique" is accurate and meaningful here.
SingleAnimationPlayer has one ticker by definition. So why not just say
"GetTicker" which obviously refers to the expected ticker.
File cc/animation/single_animation_player.cc:
Patch Set #12, Line 37:
DCHECK(!id_to_ticker_map_.empty());should actually verify that we have exactly "1" ticker.
scoped_refptr<SingleAnimationPlayer> player =
SingleAnimationPlayer::Create(id());This creates a player with a single ticker but that ticker id is different.
Don't we need to make sure that the impl side ticker has the same id?
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Yi Gu (Gerrit)
Thanks Majid for the review! PTAL at the updated patch.
I'm working on animation_player_unittest right now as we discussed yesterday. It may take some time to rewrite those tests of SingleAnimationPlayer so I just want to upload this version first so you could review.
Thanks.
7 comments:
// A ticker is added/removed to the id_to_ticker_map_ when
// AddTicker/RemoveTicker is calI think it will be useful to add some documentation on when a ticker is added to either map. […]
That is exactly how it works. Added documentation as suggested.
nit: please use a more readable name than x, y.
Done
Patch Set #12, Line 147:
er_id.fI think it will be more readable if we create local variable aliases for […]
Done
File cc/animation/animation_ticker.cc:
hmmm, shouldn't this DCHECK always fail?
It should fail. I missed it because the current tests are creating tickers in the following way:
player_->AddTicker(base::MakeUnique<AnimationTicker>(id));
which sets the animation_player_ prior to executing the DCHECK.
I've removed the DCHECK here.
File cc/animation/single_animation_player.h:
Patch Set #12, Line 59:
GetTicker() connit: I don't think "Unique" is accurate and meaningful here. […]
Done
File cc/animation/single_animation_player.cc:
Patch Set #12, Line 37:
AddTicker(base::MakeUnique<Animatio
should actually verify that we have exactly "1" ticker.
Done
DCHECK_EQ(id_to_ticker_map_.size(), 1u);
return id_to_ticker_map_.begin()->secondThis creates a player with a single ticker but that ticker id is different. […]
My mistake. I added an overloaded constructor that takes ticker_id as an extra parameter to address this problem.
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Yi Gu (Gerrit)
Patch Set 13:
(7 comments)
Thanks Majid for the review! PTAL at the updated patch.
I'm working on animation_player_unittest right now as we discussed yesterday. It may take some time to rewrite those tests of SingleAnimationPlayer so I just want to upload this version first so you could review.
Thanks.
The unit test file has been updated. PTAL. Thanks!
Majid Valipour (Gerrit)
This is very close! Thanks for sticking with it.
Some final comments but mostly nits.
17 comments:
File cc/animation/animation_player.cc:
Patch Set #14, Line 59:
playernit: s/player/ticker/
Patch Set #14, Line 126:
GetTickerById(ticker_id)Instead of calling it 3 times we should call once and hold onto the result.
Patch Set #14, Line 138:
GetTickerById(ticker_id)ditto. call once and hold onto the result.
Patch Set #14, Line 187:
it->secondnit: This is used 4 times. I think it helps to give it a name and type to improve readability.
Patch Set #14, Line 190:
it->second->element_animations()Do we actually need to care for this? Why don't we just call DetachElementFromTicker and RemoveTicker unconditionally.
Patch Set #14, Line 195:
it = tickers_impl.erase(it);why is this needed? Doesn't the DetachElementForTicker remove the map entry?
Patch Set #14, Line 261:
id_tickernit: this name is a bit confusing. I suggest perhaps renaming to |id_ticker_pair|
to make it clear or just kv as you have used elsewhere.
File cc/animation/animation_player_unittest.cc:
why is this removed?
Patch Set #14, Line 140:
AnimationIdProvider::NextTickerId()nit: why not just use a constant value e.g., 1 and remove the dependency on
AnimationIdProvider in tests.
File cc/animation/element_animations_unittest.cc:
Patch Set #14, Line 59:
SingleAnimationPlayerIs this true? We don't specifically test SingleAnimationPlayer there so I think the old
comment is more accurate.
File cc/animation/single_animation_player.h:
Patch Set #14, Line 26:
the clients and cc side animationsI think the current wording is not clear what you mean by clients.
Maybe rephrase as "cc animations clients and cc".Patch Set #14, Line 27:
supportnit: s/support/supported/
File cc/test/animation_timelines_test_common.cc:
Patch Set #14, Line 415:
(SingleAnimationPlayer*)nit: I think this needs to be a static cast.
File cc/test/layer_tree_test.cc:
(((SingleAnimationPlayer*)(it.get()))
->animation_ticker()
->HasTickingAnimation())I think we should be able to replace this with checking
"TickingAnimationsCount" and avoid the cast.Patch Set #14, Line 835:
AnimationPlayerShould this instead take in SingleAnimationPlayer?
Patch Set #14, Line 839:
(SingleAnimationPlayer*)(The cast here seems unnecessary.
File cc/trees/layer_tree_host_unittest_animation.cc:
Patch Set #14, Line 60:
(SingleAnimationPlayer*)replace with c++ static_cast<SingleAnimationPlayer*> as it is preferred. here
and elsewhere. See style guide [1][1] https://google.github.io/styleguide/cppguide.html#Casting
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Yi Gu (Gerrit)
Hi Majid. Patch has been updated. PTAL. Thanks!
In addition to the changes based on your comments, I also made the following change:
Previously I didn't call SetNeedsPushProperties upon adding/removing tickers. But I think it's necessary. The current logic of setting needs_push_properties from AnimationPlayer requires animation_timeline_ which makes sense. However, animation_timeline_ may not be set for the player when adding/removing tickers. So I changed "DCHECK(animation_timeline_)" to "if (animation_timeline_)" so that we only SetNeedsPushProperties upon adding/removing tickers if the player is attached to timeline. WDYT?
17 comments:
File cc/animation/animation_player.cc:
Patch Set #14, Line 59:
tickernit: s/player/ticker/
Done
Patch Set #14, Line 126:
nTicker* ticker = GetTic
Instead of calling it 3 times we should call once and hold onto the result.
Done
Patch Set #14, Line 138:
ticker->has_attached_ele
ditto. call once and hold onto the result.
Done
Patch Set #14, Line 187:
l.begin(),
nit: This is used 4 times. I think it helps to give it a name and type to improve readability.
Done
Patch Set #14, Line 190:
const AnimationTicker* tickerDo we actually need to care for this? Why don't we just call DetachElementFromTicker and RemoveTicke […]
DetachElementForTicker requires the ticker being "attached". However, we do not need this condition check for RemoveTicker.
why is this needed? Doesn't the DetachElementForTicker remove the map entry?
You're right. It's erased in RemoveTicker. In addition, to avoid modifying the iterator on the fly, it's better to keep a list of tickers to be removed and remove them afterwards.
Added a test for this.Patch Set #14, Line 261:
:UpdateStnit: this name is a bit confusing. I suggest perhaps renaming to |id_ticker_pair| […]
Done
File cc/animation/animation_player_unittest.cc:
why is this removed?
Removed for SAP test. Added back.
nit: why not just use a constant value e.g., 1 and remove the dependency on […]
Done
File cc/animation/element_animations_unittest.cc:
Is this true? We don't specifically test SingleAnimationPlayer there so I think the old […]
Done
File cc/animation/single_animation_player.h:
Patch Set #14, Line 26:
the cc animation clients and cc beI think the current wording is not clear what you mean by clients. […]
Done
Patch Set #14, Line 27:
supportnit: s/support/supported/
Done
File cc/test/animation_timelines_test_common.cc:
Patch Set #14, Line 415:
timeline_impl_->GetPlaye
nit: I think this needs to be a static cast.
(it.get()->TickingAnimationsCount()) {
has_unfinished_animation = true;
break;I think we should be able to replace this with checking […]
Done
Patch Set #14, Line 835:
HECK(main_task_
Should this instead take in SingleAnimationPlayer?
Done
The cast here seems unnecessary.
Done
File cc/trees/layer_tree_host_unittest_animation.cc:
Patch Set #14, Line 60:
timeline_impl_->GetPlayereplace with c++ static_cast<SingleAnimationPlayer*> as it is preferred. here […]
Done
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Majid Valipour (Gerrit)
lgtm with nits.
11 comments:
File cc/animation/animation_player.cc:
Patch Set #15, Line 161:
for_each(ticker_ids.begin(), ticker_ids.end(), [&](auto& ticker_id) {nit: may be better to use a for range loop here instead of for_each.
for_each(tickers_to_remove.begin(), tickers_to_remove.end(),
[&](auto& ticker_id) {nit: since we are looping between being() and end(), I don't really see an advantage on
using lambda and for_each. IMHO a normal for range loop is simpler, and more readable
in this case.for (auto& ticker_id : tickers_to_remove)
player_impl->RemoveTicker(ticker_id);
VS.for_each(tickers_to_remove.begin(), tickers_to_remove.end(),
[&](auto& ticker_id) { player_impl->RemoveTicker(ticker_id); });for_each(id_to_ticker_map_.begin(), id_to_ticker_map_.end(),
[&](auto& id_ticker) {nit: may be better to use a for range loop here instead of for_each + lambda.
for_each(id_to_ticker_map_.begin(), id_to_ticker_map_.end(),
[&](auto& id_ticker_pair) {nit: same concern on using a simple for range loop.
for_each(id_to_ticker_map_.begin(), id_to_ticker_map_.end(),
[&](auto& id_ticker) {nit: ditto.
Patch Set #15, Line 388:
id_to_ticker_map_.find(ticker_id)nit: we should avoid doing this lookup twice. e.g.,
auto it = id_to_ticker_map_.find(ticker_id);
return it != id_to_ticker_map_.end() ? it->second.get() : nullptr;
File cc/animation/animation_player_unittest.cc:
Patch Set #15, Line 137:
TickerTimelinenit:s/TickerTimeline/TickerAndTimeline/
Patch Set #15, Line 139:
DCHECK(player_);no need to DCHECK in unit tests.
ADD_FAILURE() << "ticker->needs_push_properties() expected to be "
<< needs_push_properties;nit: why not just use EXPECT_EQ(ticker->needs_push_properties(), needs_push_properties)?
Patch Set #15, Line 156:
return resnit: No need to return anything. Any failure that result in this being false
already results in a test failure.Patch Set #15, Line 172:
EXPECT_TRUEnit: See my comment above. No need to DCHECK here.
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Majid Valipour (Gerrit)
1 comment:
Patch Set #15, Line 12:
which owns the keyframe effectsnit: this is a bit vague and incorrect as ticker so own keyframe effects. Maybe
rewrite as "each corresponding to one keyframe effect"
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Yi Gu (Gerrit)
Thanks Majid!
Ian, could you please take a look at this patch especially the ui/compositor part? Need owner's approval. Thanks:)
12 comments:
Patch Set #15, Line 12:
each corresponding to one keyfrnit: this is a bit vague and incorrect as ticker so own keyframe effects. Maybe […]
Done
File cc/animation/animation_player.cc:
nit: may be better to use a for range loop here instead of for_each.
Done
player_impl->GetTickerById(ticker->id())) {
ticker->PushPropertiesTo(tinit: since we are looping between being() and end(), I don't really see an advantage on […]
Done
nit: may be better to use a for range loop here instead of for_each + lambda.
Done
void AnimationPlayer::AddToTicking() {
nit: same concern on using a simple for range loop.
Done
"AnimationPlayer{id=%d, element_id=%s, animations=[%s]}", id_,
GetTickerById(ticker_id)->enit: ditto.
Done
nit: we should avoid doing this lookup twice. e.g., […]
Done
File cc/animation/animation_player_unittest.cc:
Patch Set #15, Line 137:
TickerAndTimelnit:s/TickerTimeline/TickerAndTimeline/
Done
Patch Set #15, Line 139:
AnimationTicker*
no need to DCHECK in unit tests.
Done
ed_refptr<AnimationPlayer> player_impl_;
int group_id_;
nit: why not just use EXPECT_EQ(ticker->needs_push_properties(), needs_push_properties)?
Done
Patch Set #15, Line 156:
PECT_FALSEnit: No need to return anything. Any failure that result in this being false […]
Done
Patch Set #15, Line 172:
timeline_im
nit: See my comment above. No need to DCHECK here.
Done
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Yi Gu (Gerrit)
Yi Gu has assigned a change to Robert Flack.
Group effects support in cc::AnimationPlayer
The current cc/animations logic assumes a single animation has a single
keyframe effect and can only affect a single layer. To enable animations
with multiple keyframe effects, cc::AnimationPlayer need to support
multiple AnimationTickers each corresponding to one keyframe effect.
M cc/test/layer_tree_test.h
M cc/trees/layer_tree_host_common_unittest.cc
M cc/trees/layer_tree_host_unittest_animation.cc
M third_party/WebKit/Source/platform/animation/CompositorAnimationPlayer.cpp
M third_party/WebKit/Source/platform/animation/CompositorAnimationPlayer.h
M third_party/WebKit/Source/platform/animation/CompositorAnimationPlayerTest.cpp
M ui/compositor/layer_animator.cc
M ui/compositor/layer_animator.h
M ui/compositor/layer_owner_unittest.cc
M ui/compositor/layer_unittest.cc
35 files changed, 1,937 insertions(+), 935 deletions(-)
Yi Gu (Gerrit)
My previous request seems ambiguous. Ian, PTAL at the ui/compositor part. I've asked Rob to review the rest. Thanks!
Majid Valipour (Gerrit)
Patch set 17:Code-Review +1
Ian Vollick (Gerrit)
Ian Vollick (Gerrit)
Patch Set 17: Code-Review+1
ui/compositor and platform/ lgtm
Robert Flack (Gerrit)
Do we have a bug for this?
5 comments:
File cc/animation/animation_host.cc:
Patch Set #17, Line 573:
return;This seems wrong. If we call add and remove multiple times (i.e. for each AnimationTicker which is ticking) we will add the player to the ticking_players_ list once and then think that the player is no longer ticking after the first ticker stops ticking and calls RemoveFromTicking.
File cc/animation/animation_id_provider.cc:
Patch Set #17, Line 35:
return g_next_ticker_id.GetNext() + 1;It seems to me that since we always get tickers from players that the ticker id's don't need to be globally unique. I also assume that we rarely delete tickers without also deleting players and often there will only be one ticker so perhaps it would probably make more sense to save a ticker index and use a flat array of tickers on the player to reduce memory consumption and lookup time.
File cc/animation/animation_player.h:
Patch Set #17, Line 160:
but not possible otherwisenit: "but the reverse is not possible", as is the otherwise is confusing as it seems to refer to some state other than one which has not been stated.
File cc/animation/animation_player.cc:
Patch Set #17, Line 320:
if (!animation_timeline_)Why can the timeline be NULL now?
File cc/animation/animation_ticker.cc:
Patch Set #17, Line 20:
: id_(id),Need animation_player_(NULL) ?
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Yi Gu (Gerrit)
Thanks Rob. PTAL.
5 comments:
File cc/animation/animation_host.cc:
Patch Set #17, Line 573:
ticking_players_.push_back(player);This seems wrong. If we call add and remove multiple times (i.e. […]
Added is_ticking_player_ in AnimationPlayer so that we only add the player once whilst remove it iff it doesn't have any ticking ticker.
File cc/animation/animation_id_provider.cc:
It seems to me that since we always get tickers from players that the ticker id's don't need to be g […]
Done
File cc/animation/animation_player.h:
Patch Set #17, Line 160:
to_ticker_id_map_;nit: "but the reverse is not possible", as is the otherwise is confusing as it seems to refer to som […]
Done
File cc/animation/animation_player.cc:
Patch Set #17, Line 320:
int count = 0;
Why can the timeline be NULL now?
With the old logic we only need to set needs_push_properties in cases that the animation_timeline has been available.
With the new logic, we may add/remove tickers from a player and whenever we do it we need to set needs_push_properties. However, by the time we update tickers we may not set any animation_timeline to the player yet.
File cc/animation/animation_ticker.cc:
Patch Set #17, Line 20:
: animation_player_(),Need animation_player_(NULL) ?
Done
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Robert Flack (Gerrit)
3 comments:
File cc/animation/animation_player.h:
Patch Set #19, Line 123:
int NextTickerId() { return ++next_ticker_id_; }Can this just be tickers_.size()?
File cc/animation/animation_player.cc:
Patch Set #19, Line 278:
if (ticker->is_ticking())nit: Maybe use a count of ticking tickers to avoid looping?
Patch Set #19, Line 385:
[&](const auto& ticker) { return ticker_id == ticker->id(); });I thought the ticker_id was just going to be the index into the tickers vector? Do you expect lots of tickers to be created for a single animation player over time?
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Yi Gu (Gerrit)
Thanks Rob. PTAL.
3 comments:
File cc/animation/animation_player.h:
Patch Set #19, Line 123:
unsigned NextTickerId() { return tickers_.size() + 1; }Can this just be tickers_. […]
An incremental member makes test easier to write but I guess it's not worth it. Changed to tickers_.size() + 1 to match behavior of AnimationIdProvider.
File cc/animation/animation_player.cc:
Patch Set #19, Line 278:
void AnimationPlayer::AnimationRemovedFromTicking() {
nit: Maybe use a count of ticking tickers to avoid looping?
Done
I thought the ticker_id was just going to be the index into the tickers vector? Do you expect lots o […]
We don't create/remove tickers on the fly ATM. Updated.
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Robert Flack (Gerrit)
3 comments:
File cc/animation/animation_player.h:
Patch Set #20, Line 48:
ElementId element_id_of_ticker(unsigned ticker_id) const;s/unsigned/size_t
Patch Set #20, Line 123:
unsigned NextTickerId() { return tickers_.size() + 1; }Why + 1? giving it the same id as index would mean we can directly map to the index in tickers_.
File cc/animation/animation_player.cc:
Patch Set #20, Line 381:
[&](const auto& ticker) { return ticker_id == ticker->id(); });We can't randomly remove a ticker without corrupting the indices of the rest, we should either only remove all of them or change the tickers array to be nullable pointers so that we can delete without removing from the array.
Does this happen? Do we need to remove random tickers from the array?
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Yi Gu (Gerrit)
3 comments:
File cc/animation/animation_player.h:
Patch Set #20, Line 48:
ElementId element_id_of_ticker(size_t ticker_id) const;s/unsigned/size_t
Done
Why + 1? giving it the same id as index would mean we can directly map to the index in tickers_.
All other ids (player id, group id, animation id) are 1-indexed. +1 to be consistent. But using 0-indexed here may be OK given the circumstances. Done.
File cc/animation/animation_player.cc:
We can't randomly remove a ticker without corrupting the indices of the rest, we should either only […]
RemoveTicker was useful because I was thinking of adding support for add/remove tickers on the fly. Given that we probably won't need this feature, I've taken it out so that we will never remove tickers from player_impl. Rather, we create new player if necessary and push properties to its corresponding new player_impl.
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Robert Flack (Gerrit)
9 comments:
File cc/animation/animation_player.h:
Patch Set #21, Line 48:
size_tnit: We should use a typedef so that it's obvious where used that the number represents a ticker id, also because size_t is a bit of a strange type for an id but makes sense given that it maps to an index.
Patch Set #21, Line 77:
size_t ticker_id);I worry that the meaning of ticker_id is not obvious for clients of animation. What does it mean? When do you need a new ticker? At the very least I think this needs to be carefully explained in comments, but perhaps we should consider alternative names as well such as effect number/index or have an explicit API for adding a new effect (ticker).
+vollick do you have any suggestions here?
Patch Set #21, Line 80:
size_t ticker_id);With all of these *ForTicker methods, how many of them make sense to actually require a ticker_id? How often would we know the ticker id being paused? I think it may make sense to just loop over each ticker and call Pause on it.
Patch Set #21, Line 159:
numbers/number/count
File cc/animation/animation_player.h:
All other ids (player id, group id, animation id) are 1-indexed. +1 to be consistent. […]
Yes, since it's used as an index, being 0-based makes more sense. We could even call it ticker_index to be clear about this but callers don't need to know the distinction between id and index anyways.
File cc/animation/animation_player.cc:
Patch Set #21, Line 240:
size_t ticker_id) {This only seems to be used by line 235, I'd refer just merging this into the loop above than add additional per ticker methods.
Patch Set #21, Line 252:
is_ticking_player_ = true;I think is_ticking_player_ can be replaced by checking whether ticking_tickers_number_ is greater than 0.
Patch Set #21, Line 333:
std::string AnimationPlayer::ToString(size_t ticker_id) const {Shouldn't this print all tickers rather than require a ticker id?
File cc/animation/single_animation_player.h:
Patch Set #21, Line 45:
void AddAnimation(std::unique_ptr<Animation> animation);Seems like SingleAnimationPlayer isn't quite the right name given it has multiple animations. As it is right now you might call it SingleTickerPlayer but I don't think the concept of ticker is well explained / understood and perhaps SingleEffectPlayer would be a better name to relate it back to GroupEffects
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Yi Gu (Gerrit)
8 comments:
File cc/animation/animation_player.h:
Patch Set #21, Line 48:
e of hnit: We should use a typedef so that it's obvious where used that the number represents a ticker id, […]
Done
Patch Set #21, Line 77:
void DetachElementForTicker(ElementId element_id, TickerId ticker_id);I worry that the meaning of ticker_id is not obvious for clients of animation. […]
We may have 2 different keyframe effects from 2 different targets running managed by the same AnimationPlayer. That way the AnimationPlayer has 2 AnimationTickers and each of them belongs to its own Element. That's when we need a second ticker. Comments added.
I agree that the name can be improved. majidvp@ has been thinking about renaming the cc classes so that they could match their blink counterparts. e.g. AnimationPlayer -> Animation, AnimationTicker -> Effect etc.. Maybe we should change the names all together in a follow-up patch so that we won't have AnimationTicker indexed by EffectId.Patch Set #21, Line 80:
void AddAnimationForTicker(std::unique_ptr<Animation> animation,With all of these *ForTicker methods, how many of them make sense to actually require a ticker_id? H […]
Other than AbortAnimations which aborts all animations, the rest *ForTicker methods operate on a certain animation. e.g. when blink needs to pause an animation it provides an animation id. This animation is owned by one ticker, say id 1, and pausing it on ticker2 doesn't make sense. WDYT?
Patch Set #21, Line 159:
SIGN(As/number/count
Done
File cc/animation/animation_player.cc:
Patch Set #21, Line 240:
++ticking_tickers_count;This only seems to be used by line 235, I'd refer just merging this into the loop above than add add […]
Done
Patch Set #21, Line 252:
DCHECK(animation_host_);I think is_ticking_player_ can be replaced by checking whether ticking_tickers_number_ is greater th […]
Done
Shouldn't this print all tickers rather than require a ticker id?
Done. Also updated the unit test.
File cc/animation/single_animation_player.h:
Patch Set #21, Line 45:
void AddAnimation(std::unique_ptr<Animation> animation);
Seems like SingleAnimationPlayer isn't quite the right name given it has multiple animations. […]
SingleAnimationPlayer is not supposed to be parsed as SingleAnimation + Player. Rather, it should be AnimationPlayer which does Single Effect(as you mentioned last). Eventually we will have only one AnimationPlayer which handles both single and group effects. But ATM we have this class and call it SingleAP to match its base class AnimationPlayer. majidvp@ is about to rename the cc animation related classes to match their blink counterparts and "AnimationPlayer" will be renamed too. Maybe we should rename this class then?
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Robert Flack (Gerrit)
Patch set 22:Code-Review +1
3 comments:
File cc/animation/animation_player.h:
Patch Set #21, Line 77:
void DetachElementForTicker(ElementId element_id, TickerId ticker_id);We may have 2 different keyframe effects from 2 different targets running managed by the same Animat […]
Ack, doing a rename refactor to follow this up sounds reasonable.
Patch Set #21, Line 80:
void AddAnimationForTicker(std::unique_ptr<Animation> animation,
Other than AbortAnimations which aborts all animations, the rest *ForTicker methods operate on a cer […]
Oh I see, all of the calls from blink currently come from a specific keyframe effect. I assume though all of the tickers on an AnimationPlayer will be for the same animation id given that it maps to blink::Animation and represents a group effect.
File cc/animation/single_animation_player.h:
Patch Set #21, Line 45:
void AddAnimation(std::unique_ptr<Animation> animation);
SingleAnimationPlayer is not supposed to be parsed as SingleAnimation + Player. […]
We could call this SingleTickerAnimationPlayer to be explicit that it is not a single *Animation*.
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Yi Gu (Gerrit)
Thanks Rob for the review:)
3 comments:
Patch Set #21, Line 77:
void DetachElementForTicker(ElementId element_id, TickerId ticker_id);
Ack, doing a rename refactor to follow this up sounds reasonable.
Ack
Patch Set #21, Line 80:
void AddAnimationForTicker(std::unique_ptr<Animation> animation,
Oh I see, all of the calls from blink currently come from a specific keyframe effect. […]
The animation used here is cc::Animation which is different from blink::Animation. Currently 1 blink::Animation may have N cc::AnimationPlayer, each of which has M cc::Animation.
File cc/animation/single_animation_player.h:
We could call this SingleTickerAnimationPlayer to be explicit that it is not a single *Animation*.
Done
To view, visit change 742162. To unsubscribe, or for help writing mail filters, visit settings.
Yi Gu (Gerrit)
Patch set 25:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Fix conversion from size_t to int" https://chromium-review.googlesource.com/c/742162/25
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/742162/25
Bot data: {"action": "start", "triggered_at": "2018-01-25T22:26:42.0Z", "cq_cfg_revision": "9544bf3c7cb0639977b7c56b6f9db1b272cef60b", "revision": "586634ea0b38cb101a536a60e8fdc9f200d136f4"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
Yi Gu (Gerrit)
Patch set 26:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Edit commit message" https://chromium-review.googlesource.com/c/742162/26
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/742162/26
Bot data: {"action": "start", "triggered_at": "2018-01-26T22:19:47.0Z", "cq_cfg_revision": "9544bf3c7cb0639977b7c56b6f9db1b272cef60b", "revision": "6e67cba05daade204f000571226e8b465961d89d"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
chromeos-amd64-generic-rel on master.tryserver.chromium.chromiumos (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
chromeos-daisy-rel on master.tryserver.chromium.chromiumos (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
linux-chromeos-rel on master.tryserver.chromium.chromiumos (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
Yi Gu (Gerrit)
Patch set 26:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Edit commit message" https://chromium-review.googlesource.com/c/742162/26
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/742162/26
Bot data: {"action": "start", "triggered_at": "2018-01-27T19:38:38.0Z", "cq_cfg_revision": "9544bf3c7cb0639977b7c56b6f9db1b272cef60b", "revision": "6e67cba05daade204f000571226e8b465961d89d"}
Commit Bot (Gerrit)
Commit Bot merged this change.
Group effects support in cc::AnimationPlayer
The current cc/animations logic assumes a single animation has a single
keyframe effect and can only affect a single layer. To enable animations
with multiple keyframe effects, cc::AnimationPlayer need to support
multiple AnimationTickers each corresponding to one keyframe effect.
Currently there is a 1:1 relationship between AnimationPlayer and
AnimationTicker. This patch is to extend it to 1:n. Here is a summary of
changes:
- Introduce a sub-class of AnimationPlayer, a.k.a
SingleTickerAnimationPlayer, to handle the existing logic (single
effect). SingleTickerAnimationPlayer owns only one AnimationTicker as
the AnimationPlayer does today.
- Currently a AnimationTicker is created upon creating AnimationPlayer.
In this patch, tickers are created separately and added to the player
afterwards. Tickers that the player owns may belong to different targets
therefore the player needs to coordinate with AnimationHost regarding
this situation.
- Adjust existing unit tests according to the changes above.
Bug: 767043
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Ibd2ed2f396effec9fbc206ff52c7b94474342d39
Reviewed-on: https://chromium-review.googlesource.com/742162
Commit-Queue: Yi Gu <yi...@chromium.org>
Reviewed-by: Robert Flack <fla...@chromium.org>
Reviewed-by: Majid Valipour <maj...@chromium.org>
Reviewed-by: Ian Vollick <vol...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532244}
---
M cc/animation/BUILD.gn
M cc/animation/animation_host.cc
M cc/animation/animation_host.h
M cc/animation/animation_host_perftest.cc
M cc/animation/animation_player.cc
M cc/animation/animation_player.h
M cc/animation/animation_player_unittest.cc
M cc/animation/animation_ticker.cc
M cc/animation/animation_ticker.h
M cc/animation/animation_timeline.cc
M cc/animation/animation_timeline_unittest.cc
M cc/animation/element_animations_unittest.cc
M cc/animation/scroll_offset_animations_impl.cc
M cc/animation/scroll_offset_animations_impl.h
A cc/animation/single_ticker_animation_player.cc
A cc/animation/single_ticker_animation_player.h
M cc/animation/worklet_animation_player.cc
M cc/animation/worklet_animation_player.h
M cc/test/animation_test_common.cc
M cc/test/animation_test_common.h
M cc/test/animation_timelines_test_common.cc
M cc/test/animation_timelines_test_common.h
M cc/test/layer_tree_test.cc
M cc/test/layer_tree_test.h
M cc/trees/layer_tree_host_common_unittest.cc
M cc/trees/layer_tree_host_unittest_animation.cc
M third_party/WebKit/Source/platform/animation/CompositorAnimationPlayer.cpp
M third_party/WebKit/Source/platform/animation/CompositorAnimationPlayer.h
M third_party/WebKit/Source/platform/animation/CompositorAnimationPlayerTest.cpp
M ui/compositor/layer_animator.cc
M ui/compositor/layer_animator.h
M ui/compositor/layer_owner_unittest.cc
M ui/compositor/layer_unittest.cc
33 files changed, 1,909 insertions(+), 924 deletions(-)