From ec437d8d3db79459d7b19e1734e6fe309bd621e8 Mon Sep 17 00:00:00 2001 From: Eric Laurent Date: Tue, 26 Jul 2011 20:54:46 -0700 Subject: 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 --- services/audioflinger/AudioFlinger.cpp | 38 +++++++++++++++++++++------------- services/audioflinger/AudioFlinger.h | 4 +++- 2 files changed, 27 insertions(+), 15 deletions(-) diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp index cb1f9213..64030615 100644 --- 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& 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 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& 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 e2cf946d..9817c904 100644 --- 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& args); -- cgit v1.2.3