Fix audio effect framework issues

Fix two issues in audio effect framework reported by partners.

1 - Fixed duplicated audio buffer sent to effect process function when
pausing a track.
Modified Effectchain::process_l() function to clear the effect chain
input buffer before calling the effect process functions when no track
is active on the session. Previous code was clearing the buffer after
calling the process functions and  when transitioning from active
to inactive, the last processed buffer was passed again once to effect
process function before being cleared.

2 - Fixed potential mutex cross deadlock when disconnecting an effect
while playback is active. This is because EffectChain::process_l()
was calling PlaybackThread::hasAudioSession() thus creating an inversion
in the mutex lock order (EffectChain mutex locked before ThreadBase mutex).
The fix consists in removing the call to hasAudioSession() from process_l()
and requires each effect chain to keep count of the number of audio tracks
attached to it (previously only the active tracks were accounted for).

Change-Id: Iee4246694ea8c7a66c012120c629d72dd38f9c35

2 files changed, 55 insertions, 28 deletions

diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp
index f8ad5bb..e4b5cf3 100644
--- a/services/audioflinger/AudioFlinger.cpp
+++ b/services/audioflinger/AudioFlinger.cpp
@@ -1257,6 +1257,7 @@ sp<AudioFlinger::PlaybackThread::Track>  AudioFlinger::PlaybackThread::createTra
             LOGV("createTrack_l() setting main buffer %p", chain->inBuffer());
             track->setMainBuffer(chain->inBuffer());
             chain->setStrategy(AudioSystem::getStrategyForStream((audio_stream_type_t)track->type()));
+            chain->incTrackCnt();
         }
     }
     lStatus = NO_ERROR;
@@ -1340,7 +1341,7 @@ status_t AudioFlinger::PlaybackThread::addTrack_l(const sp<Track>& track)
             sp<EffectChain> chain = getEffectChain_l(track->sessionId());
             if (chain != 0) {
                 LOGV("addTrack_l() starting track on chain %p for session %d", chain.get(), track->sessionId());
-                chain->startTrack();
+                chain->incActiveTrackCnt();
             }
         }
 
@@ -1358,8 +1359,17 @@ void AudioFlinger::PlaybackThread::destroyTrack_l(const sp<Track>& track)
 {
     track->mState = TrackBase::TERMINATED;
     if (mActiveTracks.indexOf(track) < 0) {
-        mTracks.remove(track);
-        deleteTrackName_l(track->name());
+        removeTrack_l(track);
+    }
+}
+
+void AudioFlinger::PlaybackThread::removeTrack_l(const sp<Track>& track)
+{
+    mTracks.remove(track);
+    deleteTrackName_l(track->name());
+    sp<EffectChain> chain = getEffectChain_l(track->sessionId());
+    if (chain != 0) {
+        chain->decTrackCnt();
     }
 }
 
@@ -1865,12 +1875,11 @@ uint32_t AudioFlinger::MixerThread::prepareTracks_l(const SortedVector< wp<Track
                 chain = getEffectChain_l(track->sessionId());
                 if (chain != 0) {
                     LOGV("stopping track on chain %p for session Id: %d", chain.get(), track->sessionId());
-                    chain->stopTrack();
+                    chain->decActiveTrackCnt();
                 }
             }
             if (track->isTerminated()) {
-                mTracks.remove(track);
-                deleteTrackName_l(track->mName);
+                removeTrack_l(track);
             }
         }
     }
