Use an auxiliary list of animation controllers.

With this patch we accomplish the following:
 1. layer animation controllers are ref counted (so they can be shared by the two impl trees)
 2. the layer tree hosts now own a list of active animation controllers. This allows for a couple of nice things
 __a. Ticking the animation controllers no longer requires a tree walk
 __b. We will be able to support ticking of animation controllers for layers that are not yet added to the layer tree. (Support coming in a future patch).
 3. animation controllers register and unregister themselves from their respective layer tree host's list when they have an animation to tick.

BUG=162111
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171714

[Ian Vollick, 2012-12-02 17:45:48]

nduca, enne,

I think this is starting to be pretty baked. The change seems to have reduced,
rather than added, complexity, which is nice.

PTAL,

Ian

[nduca, 2012-12-03 00:07:01]

Where'd you end up with jamesr on factory vs whatever-the-alternative-was?

[Ian Vollick, 2012-12-03 02:43:37]

On 2012/12/03 00:07:01, nduca wrote:
> Where'd you end up with jamesr on factory vs whatever-the-alternative-was?

Still no word yet. I'll send a follow-up email now.

[jamesr, 2012-12-03 04:40:15]

https://codereview.chromium.org/11348256/diff/5001/cc/layer.cc
File cc/layer.cc (right):

https://codereview.chromium.org/11348256/diff/5001/cc/layer.cc#newcode367
cc/layer.cc:367: if (m_layerAnimationController->opacityLastUpdateTime() >
m_opacityLastUpdateTime)
could you explain what this logic is for? anything that's timestamp based seems
very race-prone

[Ian Vollick, 2012-12-03 14:47:03]

https://codereview.chromium.org/11348256/diff/5001/cc/layer.cc
File cc/layer.cc (right):

