AudioFlinger: fix crash when deleting pre process.

If a pre processing effect is detroyed while enabled and capture is active,
there was a possibility that the effect engine is released by the framework
while still processed by the audio HAL.

The fix consists in not releasing the engine in EffectModule::removeHandle()
but just flag the effect as being detroyed to avoid further calls to functions
on the engine effect interface.
The effect interface is then removed from the audio HAL safely in
EffectChain::removeEffect_l() while holding the EffectChain mutex.

Change-Id: I71fab30d9145062af8644f545a1f1d4d3e7e7f02

diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp
index cb1f921..6403061 100644 (file)
--- a/services/audioflinger/AudioFlinger.cpp
+++ b/services/audioflinger/AudioFlinger.cpp
@@ -3907,8 +3907,6 @@ bool AudioFlinger::RecordThread::threadLoop()
             for (size_t i = 0; i < effectChains.size(); i ++) {
                 effectChains[i]->process_l();
             }
-            // enable changes in effect chain
-            unlockEffectChains(effectChains);
 
             buffer.frameCount = mFrameCount;
             if (LIKELY(mActiveTrack->getNextBuffer(&buffer) == NO_ERROR)) {
@@ -4008,9 +4006,9 @@ bool AudioFlinger::RecordThread::threadLoop()
                 // clear the overflow.
                 usleep(kRecordThreadSleepUs);
             }
-        } else {
-            unlockEffectChains(effectChains);
         }
+        // enable changes in effect chain
+        unlockEffectChains(effectChains);
         effectChains.clear();
     }
 
@@ -5581,13 +5579,11 @@ size_t AudioFlinger::EffectModule::removeHandle(const wp<EffectHandle>& handle)
         }
     }
 
-    // Release effect engine here so that it is done immediately. Otherwise it will be released
-    // by the destructor when the last strong reference on the this object is released which can
-    // happen after next process is called on this effect.
-    if (size == 0 && mEffectInterface != NULL) {
-        // release effect engine
-        EffectRelease(mEffectInterface);
-        mEffectInterface = NULL;
+    // Prevent calls to process() and other functions on effect interface from now on.
+    // The effect engine will be released by the destructor when the last strong reference on
+    // this object is released which can happen after next process is called.
+    if (size == 0) {
+        mState = DESTROYED;
     }
 
     return size;
@@ -5637,7 +5633,7 @@ void AudioFlinger::EffectModule::updateState() {
             mState = IDLE;
         }
         break;
-    default: //IDLE , ACTIVE
+    default: //IDLE , ACTIVE, DESTROYED
         break;
     }
 }
@@ -5646,7 +5642,7 @@ void AudioFlinger::EffectModule::process()
 {
     Mutex::Autolock _l(mLock);
 
-    if (mEffectInterface == NULL ||
+    if (mState == DESTROYED || mEffectInterface == NULL ||
             mConfig.inputCfg.buffer.raw == NULL ||
             mConfig.outputCfg.buffer.raw == NULL) {
         return;
@@ -5822,6 +5818,12 @@ status_t AudioFlinger::EffectModule::start_l()
     return status;
 }
 
+status_t AudioFlinger::EffectModule::stop()
+{
+    Mutex::Autolock _l(mLock);
+    return stop_l();
+}
+
 status_t AudioFlinger::EffectModule::stop_l()
 {
     if (mEffectInterface == NULL) {
@@ -5858,7 +5860,7 @@ status_t AudioFlinger::EffectModule::command(uint32_t cmdCode,
     Mutex::Autolock _l(mLock);
 //    LOGV("command(), cmdCode: %d, mEffectInterface: %p", cmdCode, mEffectInterface);
 
-    if (mEffectInterface == NULL) {
+    if (mState == DESTROYED || mEffectInterface == NULL) {
         return NO_INIT;
     }
     status_t status = (*mEffectInterface)->command(mEffectInterface,
@@ -5907,6 +5909,8 @@ status_t AudioFlinger::EffectModule::setEnabled(bool enabled)
         case ACTIVE:
             mState = STOPPING;
             break;
+        case DESTROYED:
+            return NO_ERROR; // simply ignore as we are being destroyed
         }
         for (size_t i = 1; i < mHandles.size(); i++) {
             sp<EffectHandle> h = mHandles[i].promote();
@@ -5928,6 +5932,7 @@ bool AudioFlinger::EffectModule::isEnabled()
     case IDLE:
     case STOPPING:
     case STOPPED:
+    case DESTROYED:
     default:
         return false;
     }
@@ -5943,6 +5948,7 @@ bool AudioFlinger::EffectModule::isProcessEnabled()
         return true;
     case IDLE:
     case STARTING:
+    case DESTROYED:
     default:
         return false;
     }
@@ -6544,6 +6550,10 @@ size_t AudioFlinger::EffectChain::removeEffect_l(const sp<EffectModule>& effect)
 
     for (i = 0; i < size; i++) {
         if (effect == mEffects[i]) {
+            // calling stop here will remove pre-processing effect from the audio HAL.
+            // This is safe as we hold the EffectChain mutex which guarantees that we are not in
+            // the middle of a read from audio HAL
+            mEffects[i]->stop();
             if (type == EFFECT_FLAG_TYPE_AUXILIARY) {
                 delete[] effect->inBuffer();
             } else {

diff --git a/services/audioflinger/AudioFlinger.h b/services/audioflinger/AudioFlinger.h
index e2cf946..9817c90 100644 (file)
--- a/services/audioflinger/AudioFlinger.h
+++ b/services/audioflinger/AudioFlinger.h
@@ -995,7 +995,8 @@ private:
             STARTING,
             ACTIVE,
             STOPPING,
-            STOPPED
+            STOPPED,
+            DESTROYED
         };
 
         int         id() { return mId; }
@@ -1040,6 +1041,7 @@ private:
         status_t         setDevice(uint32_t device);
         status_t         setVolume(uint32_t *left, uint32_t *right, bool controller);
         status_t         setMode(uint32_t mode);
+        status_t         stop();
 
         status_t         dump(int fd, const Vector<String16>& args);