From 337cfcbe0a194fa8a1f0964718f8ea8190c02fdd Mon Sep 17 00:00:00 2001 From: Andy Hung Date: Fri, 18 Mar 2022 11:24:15 -0700 Subject: [PATCH] SoundPool: Clean up JNI weak reference handling Remove unnecessary JNI params and overhead. Use weak reference entirely on the native side. Test: atest SoundPoolAacTest Test: atest SoundPoolHapticTest Test: atest SoundPoolMidiTest Test: atest SoundPoolOggTest Test: atest AudioManagerTest#testSoundEffects Bug: 223815163 Change-Id: I31964b27ab3e8a96f49794e78537b165e9e84ed3 --- media/java/android/media/SoundPool.java | 16 ++--- .../jni/soundpool/android_media_SoundPool.cpp | 62 ++++++++++++++++--- 2 files changed, 56 insertions(+), 22 deletions(-) diff --git a/media/java/android/media/SoundPool.java b/media/java/android/media/SoundPool.java index 6403aab08adc0..bb086d6839eb0 100644 --- a/media/java/android/media/SoundPool.java +++ b/media/java/android/media/SoundPool.java @@ -29,7 +29,6 @@ import android.util.Log; import java.io.File; import java.io.FileDescriptor; -import java.lang.ref.WeakReference; import java.util.concurrent.atomic.AtomicReference; @@ -156,8 +155,7 @@ public class SoundPool extends PlayerBase { super(attributes, AudioPlaybackConfiguration.PLAYER_TYPE_JAM_SOUNDPOOL); // do native setup - if (native_setup(new WeakReference(this), - maxStreams, attributes, getCurrentOpPackageName()) != 0) { + if (native_setup(maxStreams, attributes, getCurrentOpPackageName()) != 0) { throw new RuntimeException("Native setup failed"); } mAttributes = attributes; @@ -510,7 +508,7 @@ public class SoundPool extends PlayerBase { private native final int _load(FileDescriptor fd, long offset, long length, int priority); - private native final int native_setup(Object weakRef, int maxStreams, + private native int native_setup(int maxStreams, @NonNull Object/*AudioAttributes*/ attributes, @NonNull String opPackageName); private native final int _play(int soundID, float leftVolume, float rightVolume, @@ -522,17 +520,11 @@ public class SoundPool extends PlayerBase { // post event from native code to message handler @SuppressWarnings("unchecked") - private static void postEventFromNative(Object ref, int msg, int arg1, int arg2, Object obj) { - SoundPool soundPool = ((WeakReference) ref).get(); - if (soundPool == null) { - return; - } - - Handler eventHandler = soundPool.mEventHandler.get(); + private void postEventFromNative(int msg, int arg1, int arg2, Object obj) { + Handler eventHandler = mEventHandler.get(); if (eventHandler == null) { return; } - Message message = eventHandler.obtainMessage(msg, arg1, arg2, obj); eventHandler.sendMessage(message); } diff --git a/media/jni/soundpool/android_media_SoundPool.cpp b/media/jni/soundpool/android_media_SoundPool.cpp index 59298dfadb268..14b0c11c986ad 100644 --- a/media/jni/soundpool/android_media_SoundPool.cpp +++ b/media/jni/soundpool/android_media_SoundPool.cpp @@ -262,10 +262,19 @@ using JObjectValue = std::remove_pointer_t; // _jobject // Note std::remove_ptr_t == NonPointerType. static_assert(std::is_same_v); +// *jweak is needed to fit the jweak into a std::shared_ptr. +// This is the Android type _jobject, but we derive this type as JWeakValue. +using JWeakValue = std::remove_pointer_t; // this is just _jobject + +// Check that jweak is really a pointer to JWeakValue. +static_assert(std::is_same_v); + // We store the ancillary data associated with a SoundPool object in a concurrent // hash map indexed on the SoundPool native object pointer. auto& getSoundPoolJavaRefManager() { - static ConcurrentHashMap> concurrentHashMap; + // Note this can store shared_ptrs to either jweak and jobject, + // as the underlying type is identical. + static ConcurrentHashMap> concurrentHashMap; return concurrentHashMap; } @@ -284,6 +293,8 @@ auto& getSoundPoolJavaRefManager() { // https://developer.android.com/training/articles/perf-jni // https://android-developers.googleblog.com/2011/11/jni-local-reference-changes-in-ics.html // +// Consider using a weak reference if this is self-referential. +[[maybe_unused]] inline auto make_shared_globalref_from_localref(JNIEnv *env, jobject localRef) { return std::shared_ptr( env->NewGlobalRef(localRef), @@ -292,6 +303,32 @@ inline auto make_shared_globalref_from_localref(JNIEnv *env, jobject localRef) { }); } +// Create a weak global reference from local ref. +inline auto make_shared_weakglobalref_from_localref(JNIEnv *env, jobject localRef) { + return std::shared_ptr( + env->NewWeakGlobalRef(localRef), + [](JWeakValue* weak) { // cannot cache env as don't know which thread we're on. + if (weak != nullptr) AndroidRuntime::getJNIEnv()->DeleteWeakGlobalRef(weak); + }); +} + +// std::unique_ptr<> does not store a type-erased deleter like std::shared_ptr<>. +// Define a lambda here to use for the std::unique_ptr<> type definition. +auto LocalRefDeleter = [](JObjectValue* object) { + if (object != nullptr) AndroidRuntime::getJNIEnv()->DeleteLocalRef(object); +}; + +// Create a local reference from another reference. +// This is a unique_ptr to avoid the temptation of sharing with other threads. +// +// This can be used to promote a WeakGlobalRef jweak into a stable LocalRef jobject. +// +inline auto make_unique_localref_from_ref(JNIEnv *env, jobject object) { + return std::unique_ptr( + env->NewLocalRef(object), + LocalRefDeleter); +} + } // namespace static const char* const kAudioAttributesClassPathName = "android/media/AudioAttributes"; @@ -438,10 +475,15 @@ static void android_media_callback(SoundPoolEvent event, SoundPool* soundPool, v return; } JNIEnv *env = AndroidRuntime::getJNIEnv(); - env->CallStaticVoidMethod( - fields.mSoundPoolClass, fields.mPostEvent, - weakRef.get(), event.mMsg, event.mArg1, event.mArg2, - nullptr /* object */); + // "promote" the WeakGlobalRef into a LocalRef. + auto javaSoundPool = make_unique_localref_from_ref(env, weakRef.get()); + if (!javaSoundPool) { + ALOGW("%s: weak reference promotes to null (release() not called?), " + "ignoring callback", __func__); + return; + } + env->CallVoidMethod(javaSoundPool.get(), fields.mPostEvent, + event.mMsg, event.mArg1, event.mArg2, nullptr /* object */); if (env->ExceptionCheck() != JNI_FALSE) { ALOGE("%s: Uncaught exception returned from Java callback", __func__); @@ -451,7 +493,7 @@ static void android_media_callback(SoundPoolEvent event, SoundPool* soundPool, v } static jint -android_media_SoundPool_native_setup(JNIEnv *env, jobject thiz, jobject weakRef, +android_media_SoundPool_native_setup(JNIEnv *env, jobject thiz, jint maxChannels, jobject jaa, jstring opPackageName) { ALOGV("android_media_SoundPool_native_setup"); @@ -485,7 +527,7 @@ android_media_SoundPool_native_setup(JNIEnv *env, jobject thiz, jobject weakRef, auto oldSoundPool = setSoundPool(env, thiz, soundPool); // register Java SoundPool WeakRef using native SoundPool * as the key, for the callback. auto oldSoundPoolJavaRef = getSoundPoolJavaRefManager().set( - soundPool.get(), make_shared_globalref_from_localref(env, weakRef)); + soundPool.get(), make_shared_weakglobalref_from_localref(env, thiz)); ALOGW_IF(oldSoundPool != nullptr, "%s: Aliased SoundPool object %p", __func__, oldSoundPool.get()); @@ -565,7 +607,7 @@ static JNINativeMethod gMethods[] = { (void *)android_media_SoundPool_setRate }, { "native_setup", - "(Ljava/lang/Object;ILjava/lang/Object;Ljava/lang/String;)I", + "(ILjava/lang/Object;Ljava/lang/String;)I", (void*)android_media_SoundPool_native_setup }, { "native_release", @@ -600,8 +642,8 @@ jint JNI_OnLoad(JavaVM* vm, void* /* reserved */) return result; } - fields.mPostEvent = env->GetStaticMethodID(clazz, "postEventFromNative", - "(Ljava/lang/Object;IIILjava/lang/Object;)V"); + fields.mPostEvent = env->GetMethodID( + clazz, "postEventFromNative", "(IIILjava/lang/Object;)V"); if (fields.mPostEvent == nullptr) { ALOGE("Can't find android/media/SoundPool.postEventFromNative"); return result;