From beaf5d919fa7986b96968d2282458aca9dc05b13 Mon Sep 17 00:00:00 2001 From: Leon Scroggins III Date: Fri, 26 Jan 2018 11:03:54 -0500 Subject: [PATCH] Fixes for animationEnd in AnimatedImageDrawable Bug: 63908092 Test: Manual If the animation is running on the render thread, post a message (in JNI) to call the listener. Use a weak reference in the native listener so that it does not create a cycle. Don't add a listener if it's already in the list. Remove the native listener when the Java listeners are all removed. Call onAnimationEnd when the animation is stopped manually. Change-Id: I5dc070089fb1cf399ac3316292592f614f2738f0 --- .../graphics/AnimatedImageDrawable.cpp | 61 ++++++++++++++----- .../drawable/AnimatedImageDrawable.java | 39 ++++++++++-- libs/hwui/hwui/AnimatedImageDrawable.cpp | 5 +- libs/hwui/hwui/AnimatedImageDrawable.h | 4 +- 4 files changed, 85 insertions(+), 24 deletions(-) diff --git a/core/jni/android/graphics/AnimatedImageDrawable.cpp b/core/jni/android/graphics/AnimatedImageDrawable.cpp index c88cf5cff04d5..ba56d592acef3 100644 --- a/core/jni/android/graphics/AnimatedImageDrawable.cpp +++ b/core/jni/android/graphics/AnimatedImageDrawable.cpp @@ -26,10 +26,11 @@ #include #include #include +#include using namespace android; -static jmethodID gAnimatedImageDrawable_postOnAnimationEndMethodID; +static jmethodID gAnimatedImageDrawable_onAnimationEndMethodID; // Note: jpostProcess holds a handle to the ImageDecoder. static jlong AnimatedImageDrawable_nCreate(JNIEnv* env, jobject /*clazz*/, @@ -123,9 +124,9 @@ static jboolean AnimatedImageDrawable_nStart(JNIEnv* env, jobject /*clazz*/, jlo return drawable->start(); } -static void AnimatedImageDrawable_nStop(JNIEnv* env, jobject /*clazz*/, jlong nativePtr) { +static jboolean AnimatedImageDrawable_nStop(JNIEnv* env, jobject /*clazz*/, jlong nativePtr) { auto* drawable = reinterpret_cast(nativePtr); - drawable->stop(); + return drawable->stop(); } // Java's LOOP_INFINITE relies on this being the same. @@ -137,33 +138,63 @@ static void AnimatedImageDrawable_nSetLoopCount(JNIEnv* env, jobject /*clazz*/, drawable->setRepetitionCount(loopCount); } -class JniAnimationEndListener : public OnAnimationEndListener { +class InvokeListener : public MessageHandler { public: - JniAnimationEndListener(JNIEnv* env, jobject javaObject) { + InvokeListener(JNIEnv* env, jobject javaObject) { LOG_ALWAYS_FATAL_IF(env->GetJavaVM(&mJvm) != JNI_OK); - mJavaObject = env->NewGlobalRef(javaObject); + // Hold a weak reference to break a cycle that would prevent GC. + mWeakRef = env->NewWeakGlobalRef(javaObject); } - ~JniAnimationEndListener() override { + ~InvokeListener() override { auto* env = get_env_or_die(mJvm); - env->DeleteGlobalRef(mJavaObject); + env->DeleteWeakGlobalRef(mWeakRef); } - void onAnimationEnd() override { + virtual void handleMessage(const Message&) override { auto* env = get_env_or_die(mJvm); - env->CallVoidMethod(mJavaObject, gAnimatedImageDrawable_postOnAnimationEndMethodID); + jobject localRef = env->NewLocalRef(mWeakRef); + if (localRef) { + env->CallVoidMethod(localRef, gAnimatedImageDrawable_onAnimationEndMethodID); + } } private: JavaVM* mJvm; - jobject mJavaObject; + jweak mWeakRef; +}; + +class JniAnimationEndListener : public OnAnimationEndListener { +public: + JniAnimationEndListener(sp&& looper, JNIEnv* env, jobject javaObject) { + mListener = new InvokeListener(env, javaObject); + mLooper = std::move(looper); + } + + void onAnimationEnd() override { mLooper->sendMessage(mListener, 0); } + +private: + sp mListener; + sp mLooper; }; static void AnimatedImageDrawable_nSetOnAnimationEndListener(JNIEnv* env, jobject /*clazz*/, jlong nativePtr, jobject jdrawable) { auto* drawable = reinterpret_cast(nativePtr); - drawable->setOnAnimationEndListener(std::unique_ptr( - new JniAnimationEndListener(env, jdrawable))); + if (!jdrawable) { + drawable->setOnAnimationEndListener(nullptr); + } else { + sp looper = Looper::getForThread(); + if (!looper.get()) { + doThrowISE(env, + "Must set AnimatedImageDrawable's AnimationCallback on a thread with a " + "looper!"); + return; + } + + drawable->setOnAnimationEndListener( + std::make_unique(std::move(looper), env, jdrawable)); + } } static long AnimatedImageDrawable_nNativeByteSize(JNIEnv* env, jobject /*clazz*/, jlong nativePtr) { @@ -186,7 +217,7 @@ static const JNINativeMethod gAnimatedImageDrawableMethods[] = { { "nSetColorFilter", "(JJ)V", (void*) AnimatedImageDrawable_nSetColorFilter }, { "nIsRunning", "(J)Z", (void*) AnimatedImageDrawable_nIsRunning }, { "nStart", "(J)Z", (void*) AnimatedImageDrawable_nStart }, - { "nStop", "(J)V", (void*) AnimatedImageDrawable_nStop }, + { "nStop", "(J)Z", (void*) AnimatedImageDrawable_nStop }, { "nSetLoopCount", "(JI)V", (void*) AnimatedImageDrawable_nSetLoopCount }, { "nSetOnAnimationEndListener", "(JLandroid/graphics/drawable/AnimatedImageDrawable;)V", (void*) AnimatedImageDrawable_nSetOnAnimationEndListener }, { "nNativeByteSize", "(J)J", (void*) AnimatedImageDrawable_nNativeByteSize }, @@ -195,7 +226,7 @@ static const JNINativeMethod gAnimatedImageDrawableMethods[] = { int register_android_graphics_drawable_AnimatedImageDrawable(JNIEnv* env) { jclass animatedImageDrawable_class = FindClassOrDie(env, "android/graphics/drawable/AnimatedImageDrawable"); - gAnimatedImageDrawable_postOnAnimationEndMethodID = GetMethodIDOrDie(env, animatedImageDrawable_class, "postOnAnimationEnd", "()V"); + gAnimatedImageDrawable_onAnimationEndMethodID = GetMethodIDOrDie(env, animatedImageDrawable_class, "onAnimationEnd", "()V"); return android::RegisterMethodsOrDie(env, "android/graphics/drawable/AnimatedImageDrawable", gAnimatedImageDrawableMethods, NELEM(gAnimatedImageDrawableMethods)); diff --git a/graphics/java/android/graphics/drawable/AnimatedImageDrawable.java b/graphics/java/android/graphics/drawable/AnimatedImageDrawable.java index bd49b87ec2002..27c8fda0d6e95 100644 --- a/graphics/java/android/graphics/drawable/AnimatedImageDrawable.java +++ b/graphics/java/android/graphics/drawable/AnimatedImageDrawable.java @@ -348,7 +348,9 @@ public class AnimatedImageDrawable extends Drawable implements Animatable2 { if (mState == null) { throw new IllegalStateException("called stop on empty AnimatedImageDrawable"); } - nStop(mState.mNativePtr); + if (nStop(mState.mNativePtr)) { + postOnAnimationEnd(); + } } // Animatable2 overrides @@ -365,21 +367,31 @@ public class AnimatedImageDrawable extends Drawable implements Animatable2 { nSetOnAnimationEndListener(mState.mNativePtr, this); } - mAnimationCallbacks.add(callback); + if (!mAnimationCallbacks.contains(callback)) { + mAnimationCallbacks.add(callback); + } } @Override public boolean unregisterAnimationCallback(@NonNull AnimationCallback callback) { - if (callback == null || mAnimationCallbacks == null) { + if (callback == null || mAnimationCallbacks == null + || !mAnimationCallbacks.remove(callback)) { return false; } - return mAnimationCallbacks.remove(callback); + if (mAnimationCallbacks.isEmpty()) { + clearAnimationCallbacks(); + } + + return true; } @Override public void clearAnimationCallbacks() { - mAnimationCallbacks = null; + if (mAnimationCallbacks != null) { + mAnimationCallbacks = null; + nSetOnAnimationEndListener(mState.mNativePtr, null); + } } private void postOnAnimationStart() { @@ -413,6 +425,21 @@ public class AnimatedImageDrawable extends Drawable implements Animatable2 { return mHandler; } + /** + * Called by JNI. + * + * The JNI code has already posted this to the thread that created the + * callback, so no need to post. + */ + @SuppressWarnings("unused") + private void onAnimationEnd() { + if (mAnimationCallbacks != null) { + for (Animatable2.AnimationCallback callback : mAnimationCallbacks) { + callback.onAnimationEnd(this); + } + } + } + private static native long nCreate(long nativeImageDecoder, @Nullable ImageDecoder decoder, int width, int height, Rect cropRect) @@ -432,7 +459,7 @@ public class AnimatedImageDrawable extends Drawable implements Animatable2 { @FastNative private static native boolean nStart(long nativePtr); @FastNative - private static native void nStop(long nativePtr); + private static native boolean nStop(long nativePtr); @FastNative private static native void nSetLoopCount(long nativePtr, int loopCount); // Pass the drawable down to native so it can call onAnimationEnd. diff --git a/libs/hwui/hwui/AnimatedImageDrawable.cpp b/libs/hwui/hwui/AnimatedImageDrawable.cpp index 5356d3bfc7c9a..2bded9b3a2d88 100644 --- a/libs/hwui/hwui/AnimatedImageDrawable.cpp +++ b/libs/hwui/hwui/AnimatedImageDrawable.cpp @@ -48,8 +48,10 @@ bool AnimatedImageDrawable::start() { return true; } -void AnimatedImageDrawable::stop() { +bool AnimatedImageDrawable::stop() { + bool wasRunning = mRunning; mRunning = false; + return wasRunning; } bool AnimatedImageDrawable::isRunning() { @@ -180,7 +182,6 @@ void AnimatedImageDrawable::onDraw(SkCanvas* canvas) { if (finalFrame) { if (mEndListener) { mEndListener->onAnimationEnd(); - mEndListener = nullptr; } } } diff --git a/libs/hwui/hwui/AnimatedImageDrawable.h b/libs/hwui/hwui/AnimatedImageDrawable.h index 9d84ed5f44706..2fd6f40b71b5a 100644 --- a/libs/hwui/hwui/AnimatedImageDrawable.h +++ b/libs/hwui/hwui/AnimatedImageDrawable.h @@ -68,7 +68,9 @@ public: // Returns true if the animation was started; false otherwise (e.g. it was // already running) bool start(); - void stop(); + // Returns true if the animation was stopped; false otherwise (e.g. it was + // already stopped) + bool stop(); bool isRunning(); void setRepetitionCount(int count) { mSkAnimatedImage->setRepetitionCount(count); }