audioflinger: fix pre processing effect leak

When a capture thread was closed, the effects attached to this thread
were left dangling and the associated effect chain destroyed.
When their last client was disconnected, the effects were not released
properly from the effect library because the destruction process could
not be completed without the effect being attached to a thread.

A similar problem prevented a RecordTrack to be properly released if
its client was destroyed after the capture thread.

The fix consists in allowing the effect or record track to be properly
released even if its parent thread cannot be promoted.

Also save any effect chain still present on a closed capture thread
in case a new client wants to reuse the effects on the same session later.

Bug: 17110064.
Change-Id: I5cd644daa357afd1f3548f9bcb28e6152d95fdb8

8 files changed, 133 insertions, 37 deletions

diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp
index 1843722..9cfbe6a 100644
--- a/services/audioflinger/AudioFlinger.cpp
+++ b/services/audioflinger/AudioFlinger.cpp
@@ -418,6 +418,13 @@ status_t AudioFlinger::dump(int fd, const Vector<String16>& args)
             mRecordThreads.valueAt(i)->dump(fd, args);
         }
 
+        // dump orphan effect chains
+        if (mOrphanEffectChains.size() != 0) {
+            write(fd, "  Orphan Effect Chains\n", strlen("  Orphan Effect Chains\n"));
+            for (size_t i = 0; i < mOrphanEffectChains.size(); i++) {
+                mOrphanEffectChains.valueAt(i)->dump(fd, args);
+            }
+        }
         // dump all hardware devs
         for (size_t i = 0; i < mAudioHwDevs.size(); i++) {
             audio_hw_device_t *dev = mAudioHwDevs.valueAt(i)->hwDevice();
@@ -1416,7 +1423,7 @@ sp<IAudioRecord> AudioFlinger::openRecord(
                 *sessionId = lSessionId;
             }
         }
-        ALOGV("openRecord() lSessionId: %d", lSessionId);
+        ALOGV("openRecord() lSessionId: %d input %d", lSessionId, input);
 
         // TODO: the uid should be passed in as a parameter to openRecord
         recordTrack = thread->createRecordTrack_l(client, sampleRate, format, channelMask,
@@ -2022,6 +2029,16 @@ status_t AudioFlinger::closeInput_nonvirtual(audio_io_handle_t input)
         }
 
         ALOGV("closeInput() %d", input);
+        {
+            // If we still have effect chains, it means that a client still holds a handle
+            // on at least one effect. We must keep the chain alive in case a new record
+            // thread is opened for a new capture on the same session
+            Mutex::Autolock _sl(thread->mLock);
+            Vector< sp<EffectChain> > effectChains = thread->getEffectChains_l();
+            for (size_t i = 0; i < effectChains.size(); i++) {
+                putOrphanEffectChain_l(effectChains[i]);
+            }
+        }
         audioConfigChanged(AudioSystem::INPUT_CLOSED, input, NULL);
         mRecordThreads.removeItem(input);
     }
@@ -2451,6 +2468,13 @@ sp<IEffect> AudioFlinger::createEffect(
                 lStatus = BAD_VALUE;
                 goto Exit;
             }
+        } else {
+            // Check if one effect chain was awaiting for an effect to be created on this
+            // session and used it instead of creating a new one.
+            sp<EffectChain> chain = getOrphanEffectChain_l((audio_session_t)sessionId);
+            if (chain != 0) {
+                thread->addEffectChain_l(chain);
+            }
         }
 
         sp<Client> client = registerPid(pid);
@@ -2623,6 +2647,49 @@ void AudioFlinger::onNonOffloadableGlobalEffectEnable()
 
 }
 
