From cab112421da6e8eac19ffddbbe3d76067cffee78 Mon Sep 17 00:00:00 2001 From: Eric Laurent Date: Thu, 15 Jul 2010 12:50:15 -0700 Subject: Several improvements in audio effects volume control. - Fixed crash when deleting an effect chained before an effect having volume control - Changed EFFECT_FLAG_VOLUME_CTRL to implicitely include EFFECT_FLAG_VOLUME_IND (not need to set both in effect descriptor). - Volume control changes from one effect to another if needed according to effect enable state - EFFECT_CMD_SET_VOLUME is only sent when their is an actual change in volume Change-Id: Ieebaf09157e2627366023569d95516646e03e26c --- include/media/EffectApi.h | 2 +- services/audioflinger/AudioFlinger.cpp | 124 +++++++++++++++++---------------- services/audioflinger/AudioFlinger.h | 19 ++--- 3 files changed, 74 insertions(+), 71 deletions(-) diff --git a/include/media/EffectApi.h b/include/media/EffectApi.h index 9f3d0b6..8c120e5 100644 --- a/include/media/EffectApi.h +++ b/include/media/EffectApi.h @@ -455,7 +455,7 @@ enum effect_command_e { //-------------------------------------------------------------------------------------------------- // description: // Set and get volume. Used by audio framework to delegate volume control to effect engine. -// The effect implementation must set EFFECT_FLAG_VOLUME_IND and/or EFFECT_FLAG_VOLUME_CTRL flag in +// The effect implementation must set EFFECT_FLAG_VOLUME_IND or EFFECT_FLAG_VOLUME_CTRL flag in // its descriptor to receive this command before every call to process() function // If EFFECT_FLAG_VOLUME_CTRL flag is set in the effect descriptor, the effect engine must return // the volume that should be applied before the effect is processed. The overall volume (the volume diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp index 771d885..7e528af 100644 --- a/services/audioflinger/AudioFlinger.cpp +++ b/services/audioflinger/AudioFlinger.cpp @@ -1402,7 +1402,7 @@ void AudioFlinger::PlaybackThread::setMode(uint32_t mode) Mutex::Autolock _l(mLock); size_t size = mEffectChains.size(); for (size_t i = 0; i < size; i++) { - mEffectChains[i]->setMode(mode); + mEffectChains[i]->setMode_l(mode); } } @@ -1503,8 +1503,8 @@ bool AudioFlinger::MixerThread::threadLoop() // prevent any changes in effect chain list and in each effect chain // during mixing and effect process as the audio buffers could be deleted // or modified if an effect is created or deleted - effectChains = mEffectChains; lockEffectChains_l(); + effectChains = mEffectChains; } if (LIKELY(mixerStatus == MIXER_TRACKS_READY)) { @@ -1628,7 +1628,7 @@ uint32_t AudioFlinger::MixerThread::prepareTracks_l(const SortedVector< wp chain = getEffectChain_l(0); if (chain != 0) { uint32_t v = (uint32_t)(masterVolume * (1 << 24)); - chain->setVolume(&v, &v); + chain->setVolume_l(&v, &v); masterVolume = (float)((v + (1 << 23)) >> 24); chain.clear(); } @@ -1706,7 +1706,7 @@ uint32_t AudioFlinger::MixerThread::prepareTracks_l(const SortedVector< wpvolume[1]) << 12; // Delegate volume control to effect in track effect chain if needed - if (chain != 0 && chain->setVolume(&vl, &vr)) { + if (chain != 0 && chain->setVolume_l(&vl, &vr)) { // Do not ramp volume is volume is controlled by effect param = AudioMixer::VOLUME; } @@ -1885,7 +1885,7 @@ bool AudioFlinger::MixerThread::checkForNewParameters_l() // aware of attached audio device. mDevice = (uint32_t)value; for (size_t i = 0; i < mEffectChains.size(); i++) { - mEffectChains[i]->setDevice(mDevice); + mEffectChains[i]->setDevice_l(mDevice); } } @@ -2198,7 +2198,7 @@ bool AudioFlinger::DirectOutputThread::threadLoop() // there is one, the track is connected to it if (!effectChains.isEmpty()) { // Do not ramp volume is volume is controlled by effect - if(effectChains[0]->setVolume(&vl, &vr)) { + if(effectChains[0]->setVolume_l(&vl, &vr)) { rampVolume = false; } } @@ -2505,8 +2505,8 @@ bool AudioFlinger::DuplicatingThread::threadLoop() // prevent any changes in effect chain list and in each effect chain // during mixing and effect process as the audio buffers could be deleted // or modified if an effect is created or deleted - effectChains = mEffectChains; lockEffectChains_l(); + effectChains = mEffectChains; } if (LIKELY(mixerStatus == MIXER_TRACKS_READY)) { @@ -4744,7 +4744,7 @@ sp AudioFlinger::PlaybackThread::createEffect_l( chain = new EffectChain(this, sessionId); addEffectChain_l(chain); } else { - effect = chain->getEffectFromDesc(desc); + effect = chain->getEffectFromDesc_l(desc); } LOGV("createEffect_l() got effect %p on chain %p", effect == 0 ? 0 : effect.get(), chain.get()); @@ -4762,7 +4762,7 @@ sp AudioFlinger::PlaybackThread::createEffect_l( if (lStatus != NO_ERROR) { goto Exit; } - lStatus = chain->addEffect(effect); + lStatus = chain->addEffect_l(effect); if (lStatus != NO_ERROR) { goto Exit; } @@ -4782,7 +4782,8 @@ sp AudioFlinger::PlaybackThread::createEffect_l( Exit: if (lStatus != NO_ERROR && lStatus != ALREADY_EXISTS) { if (effectCreated) { - if (chain->removeEffect(effect) == 0) { + Mutex::Autolock _l(mLock); + if (chain->removeEffect_l(effect) == 0) { removeEffectChain_l(chain); } } @@ -4810,7 +4811,7 @@ void AudioFlinger::PlaybackThread::disconnectEffect(const sp< EffectModule>& eff sp chain = effect->chain().promote(); if (chain != 0) { // remove effect chain if remove last effect - if (chain->removeEffect(effect) == 0) { + if (chain->removeEffect_l(effect) == 0) { removeEffectChain_l(chain); } } @@ -4920,7 +4921,7 @@ sp AudioFlinger::PlaybackThread::getEffect_l(int ses sp chain = getEffectChain_l(sessionId); if (chain != 0) { - effect = chain->getEffectFromId(effectId); + effect = chain->getEffectFromId_l(effectId); } return effect; } @@ -5365,7 +5366,9 @@ status_t AudioFlinger::EffectModule::setVolume(uint32_t *left, uint32_t *right, // Send volume indication if EFFECT_FLAG_VOLUME_IND is set and read back altered volume // if controller flag is set (Note that controller == TRUE => EFFECT_FLAG_VOLUME_CTRL set) - if ((mDescriptor.flags & EFFECT_FLAG_VOLUME_MASK) & (EFFECT_FLAG_VOLUME_CTRL|EFFECT_FLAG_VOLUME_IND)) { + if (isEnabled() && + (mDescriptor.flags & EFFECT_FLAG_VOLUME_MASK) == EFFECT_FLAG_VOLUME_CTRL || + (mDescriptor.flags & EFFECT_FLAG_VOLUME_MASK) == EFFECT_FLAG_VOLUME_IND) { status_t cmdStatus; uint32_t volume[2]; uint32_t *pVolume = NULL; @@ -5745,7 +5748,8 @@ void AudioFlinger::EffectHandle::dump(char* buffer, size_t size) AudioFlinger::EffectChain::EffectChain(const wp& wThread, int sessionId) - : mThread(wThread), mSessionId(sessionId), mVolumeCtrlIdx(-1), mActiveTrackCnt(0), mOwnInBuffer(false) + : mThread(wThread), mSessionId(sessionId), mActiveTrackCnt(0), mOwnInBuffer(false), + mVolumeCtrlIdx(-1), mLeftVolume(0), mRightVolume(0) { } @@ -5758,7 +5762,8 @@ AudioFlinger::EffectChain::~EffectChain() } -sp AudioFlinger::EffectChain::getEffectFromDesc(effect_descriptor_t *descriptor) +// getEffectFromDesc_l() must be called with PlaybackThread::mLock held +sp AudioFlinger::EffectChain::getEffectFromDesc_l(effect_descriptor_t *descriptor) { sp effect; size_t size = mEffects.size(); @@ -5772,7 +5777,8 @@ sp AudioFlinger::EffectChain::getEffectFromDesc(effe return effect; } -sp AudioFlinger::EffectChain::getEffectFromId(int id) +// getEffectFromId_l() must be called with PlaybackThread::mLock held +sp AudioFlinger::EffectChain::getEffectFromId_l(int id) { sp effect; size_t size = mEffects.size(); @@ -5807,7 +5813,8 @@ void AudioFlinger::EffectChain::process_l() } } -status_t AudioFlinger::EffectChain::addEffect(sp& effect) +// addEffect_l() must be called with PlaybackThread::mLock held +status_t AudioFlinger::EffectChain::addEffect_l(sp& effect) { effect_descriptor_t desc = effect->desc(); uint32_t insertPref = desc.flags & EFFECT_FLAG_INSERT_MASK; @@ -5860,7 +5867,7 @@ status_t AudioFlinger::EffectChain::addEffect(sp& effect) // check invalid effect chaining combinations if (insertPref == EFFECT_FLAG_INSERT_EXCLUSIVE || iPref == EFFECT_FLAG_INSERT_EXCLUSIVE) { - LOGW("addEffect() could not insert effect %s: exclusive conflict with %s", desc.name, d.name); + LOGW("addEffect_l() could not insert effect %s: exclusive conflict with %s", desc.name, d.name); return INVALID_OPERATION; } // remember position of first insert effect and by default @@ -5910,22 +5917,17 @@ status_t AudioFlinger::EffectChain::addEffect(sp& effect) effect->setOutBuffer(mInBuffer); } mEffects.insertAt(effect, idx_insert); - // Always give volume control to last effect in chain with volume control capability - if (((desc.flags & EFFECT_FLAG_VOLUME_MASK) & EFFECT_FLAG_VOLUME_CTRL) && - mVolumeCtrlIdx < idx_insert) { - mVolumeCtrlIdx = idx_insert; - } - LOGV("addEffect() effect %p, added in chain %p at rank %d", effect.get(), this, idx_insert); + LOGV("addEffect_l() effect %p, added in chain %p at rank %d", effect.get(), this, idx_insert); } effect->configure(); return NO_ERROR; } -size_t AudioFlinger::EffectChain::removeEffect(const sp& effect) +// removeEffect_l() must be called with PlaybackThread::mLock held +size_t AudioFlinger::EffectChain::removeEffect_l(const sp& effect) { Mutex::Autolock _l(mLock); - int size = (int)mEffects.size(); int i; uint32_t type = effect->desc().flags & EFFECT_FLAG_TYPE_MASK; @@ -5941,26 +5943,16 @@ size_t AudioFlinger::EffectChain::removeEffect(const sp& effect) } } mEffects.removeAt(i); - LOGV("removeEffect() effect %p, removed from chain %p at rank %d", effect.get(), this, i); + LOGV("removeEffect_l() effect %p, removed from chain %p at rank %d", effect.get(), this, i); break; } } - // Return volume control to last effect in chain with volume control capability - if (mVolumeCtrlIdx == i) { - size = (int)mEffects.size(); - for (i = size; i > 0; i--) { - if ((mEffects[i - 1]->desc().flags & EFFECT_FLAG_VOLUME_MASK) & EFFECT_FLAG_VOLUME_CTRL) { - break; - } - } - // mVolumeCtrlIdx reset to -1 if no effect found with volume control flag set - mVolumeCtrlIdx = i - 1; - } return mEffects.size(); } -void AudioFlinger::EffectChain::setDevice(uint32_t device) +// setDevice_l() must be called with PlaybackThread::mLock held +void AudioFlinger::EffectChain::setDevice_l(uint32_t device) { size_t size = mEffects.size(); for (size_t i = 0; i < size; i++) { @@ -5968,7 +5960,8 @@ void AudioFlinger::EffectChain::setDevice(uint32_t device) } } -void AudioFlinger::EffectChain::setMode(uint32_t mode) +// setMode_l() must be called with PlaybackThread::mLock held +void AudioFlinger::EffectChain::setMode_l(uint32_t mode) { size_t size = mEffects.size(); for (size_t i = 0; i < size; i++) { @@ -5976,15 +5969,35 @@ void AudioFlinger::EffectChain::setMode(uint32_t mode) } } -bool AudioFlinger::EffectChain::setVolume(uint32_t *left, uint32_t *right) +// setVolume_l() must be called with PlaybackThread::mLock held +bool AudioFlinger::EffectChain::setVolume_l(uint32_t *left, uint32_t *right) { uint32_t newLeft = *left; uint32_t newRight = *right; bool hasControl = false; + int ctrlIdx = -1; + size_t size = mEffects.size(); - // first get volume update from volume controller - if (mVolumeCtrlIdx >= 0) { - mEffects[mVolumeCtrlIdx]->setVolume(&newLeft, &newRight, true); + // first update volume controller + for (size_t i = size; i > 0; i--) { + if (mEffects[i - 1]->isEnabled() && + (mEffects[i - 1]->desc().flags & EFFECT_FLAG_VOLUME_MASK) == EFFECT_FLAG_VOLUME_CTRL) { + ctrlIdx = i - 1; + break; + } + } + + if (ctrlIdx == mVolumeCtrlIdx && *left == mLeftVolume && *right == mRightVolume) { + return false; + } + + mVolumeCtrlIdx = ctrlIdx; + mLeftVolume = *left; + mRightVolume = *right; + + // second get volume update from volume controller + if (ctrlIdx >= 0) { + mEffects[ctrlIdx]->setVolume(&newLeft, &newRight, true); hasControl = true; } // then indicate volume to all other effects in chain. @@ -5992,11 +6005,11 @@ bool AudioFlinger::EffectChain::setVolume(uint32_t *left, uint32_t *right) // and requested volume to effects after controller uint32_t lVol = newLeft; uint32_t rVol = newRight; - size_t size = mEffects.size(); + for (size_t i = 0; i < size; i++) { - if ((int)i == mVolumeCtrlIdx) continue; - // this also works for mVolumeCtrlIdx == -1 when there is no volume controller - if ((int)i > mVolumeCtrlIdx) { + if ((int)i == ctrlIdx) continue; + // this also works for ctrlIdx == -1 when there is no volume controller + if ((int)i > ctrlIdx) { lVol = *left; rVol = *right; } @@ -6008,16 +6021,6 @@ bool AudioFlinger::EffectChain::setVolume(uint32_t *left, uint32_t *right) return hasControl; } -sp AudioFlinger::EffectChain::getVolumeController() -{ - sp effect; - if (mVolumeCtrlIdx >= 0) { - effect = mEffects[mVolumeCtrlIdx]; - } - return effect; -} - - status_t AudioFlinger::EffectChain::dump(int fd, const Vector& args) { const size_t SIZE = 256; @@ -6033,12 +6036,11 @@ status_t AudioFlinger::EffectChain::dump(int fd, const Vector& args) result.append("\tCould not lock mutex:\n"); } - result.append("\tNum fx In buffer Out buffer Vol ctrl Active tracks:\n"); - snprintf(buffer, SIZE, "\t%02d 0x%08x 0x%08x %02d %d\n", + result.append("\tNum fx In buffer Out buffer Active tracks:\n"); + snprintf(buffer, SIZE, "\t%02d 0x%08x 0x%08x %d\n", mEffects.size(), (uint32_t)mInBuffer, (uint32_t)mOutBuffer, - (mVolumeCtrlIdx == -1) ? 0 : mEffects[mVolumeCtrlIdx]->id(), mActiveTrackCnt); result.append(buffer); write(fd, result.string(), result.size()); diff --git a/services/audioflinger/AudioFlinger.h b/services/audioflinger/AudioFlinger.h index 7013d76..4507a48 100644 --- a/services/audioflinger/AudioFlinger.h +++ b/services/audioflinger/AudioFlinger.h @@ -1061,18 +1061,17 @@ private: mLock.unlock(); } - status_t addEffect(sp& handle); - size_t removeEffect(const sp& handle); + status_t addEffect_l(sp& handle); + size_t removeEffect_l(const sp& handle); int sessionId() { return mSessionId; } - sp getEffectFromDesc(effect_descriptor_t *descriptor); - sp getEffectFromId(int id); - sp getVolumeController(); - bool setVolume(uint32_t *left, uint32_t *right); - void setDevice(uint32_t device); - void setMode(uint32_t mode); + sp getEffectFromDesc_l(effect_descriptor_t *descriptor); + sp getEffectFromId_l(int id); + bool setVolume_l(uint32_t *left, uint32_t *right); + void setDevice_l(uint32_t device); + void setMode_l(uint32_t mode); void setInBuffer(int16_t *buffer, bool ownsBuffer = false) { @@ -1106,9 +1105,11 @@ private: int mSessionId; // audio session ID int16_t *mInBuffer; // chain input buffer int16_t *mOutBuffer; // chain output buffer - int mVolumeCtrlIdx; // index of insert effect having control over volume int mActiveTrackCnt; // number of active 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 + uint32_t mRightVolume; // previous volume on right channel }; friend class RecordThread; -- cgit v1.1