https://codereview.chromium.org/11348256/diff/5001/cc/layer.cc#newcode367
cc/layer.cc:367: if (m_layerAnimationController->opacityLastUpdateTime() >
m_opacityLastUpdateTime)
On 2012/12/03 04:40:15, jamesr wrote:
> could you explain what this logic is for? anything that's timestamp based
seems
> very race-prone
Sure. Animation controllers used to push new values to their layers. This was
racey as well, in that whichever party modified a layer last (either WebCore or
an animation controller) clobbered the value. Now that we're going to be sharing
controllers between LayerImpls, I've switched to a pull model. To replicate the
old behavior, layers return whichever value is fresher (either the value from
WebCore or the animation controller's). I use timestamps to make this
determination.

[enne (OOO), 2012-12-04 01:32:16]

https://codereview.chromium.org/11348256/diff/5001/cc/layer.cc
File cc/layer.cc (right):

https://codereview.chromium.org/11348256/diff/5001/cc/layer.cc#newcode367
cc/layer.cc:367: if (m_layerAnimationController->opacityLastUpdateTime() >
m_opacityLastUpdateTime)
On 2012/12/03 14:47:03, vollick wrote:
> On 2012/12/03 04:40:15, jamesr wrote:
> > could you explain what this logic is for? anything that's timestamp based
> seems
> > very race-prone
> Sure. Animation controllers used to push new values to their layers. This was
> racey as well, in that whichever party modified a layer last (either WebCore
or
> an animation controller) clobbered the value. Now that we're going to be
sharing
> controllers between LayerImpls, I've switched to a pull model. To replicate
the
> old behavior, layers return whichever value is fresher (either the value from
> WebCore or the animation controller's). I use timestamps to make this
> determination.

This is really strange to my eyes too.  If the opacity is animating, shouldn't
that control the opacity no matter what?  What does it mean to manually set the
opacity on something that's also being animated?

[nduca, 2012-12-04 04:00:43]

monotonicallyIncreasingTime is actually a lie. Its monotonically non decreasing.
meaning you can ask for it twice in a row and get the same value. it just will
never go down.

if you need to guarantee ordering, use sequence numbers, I'm afraid.

[nduca, 2012-12-04 05:40:26]

how does a layer moving between trees work in this?

https://codereview.chromium.org/11348256/diff/5001/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://codereview.chromium.org/11348256/diff/5001/cc/layer_tree_host.h#newco...
cc/layer_tree_host.h:245: // AnimationRegistar implementation.
so, walk me through the benefit of this being LTH rather than ref-counted
global?

[nduca, 2012-12-04 05:51:13]

https://codereview.chromium.org/11348256/diff/5001/cc/layer_animation_control...
File cc/layer_animation_controller.cc (right):

https://codereview.chromium.org/11348256/diff/5001/cc/layer_animation_control...
cc/layer_animation_controller.cc:169: return;
Oh i get it nao. Thanks.

Can we split out registration from activation? E.g. you're always registered and
then when your activation state changes, you just tell the registrar that you're
on or off at a given moment.

Also, Registrar->AnimationController? It sorta makes sense if you think of it...

[Ian Vollick, 2012-12-04 19:49:06]

https://codereview.chromium.org/11348256/diff/5001/cc/layer.cc
File cc/layer.cc (right):

https://codereview.chromium.org/11348256/diff/5001/cc/layer.cc#newcode367
cc/layer.cc:367: if (m_layerAnimationController->opacityLastUpdateTime() >
m_opacityLastUpdateTime)
On 2012/12/04 01:32:16, enne wrote:
> On 2012/12/03 14:47:03, vollick wrote:
> > On 2012/12/03 04:40:15, jamesr wrote:
> > > could you explain what this logic is for? anything that's timestamp based
> > seems
> > > very race-prone
> > Sure. Animation controllers used to push new values to their layers. This
was
> > racey as well, in that whichever party modified a layer last (either WebCore
> or
> > an animation controller) clobbered the value. Now that we're going to be
> sharing
> > controllers between LayerImpls, I've switched to a pull model. To replicate
> the
> > old behavior, layers return whichever value is fresher (either the value
from
> > WebCore or the animation controller's). I use timestamps to make this
> > determination.
> 
> This is really strange to my eyes too.  If the opacity is animating, shouldn't
> that control the opacity no matter what?  What does it mean to manually set
the
> opacity on something that's also being animated?

Yes, it should. In fact, in my latest patch we explicitly ignore transform and
opacity updates if the values are animating. The problem these timestamps were
trying to address was not how to deal with setFoo when foo was animating, but
how a layer can decide which foo (it's own copy, or the animator's) was fresher
after an animation had completed, since it had no way of knowing which came
last, the animation or the call to setFoo.

The newest patch simplifies the situation by making the animation controller the
manager of its layer's opacity and transform. This has a number of benefits. It
saves space -- we no longer have to have redundant copies of the transform and
opacity lying around. It also encapsulates the logic for dealing with setFoo
when foo is animating nicely within the animator. Since the animator has all the
knowledge it needs to determine if the animated or set value is fresher, there
is no longer a need for timestamps/sequence #'s, etc. It also simplifies logic
in the layer classes. When a layer is asked to set opacity, it just passes it
along to its animation controller. When it's asked for this value, it just
passes back the value from its controller.

https://codereview.chromium.org/11348256/diff/5001/cc/layer_animation_control...
File cc/layer_animation_controller.cc (right):

https://codereview.chromium.org/11348256/diff/5001/cc/layer_animation_control...
cc/layer_animation_controller.cc:169: return;
On 2012/12/04 05:51:13, nduca wrote:
> Oh i get it nao. Thanks.
> 
> Can we split out registration from activation? E.g. you're always registered
and
> then when your activation state changes, you just tell the registrar that
you're
> on or off at a given moment.

Naming is hard. Perhaps registration is the wrong word for the concept I'm
trying to articulate here. What I mean by register is
register-as-an-active-animating-controller. Perhaps the two registrar functions
could be named Activate/Deactivate rather than Register/Unregister.

I don't think that splitting out these two concepts makes sense. As I understand
your comment, you take 'registered with' to mean 'associated with'. That
association is implied by having a non-null registrar pointer in
LayerAnimationController. The only idea I care about here is activation and
deactivation of controllers. Rather than splitting out registration from
activation, I think we should just rename registration to something that more
clearly means what I originally intended.

> 
> Also, Registrar->AnimationController? It sorta makes sense if you think of
it...

I like this, but if we made this change, I think we should also
LayerAnimationController -> LayerAnimator. What do you think? If we make this
switch, I'd like to do it in a second, brain-dead, sed patch so that it doesn't
hide the 'real' changes in this CL.

[enne (OOO), 2012-12-04 22:27:07]

What user of the API is calling setFoo when Foo is animating?  Is this something
that happens in practice (and if so, how?) or is this just a potential issue
given the way this API works?

[nduca, 2012-12-04 22:29:04]

I get your point, but activation is orthogonal from which host a controller is
associated with.  I have had a lot of success with keeping things orthogonal
even when they technically be merged. Can you give what I suggested a try?

[Ian Vollick, 2012-12-05 02:14:31]

On 2012/12/04 22:27:07, enne wrote:
> What user of the API is calling setFoo when Foo is animating?  Is this
something
> that happens in practice (and if so, how?) or is this just a potential issue
> given the way this API works?

I think this is only an issue during commit. We used to have guards in
Layer::pushPropertiesTo to prevent overwriting an animated value -- these same
checks just got pushed to the animation controller.

[Ian Vollick, 2012-12-05 02:15:59]

On 2012/12/04 22:29:04, nduca wrote:
> I get your point, but activation is orthogonal from which host a controller is
> associated with.  I have had a lot of success with keeping things orthogonal
> even when they technically be merged. Can you give what I suggested a try?

Sorry, I honestly didn't mean to be difficult -- I hadn't understood that by
decoupling the concepts you meant to try using global lists of active
controllers. I've uploaded a cl that does this here
https://codereview.chromium.org/11443004/

[Ian Vollick, 2012-12-05 15:30:00]

On 2012/12/05 02:15:59, vollick wrote:
> On 2012/12/04 22:29:04, nduca wrote:
> > I get your point, but activation is orthogonal from which host a controller
is
> > associated with.  I have had a lot of success with keeping things orthogonal
> > even when they technically be merged. Can you give what I suggested a try?
> 
Kk, I've added registration/unregistration support in the latest cl. Is this
closer to what you were thinking?

[nduca, 2012-12-05 20:15:56]

I like this. Do you, Ian?

https://codereview.chromium.org/11348256/diff/18001/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://codereview.chromium.org/11348256/diff/18001/cc/layer_tree_host.h#newc...
cc/layer_tree_host.h:261: virtual void
DeactivateAnimationController(LayerAnimationController*) OVERRIDE;
DidActivate, DidDeactivate. Written as Activate, it sounds like it is doing work
to activate the controller vs being notified that it already has become active.

[Ian Vollick, 2012-12-06 00:33:22]

On 2012/12/05 20:15:56, nduca wrote:
> I like this. Do you, Ian?

Yup!

> 
> https://codereview.chromium.org/11348256/diff/18001/cc/layer_tree_host.h
> File cc/layer_tree_host.h (right):
> 
>
https://codereview.chromium.org/11348256/diff/18001/cc/layer_tree_host.h#newc...
> cc/layer_tree_host.h:261: virtual void
> DeactivateAnimationController(LayerAnimationController*) OVERRIDE;
> DidActivate, DidDeactivate. Written as Activate, it sounds like it is doing
work
> to activate the controller vs being notified that it already has become
active.

SG. Done.

[nduca, 2012-12-06 00:58:45]

lgtm

[commit-bot: I haz the power, 2012-12-06 21:30:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/11348256/27019

[commit-bot: I haz the power, 2012-12-06 21:31:12]

Failed to apply patch for cc/layer_tree_host.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/layer_tree_host.cc
  Hunk #4 FAILED at 390.
  1 out of 7 hunks FAILED -- saving rejects to file cc/layer_tree_host.cc.rej

Patch:       cc/layer_tree_host.cc
Index: cc/layer_tree_host.cc
diff --git a/cc/layer_tree_host.cc b/cc/layer_tree_host.cc
index
54c1febd343de50360d14ea4560bd452a1721ce8..23baf0245417b3c734b554a08a38178230c4ce39
100644
--- a/cc/layer_tree_host.cc
+++ b/cc/layer_tree_host.cc
@@ -7,6 +7,7 @@
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
 #include "base/message_loop.h"
+#include "base/stl_util.h"
 #include "base/string_number_conversions.h"
 #include "cc/font_atlas.h"
 #include "cc/heads_up_display_layer.h"
@@ -177,7 +178,6 @@ scoped_ptr<LayerTreeHost>
LayerTreeHost::create(LayerTreeHostClient* client, con
 
 LayerTreeHost::LayerTreeHost(LayerTreeHostClient* client, const
LayerTreeSettings& settings)
     : m_animating(false)
-    , m_needsAnimateLayers(false)
     , m_needsFullTreeSync(true)
     , m_client(client)
     , m_commitNumber(0)
@@ -235,6 +235,9 @@ LayerTreeHost::~LayerTreeHost()
     RateLimiterMap::iterator it = m_rateLimiters.begin();
     if (it != m_rateLimiters.end())
         it->second->stop();
+
+    if (m_rootLayer)
+        m_rootLayer = 0;
 }
 
 void LayerTreeHost::setSurfaceReady()
@@ -387,11 +390,6 @@ void
LayerTreeHost::finishCommitOnImplThread(LayerTreeHostImpl* hostImpl)
     else
         hostImpl->setHudLayer(0);
 
-    // We may have added an animation during the tree sync. This will cause
both layer tree hosts
-    // to visit their controllers.
-    if (rootLayer() && m_needsAnimateLayers)
-        hostImpl->setNeedsAnimateLayers();
-
     hostImpl->setSourceFrameNumber(commitNumber());
     hostImpl->setViewportSize(layoutViewportSize(), deviceViewportSize());
     hostImpl->setDeviceScaleFactor(deviceScaleFactor());
@@ -525,12 +523,6 @@ void
LayerTreeHost::setAnimationEvents(scoped_ptr<AnimationEventsVector> events,
     setAnimationEventsRecursive(*events.get(), m_rootLayer.get(),
wallClockTime);
 }
 
-void LayerTreeHost::didAddAnimation()
-{
-    m_needsAnimateLayers = true;
-    m_proxy->didAddAnimation();
-}
-
 void LayerTreeHost::setRootLayer(scoped_refptr<Layer> rootLayer)
 {
     if (m_rootLayer == rootLayer)
@@ -933,33 +925,16 @@ void LayerTreeHost::setDeviceScaleFactor(float
deviceScaleFactor)
 
 void LayerTreeHost::animateLayers(base::TimeTicks time)
 {
-    if (!m_settings.acceleratedAnimationEnabled || !m_needsAnimateLayers)
+    if (!m_settings.acceleratedAnimationEnabled ||
m_activeAnimationControllers.empty())
         return;
 
     TRACE_EVENT0("cc", "LayerTreeHostImpl::animateLayers");
-    m_needsAnimateLayers = animateLayersRecursive(m_rootLayer.get(), time);
-}
 
-bool LayerTreeHost::animateLayersRecursive(Layer* current, base::TimeTicks
time)
-{
-    if (!current)
-        return false;
-
-    bool subtreeNeedsAnimateLayers = false;
-    LayerAnimationController* currentController =
current->layerAnimationController();
     double monotonicTime = (time - base::TimeTicks()).InSecondsF();
-    currentController->animate(monotonicTime, 0);
-
-    // If the current controller still has an active animation, we must
continue animating layers.
-    if (currentController->hasActiveAnimation())
-         subtreeNeedsAnimateLayers = true;
 
-    for (size_t i = 0; i < current->children().size(); ++i) {
-        if (animateLayersRecursive(current->children()[i].get(), time))
-            subtreeNeedsAnimateLayers = true;
-    }
-
-    return subtreeNeedsAnimateLayers;
+    AnimationControllerSet copy = m_activeAnimationControllers;
+    for (AnimationControllerSet::iterator iter = copy.begin(); iter !=
copy.end(); ++iter)
+        (*iter)->animate(monotonicTime, 0);
 }
 
 void LayerTreeHost::setAnimationEventsRecursive(const AnimationEventsVector&
events, Layer* layer, base::Time wallClockTime)
@@ -980,4 +955,30 @@ void LayerTreeHost::setAnimationEventsRecursive(const
AnimationEventsVector& eve
         setAnimationEventsRecursive(events,
layer->children()[childIndex].get(), wallClockTime);
 }
 
+void LayerTreeHost::DidActivateAnimationController(LayerAnimationController*
controller) {
+    // Controllers register themselves when they have new animations. We need
+    // to commit in this case.
+    setNeedsCommit();
+    m_activeAnimationControllers.insert(controller);
+}
+
+void LayerTreeHost::DidDeactivateAnimationController(LayerAnimationController*
controller) {
+    if (ContainsKey(m_activeAnimationControllers, controller))
+        m_activeAnimationControllers.erase(controller);
+}
+
+void LayerTreeHost::RegisterAnimationController(LayerAnimationController*
controller) {
+#if !defined(NDEBUG)
+    m_allAnimationControllers.insert(controller);
+#endif
+}
+
+void LayerTreeHost::UnregisterAnimationController(LayerAnimationController*
controller) {
+#if !defined(NDEBUG)
+    if (ContainsKey(m_allAnimationControllers, controller))
+        m_allAnimationControllers.erase(controller);
+#endif
+    DidDeactivateAnimationController(controller);
+}
+
 }  // namespace cc

[commit-bot: I haz the power, 2012-12-07 00:25:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/11348256/37002

[commit-bot: I haz the power, 2012-12-07 00:31:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/11348256/36026

[commit-bot: I haz the power, 2012-12-07 07:37:13]

Change committed as 171714