+status_t AudioFlinger::putOrphanEffectChain_l(const sp<AudioFlinger::EffectChain>& chain)
+{
+    audio_session_t session = (audio_session_t)chain->sessionId();
+    ssize_t index = mOrphanEffectChains.indexOfKey(session);
+    ALOGV("putOrphanEffectChain_l session %d index %d", session, index);
+    if (index >= 0) {
+        ALOGW("putOrphanEffectChain_l chain for session %d already present", session);
+        return ALREADY_EXISTS;
+    }
+    mOrphanEffectChains.add(session, chain);
+    return NO_ERROR;
+}
+
+sp<AudioFlinger::EffectChain> AudioFlinger::getOrphanEffectChain_l(audio_session_t session)
+{
+    sp<EffectChain> chain;
+    ssize_t index = mOrphanEffectChains.indexOfKey(session);
+    ALOGV("getOrphanEffectChain_l session %d index %d", session, index);
+    if (index >= 0) {
+        chain = mOrphanEffectChains.valueAt(index);
+        mOrphanEffectChains.removeItemsAt(index);
+    }
+    return chain;
+}
+
+bool AudioFlinger::updateOrphanEffectChains(const sp<AudioFlinger::EffectModule>& effect)
+{
+    Mutex::Autolock _l(mLock);
+    audio_session_t session = (audio_session_t)effect->sessionId();
+    ssize_t index = mOrphanEffectChains.indexOfKey(session);
+    ALOGV("updateOrphanEffectChains session %d index %d", session, index);
+    if (index >= 0) {
+        sp<EffectChain> chain = mOrphanEffectChains.valueAt(index);
+        if (chain->removeEffect_l(effect) == 0) {
+            ALOGV("updateOrphanEffectChains removing effect chain at index %d", index);
+            mOrphanEffectChains.removeItemsAt(index);
+        }
+        return true;
+    }
+    return false;
+}
+
+
 struct Entry {
 #define MAX_NAME 32     // %Y%m%d%H%M%S_%d.wav
     char mName[MAX_NAME];
diff --git a/services/audioflinger/AudioFlinger.h b/services/audioflinger/AudioFlinger.h
index 753314f..1003017 100644
--- a/services/audioflinger/AudioFlinger.h
+++ b/services/audioflinger/AudioFlinger.h
@@ -569,6 +569,23 @@ private:
                 bool isNonOffloadableGlobalEffectEnabled_l();
                 void onNonOffloadableGlobalEffectEnable();
 
+                // Store an effect chain to mOrphanEffectChains keyed vector.
+                // Called when a thread exits and effects are still attached to it.
+                // If effects are later created on the same session, they will reuse the same
+                // effect chain and same instances in the effect library.
+                // return ALREADY_EXISTS if a chain with the same session already exists in
+                // mOrphanEffectChains. Note that this should never happen as there is only one
+                // chain for a given session and it is attached to only one thread at a time.
+                status_t        putOrphanEffectChain_l(const sp<EffectChain>& chain);
+                // Get an effect chain for the specified session in mOrphanEffectChains and remove
+                // it if found. Returns 0 if not found (this is the most common case).
+                sp<EffectChain> getOrphanEffectChain_l(audio_session_t session);
+                // Called when the last effect handle on an effect instance is removed. If this
+                // effect belongs to an effect chain in mOrphanEffectChains, the chain is updated
+                // and removed from mOrphanEffectChains if it does not contain any effect.
+                // Return true if the effect was found in mOrphanEffectChains, false otherwise.
+                bool            updateOrphanEffectChains(const sp<EffectModule>& effect);
+
     class AudioHwDevice {
     public:
         enum Flags {
@@ -713,6 +730,9 @@ private:
                 Vector < sp<SyncEvent> > mPendingSyncEvents; // sync events awaiting for a session
                                                              // to be created
 
+                // Effect chains without a valid thread
+                DefaultKeyedVector< audio_session_t , sp<EffectChain> > mOrphanEffectChains;
+
 private:
     sp<Client>  registerPid(pid_t pid);    // always returns non-0
 
diff --git a/services/audioflinger/Effects.cpp b/services/audioflinger/Effects.cpp
index 365f271..15f1f23 100644
--- a/services/audioflinger/Effects.cpp
+++ b/services/audioflinger/Effects.cpp
@@ -68,7 +68,8 @@ AudioFlinger::EffectModule::EffectModule(ThreadBase *thread,
       mStatus(NO_INIT), mState(IDLE),
       // mMaxDisableWaitCnt is set by configure() and not used before then
       // mDisableWaitCnt is set by process() and updateState() and not used before then
-      mSuspended(false)
+      mSuspended(false),
+      mAudioFlinger(thread->mAudioFlinger)
 {
     ALOGV("Constructor %p", this);
     int lStatus;
@@ -197,9 +198,19 @@ size_t AudioFlinger::EffectModule::disconnect(EffectHandle *handle, bool unpinIf
     // destructor before we exit
     sp<EffectModule> keep(this);
     {
-        sp<ThreadBase> thread = mThread.promote();
-        if (thread != 0) {
-            thread->disconnectEffect(keep, handle, unpinIfLast);
+        if (removeHandle(handle) == 0) {
+            if (!isPinned() || unpinIfLast) {
+                sp<ThreadBase> thread = mThread.promote();
+                if (thread != 0) {
+                    Mutex::Autolock _l(thread->mLock);
+                    thread->removeEffect_l(this);
+                }
+                sp<AudioFlinger> af = mAudioFlinger.promote();
+                if (af != 0) {
+                    af->updateOrphanEffectChains(this);
+                }
+                AudioSystem::unregisterEffect(mId);
+            }
         }
     }
     return mHandles.size();
@@ -1911,4 +1922,13 @@ bool AudioFlinger::EffectChain::isNonOffloadableEnabled()
     return false;
 }
 
+void AudioFlinger::EffectChain::setThread(const sp<ThreadBase>& thread)
+{
+    Mutex::Autolock _l(mLock);
+    mThread = thread;
+    for (size_t i = 0; i < mEffects.size(); i++) {
+        mEffects[i]->setThread(thread);
+    }
+}
+
 }; // namespace android
diff --git a/services/audioflinger/Effects.h b/services/audioflinger/Effects.h
index 4170fd4..eaf90e7 100644
--- a/services/audioflinger/Effects.h
+++ b/services/audioflinger/Effects.h
@@ -153,6 +153,7 @@ mutable Mutex               mLock;      // mutex for process, commands and handl
     uint32_t mDisableWaitCnt;       // current process() calls count during disable period.
     bool     mSuspended;            // effect is suspended: temporarily disabled by framework
     bool     mOffloaded;            // effect is currently offloaded to the audio DSP
+    wp<AudioFlinger>    mAudioFlinger;
 };
 
 // The EffectHandle class implements the IEffect interface. It provides resources
@@ -347,6 +348,8 @@ protected:
 
     void clearInputBuffer_l(sp<ThreadBase> thread);
 
+    void setThread(const sp<ThreadBase>& thread);
+
     wp<ThreadBase> mThread;     // parent mixer thread
     Mutex mLock;                // mutex protecting effect list
     Vector< sp<EffectModule> > mEffects; // list of effect modules
diff --git a/services/audioflinger/Threads.cpp b/services/audioflinger/Threads.cpp
index 97b1753..3d17c89 100644
--- a/services/audioflinger/Threads.cpp
+++ b/services/audioflinger/Threads.cpp
@@ -1147,21 +1147,6 @@ void AudioFlinger::ThreadBase::setMode(audio_mode_t mode)
     }
 }
 
-void AudioFlinger::ThreadBase::disconnectEffect(const sp<EffectModule>& effect,
-                                                    EffectHandle *handle,
-                                                    bool unpinIfLast) {
-
-    Mutex::Autolock _l(mLock);
-    ALOGV("disconnectEffect() %p effect %p", this, effect.get());
-    // delete the effect module if removing last handle on it
-    if (effect->removeHandle(handle) == 0) {
-        if (!effect->isPinned() || unpinIfLast) {
-            removeEffect_l(effect);
-            AudioSystem::unregisterEffect(effect->id());
-        }
-    }
-}
-
 void AudioFlinger::ThreadBase::getAudioPortConfig(struct audio_port_config *config)
 {
     config->type = AUDIO_PORT_TYPE_MIX;
@@ -2278,7 +2263,7 @@ status_t AudioFlinger::PlaybackThread::addEffectChain_l(const sp<EffectChain>& c
             }
         }
     }
-
+    chain->setThread(this);
     chain->setInBuffer(buffer, ownsBuffer);
     chain->setOutBuffer(reinterpret_cast<int16_t*>(mEffectBufferEnabled
             ? mEffectBuffer : mSinkBuffer));
@@ -6188,10 +6173,11 @@ status_t AudioFlinger::RecordThread::addEffectChain_l(const sp<EffectChain>& cha
 {
     // only one chain per input thread
     if (mEffectChains.size() != 0) {
+        ALOGW("addEffectChain_l() already one chain %p on thread %p", chain.get(), this);
         return INVALID_OPERATION;
     }
     ALOGV("addEffectChain_l() %p on thread %p", chain.get(), this);
-
+    chain->setThread(this);
     chain->setInBuffer(NULL);
     chain->setOutBuffer(NULL);
 
diff --git a/services/audioflinger/Threads.h b/services/audioflinger/Threads.h
index 648502b..fd025b5 100644
--- a/services/audioflinger/Threads.h
+++ b/services/audioflinger/Threads.h
@@ -283,9 +283,6 @@ public:
                                     effect_descriptor_t *desc,
                                     int *enabled,
                                     status_t *status /*non-NULL*/);
-                void disconnectEffect(const sp< EffectModule>& effect,
-                                      EffectHandle *handle,
-                                      bool unpinIfLast);
 
                 // return values for hasAudioSession (bit field)
                 enum effect_state {
diff --git a/services/audioflinger/TrackBase.h b/services/audioflinger/TrackBase.h
index 864daa5..98bf96e 100644
--- a/services/audioflinger/TrackBase.h
+++ b/services/audioflinger/TrackBase.h
@@ -166,6 +166,7 @@ protected:
     sp<NBAIO_Source>    mTeeSource;
     bool                mTerminated;
     track_type          mType;      // must be one of TYPE_DEFAULT, TYPE_OUTPUT, TYPE_PATCH ...
+    audio_io_handle_t   mThreadIoHandle; // I/O handle of the thread the track is attached to
 };
 
 // PatchProxyBufferProvider interface is implemented by PatchTrack and PatchRecord.
diff --git a/services/audioflinger/Tracks.cpp b/services/audioflinger/Tracks.cpp
index 6cbab04..c0a75b9 100644
--- a/services/audioflinger/Tracks.cpp
+++ b/services/audioflinger/Tracks.cpp
@@ -96,7 +96,8 @@ AudioFlinger::ThreadBase::TrackBase::TrackBase(
         mServerProxy(NULL),
         mId(android_atomic_inc(&nextTrackId)),
         mTerminated(false),
-        mType(type)
+        mType(type),
+        mThreadIoHandle(thread->id())
 {
     // if the caller is us, trust the specified uid
     if (IPCThreadState::self()->getCallingPid() != getpid_cached || clientUid == -1) {
@@ -482,14 +483,15 @@ void AudioFlinger::PlaybackThread::Track::destroy()
     // this Track with its member mTrack.
     sp<Track> keep(this);
     { // scope for mLock
+        bool wasActive = false;
         sp<ThreadBase> thread = mThread.promote();
         if (thread != 0) {
             Mutex::Autolock _l(thread->mLock);
             PlaybackThread *playbackThread = (PlaybackThread *)thread.get();
-            bool wasActive = playbackThread->destroyTrack_l(this);
-            if (isExternalTrack() && !wasActive) {
-                AudioSystem::releaseOutput(thread->id());
-            }
+            wasActive = playbackThread->destroyTrack_l(this);
+        }
+        if (isExternalTrack() && !wasActive) {
+            AudioSystem::releaseOutput(mThreadIoHandle);
         }
     }
 }
@@ -2050,7 +2052,7 @@ void AudioFlinger::RecordThread::RecordTrack::stop()
     if (thread != 0) {
         RecordThread *recordThread = (RecordThread *)thread.get();
         if (recordThread->stop(this) && isExternalTrack()) {
-            AudioSystem::stopInput(recordThread->id(), (audio_session_t)mSessionId);
+            AudioSystem::stopInput(mThreadIoHandle, (audio_session_t)mSessionId);
         }
     }
 }
@@ -2060,14 +2062,14 @@ void AudioFlinger::RecordThread::RecordTrack::destroy()
     // see comments at AudioFlinger::PlaybackThread::Track::destroy()
     sp<RecordTrack> keep(this);
     {
+        if (isExternalTrack()) {
+            if (mState == ACTIVE || mState == RESUMING) {
+                AudioSystem::stopInput(mThreadIoHandle, (audio_session_t)mSessionId);
+            }
+            AudioSystem::releaseInput(mThreadIoHandle, (audio_session_t)mSessionId);
+        }
         sp<ThreadBase> thread = mThread.promote();
         if (thread != 0) {
-            if (isExternalTrack()) {
-                if (mState == ACTIVE || mState == RESUMING) {
-                    AudioSystem::stopInput(thread->id(), (audio_session_t)mSessionId);
-                }
-                AudioSystem::releaseInput(thread->id(), (audio_session_t)mSessionId);
-            }
             Mutex::Autolock _l(thread->mLock);
             RecordThread *recordThread = (RecordThread *) thread.get();
             recordThread->destroyTrack_l(this);