diff options
8 files changed, 252 insertions, 46 deletions
diff --git a/media/jni/audioeffect/android_media_AudioEffect.cpp b/media/jni/audioeffect/android_media_AudioEffect.cpp index 0d53ab152129..2691983a0e3a 100644 --- a/media/jni/audioeffect/android_media_AudioEffect.cpp +++ b/media/jni/audioeffect/android_media_AudioEffect.cpp @@ -14,8 +14,8 @@ * limitations under the License. */ - #include <stdio.h> +#include <unordered_set> //#define LOG_NDEBUG 0 #define LOG_TAG "AudioEffects-JNI" @@ -58,22 +58,15 @@ static fields_t fields; struct effect_callback_cookie { jclass audioEffect_class; // AudioEffect class jobject audioEffect_ref; // AudioEffect object instance - }; + bool busy; + Condition cond; +}; // ---------------------------------------------------------------------------- -class AudioEffectJniStorage { - public: - effect_callback_cookie mCallbackData; - - AudioEffectJniStorage() { - } - - ~AudioEffectJniStorage() { - } - +struct AudioEffectJniStorage { + effect_callback_cookie mCallbackData{}; }; - jint AudioEffectJni::translateNativeErrorToJava(int code) { switch(code) { case NO_ERROR: @@ -101,6 +94,7 @@ jint AudioEffectJni::translateNativeErrorToJava(int code) { } static Mutex sLock; +static std::unordered_set<effect_callback_cookie*> sAudioEffectCallBackCookies; // ---------------------------------------------------------------------------- static void effectCallback(int event, void* user, void *info) { @@ -121,7 +115,13 @@ static void effectCallback(int event, void* user, void *info) { ALOGW("effectCallback error user %p, env %p", user, env); return; } - + { + Mutex::Autolock l(sLock); + if (sAudioEffectCallBackCookies.count(callbackInfo) == 0) { + return; + } + callbackInfo->busy = true; + } ALOGV("effectCallback: callbackInfo %p, audioEffect_ref %p audioEffect_class %p", callbackInfo, callbackInfo->audioEffect_ref, @@ -188,6 +188,11 @@ effectCallback_Exit: env->ExceptionDescribe(); env->ExceptionClear(); } + { + Mutex::Autolock l(sLock); + callbackInfo->busy = false; + callbackInfo->cond.broadcast(); + } } // ---------------------------------------------------------------------------- @@ -396,6 +401,10 @@ android_media_AudioEffect_native_setup(JNIEnv *env, jobject thiz, jobject weak_t setAudioEffect(env, thiz, lpAudioEffect); } + { + Mutex::Autolock l(sLock); + sAudioEffectCallBackCookies.insert(&lpJniStorage->mCallbackData); + } env->SetLongField(thiz, fields.fidJniData, (jlong)lpJniStorage); return (jint) AUDIOEFFECT_SUCCESS; @@ -427,6 +436,7 @@ setup_failure: // ---------------------------------------------------------------------------- +#define CALLBACK_COND_WAIT_TIMEOUT_MS 1000 static void android_media_AudioEffect_native_release(JNIEnv *env, jobject thiz) { sp<AudioEffect> lpAudioEffect = setAudioEffect(env, thiz, 0); if (lpAudioEffect == 0) { @@ -442,7 +452,17 @@ static void android_media_AudioEffect_native_release(JNIEnv *env, jobject thiz) env->SetLongField(thiz, fields.fidJniData, 0); if (lpJniStorage) { - ALOGV("deleting pJniStorage: %p\n", lpJniStorage); + Mutex::Autolock l(sLock); + effect_callback_cookie *lpCookie = &lpJniStorage->mCallbackData; + ALOGV("deleting lpJniStorage: %p\n", lpJniStorage); + sAudioEffectCallBackCookies.erase(lpCookie); + while (lpCookie->busy) { + if (lpCookie->cond.waitRelative(sLock, + milliseconds(CALLBACK_COND_WAIT_TIMEOUT_MS)) != + NO_ERROR) { + break; + } + } env->DeleteGlobalRef(lpJniStorage->mCallbackData.audioEffect_class); env->DeleteGlobalRef(lpJniStorage->mCallbackData.audioEffect_ref); delete lpJniStorage; diff --git a/media/jni/audioeffect/android_media_Visualizer.cpp b/media/jni/audioeffect/android_media_Visualizer.cpp index 4c5970a30a05..609fafee4d6a 100644 --- a/media/jni/audioeffect/android_media_Visualizer.cpp +++ b/media/jni/audioeffect/android_media_Visualizer.cpp @@ -15,6 +15,7 @@ */ #include <stdio.h> +#include <unordered_set> //#define LOG_NDEBUG 0 #define LOG_TAG "visualizers-JNI" @@ -63,6 +64,12 @@ struct visualizer_callback_cookie { jclass visualizer_class; // Visualizer class jobject visualizer_ref; // Visualizer object instance + // 'busy_count' and 'cond' together with 'sLock' are used to serialize + // concurrent access to the callback cookie from 'setup'/'release' + // and the callback. + int busy_count; + Condition cond; + // Lazily allocated arrays used to hold callback data provided to java // applications. These arrays are allocated during the first callback and // reallocated when the size of the callback data changes. Allocating on @@ -70,14 +77,12 @@ struct visualizer_callback_cookie { // reference to the provided data (they need to make a copy if they want to // hold onto outside of the callback scope), but it avoids GC thrash caused // by constantly allocating and releasing arrays to hold callback data. + // 'callback_data_lock' must never be held at the same time with 'sLock'. Mutex callback_data_lock; jbyteArray waveform_data; jbyteArray fft_data; - visualizer_callback_cookie() { - waveform_data = NULL; - fft_data = NULL; - } + // Assumes use of default initialization by the client. ~visualizer_callback_cookie() { cleanupBuffers(); @@ -102,15 +107,8 @@ struct visualizer_callback_cookie { }; // ---------------------------------------------------------------------------- -class VisualizerJniStorage { - public: - visualizer_callback_cookie mCallbackData; - - VisualizerJniStorage() { - } - - ~VisualizerJniStorage() { - } +struct VisualizerJniStorage { + visualizer_callback_cookie mCallbackData{}; }; @@ -136,6 +134,7 @@ static jint translateError(int code) { } static Mutex sLock; +static std::unordered_set<visualizer_callback_cookie*> sVisualizerCallBackCookies; // ---------------------------------------------------------------------------- static void ensureArraySize(JNIEnv *env, jbyteArray *array, uint32_t size) { @@ -173,11 +172,19 @@ static void captureCallback(void* user, return; } + { + Mutex::Autolock l(sLock); + if (sVisualizerCallBackCookies.count(callbackInfo) == 0) { + return; + } + callbackInfo->busy_count++; + } ALOGV("captureCallback: callbackInfo %p, visualizer_ref %p visualizer_class %p", callbackInfo, callbackInfo->visualizer_ref, callbackInfo->visualizer_class); + { AutoMutex lock(&callbackInfo->callback_data_lock); if (waveformSize != 0 && waveform != NULL) { @@ -219,11 +226,17 @@ static void captureCallback(void* user, jArray); } } + } // callback_data_lock scope if (env->ExceptionCheck()) { env->ExceptionDescribe(); env->ExceptionClear(); } + { + Mutex::Autolock l(sLock); + callbackInfo->busy_count--; + callbackInfo->cond.broadcast(); + } } // ---------------------------------------------------------------------------- @@ -332,16 +345,41 @@ static void android_media_visualizer_effect_callback(int32_t event, void *info) { if ((event == AudioEffect::EVENT_ERROR) && (*((status_t*)info) == DEAD_OBJECT)) { - VisualizerJniStorage* lpJniStorage = (VisualizerJniStorage*)user; - visualizer_callback_cookie* callbackInfo = &lpJniStorage->mCallbackData; + visualizer_callback_cookie* callbackInfo = + (visualizer_callback_cookie *)user; JNIEnv *env = AndroidRuntime::getJNIEnv(); + if (!user || !env) { + ALOGW("effectCallback error user %p, env %p", user, env); + return; + } + { + Mutex::Autolock l(sLock); + if (sVisualizerCallBackCookies.count(callbackInfo) == 0) { + return; + } + callbackInfo->busy_count++; + } + ALOGV("effectCallback: callbackInfo %p, visualizer_ref %p visualizer_class %p", + callbackInfo, + callbackInfo->visualizer_ref, + callbackInfo->visualizer_class); + env->CallStaticVoidMethod( callbackInfo->visualizer_class, fields.midPostNativeEvent, callbackInfo->visualizer_ref, NATIVE_EVENT_SERVER_DIED, 0, NULL); + if (env->ExceptionCheck()) { + env->ExceptionDescribe(); + env->ExceptionClear(); + } + { + Mutex::Autolock l(sLock); + callbackInfo->busy_count--; + callbackInfo->cond.broadcast(); + } } } @@ -389,7 +427,7 @@ android_media_visualizer_native_setup(JNIEnv *env, jobject thiz, jobject weak_th } lpVisualizer->set(0, android_media_visualizer_effect_callback, - lpJniStorage, + &lpJniStorage->mCallbackData, (audio_session_t) sessionId); lStatus = translateError(lpVisualizer->initCheck()); @@ -410,6 +448,10 @@ android_media_visualizer_native_setup(JNIEnv *env, jobject thiz, jobject weak_th setVisualizer(env, thiz, lpVisualizer); + { + Mutex::Autolock l(sLock); + sVisualizerCallBackCookies.insert(&lpJniStorage->mCallbackData); + } env->SetLongField(thiz, fields.fidJniData, (jlong)lpJniStorage); return VISUALIZER_SUCCESS; @@ -432,13 +474,15 @@ setup_failure: } // ---------------------------------------------------------------------------- +#define CALLBACK_COND_WAIT_TIMEOUT_MS 1000 static void android_media_visualizer_native_release(JNIEnv *env, jobject thiz) { - { //limit scope so that lpVisualizer is deleted before JNI storage data. + { sp<Visualizer> lpVisualizer = setVisualizer(env, thiz, 0); if (lpVisualizer == 0) { return; } lpVisualizer->release(); + // Visualizer can still can be held by AudioEffect::EffectClient } // delete the JNI data VisualizerJniStorage* lpJniStorage = @@ -449,9 +493,22 @@ static void android_media_visualizer_native_release(JNIEnv *env, jobject thiz) env->SetLongField(thiz, fields.fidJniData, 0); if (lpJniStorage) { + { + Mutex::Autolock l(sLock); + visualizer_callback_cookie *lpCookie = &lpJniStorage->mCallbackData; ALOGV("deleting pJniStorage: %p\n", lpJniStorage); + sVisualizerCallBackCookies.erase(lpCookie); + while (lpCookie->busy_count > 0) { + if (lpCookie->cond.waitRelative(sLock, + milliseconds(CALLBACK_COND_WAIT_TIMEOUT_MS)) != + NO_ERROR) { + break; + } + } + ALOG_ASSERT(lpCookie->busy_count == 0, "Unbalanced busy_count inc/dec"); env->DeleteGlobalRef(lpJniStorage->mCallbackData.visualizer_class); env->DeleteGlobalRef(lpJniStorage->mCallbackData.visualizer_ref); + } // sLock scope delete lpJniStorage; } } @@ -707,4 +764,3 @@ int register_android_media_visualizer(JNIEnv *env) { return AndroidRuntime::registerNativeMethods(env, kClassPathName, gMethods, NELEM(gMethods)); } - diff --git a/media/tests/EffectsTest/AndroidManifest.xml b/media/tests/EffectsTest/AndroidManifest.xml index 9b59891fb279..ad0c10ee3807 100644 --- a/media/tests/EffectsTest/AndroidManifest.xml +++ b/media/tests/EffectsTest/AndroidManifest.xml @@ -13,6 +13,10 @@ See the License for the specific language governing permissions and limitations under the License. --> +<!-- +Make sure to enable access to the mic in settings and run: +adb shell am compat enable ALLOW_TEST_API_ACCESS com.android.effectstest +--> <manifest xmlns:android="http://schemas.android.com/apk/res/android" package="com.android.effectstest"> diff --git a/media/tests/EffectsTest/res/layout/bassboosttest.xml b/media/tests/EffectsTest/res/layout/bassboosttest.xml index ac912c84d107..5f9132cb8cc7 100644 --- a/media/tests/EffectsTest/res/layout/bassboosttest.xml +++ b/media/tests/EffectsTest/res/layout/bassboosttest.xml @@ -187,6 +187,11 @@ android:layout_height="wrap_content" android:scaleType="fitXY"/> + <Button android:id="@+id/hammer_on_release_bug" + android:layout_width="fill_parent" android:layout_height="wrap_content" + android:text="@string/hammer_on_release_bug_name"> + </Button> + </LinearLayout> </ScrollView> diff --git a/media/tests/EffectsTest/res/layout/visualizertest.xml b/media/tests/EffectsTest/res/layout/visualizertest.xml index 18d7a3648fbf..85dabbc115f3 100644 --- a/media/tests/EffectsTest/res/layout/visualizertest.xml +++ b/media/tests/EffectsTest/res/layout/visualizertest.xml @@ -175,6 +175,11 @@ </LinearLayout> + <Button android:id="@+id/hammer_on_release_bug" + android:layout_width="fill_parent" android:layout_height="wrap_content" + android:text="@string/hammer_on_release_bug_name"> + </Button> + <ImageView android:src="@android:drawable/divider_horizontal_dark" android:layout_width="fill_parent" diff --git a/media/tests/EffectsTest/res/values/strings.xml b/media/tests/EffectsTest/res/values/strings.xml index 7c12da1274e3..a44c7e93382a 100644 --- a/media/tests/EffectsTest/res/values/strings.xml +++ b/media/tests/EffectsTest/res/values/strings.xml @@ -37,4 +37,6 @@ <string name="send_level_name">Send Level</string> <!-- Toggles use of a multi-threaded client for an effect [CHAR LIMIT=24] --> <string name="effect_multithreaded">Multithreaded Use</string> + <!-- Runs a stress test for a bug related to simultaneous release of multiple effect instances [CHAR LIMIT=24] --> + <string name="hammer_on_release_bug_name">Hammer on release()</string> </resources> diff --git a/media/tests/EffectsTest/src/com/android/effectstest/BassBoostTest.java b/media/tests/EffectsTest/src/com/android/effectstest/BassBoostTest.java index cce2acc5869a..a207bf1d5359 100644 --- a/media/tests/EffectsTest/src/com/android/effectstest/BassBoostTest.java +++ b/media/tests/EffectsTest/src/com/android/effectstest/BassBoostTest.java @@ -17,29 +17,24 @@ package com.android.effectstest; import android.app.Activity; -import android.content.Context; -import android.content.Intent; +import android.media.audiofx.AudioEffect; +import android.media.audiofx.BassBoost; import android.os.Bundle; import android.util.Log; import android.view.KeyEvent; -import android.view.Menu; -import android.view.View.OnClickListener; import android.view.View; -import android.view.ViewGroup; +import android.view.View.OnClickListener; import android.widget.Button; -import android.widget.TextView; +import android.widget.CompoundButton; +import android.widget.CompoundButton.OnCheckedChangeListener; import android.widget.EditText; import android.widget.SeekBar; +import android.widget.TextView; import android.widget.ToggleButton; -import android.widget.CompoundButton; -import android.widget.CompoundButton.OnCheckedChangeListener; -import java.nio.ByteOrder; + import java.nio.ByteBuffer; +import java.nio.ByteOrder; import java.util.HashMap; -import java.util.Map; - -import android.media.audiofx.BassBoost; -import android.media.audiofx.AudioEffect; public class BassBoostTest extends Activity implements OnCheckedChangeListener { @@ -78,6 +73,9 @@ public class BassBoostTest extends Activity implements OnCheckedChangeListener { mReleaseButton = (ToggleButton)findViewById(R.id.bbReleaseButton); mOnOffButton = (ToggleButton)findViewById(R.id.bassboostOnOff); + final Button hammerReleaseTest = (Button) findViewById(R.id.hammer_on_release_bug); + hammerReleaseTest.setEnabled(false); + getEffect(sSession); if (mBassBoost != null) { @@ -93,6 +91,14 @@ public class BassBoostTest extends Activity implements OnCheckedChangeListener { mStrength = new BassBoostParam(mBassBoost, 0, 1000, seekBar, textView); seekBar.setOnSeekBarChangeListener(mStrength); mStrength.setEnabled(mBassBoost.getStrengthSupported()); + + hammerReleaseTest.setEnabled(true); + hammerReleaseTest.setOnClickListener(new OnClickListener() { + @Override + public void onClick(View v) { + runHammerReleaseTest(hammerReleaseTest); + } + }); } } @@ -273,4 +279,52 @@ public class BassBoostTest extends Activity implements OnCheckedChangeListener { } } + // Stress-tests releasing of AudioEffect by doing repeated creation + // and subsequent releasing. Also forces emission of callbacks from + // the AudioFlinger by setting a control status listener. Since all + // effect instances are bound to the same session, the AF will + // notify them about the change in their status. This can reveal racy + // behavior w.r.t. releasing. + class HammerReleaseTest extends Thread { + private static final int NUM_EFFECTS = 10; + private static final int NUM_ITERATIONS = 100; + private final int mSession; + private final Runnable mOnComplete; + + HammerReleaseTest(int session, Runnable onComplete) { + mSession = session; + mOnComplete = onComplete; + } + + @Override + public void run() { + Log.w(TAG, "HammerReleaseTest started"); + BassBoost[] effects = new BassBoost[NUM_EFFECTS]; + for (int i = 0; i < NUM_ITERATIONS; i++) { + for (int j = 0; j < NUM_EFFECTS; j++) { + effects[j] = new BassBoost(0, mSession); + effects[j].setControlStatusListener(mEffectListener); + yield(); + } + for (int j = NUM_EFFECTS - 1; j >= 0; j--) { + Log.w(TAG, "HammerReleaseTest releasing effect " + (Object) effects[j]); + effects[j].release(); + effects[j] = null; + yield(); + } + } + Log.w(TAG, "HammerReleaseTest ended"); + runOnUiThread(mOnComplete); + } + } + + private void runHammerReleaseTest(Button controlButton) { + controlButton.setEnabled(false); + HammerReleaseTest thread = new HammerReleaseTest(sSession, + () -> { + controlButton.setEnabled(true); + }); + thread.start(); + } + } diff --git a/media/tests/EffectsTest/src/com/android/effectstest/VisualizerTest.java b/media/tests/EffectsTest/src/com/android/effectstest/VisualizerTest.java index 2e141c5ef7c8..dcfe11a79ef9 100644 --- a/media/tests/EffectsTest/src/com/android/effectstest/VisualizerTest.java +++ b/media/tests/EffectsTest/src/com/android/effectstest/VisualizerTest.java @@ -17,6 +17,7 @@ package com.android.effectstest; import android.app.Activity; +import android.media.audiofx.Visualizer; import android.os.Bundle; import android.os.Handler; import android.os.Looper; @@ -24,6 +25,8 @@ import android.os.Message; import android.util.Log; import android.view.KeyEvent; import android.view.View; +import android.view.View.OnClickListener; +import android.widget.Button; import android.widget.CompoundButton; import android.widget.CompoundButton.OnCheckedChangeListener; import android.widget.EditText; @@ -74,11 +77,22 @@ public class VisualizerTest extends Activity implements OnCheckedChangeListener mCallbackOn = false; mCallbackButton.setChecked(mCallbackOn); + final Button hammerReleaseTest = (Button) findViewById(R.id.hammer_on_release_bug); + hammerReleaseTest.setEnabled(false); + mMultithreadedButton.setOnCheckedChangeListener(this); if (getEffect(sSession) != null) { mReleaseButton.setOnCheckedChangeListener(this); mOnOffButton.setOnCheckedChangeListener(this); mCallbackButton.setOnCheckedChangeListener(this); + + hammerReleaseTest.setEnabled(true); + hammerReleaseTest.setOnClickListener(new OnClickListener() { + @Override + public void onClick(View v) { + runHammerReleaseTest(hammerReleaseTest); + } + }); } } @@ -214,4 +228,50 @@ public class VisualizerTest extends Activity implements OnCheckedChangeListener } } + // Stress-tests releasing of AudioEffect by doing repeated creation + // and subsequent releasing. Unlike a similar class in BassBoostTest, + // this one doesn't sets a control status listener because Visualizer + // doesn't inherit from AudioEffect and doesn't implement this method + // by itself. + class HammerReleaseTest extends Thread { + private static final int NUM_EFFECTS = 10; + private static final int NUM_ITERATIONS = 100; + private final int mSession; + private final Runnable mOnComplete; + + HammerReleaseTest(int session, Runnable onComplete) { + mSession = session; + mOnComplete = onComplete; + } + + @Override + public void run() { + Log.w(TAG, "HammerReleaseTest started"); + Visualizer[] effects = new Visualizer[NUM_EFFECTS]; + for (int i = 0; i < NUM_ITERATIONS; i++) { + for (int j = 0; j < NUM_EFFECTS; j++) { + effects[j] = new Visualizer(mSession); + yield(); + } + for (int j = NUM_EFFECTS - 1; j >= 0; j--) { + Log.w(TAG, "HammerReleaseTest releasing effect " + (Object) effects[j]); + effects[j].release(); + effects[j] = null; + yield(); + } + } + Log.w(TAG, "HammerReleaseTest ended"); + runOnUiThread(mOnComplete); + } + } + + private void runHammerReleaseTest(Button controlButton) { + controlButton.setEnabled(false); + HammerReleaseTest thread = new HammerReleaseTest(sSession, + () -> { + controlButton.setEnabled(true); + }); + thread.start(); + } + } |