audio policy: fix lockup during mediaserver restart

When pre or post processing effects are applied by audio policy service
automatically there is a potential lockup if the first call into
AudioPolicyService is made while creating one of those effects.
This is because effects are created with AudioPolicyService mutex held and
effect creation calls into methods (e.g registerClient()) which also acquire
the mutex.

The fix consists in adding a new mutex to AudioPolicyEffects class and not
hold the AudioPolicyService mutex when calling methods in of class.

Bug: 17830596.
Change-Id: Ie61c3671d3147f46e99ba208f39c1a3bf180779f

5 files changed, 186 insertions, 115 deletions

diff --git a/services/audiopolicy/AudioPolicyEffects.cpp b/services/audiopolicy/AudioPolicyEffects.cpp
index c45acd0..3c1c042 100644
--- a/services/audiopolicy/AudioPolicyEffects.cpp
+++ b/services/audiopolicy/AudioPolicyEffects.cpp
@@ -87,6 +87,7 @@ status_t AudioPolicyEffects::addInputEffects(audio_io_handle_t input,
     audio_source_t aliasSource = (inputSource == AUDIO_SOURCE_HOTWORD) ?
                                     AUDIO_SOURCE_VOICE_RECOGNITION : inputSource;
 
+    Mutex::Autolock _l(mLock);
     ssize_t index = mInputSources.indexOfKey(aliasSource);
     if (index < 0) {
         ALOGV("addInputEffects(): no processing needs to be attached to this source");
@@ -122,7 +123,7 @@ status_t AudioPolicyEffects::addInputEffects(audio_io_handle_t input,
         ALOGV("addInputEffects(): added Fx %s on source: %d", effect->mName, (int32_t)aliasSource);
         inputDesc->mEffects.add(fx);
     }
-    setProcessorEnabled(inputDesc, true);
+    inputDesc->setProcessorEnabled(true);
 
     return status;
 }
@@ -132,6 +133,7 @@ status_t AudioPolicyEffects::releaseInputEffects(audio_io_handle_t input)
 {
     status_t status = NO_ERROR;
 
+    Mutex::Autolock _l(mLock);
     ssize_t index = mInputs.indexOfKey(input);
     if (index < 0) {
         return status;
@@ -140,7 +142,7 @@ status_t AudioPolicyEffects::releaseInputEffects(audio_io_handle_t input)
     inputDesc->mRefCount--;
     ALOGV("releaseInputEffects(): input: %d, refCount: %d", input, inputDesc->mRefCount);
     if (inputDesc->mRefCount == 0) {
-        setProcessorEnabled(inputDesc, false);
+        inputDesc->setProcessorEnabled(false);
         delete inputDesc;
         mInputs.removeItemsAt(index);
         ALOGV("releaseInputEffects(): all effects released");
@@ -154,6 +156,7 @@ status_t AudioPolicyEffects::queryDefaultInputEffects(int audioSession,
 {
     status_t status = NO_ERROR;
 
+    Mutex::Autolock _l(mLock);
     size_t index;
     for (index = 0; index < mInputs.size(); index++) {
         if (mInputs.valueAt(index)->mSessionId == audioSession) {
@@ -186,6 +189,7 @@ status_t AudioPolicyEffects::queryDefaultOutputSessionEffects(int audioSession,
 {
     status_t status = NO_ERROR;
 
+    Mutex::Autolock _l(mLock);
     size_t index;
     for (index = 0; index < mOutputSessions.size(); index++) {
         if (mOutputSessions.valueAt(index)->mSessionId == audioSession) {
@@ -218,6 +222,7 @@ status_t AudioPolicyEffects::addOutputSessionEffects(audio_io_handle_t output,
 {
     status_t status = NO_ERROR;
 
+    Mutex::Autolock _l(mLock);
     // create audio processors according to stream
     ssize_t index = mOutputStreams.indexOfKey(stream);
     if (index < 0) {
@@ -254,7 +259,7 @@ status_t AudioPolicyEffects::addOutputSessionEffects(audio_io_handle_t output,
         procDesc->mEffects.add(fx);
     }
 
-    setProcessorEnabled(procDesc, true);
+    procDesc->setProcessorEnabled(true);
 
     return status;
 }
@@ -267,6 +272,7 @@ status_t AudioPolicyEffects::releaseOutputSessionEffects(audio_io_handle_t outpu
     (void) output; // argument not used for now
     (void) stream; // argument not used for now
 
+    Mutex::Autolock _l(mLock);
     ssize_t index = mOutputSessions.indexOfKey(audioSession);
     if (index < 0) {
         ALOGV("releaseOutputSessionEffects: no output processing was attached to this stream");
@@ -277,7 +283,7 @@ status_t AudioPolicyEffects::releaseOutputSessionEffects(audio_io_handle_t outpu
     procDesc->mRefCount--;
     ALOGV("releaseOutputSessionEffects(): session: %d, refCount: %d", audioSession, procDesc->mRefCount);
     if (procDesc->mRefCount == 0) {
-        setProcessorEnabled(procDesc, false);
+        procDesc->setProcessorEnabled(false);
         procDesc->mEffects.clear();
         delete procDesc;
         mOutputSessions.removeItemsAt(index);
@@ -288,11 +294,10 @@ status_t AudioPolicyEffects::releaseOutputSessionEffects(audio_io_handle_t outpu
 }
 
 
-void AudioPolicyEffects::setProcessorEnabled(const EffectVector *effectVector, bool enabled)
+void AudioPolicyEffects::EffectVector::setProcessorEnabled(bool enabled)
 {
-    const Vector<sp<AudioEffect> > &fxVector = effectVector->mEffects;
-    for (size_t i = 0; i < fxVector.size(); i++) {
-        fxVector.itemAt(i)->setEnabled(enabled);
+    for (size_t i = 0; i < mEffects.size(); i++) {
+        mEffects.itemAt(i)->setEnabled(enabled);
     }
 }
 
@@ -313,7 +318,7 @@ void AudioPolicyEffects::setProcessorEnabled(const EffectVector *effectVector, b
 
 // returns the audio_source_t enum corresponding to the input source name or
 // AUDIO_SOURCE_CNT is no match found
-audio_source_t AudioPolicyEffects::inputSourceNameToEnum(const char *name)
+/*static*/ audio_source_t AudioPolicyEffects::inputSourceNameToEnum(const char *name)
 {
     int i;
     for (i = AUDIO_SOURCE_MIC; i < AUDIO_SOURCE_CNT; i++) {
diff --git a/services/audiopolicy/AudioPolicyEffects.h b/services/audiopolicy/AudioPolicyEffects.h
index dbe0d0e..6b0d538 100644
--- a/services/audiopolicy/AudioPolicyEffects.h
+++ b/services/audiopolicy/AudioPolicyEffects.h
@@ -45,6 +45,10 @@ public:
 	         AudioPolicyEffects();
     virtual ~AudioPolicyEffects();
 
+    // NOTE: methods on AudioPolicyEffects should never be called with the AudioPolicyService
+    // main mutex (mLock) held as they will indirectly call back into AudioPolicyService when
+    // managing audio effects.
+
     // Return a list of effect descriptors for default input effects
     // associated with audioSession
     status_t queryDefaultInputEffects(int audioSession,
@@ -133,6 +137,10 @@ private:
     public:
         EffectVector(int session) : mSessionId(session), mRefCount(0) {}
         /*virtual*/ ~EffectVector() {}
+
+        // Enable or disable all effects in effect vector
+        void setProcessorEnabled(bool enabled);
+
         const int mSessionId;
         // AudioPolicyManager keeps mLock, no need for lock on reference count here
         int mRefCount;
@@ -141,14 +149,11 @@ private:
 
 
     static const char * const kInputSourceNames[AUDIO_SOURCE_CNT -1];
-    audio_source_t inputSourceNameToEnum(const char *name);
+    static audio_source_t inputSourceNameToEnum(const char *name);
 
     static const char *kStreamNames[AUDIO_STREAM_CNT+1]; //+1 required as streams start from -1
     audio_stream_type_t streamNameToEnum(const char *name);
 
-    // Enable or disable all effects in effect vector
-    void setProcessorEnabled(const EffectVector *effectVector, bool enabled);
-
     // Parse audio_effects.conf
     status_t loadAudioEffectConfig(const char *path);
 
@@ -173,6 +178,8 @@ private:
                          size_t *curSize,
                          size_t *totSize);
 
+    // protects access to mInputSources, mInputs, mOutputStreams, mOutputSessions
+    Mutex mLock;
     // Automatic input effects are configured per audio_source_t
     KeyedVector< audio_source_t, EffectDescVector* > mInputSources;
     // Automatic input effects are unique for audio_io_handle_t
diff --git a/services/audiopolicy/AudioPolicyInterfaceImpl.cpp b/services/audiopolicy/AudioPolicyInterfaceImpl.cpp
index b212ca6..6cd0ac8 100644
--- a/services/audiopolicy/AudioPolicyInterfaceImpl.cpp
+++ b/services/audiopolicy/AudioPolicyInterfaceImpl.cpp
@@ -162,14 +162,19 @@ status_t AudioPolicyService::startOutput(audio_io_handle_t output,
         return NO_INIT;
     }
     ALOGV("startOutput()");
-    Mutex::Autolock _l(mLock);
-
-    // create audio processors according to stream
-    status_t status = mAudioPolicyEffects->addOutputSessionEffects(output, stream, session);
-    if (status != NO_ERROR && status != ALREADY_EXISTS) {
-        ALOGW("Failed to add effects on session %d", session);
+    sp<AudioPolicyEffects>audioPolicyEffects;
+    {
+        Mutex::Autolock _l(mLock);
+        audioPolicyEffects = mAudioPolicyEffects;
+    }
+    if (audioPolicyEffects != 0) {
+        // create audio processors according to stream
+        status_t status = audioPolicyEffects->addOutputSessionEffects(output, stream, session);
+        if (status != NO_ERROR && status != ALREADY_EXISTS) {
+            ALOGW("Failed to add effects on session %d", session);
+        }
     }
-
+    Mutex::Autolock _l(mLock);
     return mAudioPolicyManager->startOutput(output, stream, session);
 }
 
@@ -190,14 +195,19 @@ status_t  AudioPolicyService::doStopOutput(audio_io_handle_t output,
                                       int session)
 {
     ALOGV("doStopOutput from tid %d", gettid());
-    Mutex::Autolock _l(mLock);
-
-    // release audio processors from the stream
-    status_t status = mAudioPolicyEffects->releaseOutputSessionEffects(output, stream, session);
-    if (status != NO_ERROR && status != ALREADY_EXISTS) {
-        ALOGW("Failed to release effects on session %d", session);
+    sp<AudioPolicyEffects>audioPolicyEffects;
+    {
+        Mutex::Autolock _l(mLock);
+        audioPolicyEffects = mAudioPolicyEffects;
+    }
+    if (audioPolicyEffects != 0) {
+        // release audio processors from the stream
+        status_t status = audioPolicyEffects->releaseOutputSessionEffects(output, stream, session);
+        if (status != NO_ERROR && status != ALREADY_EXISTS) {
+            ALOGW("Failed to release effects on session %d", session);
+        }
     }
-
+    Mutex::Autolock _l(mLock);
     return mAudioPolicyManager->stopOutput(output, stream, session);
 }
 
@@ -235,23 +245,26 @@ audio_io_handle_t AudioPolicyService::getInput(audio_source_t inputSource,
     if ((inputSource == AUDIO_SOURCE_HOTWORD) && !captureHotwordAllowed()) {
         return 0;
     }
-
-    Mutex::Autolock _l(mLock);
-    // the audio_in_acoustics_t parameter is ignored by get_input()
-    audio_io_handle_t input = mAudioPolicyManager->getInput(inputSource, samplingRate,
-                                                   format, channelMask,
-                                                   (audio_session_t)audioSession, flags);
-
+    audio_io_handle_t input;
+    sp<AudioPolicyEffects>audioPolicyEffects;
+    {
+        Mutex::Autolock _l(mLock);
+        // the audio_in_acoustics_t parameter is ignored by get_input()
+        input = mAudioPolicyManager->getInput(inputSource, samplingRate,
+                                                       format, channelMask,
+                                                       (audio_session_t)audioSession, flags);
+        audioPolicyEffects = mAudioPolicyEffects;
+    }
     if (input == 0) {
         return input;
     }
-
-    // create audio pre processors according to input source
-    status_t status = mAudioPolicyEffects->addInputEffects(input, inputSource, audioSession);
-    if (status != NO_ERROR && status != ALREADY_EXISTS) {
-        ALOGW("Failed to add effects on input %d", input);
+    if (audioPolicyEffects != 0) {
+        // create audio pre processors according to input source
+        status_t status = audioPolicyEffects->addInputEffects(input, inputSource, audioSession);
+        if (status != NO_ERROR && status != ALREADY_EXISTS) {
+            ALOGW("Failed to add effects on input %d", input);
+        }
     }
-
     return input;
 }
 
@@ -283,13 +296,18 @@ void AudioPolicyService::releaseInput(audio_io_handle_t input,
     if (mAudioPolicyManager == NULL) {
         return;
     }
-    Mutex::Autolock _l(mLock);
-    mAudioPolicyManager->releaseInput(input, session);
-
-    // release audio processors from the input
-    status_t status = mAudioPolicyEffects->releaseInputEffects(input);
-    if(status != NO_ERROR) {
-        ALOGW("Failed to release effects on input %d", input);
+    sp<AudioPolicyEffects>audioPolicyEffects;
+    {
+        Mutex::Autolock _l(mLock);
+        mAudioPolicyManager->releaseInput(input, session);
+        audioPolicyEffects = mAudioPolicyEffects;
+    }
+    if (audioPolicyEffects != 0) {
+        // release audio processors from the input
+        status_t status = audioPolicyEffects->releaseInputEffects(input);
+        if(status != NO_ERROR) {
+            ALOGW("Failed to release effects on input %d", input);
+        }
     }
 }
 
@@ -437,9 +455,16 @@ status_t AudioPolicyService::queryDefaultPreProcessing(int audioSession,
         *count = 0;
         return NO_INIT;
     }
-    Mutex::Autolock _l(mLock);
-
-    return mAudioPolicyEffects->queryDefaultInputEffects(audioSession, descriptors, count);
+    sp<AudioPolicyEffects>audioPolicyEffects;
+    {
+        Mutex::Autolock _l(mLock);
+        audioPolicyEffects = mAudioPolicyEffects;
+    }
+    if (audioPolicyEffects == 0) {
+        *count = 0;
+        return NO_INIT;
+    }
+    return audioPolicyEffects->queryDefaultInputEffects(audioSession, descriptors, count);
 }
 
 bool AudioPolicyService::isOffloadSupported(const audio_offload_info_t& info)
diff --git a/services/audiopolicy/AudioPolicyInterfaceImplLegacy.cpp b/services/audiopolicy/AudioPolicyInterfaceImplLegacy.cpp
index 1e40bc3..9f66fd1 100644
--- a/services/audiopolicy/AudioPolicyInterfaceImplLegacy.cpp
+++ b/services/audiopolicy/AudioPolicyInterfaceImplLegacy.cpp
@@ -150,14 +150,20 @@ status_t AudioPolicyService::startOutput(audio_io_handle_t output,
         return NO_INIT;
     }
     ALOGV("startOutput()");
-    Mutex::Autolock _l(mLock);
-
     // create audio processors according to stream
-    status_t status = mAudioPolicyEffects->addOutputSessionEffects(output, stream, session);
-    if (status != NO_ERROR && status != ALREADY_EXISTS) {
-        ALOGW("Failed to add effects on session %d", session);
+    sp<AudioPolicyEffects>audioPolicyEffects;
+    {
+        Mutex::Autolock _l(mLock);
+        audioPolicyEffects = mAudioPolicyEffects;
+    }
+    if (audioPolicyEffects != 0) {
+        status_t status = audioPolicyEffects->addOutputSessionEffects(output, stream, session);
+        if (status != NO_ERROR && status != ALREADY_EXISTS) {
+            ALOGW("Failed to add effects on session %d", session);
+        }
     }
 
+    Mutex::Autolock _l(mLock);
     return mpAudioPolicy->start_output(mpAudioPolicy, output, stream, session);
 }
 
@@ -178,14 +184,19 @@ status_t  AudioPolicyService::doStopOutput(audio_io_handle_t output,
                                       int session)
 {
     ALOGV("doStopOutput from tid %d", gettid());
-    Mutex::Autolock _l(mLock);
-
     // release audio processors from the stream
-    status_t status = mAudioPolicyEffects->releaseOutputSessionEffects(output, stream, session);
-    if (status != NO_ERROR && status != ALREADY_EXISTS) {
-        ALOGW("Failed to release effects on session %d", session);
+    sp<AudioPolicyEffects>audioPolicyEffects;
+    {
+        Mutex::Autolock _l(mLock);
+        audioPolicyEffects = mAudioPolicyEffects;
     }
-
+    if (audioPolicyEffects != 0) {
+        status_t status = audioPolicyEffects->releaseOutputSessionEffects(output, stream, session);
+        if (status != NO_ERROR && status != ALREADY_EXISTS) {
+            ALOGW("Failed to release effects on session %d", session);
+        }
+    }
+    Mutex::Autolock _l(mLock);
     return mpAudioPolicy->stop_output(mpAudioPolicy, output, stream, session);
 }
 
@@ -224,21 +235,26 @@ audio_io_handle_t AudioPolicyService::getInput(audio_source_t inputSource,
         return 0;
     }
 
-    Mutex::Autolock _l(mLock);
-    // the audio_in_acoustics_t parameter is ignored by get_input()
-    audio_io_handle_t input = mpAudioPolicy->get_input(mpAudioPolicy, inputSource, samplingRate,
-                                                   format, channelMask, (audio_in_acoustics_t) 0);
-
+    audio_io_handle_t input;
+    sp<AudioPolicyEffects>audioPolicyEffects;
+    {
+        Mutex::Autolock _l(mLock);
+        // the audio_in_acoustics_t parameter is ignored by get_input()
+        input = mpAudioPolicy->get_input(mpAudioPolicy, inputSource, samplingRate,
+                                             format, channelMask, (audio_in_acoustics_t) 0);
+        audioPolicyEffects = mAudioPolicyEffects;
+    }
     if (input == 0) {
         return input;
     }
 
-    // create audio pre processors according to input source
-    status_t status = mAudioPolicyEffects->addInputEffects(input, inputSource, audioSession);
-    if (status != NO_ERROR && status != ALREADY_EXISTS) {
-        ALOGW("Failed to add effects on input %d", input);
+    if (audioPolicyEffects != 0) {
+        // create audio pre processors according to input source
+        status_t status = audioPolicyEffects->addInputEffects(input, inputSource, audioSession);
+        if (status != NO_ERROR && status != ALREADY_EXISTS) {
+            ALOGW("Failed to add effects on input %d", input);
+        }
     }
-
     return input;
 }
 
@@ -270,13 +286,19 @@ void AudioPolicyService::releaseInput(audio_io_handle_t input,
     if (mpAudioPolicy == NULL) {
         return;
     }
-    Mutex::Autolock _l(mLock);
-    mpAudioPolicy->release_input(mpAudioPolicy, input);
 
-    // release audio processors from the input
-    status_t status = mAudioPolicyEffects->releaseInputEffects(input);
-    if(status != NO_ERROR) {
-        ALOGW("Failed to release effects on input %d", input);
+    sp<AudioPolicyEffects>audioPolicyEffects;
+    {
+        Mutex::Autolock _l(mLock);
+        mpAudioPolicy->release_input(mpAudioPolicy, input);
+        audioPolicyEffects = mAudioPolicyEffects;
+    }
+    if (audioPolicyEffects != 0) {
+        // release audio processors from the input
+        status_t status = audioPolicyEffects->releaseInputEffects(input);
+        if(status != NO_ERROR) {
+            ALOGW("Failed to release effects on input %d", input);
+        }
     }
 }
 
@@ -437,9 +459,16 @@ status_t AudioPolicyService::queryDefaultPreProcessing(int audioSession,
         *count = 0;
         return NO_INIT;
     }
-    Mutex::Autolock _l(mLock);
-
-    return mAudioPolicyEffects->queryDefaultInputEffects(audioSession, descriptors, count);
+    sp<AudioPolicyEffects>audioPolicyEffects;
+    {
+        Mutex::Autolock _l(mLock);
+        audioPolicyEffects = mAudioPolicyEffects;
+    }
+    if (audioPolicyEffects == 0) {
+        *count = 0;
+        return NO_INIT;
+    }
+    return audioPolicyEffects->queryDefaultInputEffects(audioSession, descriptors, count);
 }
 
 bool AudioPolicyService::isOffloadSupported(const audio_offload_info_t& info)
diff --git a/services/audiopolicy/AudioPolicyService.cpp b/services/audiopolicy/AudioPolicyService.cpp
index 647cda4..4761a13 100644
--- a/services/audiopolicy/AudioPolicyService.cpp
+++ b/services/audiopolicy/AudioPolicyService.cpp
@@ -66,51 +66,56 @@ AudioPolicyService::AudioPolicyService()
     int forced_val;
     int rc;
 
-    Mutex::Autolock _l(mLock);
+    {
+        Mutex::Autolock _l(mLock);
 
-    // start tone playback thread
-    mTonePlaybackThread = new AudioCommandThread(String8("ApmTone"), this);
-    // start audio commands thread
-    mAudioCommandThread = new AudioCommandThread(String8("ApmAudio"), this);
-    // start output activity command thread
-    mOutputCommandThread = new AudioCommandThread(String8("ApmOutput"), this);
+        // start tone playback thread
+        mTonePlaybackThread = new AudioCommandThread(String8("ApmTone"), this);
+        // start audio commands thread
+        mAudioCommandThread = new AudioCommandThread(String8("ApmAudio"), this);
+        // start output activity command thread
+        mOutputCommandThread = new AudioCommandThread(String8("ApmOutput"), this);
 
 #ifdef USE_LEGACY_AUDIO_POLICY
-    ALOGI("AudioPolicyService CSTOR in legacy mode");
+        ALOGI("AudioPolicyService CSTOR in legacy mode");
 
-    /* instantiate the audio policy manager */
-    rc = hw_get_module(AUDIO_POLICY_HARDWARE_MODULE_ID, &module);
-    if (rc) {
-        return;
-    }
-    rc = audio_policy_dev_open(module, &mpAudioPolicyDev);
-    ALOGE_IF(rc, "couldn't open audio policy device (%s)", strerror(-rc));
-    if (rc) {
-        return;
-    }
+        /* instantiate the audio policy manager */
+        rc = hw_get_module(AUDIO_POLICY_HARDWARE_MODULE_ID, &module);
+        if (rc) {
+            return;
+        }
+        rc = audio_policy_dev_open(module, &mpAudioPolicyDev);
+        ALOGE_IF(rc, "couldn't open audio policy device (%s)", strerror(-rc));
+        if (rc) {
+            return;
+        }
 
-    rc = mpAudioPolicyDev->create_audio_policy(mpAudioPolicyDev, &aps_ops, this,
-                                               &mpAudioPolicy);
-    ALOGE_IF(rc, "couldn't create audio policy (%s)", strerror(-rc));
-    if (rc) {
-        return;
-    }
+        rc = mpAudioPolicyDev->create_audio_policy(mpAudioPolicyDev, &aps_ops, this,
+                                                   &mpAudioPolicy);
+        ALOGE_IF(rc, "couldn't create audio policy (%s)", strerror(-rc));
+        if (rc) {
+            return;
+        }
 
-    rc = mpAudioPolicy->init_check(mpAudioPolicy);
-    ALOGE_IF(rc, "couldn't init_check the audio policy (%s)", strerror(-rc));
-    if (rc) {
-        return;
-    }
-    ALOGI("Loaded audio policy from %s (%s)", module->name, module->id);
+        rc = mpAudioPolicy->init_check(mpAudioPolicy);
+        ALOGE_IF(rc, "couldn't init_check the audio policy (%s)", strerror(-rc));
+        if (rc) {
+            return;
+        }
+        ALOGI("Loaded audio policy from %s (%s)", module->name, module->id);
 #else
-    ALOGI("AudioPolicyService CSTOR in new mode");
+        ALOGI("AudioPolicyService CSTOR in new mode");
 
-    mAudioPolicyClient = new AudioPolicyClient(this);
-    mAudioPolicyManager = createAudioPolicyManager(mAudioPolicyClient);
+        mAudioPolicyClient = new AudioPolicyClient(this);
+        mAudioPolicyManager = createAudioPolicyManager(mAudioPolicyClient);
 #endif
-
+    }
     // load audio processing modules
-    mAudioPolicyEffects = new AudioPolicyEffects();
+    sp<AudioPolicyEffects>audioPolicyEffects = new AudioPolicyEffects();
+    {
+        Mutex::Autolock _l(mLock);
+        mAudioPolicyEffects = audioPolicyEffects;
+    }
 }
 
 AudioPolicyService::~AudioPolicyService()