refactor mutexes for audio effects in audio flinger and audio policy
Remove effect specific mutex (mEffectLock) in AudioPolicyService: Due to
concurrent capture (among other reasons), it is necessary that the audio
policy manager state preserved by mLock includes audio effects
registration and enabling.
Moved all audio policy API calls from audio flinger out of locked regions
for audio flinger, thread and effects mutexes to avoid cross deadlocks
between audioflinger and audio policy manager:
- centralized audio policy API calls in EffectModule::updatePolicyState()
- the enabled state now reflects the state requested by the controlling
handle, not the actual effect processing state: a suspended effect is
now considered enabled.
A new audio policy manager API moveEffectsToIo() is added to atomically
handle moving effects to a new input or output without having to call
unregister > register > enable sequence.
Also fix assert in setStreamVolume to match volume group refactoring
in audio policy manager.
Bug: 128419018
Test: CTS tests for audio effects.
Test: manual tests with Duo calls, Play Music, Youtube, notifications
with and without Bluetooth and wired headset.
Change-Id: I8bd3af81026c55b6be283b3a9b41fe4998e060fd
diff --git a/services/audiopolicy/AudioPolicyInterface.h b/services/audiopolicy/AudioPolicyInterface.h
index a2cf7aa..caf0d7e 100644
--- a/services/audiopolicy/AudioPolicyInterface.h
+++ b/services/audiopolicy/AudioPolicyInterface.h
@@ -189,6 +189,7 @@
int id) = 0;
virtual status_t unregisterEffect(int id) = 0;
virtual status_t setEffectEnabled(int id, bool enabled) = 0;
+ virtual status_t moveEffectsToIo(const std::vector<int>& ids, audio_io_handle_t io) = 0;
virtual bool isStreamActive(audio_stream_type_t stream, uint32_t inPastMs = 0) const = 0;
virtual bool isStreamActiveRemotely(audio_stream_type_t stream,
diff --git a/services/audiopolicy/common/managerdefinitions/include/EffectDescriptor.h b/services/audiopolicy/common/managerdefinitions/include/EffectDescriptor.h
index 7f01dc5..c729ef9 100644
--- a/services/audiopolicy/common/managerdefinitions/include/EffectDescriptor.h
+++ b/services/audiopolicy/common/managerdefinitions/include/EffectDescriptor.h
@@ -65,6 +65,11 @@
uint32_t getMaxEffectsMemory() const;
bool isNonOffloadableEffectEnabled() const;
+ void moveEffects(audio_session_t session,
+ audio_io_handle_t srcOutput,
+ audio_io_handle_t dstOutput);
+ void moveEffects(const std::vector<int>& ids, audio_io_handle_t dstOutput);
+
void dump(String8 *dst, int spaces = 0, bool verbose = true) const;
private:
diff --git a/services/audiopolicy/common/managerdefinitions/src/EffectDescriptor.cpp b/services/audiopolicy/common/managerdefinitions/src/EffectDescriptor.cpp
index 89f9899..627fa8d 100644
--- a/services/audiopolicy/common/managerdefinitions/src/EffectDescriptor.cpp
+++ b/services/audiopolicy/common/managerdefinitions/src/EffectDescriptor.cpp
@@ -174,6 +174,32 @@
return MAX_EFFECTS_MEMORY;
}
+void EffectDescriptorCollection::moveEffects(audio_session_t session,
+ audio_io_handle_t srcOutput,
+ audio_io_handle_t dstOutput)
+{
+ ALOGV("%s session %d srcOutput %d dstOutput %d", __func__, session, srcOutput, dstOutput);
+ for (size_t i = 0; i < size(); i++) {
+ sp<EffectDescriptor> effect = valueAt(i);
+ if (effect->mSession == session && effect->mIo == srcOutput) {
+ effect->mIo = dstOutput;
+ }
+ }
+}
+
+void EffectDescriptorCollection::moveEffects(const std::vector<int>& ids,
+ audio_io_handle_t dstOutput)
+{
+ ALOGV("%s num effects %zu, first ID %d, dstOutput %d",
+ __func__, ids.size(), ids.size() ? ids[0] : 0, dstOutput);
+ for (size_t i = 0; i < size(); i++) {
+ sp<EffectDescriptor> effect = valueAt(i);
+ if (std::find(begin(ids), end(ids), effect->mId) != end(ids)) {
+ effect->mIo = dstOutput;
+ }
+ }
+}
+
void EffectDescriptorCollection::dump(String8 *dst, int spaces, bool verbose) const
{
if (verbose) {
diff --git a/services/audiopolicy/managerdefault/AudioPolicyManager.cpp b/services/audiopolicy/managerdefault/AudioPolicyManager.cpp
index af29f87..d8fbc38 100644
--- a/services/audiopolicy/managerdefault/AudioPolicyManager.cpp
+++ b/services/audiopolicy/managerdefault/AudioPolicyManager.cpp
@@ -2675,6 +2675,7 @@
}
if (output != mMusicEffectOutput) {
+ mEffects.moveEffects(AUDIO_SESSION_OUTPUT_MIX, mMusicEffectOutput, output);
mpClientInterface->moveEffects(AUDIO_SESSION_OUTPUT_MIX, mMusicEffectOutput, output);
mMusicEffectOutput = output;
}
@@ -2734,6 +2735,13 @@
return status;
}
+
+status_t AudioPolicyManager::moveEffectsToIo(const std::vector<int>& ids, audio_io_handle_t io)
+{
+ mEffects.moveEffects(ids, io);
+ return NO_ERROR;
+}
+
bool AudioPolicyManager::isStreamActive(audio_stream_type_t stream, uint32_t inPastMs) const
{
return mOutputs.isActive(toVolumeSource(stream), inPastMs);
diff --git a/services/audiopolicy/managerdefault/AudioPolicyManager.h b/services/audiopolicy/managerdefault/AudioPolicyManager.h
index de447fb..fd88841 100644
--- a/services/audiopolicy/managerdefault/AudioPolicyManager.h
+++ b/services/audiopolicy/managerdefault/AudioPolicyManager.h
@@ -200,6 +200,7 @@
int id);
virtual status_t unregisterEffect(int id);
virtual status_t setEffectEnabled(int id, bool enabled);
+ status_t moveEffectsToIo(const std::vector<int>& ids, audio_io_handle_t io) override;
virtual bool isStreamActive(audio_stream_type_t stream, uint32_t inPastMs = 0) const;
// return whether a stream is playing remotely, override to change the definition of
diff --git a/services/audiopolicy/service/AudioPolicyEffects.h b/services/audiopolicy/service/AudioPolicyEffects.h
index 6ad01f7..dcf093b 100644
--- a/services/audiopolicy/service/AudioPolicyEffects.h
+++ b/services/audiopolicy/service/AudioPolicyEffects.h
@@ -225,6 +225,8 @@
size_t *totSize);
// protects access to mInputSources, mInputSessions, mOutputStreams, mOutputSessions
+ // never hold AudioPolicyService::mLock when calling AudioPolicyEffects methods as
+ // those can call back into AudioPolicyService methods and try to acquire the mutex
Mutex mLock;
// Automatic input effects are configured per audio_source_t
KeyedVector< audio_source_t, EffectDescVector* > mInputSources;
diff --git a/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp b/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp
index 2eb272e..2e47eb6 100644
--- a/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp
+++ b/services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp
@@ -797,7 +797,7 @@
if (mAudioPolicyManager == NULL) {
return NO_INIT;
}
- Mutex::Autolock _l(mEffectsLock);
+ Mutex::Autolock _l(mLock);
AutoCallerClear acc;
return mAudioPolicyManager->registerEffect(desc, io, strategy, session, id);
}
@@ -807,7 +807,7 @@
if (mAudioPolicyManager == NULL) {
return NO_INIT;
}
- Mutex::Autolock _l(mEffectsLock);
+ Mutex::Autolock _l(mLock);
AutoCallerClear acc;
return mAudioPolicyManager->unregisterEffect(id);
}
@@ -817,11 +817,21 @@
if (mAudioPolicyManager == NULL) {
return NO_INIT;
}
- Mutex::Autolock _l(mEffectsLock);
+ Mutex::Autolock _l(mLock);
AutoCallerClear acc;
return mAudioPolicyManager->setEffectEnabled(id, enabled);
}
+status_t AudioPolicyService::moveEffectsToIo(const std::vector<int>& ids, audio_io_handle_t io)
+{
+ if (mAudioPolicyManager == NULL) {
+ return NO_INIT;
+ }
+ Mutex::Autolock _l(mLock);
+ AutoCallerClear acc;
+ return mAudioPolicyManager->moveEffectsToIo(ids, io);
+}
+
bool AudioPolicyService::isStreamActive(audio_stream_type_t stream, uint32_t inPastMs) const
{
if (uint32_t(stream) >= AUDIO_STREAM_PUBLIC_CNT) {
@@ -973,8 +983,6 @@
return false;
}
Mutex::Autolock _l(mLock);
- Mutex::Autolock _le(mEffectsLock); // isOffloadSupported queries for
- // non-offloadable effects
AutoCallerClear acc;
return mAudioPolicyManager->isOffloadSupported(info);
}
diff --git a/services/audiopolicy/service/AudioPolicyService.h b/services/audiopolicy/service/AudioPolicyService.h
index 189322f..0842649 100644
--- a/services/audiopolicy/service/AudioPolicyService.h
+++ b/services/audiopolicy/service/AudioPolicyService.h
@@ -134,6 +134,7 @@
int id);
virtual status_t unregisterEffect(int id);
virtual status_t setEffectEnabled(int id, bool enabled);
+ status_t moveEffectsToIo(const std::vector<int>& ids, audio_io_handle_t io) override;
virtual bool isStreamActive(audio_stream_type_t stream, uint32_t inPastMs = 0) const;
virtual bool isStreamActiveRemotely(audio_stream_type_t stream, uint32_t inPastMs = 0) const;
virtual bool isSourceActive(audio_source_t source) const;
@@ -810,7 +811,6 @@
mutable Mutex mLock; // prevents concurrent access to AudioPolicy manager functions changing
// device connection state or routing
- mutable Mutex mEffectsLock; // serialize access to Effect state within APM.
// Note: lock acquisition order is always mLock > mEffectsLock:
// mLock protects AudioPolicyManager methods that can call into audio flinger
// and possibly back in to audio policy service and acquire mEffectsLock.
@@ -824,6 +824,8 @@
DefaultKeyedVector< int64_t, sp<NotificationClient> > mNotificationClients;
Mutex mNotificationClientsLock; // protects mNotificationClients
// Manage all effects configured in audio_effects.conf
+ // never hold AudioPolicyService::mLock when calling AudioPolicyEffects methods as
+ // those can call back into AudioPolicyService methods and try to acquire the mutex
sp<AudioPolicyEffects> mAudioPolicyEffects;
audio_mode_t mPhoneState;