@@ -1956,7 +1965,7 @@ bool AudioFlinger::MixerThread::checkForNewParameters_l()
         if (param.getInt(String8(AudioParameter::keyRouting), value) == NO_ERROR) {
             // when changing the audio output device, call addBatteryData to notify
             // the change
-            if (mDevice != value) {
+            if ((int)mDevice != value) {
                 uint32_t params = 0;
                 // check whether speaker is on
                 if (value & AUDIO_DEVICE_OUT_SPEAKER) {
@@ -2348,11 +2357,10 @@ bool AudioFlinger::DirectOutputThread::threadLoop()
                 if (!effectChains.isEmpty()) {
                     LOGV("stopping track on chain %p for session Id: %d", effectChains[0].get(),
                             trackToRemove->sessionId());
-                    effectChains[0]->stopTrack();
+                    effectChains[0]->decActiveTrackCnt();
                 }
                 if (trackToRemove->isTerminated()) {
-                    mTracks.remove(trackToRemove);
-                    deleteTrackName_l(trackToRemove->mName);
+                    removeTrack_l(trackToRemove);
                 }
             }
 
@@ -5150,6 +5158,7 @@ status_t AudioFlinger::PlaybackThread::addEffectChain_l(const sp<EffectChain>& c
             if (session == track->sessionId()) {
                 LOGV("addEffectChain_l() track->setMainBuffer track %p buffer %p", track.get(), buffer);
                 track->setMainBuffer(buffer);
+                chain->incTrackCnt();
             }
         }
 
@@ -5159,7 +5168,7 @@ status_t AudioFlinger::PlaybackThread::addEffectChain_l(const sp<EffectChain>& c
             if (track == 0) continue;
             if (session == track->sessionId()) {
                 LOGV("addEffectChain_l() activating track %p on session %d", track.get(), session);
-                chain->startTrack();
+                chain->incActiveTrackCnt();
             }
         }
     }
@@ -5195,11 +5204,23 @@ size_t AudioFlinger::PlaybackThread::removeEffectChain_l(const sp<EffectChain>&
     for (size_t i = 0; i < mEffectChains.size(); i++) {
         if (chain == mEffectChains[i]) {
             mEffectChains.removeAt(i);
+            // detach all active tracks from the chain
+            for (size_t i = 0 ; i < mActiveTracks.size() ; ++i) {
+                sp<Track> track = mActiveTracks[i].promote();
+                if (track == 0) continue;
+                if (session == track->sessionId()) {
+                    LOGV("removeEffectChain_l(): stopping track on chain %p for session Id: %d",
+                            chain.get(), session);
+                    chain->decActiveTrackCnt();
+                }
+            }
+
             // detach all tracks with same session ID from this chain
             for (size_t i = 0; i < mTracks.size(); ++i) {
                 sp<Track> track = mTracks[i];
                 if (session == track->sessionId()) {
                     track->setMainBuffer(mMixBuffer);
+                    chain->decTrackCnt();
                 }
             }
             break;
@@ -5481,7 +5502,7 @@ void AudioFlinger::EffectModule::process()
         // If an insert effect is idle and input buffer is different from output buffer,
         // accumulate input onto output
         sp<EffectChain> chain = mChain.promote();
-        if (chain != 0 && chain->activeTracks() != 0) {
+        if (chain != 0 && chain->activeTrackCnt() != 0) {
             size_t frameCnt = mConfig.inputCfg.buffer.frameCount * 2;  //always stereo here
             int16_t *in = mConfig.inputCfg.buffer.s16;
             int16_t *out = mConfig.outputCfg.buffer.s16;
@@ -6157,9 +6178,9 @@ void AudioFlinger::EffectHandle::dump(char* buffer, size_t size)
 
 AudioFlinger::EffectChain::EffectChain(const wp<ThreadBase>& wThread,
                                         int sessionId)
-    : mThread(wThread), mSessionId(sessionId), mActiveTrackCnt(0), mOwnInBuffer(false),
-            mVolumeCtrlIdx(-1), mLeftVolume(UINT_MAX), mRightVolume(UINT_MAX),
-            mNewLeftVolume(UINT_MAX), mNewRightVolume(UINT_MAX)
+    : mThread(wThread), mSessionId(sessionId), mActiveTrackCnt(0), mTrackCnt(0),
+      mOwnInBuffer(false), mVolumeCtrlIdx(-1), mLeftVolume(UINT_MAX), mRightVolume(UINT_MAX),
+      mNewLeftVolume(UINT_MAX), mNewRightVolume(UINT_MAX)
 {
     mStrategy = AudioSystem::getStrategyForStream(AUDIO_STREAM_MUSIC);
 }
@@ -6216,8 +6237,15 @@ void AudioFlinger::EffectChain::process_l()
             (mSessionId == AUDIO_SESSION_OUTPUT_STAGE);
     bool tracksOnSession = false;
     if (!isGlobalSession) {
-        tracksOnSession =
-                playbackThread->hasAudioSession(mSessionId) & PlaybackThread::TRACK_SESSION;
+        tracksOnSession = (trackCnt() != 0);
+    }
+
+    // if no track is active, input buffer must be cleared here as the mixer process
+    // will not do it
+    if (tracksOnSession &&
+            activeTrackCnt() == 0) {
+        size_t numSamples = playbackThread->frameCount() * playbackThread->channelCount();
+        memset(mInBuffer, 0, numSamples * sizeof(int16_t));
     }
 
     size_t size = mEffects.size();
@@ -6230,13 +6258,6 @@ void AudioFlinger::EffectChain::process_l()
     for (size_t i = 0; i < size; i++) {
         mEffects[i]->updateState();
     }
-    // if no track is active, input buffer must be cleared here as the mixer process
-    // will not do it
-    if (tracksOnSession &&
-        activeTracks() == 0) {
-        size_t numSamples = playbackThread->frameCount() * playbackThread->channelCount();
-        memset(mInBuffer, 0, numSamples * sizeof(int16_t));
-    }
 }
 
 // addEffect_l() must be called with PlaybackThread::mLock held
diff --git a/services/audioflinger/AudioFlinger.h b/services/audioflinger/AudioFlinger.h
index 22e5116..b324d49 100644
--- a/services/audioflinger/AudioFlinger.h
+++ b/services/audioflinger/AudioFlinger.h
@@ -684,6 +684,7 @@ private:
 
         status_t    addTrack_l(const sp<Track>& track);
         void        destroyTrack_l(const sp<Track>& track);
+        void        removeTrack_l(const sp<Track>& track);
 
         void        readOutputParameters();
 
@@ -1134,9 +1135,13 @@ private:
             return mOutBuffer;
         }
 
-        void startTrack() {mActiveTrackCnt++;}
-        void stopTrack() {mActiveTrackCnt--;}
-        int activeTracks() { return mActiveTrackCnt;}
+        void incTrackCnt() { android_atomic_inc(&mTrackCnt); }
+        void decTrackCnt() { android_atomic_dec(&mTrackCnt); }
+        int32_t trackCnt() { return mTrackCnt;}
+
+        void incActiveTrackCnt() { android_atomic_inc(&mActiveTrackCnt); }
+        void decActiveTrackCnt() { android_atomic_dec(&mActiveTrackCnt); }
+        int32_t activeTrackCnt() { return mActiveTrackCnt;}
 
         uint32_t strategy() { return mStrategy; }
         void setStrategy(uint32_t strategy)
@@ -1155,7 +1160,8 @@ private:
         int mSessionId;             // audio session ID
         int16_t *mInBuffer;         // chain input buffer
         int16_t *mOutBuffer;        // chain output buffer
-        int mActiveTrackCnt;        // number of active tracks connected
+        volatile int32_t mActiveTrackCnt;  // number of active tracks connected
+        volatile int32_t mTrackCnt;        // number of tracks connected
         bool mOwnInBuffer;          // true if the chain owns its input buffer
         int mVolumeCtrlIdx;         // index of insert effect having control over volume
         uint32_t mLeftVolume;       // previous volume on left channel