Revert "Revert "Check RenderNode's owning view before attaching animators""

This reverts commit eb40178af3b7c8d925eaf6c1aa0bef739c8ea47e.

Change-Id: I6838ecb35b50847746ee66ac204f14eb5f579b91

8 files changed, 86 insertions, 37 deletions

diff --git a/core/java/android/view/RenderNode.java b/core/java/android/view/RenderNode.java
index 7017ff5a3fa0..a4cb7035f2f9 100644
--- a/core/java/android/view/RenderNode.java
+++ b/core/java/android/view/RenderNode.java
@@ -775,6 +775,10 @@ public class RenderNode {
         mOwningView.mAttachInfo.mViewRootImpl.registerAnimatingRenderNode(this);
     }
 
+    public boolean isAttached() {
+        return mOwningView != null && mOwningView.mAttachInfo != null;
+    }
+
     public void addAnimator(AnimatedVectorDrawable.VectorDrawableAnimator animatorSet) {
         if (mOwningView == null || mOwningView.mAttachInfo == null) {
             throw new IllegalStateException("Cannot start this animator on a detached view!");
diff --git a/graphics/java/android/graphics/drawable/AnimatedVectorDrawable.java b/graphics/java/android/graphics/drawable/AnimatedVectorDrawable.java
index 99bc306821ef..77748a8b12ea 100644
--- a/graphics/java/android/graphics/drawable/AnimatedVectorDrawable.java
+++ b/graphics/java/android/graphics/drawable/AnimatedVectorDrawable.java
@@ -778,7 +778,6 @@ public class AnimatedVectorDrawable extends Drawable implements Animatable2 {
         private boolean mShouldIgnoreInvalidAnim;
         // TODO: Consider using NativeAllocationRegistery to track native allocation
         private final VirtualRefBasePtr mSetRefBasePtr;
-        private WeakReference<RenderNode> mTarget = null;
         private WeakReference<RenderNode> mLastSeenTarget = null;
         private int mLastListenerId = 0;
         private int mPendingAnimationAction = NONE;
@@ -1048,16 +1047,13 @@ public class AnimatedVectorDrawable extends Drawable implements Animatable2 {
             }
         }
 
-        private boolean setTarget(RenderNode node) {
-            node.addAnimator(this);
-            mTarget = new WeakReference<RenderNode>(node);
-            return true;
-        }
-
         private boolean useLastSeenTarget() {
-            if (mLastSeenTarget != null && mLastSeenTarget.get() != null) {
-                setTarget(mLastSeenTarget.get());
-                return true;
+            if (mLastSeenTarget != null) {
+                final RenderNode target = mLastSeenTarget.get();
+                if (target != null && target.isAttached()) {
+                    target.addAnimator(this);
+                    return true;
+                }
             }
             return false;
         }
@@ -1155,7 +1151,6 @@ public class AnimatedVectorDrawable extends Drawable implements Animatable2 {
             if (mListener != null) {
                 mListener.onAnimationEnd(null);
             }
-            mTarget = null;
         }
 
         // onFinished: should be called from native
diff --git a/libs/hwui/Animator.cpp b/libs/hwui/Animator.cpp
index 372bcb3b2375..294edb6ecd46 100644
--- a/libs/hwui/Animator.cpp
+++ b/libs/hwui/Animator.cpp
@@ -33,6 +33,7 @@ namespace uirenderer {
 
 BaseRenderNodeAnimator::BaseRenderNodeAnimator(float finalValue)
         : mTarget(nullptr)
+        , mStagingTarget(nullptr)
         , mFinalValue(finalValue)
         , mDeltaValue(0)
         , mFromValue(0)
@@ -82,7 +83,7 @@ void BaseRenderNodeAnimator::setStartDelay(nsecs_t startDelay) {
 }
 
 void BaseRenderNodeAnimator::attach(RenderNode* target) {
-    mTarget = target;
+    mStagingTarget = target;
     onAttached();
 }
 
@@ -145,6 +146,15 @@ void BaseRenderNodeAnimator::resolveStagingRequest(Request request) {
 }
 
 void BaseRenderNodeAnimator::pushStaging(AnimationContext& context) {
+    if (mStagingTarget) {
+        RenderNode* oldTarget = mTarget;
+        mTarget = mStagingTarget;
+        mStagingTarget = nullptr;
+        if (oldTarget && oldTarget != mTarget) {
+            oldTarget->onAnimatorTargetChanged(this);
+        }
+    }
+
     if (!mHasStartValue) {
         doSetStartValue(getValue(mTarget));
     }
@@ -195,6 +205,7 @@ void BaseRenderNodeAnimator::pushStaging(AnimationContext& context) {
             }
         }
     }
+    onPushStaging();
 }
 
 void BaseRenderNodeAnimator::transitionToRunning(AnimationContext& context) {
@@ -309,18 +320,36 @@ RenderPropertyAnimator::RenderPropertyAnimator(RenderProperty property, float fi
 
 void RenderPropertyAnimator::onAttached() {
     if (!mHasStartValue
-            && mTarget->isPropertyFieldDirty(mPropertyAccess->dirtyMask)) {
-        setStartValue((mTarget->stagingProperties().*mPropertyAccess->getter)());
+            && mStagingTarget->isPropertyFieldDirty(mPropertyAccess->dirtyMask)) {
+        setStartValue((mStagingTarget->stagingProperties().*mPropertyAccess->getter)());
     }
 }
 
 void RenderPropertyAnimator::onStagingPlayStateChanged() {
     if (mStagingPlayState == PlayState::Running) {
-        (mTarget->mutateStagingProperties().*mPropertyAccess->setter)(finalValue());
+        if (mStagingTarget) {
+            (mStagingTarget->mutateStagingProperties().*mPropertyAccess->setter)(finalValue());
+        } else {
+            // In the case of start delay where stagingTarget has been sync'ed over and null'ed
+            // we delay the properties update to push staging.
+            mShouldUpdateStagingProperties = true;
+        }
     } else if (mStagingPlayState == PlayState::Finished) {
         // We're being canceled, so make sure that whatever values the UI thread
         // is observing for us is pushed over
+        mShouldSyncPropertyFields = true;
+    }
+}
+
+void RenderPropertyAnimator::onPushStaging() {
+    if (mShouldUpdateStagingProperties) {
+        (mTarget->mutateStagingProperties().*mPropertyAccess->setter)(finalValue());
+        mShouldUpdateStagingProperties = false;
+    }
+
+    if (mShouldSyncPropertyFields) {
         mTarget->setPropertyFieldsDirty(dirtyMask());
+        mShouldSyncPropertyFields = false;
     }
 }
 
diff --git a/libs/hwui/Animator.h b/libs/hwui/Animator.h
index fcbc11b0306c..fdae0f32d4e6 100644
--- a/libs/hwui/Animator.h
+++ b/libs/hwui/Animator.h
@@ -85,6 +85,7 @@ public:
 
     void forceEndNow(AnimationContext& context);
     RenderNode* target() { return mTarget; }
+    RenderNode* stagingTarget() { return mStagingTarget; }
 
 protected:
     // PlayState is used by mStagingPlayState and mPlayState to track the state initiated from UI
@@ -123,8 +124,10 @@ protected:
 
     virtual void onStagingPlayStateChanged() {}
     virtual void onPlayTimeChanged(nsecs_t playTime) {}
+    virtual void onPushStaging() {}
 
     RenderNode* mTarget;
+    RenderNode* mStagingTarget;
 
     float mFinalValue;
     float mDeltaValue;
@@ -188,6 +191,7 @@ protected:
     virtual void setValue(RenderNode* target, float value) override;
     virtual void onAttached() override;
     virtual void onStagingPlayStateChanged() override;
+    virtual void onPushStaging() override;
 
 private:
     typedef bool (RenderProperties::*SetFloatProperty)(float value);
@@ -197,6 +201,8 @@ private:
     const PropertyAccessors* mPropertyAccess;
 
     static const PropertyAccessors PROPERTY_ACCESSOR_LUT[];
+    bool mShouldSyncPropertyFields = false;
+    bool mShouldUpdateStagingProperties = false;
 };
 
 class CanvasPropertyPrimitiveAnimator : public BaseRenderNodeAnimator {
diff --git a/libs/hwui/AnimatorManager.cpp b/libs/hwui/AnimatorManager.cpp
index 2b49b4743d89..2198fcc95fe5 100644
--- a/libs/hwui/AnimatorManager.cpp
+++ b/libs/hwui/AnimatorManager.cpp
@@ -42,7 +42,23 @@ AnimatorManager::~AnimatorManager() {
 }
 
 void AnimatorManager::addAnimator(const sp<BaseRenderNodeAnimator>& animator) {
+    RenderNode* stagingTarget = animator->stagingTarget();
+    if (stagingTarget == &mParent) {
+        return;
+    }
     mNewAnimators.emplace_back(animator.get());
+    // If the animator is already attached to other RenderNode, remove it from that RenderNode's
+    // new animator list. This ensures one animator only ends up in one newAnimatorList during one
+    // frame, even when it's added multiple times to multiple targets.
+    if (stagingTarget) {
+        stagingTarget->removeAnimator(animator);
+    }
+    animator->attach(&mParent);
+}
+
+void AnimatorManager::removeAnimator(const sp<BaseRenderNodeAnimator>& animator) {
+    mNewAnimators.erase(std::remove(mNewAnimators.begin(), mNewAnimators.end(), animator),
+            mNewAnimators.end());
 }
 
 void AnimatorManager::setAnimationHandle(AnimationHandle* handle) {
@@ -58,21 +74,12 @@ void AnimatorManager::pushStaging() {
         LOG_ALWAYS_FATAL_IF(!mAnimationHandle,
                 "Trying to start new animators on %p (%s) without an animation handle!",
                 &mParent, mParent.getName());
-        // Only add animators that are not already in the on-going animator list.
-        for (auto& animator : mNewAnimators) {
-            RenderNode* targetRenderNode = animator->target();
-            if (targetRenderNode == &mParent) {
-                // Animator already in the animator list: skip adding again
-                continue;
-            }
 
-            if (targetRenderNode){
-                // If the animator is already in another RenderNode's animator list, remove animator from
-                // that list and add animator to current RenderNode's list.
-                targetRenderNode->animators().removeActiveAnimator(animator);
+        // Only add new animators that are not already in the mAnimators list
+        for (auto& anim : mNewAnimators) {
+            if (anim->target() != &mParent) {
+                mAnimators.push_back(std::move(anim));
             }
-            animator->attach(&mParent);
-            mAnimators.push_back(std::move(animator));
         }
         mNewAnimators.clear();
     }
@@ -81,6 +88,11 @@ void AnimatorManager::pushStaging() {
     }
 }
 
+void AnimatorManager::onAnimatorTargetChanged(BaseRenderNodeAnimator* animator) {
+    LOG_ALWAYS_FATAL_IF(animator->target() == &mParent, "Target has not been changed");
+    mAnimators.erase(std::remove(mAnimators.begin(), mAnimators.end(), animator), mAnimators.end());
+}
+
 class AnimateFunctor {
 public:
     AnimateFunctor(TreeInfo& info, AnimationContext& context)
@@ -154,10 +166,6 @@ void AnimatorManager::endAllStagingAnimators() {
     mNewAnimators.clear();
 }
 
-void AnimatorManager::removeActiveAnimator(const sp<BaseRenderNodeAnimator>& animator) {
-    std::remove(mAnimators.begin(), mAnimators.end(), animator);
-}
-
 class EndActiveAnimatorsFunctor {
 public:
     EndActiveAnimatorsFunctor(AnimationContext& context) : mContext(context) {}
diff --git a/libs/hwui/AnimatorManager.h b/libs/hwui/AnimatorManager.h
index c24ef47a4644..61f6179d217c 100644
--- a/libs/hwui/AnimatorManager.h
+++ b/libs/hwui/AnimatorManager.h
@@ -39,11 +39,13 @@ public:
     ~AnimatorManager();
 
     void addAnimator(const sp<BaseRenderNodeAnimator>& animator);
+    void removeAnimator(const sp<BaseRenderNodeAnimator>& animator);
 
     void setAnimationHandle(AnimationHandle* handle);
     bool hasAnimationHandle() { return mAnimationHandle; }
 
     void pushStaging();
+    void onAnimatorTargetChanged(BaseRenderNodeAnimator* animator);
 
     // Returns the combined dirty mask of all animators run
     uint32_t animate(TreeInfo& info);
@@ -62,15 +64,10 @@ public:
 private:
     uint32_t animateCommon(TreeInfo& info);
 
-    // This would remove the animator from mAnimators list. It should only be called during
-    // push staging.
-    void removeActiveAnimator(const sp<BaseRenderNodeAnimator>& animator);
-
     RenderNode& mParent;
     AnimationHandle* mAnimationHandle;
 
     // To improve the efficiency of resizing & removing from the vector
-    // use manual ref counting instead of sp<>.
     std::vector< sp<BaseRenderNodeAnimator> > mNewAnimators;
     std::vector< sp<BaseRenderNodeAnimator> > mAnimators;
 };
diff --git a/libs/hwui/RenderNode.cpp b/libs/hwui/RenderNode.cpp
index bade216b3b21..9ac76a4339e1 100644
--- a/libs/hwui/RenderNode.cpp
+++ b/libs/hwui/RenderNode.cpp
@@ -218,6 +218,10 @@ void RenderNode::addAnimator(const sp<BaseRenderNodeAnimator>& animator) {
     mAnimatorManager.addAnimator(animator);
 }
 
+void RenderNode::removeAnimator(const sp<BaseRenderNodeAnimator>& animator) {
+    mAnimatorManager.removeAnimator(animator);
+}
+
 void RenderNode::damageSelf(TreeInfo& info) {
     if (isRenderable()) {
         if (properties().getClipDamageToBounds()) {
diff --git a/libs/hwui/RenderNode.h b/libs/hwui/RenderNode.h
index f248de54acba..e0376450178f 100644
--- a/libs/hwui/RenderNode.h
+++ b/libs/hwui/RenderNode.h
@@ -187,6 +187,12 @@ public:
 
     // UI thread only!
     ANDROID_API void addAnimator(const sp<BaseRenderNodeAnimator>& animator);
+    void removeAnimator(const sp<BaseRenderNodeAnimator>& animator);
+
+    // This can only happen during pushStaging()
+    void onAnimatorTargetChanged(BaseRenderNodeAnimator* animator) {
+        mAnimatorManager.onAnimatorTargetChanged(animator);
+    }
 
     AnimatorManager& animators() { return mAnimatorManager; }