AudioPolicyEffects: Add thread safety annotations
Modernize sp<> creation.
Clean up comments.
Regroup and update legacy effect parsing methods to be static
(but do not assume Effect .conf files are supported as they
are not used for HIDL or AIDL HALs, enforced by VTS since Android P).
Test: atest CtsMediaAudioTestCases
Bug: 317816718
Change-Id: I3eff2e555863ba2c40bce8232441e9def64e55a2
diff --git a/services/audiopolicy/service/AudioPolicyEffects.cpp b/services/audiopolicy/service/AudioPolicyEffects.cpp
index 42f7899..0b42c50 100644
--- a/services/audiopolicy/service/AudioPolicyEffects.cpp
+++ b/services/audiopolicy/service/AudioPolicyEffects.cpp
@@ -42,15 +42,19 @@
// ----------------------------------------------------------------------------
AudioPolicyEffects::AudioPolicyEffects(const sp<EffectsFactoryHalInterface>& effectsFactoryHal) {
+ // Note: clang thread-safety permits the ctor to call guarded _l methods without
+ // acquiring the associated mutex capability as standard practice is to assume
+ // single threaded construction and destruction.
+
// load xml config with effectsFactoryHal
- status_t loadResult = loadAudioEffectConfig(effectsFactoryHal);
+ status_t loadResult = loadAudioEffectConfig_ll(effectsFactoryHal);
if (loadResult < 0) {
ALOGW("Failed to query effect configuration, fallback to load .conf");
// load automatic audio effect modules
if (access(AUDIO_EFFECT_VENDOR_CONFIG_FILE, R_OK) == 0) {
- loadAudioEffectConfigLegacy(AUDIO_EFFECT_VENDOR_CONFIG_FILE);
+ loadAudioEffectConfigLegacy_l(AUDIO_EFFECT_VENDOR_CONFIG_FILE);
} else if (access(AUDIO_EFFECT_DEFAULT_CONFIG_FILE, R_OK) == 0) {
- loadAudioEffectConfigLegacy(AUDIO_EFFECT_DEFAULT_CONFIG_FILE);
+ loadAudioEffectConfigLegacy_l(AUDIO_EFFECT_DEFAULT_CONFIG_FILE);
}
} else if (loadResult > 0) {
ALOGE("Effect config is partially invalid, skipped %d elements", loadResult);
@@ -540,7 +544,8 @@
// Audio processing configuration
// ----------------------------------------------------------------------------
-/*static*/ const char * const AudioPolicyEffects::kInputSourceNames[AUDIO_SOURCE_CNT -1] = {
+// we keep to const char* instead of std::string_view as comparison is believed faster.
+constexpr const char* kInputSourceNames[AUDIO_SOURCE_CNT - 1] = {
MIC_SRC_TAG,
VOICE_UL_SRC_TAG,
VOICE_DL_SRC_TAG,
@@ -567,7 +572,8 @@
return (audio_source_t)i;
}
-const char *AudioPolicyEffects::kStreamNames[AUDIO_STREAM_PUBLIC_CNT+1] = {
+// +1 as enum starts from -1
+constexpr const char* kStreamNames[AUDIO_STREAM_PUBLIC_CNT + 1] = {
AUDIO_STREAM_DEFAULT_TAG,
AUDIO_STREAM_VOICE_CALL_TAG,
AUDIO_STREAM_SYSTEM_TAG,
@@ -584,6 +590,7 @@
// returns the audio_stream_t enum corresponding to the output stream name or
// AUDIO_STREAM_PUBLIC_CNT is no match found
+/* static */
audio_stream_type_t AudioPolicyEffects::streamNameToEnum(const char *name)
{
int i;
@@ -600,6 +607,7 @@
// Audio Effect Config parser
// ----------------------------------------------------------------------------
+/* static */
size_t AudioPolicyEffects::growParamSize(char **param,
size_t size,
size_t *curSize,
@@ -623,7 +631,7 @@
return pos;
}
-
+/* static */
size_t AudioPolicyEffects::readParamValue(cnode *node,
char **param,
size_t *curSize,
@@ -692,6 +700,7 @@
return len;
}
+/* static */
effect_param_t *AudioPolicyEffects::loadEffectParameter(cnode *root)
{
cnode *param;
@@ -767,6 +776,7 @@
return NULL;
}
+/* static */
void AudioPolicyEffects::loadEffectParameters(cnode *root, Vector <effect_param_t *>& params)
{
cnode *node = root->first_child;
@@ -780,7 +790,7 @@
}
}
-
+/* static */
AudioPolicyEffects::EffectDescVector *AudioPolicyEffects::loadEffectConfig(
cnode *root,
const Vector <EffectDesc *>& effects)
@@ -820,7 +830,7 @@
return desc;
}
-status_t AudioPolicyEffects::loadInputEffectConfigurations(cnode *root,
+status_t AudioPolicyEffects::loadInputEffectConfigurations_l(cnode* root,
const Vector <EffectDesc *>& effects)
{
cnode *node = config_find(root, PREPROCESSING_TAG);
@@ -831,11 +841,11 @@
while (node) {
audio_source_t source = inputSourceNameToEnum(node->name);
if (source == AUDIO_SOURCE_CNT) {
- ALOGW("loadInputSources() invalid input source %s", node->name);
+ ALOGW("%s() invalid input source %s", __func__, node->name);
node = node->next;
continue;
}
- ALOGV("loadInputSources() loading input source %s", node->name);
+ ALOGV("%s() loading input source %s", __func__, node->name);
EffectDescVector *desc = loadEffectConfig(node, effects);
if (desc == NULL) {
node = node->next;
@@ -847,7 +857,7 @@
return NO_ERROR;
}
-status_t AudioPolicyEffects::loadStreamEffectConfigurations(cnode *root,
+status_t AudioPolicyEffects::loadStreamEffectConfigurations_l(cnode* root,
const Vector <EffectDesc *>& effects)
{
cnode *node = config_find(root, OUTPUT_SESSION_PROCESSING_TAG);
@@ -858,11 +868,11 @@
while (node) {
audio_stream_type_t stream = streamNameToEnum(node->name);
if (stream == AUDIO_STREAM_PUBLIC_CNT) {
- ALOGW("loadStreamEffectConfigurations() invalid output stream %s", node->name);
+ ALOGW("%s() invalid output stream %s", __func__, node->name);
node = node->next;
continue;
}
- ALOGV("loadStreamEffectConfigurations() loading output stream %s", node->name);
+ ALOGV("%s() loading output stream %s", __func__, node->name);
EffectDescVector *desc = loadEffectConfig(node, effects);
if (desc == NULL) {
node = node->next;
@@ -874,6 +884,7 @@
return NO_ERROR;
}
+/* static */
AudioPolicyEffects::EffectDesc *AudioPolicyEffects::loadEffect(cnode *root)
{
cnode *node = config_find(root, UUID_TAG);
@@ -888,6 +899,7 @@
return new EffectDesc(root->name, uuid);
}
+/* static */
status_t AudioPolicyEffects::loadEffects(cnode *root, Vector <EffectDesc *>& effects)
{
cnode *node = config_find(root, EFFECTS_TAG);
@@ -908,7 +920,7 @@
return NO_ERROR;
}
-status_t AudioPolicyEffects::loadAudioEffectConfig(
+status_t AudioPolicyEffects::loadAudioEffectConfig_ll(
const sp<EffectsFactoryHalInterface>& effectsFactoryHal) {
if (!effectsFactoryHal) {
ALOGE("%s Null EffectsFactoryHalInterface", __func__);
@@ -944,18 +956,17 @@
}
};
+ // access to mInputSources and mOutputStreams requires mMutex;
loadProcessingChain(processings->preprocess, mInputSources);
loadProcessingChain(processings->postprocess, mOutputStreams);
- {
- audio_utils::lock_guard _l(mMutex);
- loadDeviceProcessingChain(processings->deviceprocess, mDeviceEffects);
- }
+ // access to mDeviceEffects requires mDeviceEffectsMutex
+ loadDeviceProcessingChain(processings->deviceprocess, mDeviceEffects);
return skippedElements;
}
-status_t AudioPolicyEffects::loadAudioEffectConfigLegacy(const char *path)
+status_t AudioPolicyEffects::loadAudioEffectConfigLegacy_l(const char *path)
{
cnode *root;
char *data;
@@ -969,8 +980,10 @@
Vector <EffectDesc *> effects;
loadEffects(root, effects);
- loadInputEffectConfigurations(root, effects);
- loadStreamEffectConfigurations(root, effects);
+
+ // requires mMutex
+ loadInputEffectConfigurations_l(root, effects);
+ loadStreamEffectConfigurations_l(root, effects);
for (size_t i = 0; i < effects.size(); i++) {
delete effects[i];
@@ -985,7 +998,7 @@
void AudioPolicyEffects::initDefaultDeviceEffects()
{
- audio_utils::lock_guard _l(mMutex);
+ std::lock_guard _l(mDeviceEffectsMutex);
for (const auto& deviceEffectsIter : mDeviceEffects) {
const auto& deviceEffects = deviceEffectsIter.second;
for (const auto& effectDesc : deviceEffects->mEffectDescriptors->mEffects) {
diff --git a/services/audiopolicy/service/AudioPolicyEffects.h b/services/audiopolicy/service/AudioPolicyEffects.h
index 7f41f09..bbfad41 100644
--- a/services/audiopolicy/service/AudioPolicyEffects.h
+++ b/services/audiopolicy/service/AudioPolicyEffects.h
@@ -14,8 +14,7 @@
* limitations under the License.
*/
-#ifndef ANDROID_AUDIOPOLICYEFFECTS_H
-#define ANDROID_AUDIOPOLICYEFFECTS_H
+#pragma once
#include <stdlib.h>
#include <stdio.h>
@@ -57,7 +56,7 @@
// First it will look whether vendor specific file exists,
// otherwise it will parse the system default file.
explicit AudioPolicyEffects(const sp<EffectsFactoryHalInterface>& effectsFactoryHal);
- virtual ~AudioPolicyEffects();
+ ~AudioPolicyEffects() override;
// NOTE: methods on AudioPolicyEffects should never be called with the AudioPolicyService
// main mutex (mMutex) held as they will indirectly call back into AudioPolicyService when
@@ -67,34 +66,34 @@
// associated with audioSession
status_t queryDefaultInputEffects(audio_session_t audioSession,
effect_descriptor_t *descriptors,
- uint32_t *count);
+ uint32_t* count) EXCLUDES_AudioPolicyEffects_Mutex;
// Add all input effects associated with this input
// Effects are attached depending on the audio_source_t
status_t addInputEffects(audio_io_handle_t input,
audio_source_t inputSource,
- audio_session_t audioSession);
+ audio_session_t audioSession) EXCLUDES_AudioPolicyEffects_Mutex;
// Add all input effects associated to this input
status_t releaseInputEffects(audio_io_handle_t input,
- audio_session_t audioSession);
+ audio_session_t audioSession) EXCLUDES_AudioPolicyEffects_Mutex;
// Return a list of effect descriptors for default output effects
// associated with audioSession
status_t queryDefaultOutputSessionEffects(audio_session_t audioSession,
effect_descriptor_t *descriptors,
- uint32_t *count);
+ uint32_t* count) EXCLUDES_AudioPolicyEffects_Mutex;
// Add all output effects associated to this output
// Effects are attached depending on the audio_stream_type_t
status_t addOutputSessionEffects(audio_io_handle_t output,
audio_stream_type_t stream,
- audio_session_t audioSession);
+ audio_session_t audioSession) EXCLUDES_AudioPolicyEffects_Mutex;
// release all output effects associated with this output stream and audiosession
status_t releaseOutputSessionEffects(audio_io_handle_t output,
audio_stream_type_t stream,
- audio_session_t audioSession);
+ audio_session_t audioSession) EXCLUDES_AudioPolicyEffects_Mutex;
// Add the effect to the list of default effects for sources of type |source|.
status_t addSourceDefaultEffect(const effect_uuid_t *type,
@@ -102,7 +101,7 @@
const effect_uuid_t *uuid,
int32_t priority,
audio_source_t source,
- audio_unique_id_t* id);
+ audio_unique_id_t* id) EXCLUDES_AudioPolicyEffects_Mutex;
// Add the effect to the list of default effects for streams of a given usage.
status_t addStreamDefaultEffect(const effect_uuid_t *type,
@@ -110,18 +109,20 @@
const effect_uuid_t *uuid,
int32_t priority,
audio_usage_t usage,
- audio_unique_id_t* id);
+ audio_unique_id_t* id) EXCLUDES_AudioPolicyEffects_Mutex;
// Remove the default source effect from wherever it's attached.
- status_t removeSourceDefaultEffect(audio_unique_id_t id);
+ status_t removeSourceDefaultEffect(audio_unique_id_t id) EXCLUDES_AudioPolicyEffects_Mutex;
// Remove the default stream effect from wherever it's attached.
- status_t removeStreamDefaultEffect(audio_unique_id_t id);
+ status_t removeStreamDefaultEffect(audio_unique_id_t id) EXCLUDES_AudioPolicyEffects_Mutex;
+ // Called by AudioPolicyService::onFirstRef() to load device effects
+ // on a separate worker thread.
+ // TODO(b/319515492) move this initialization after AudioPolicyService::onFirstRef().
void setDefaultDeviceEffects();
private:
- void initDefaultDeviceEffects();
// class to store the description of an effects and its parameters
// as defined in audio_effects.conf
@@ -232,65 +233,84 @@
};
- static const char * const kInputSourceNames[AUDIO_SOURCE_CNT -1];
+ // Called on an async thread because it creates AudioEffects
+ // which register with AudioFlinger and AudioPolicy.
+ // We must therefore exclude the EffectHandle_Mutex.
+ void initDefaultDeviceEffects() EXCLUDES(mDeviceEffectsMutex) EXCLUDES_EffectHandle_Mutex;
+
+ status_t loadAudioEffectConfig_ll(const sp<EffectsFactoryHalInterface>& effectsFactoryHal)
+ REQUIRES(mMutex, mDeviceEffectsMutex);
+
+ // Legacy: Begin methods below.
+ // Parse audio_effects.conf - called from constructor.
+ status_t loadAudioEffectConfigLegacy_l(const char* path) REQUIRES(mMutex);
+
+ // Legacy: Load all automatic effect configurations
+ status_t loadInputEffectConfigurations_l(
+ cnode* root, const Vector<EffectDesc*>& effects) REQUIRES(mMutex);
+ status_t loadStreamEffectConfigurations_l(
+ cnode* root, const Vector<EffectDesc*>& effects) REQUIRES(mMutex);
+
+ // Legacy: static methods below.
+
static audio_source_t inputSourceNameToEnum(const char *name);
- static const char *kStreamNames[AUDIO_STREAM_PUBLIC_CNT+1]; //+1 required as streams start from -1
- audio_stream_type_t streamNameToEnum(const char *name);
-
- // Parse audio_effects.conf
- status_t loadAudioEffectConfigLegacy(const char *path);
- status_t loadAudioEffectConfig(const sp<EffectsFactoryHalInterface>& effectsFactoryHal);
+ static audio_stream_type_t streamNameToEnum(const char* name);
// Load all effects descriptors in configuration file
- status_t loadEffects(cnode *root, Vector <EffectDesc *>& effects);
- EffectDesc *loadEffect(cnode *root);
-
- // Load all automatic effect configurations
- status_t loadInputEffectConfigurations(cnode *root, const Vector <EffectDesc *>& effects);
- status_t loadStreamEffectConfigurations(cnode *root, const Vector <EffectDesc *>& effects);
- EffectDescVector *loadEffectConfig(cnode *root, const Vector <EffectDesc *>& effects);
+ static status_t loadEffects(cnode* root, Vector<EffectDesc*>& effects);
+ static EffectDesc* loadEffect(cnode* root);
+ static EffectDescVector *loadEffectConfig(cnode *root, const Vector <EffectDesc *>& effects);
// Load all automatic effect parameters
- void loadEffectParameters(cnode *root, Vector <effect_param_t *>& params);
- effect_param_t *loadEffectParameter(cnode *root);
- size_t readParamValue(cnode *node,
+ static void loadEffectParameters(cnode* root, Vector<effect_param_t*>& params);
+ static effect_param_t* loadEffectParameter(cnode* root);
+ static size_t readParamValue(cnode* node,
char **param,
size_t *curSize,
size_t *totSize);
- size_t growParamSize(char **param,
+ static size_t growParamSize(char** param,
size_t size,
size_t *curSize,
size_t *totSize);
+ // Legacy: End methods above.
+
// protects access to mInputSources, mInputSessions, mOutputStreams, mOutputSessions
// never hold AudioPolicyService::mMutex when calling AudioPolicyEffects methods as
// those can call back into AudioPolicyService methods and try to acquire the mutex
mutable audio_utils::mutex mMutex{audio_utils::MutexOrder::kAudioPolicyEffects_Mutex};
// Automatic input effects are configured per audio_source_t
- KeyedVector< audio_source_t, EffectDescVector* > mInputSources;
- // Automatic input effects are unique for audio_io_handle_t
- KeyedVector< audio_session_t, EffectVector* > mInputSessions;
+ KeyedVector<audio_source_t, EffectDescVector*> mInputSources GUARDED_BY(mMutex);
+ // Automatic input effects are unique for an audio_session_t.
+ KeyedVector<audio_session_t, EffectVector*> mInputSessions GUARDED_BY(mMutex);
// Automatic output effects are organized per audio_stream_type_t
- KeyedVector< audio_stream_type_t, EffectDescVector* > mOutputStreams;
- // Automatic output effects are unique for audiosession ID
- KeyedVector< audio_session_t, EffectVector* > mOutputSessions;
+ KeyedVector<audio_stream_type_t, EffectDescVector*> mOutputStreams GUARDED_BY(mMutex);
+ // Automatic output effects are unique for an audio_session_t.
+ KeyedVector<audio_session_t, EffectVector*> mOutputSessions GUARDED_BY(mMutex);
/**
* @brief mDeviceEffects map of device effects indexed by the device address
*/
- std::map<std::string, std::unique_ptr<DeviceEffects>> mDeviceEffects GUARDED_BY(mMutex);
+
+ // mDeviceEffects is never accessed through AudioPolicyEffects methods.
+ // We keep a separate mutex here to catch future methods attempting to access this variable.
+ std::mutex mDeviceEffectsMutex;
+ std::map<std::string, std::unique_ptr<DeviceEffects>> mDeviceEffects
+ GUARDED_BY(mDeviceEffectsMutex);
/**
* Device Effect initialization must be asynchronous: the audio_policy service parses and init
* effect on first reference. AudioFlinger will handle effect creation and register these
* effect on audio_policy service.
- * We must store the reference of the furture garantee real asynchronous operation.
+ *
+ * The future is associated with the std::async launched thread - no need to lock as
+ * it is only set once on init. Due to the async nature, it is conceivable that
+ * some device effects are not available immediately after AudioPolicyService::onFirstRef()
+ * while the effects are being created.
*/
std::future<void> mDefaultDeviceEffectFuture;
};
} // namespace android
-
-#endif // ANDROID_AUDIOPOLICYEFFECTS_H
diff --git a/services/audiopolicy/service/AudioPolicyService.cpp b/services/audiopolicy/service/AudioPolicyService.cpp
index 835a617..57e2718 100644
--- a/services/audiopolicy/service/AudioPolicyService.cpp
+++ b/services/audiopolicy/service/AudioPolicyService.cpp
@@ -280,9 +280,9 @@
// load audio processing modules
const sp<EffectsFactoryHalInterface> effectsFactoryHal = EffectsFactoryHalInterface::create();
- sp<AudioPolicyEffects> audioPolicyEffects = new AudioPolicyEffects(effectsFactoryHal);
- sp<UidPolicy> uidPolicy = new UidPolicy(this);
- sp<SensorPrivacyPolicy> sensorPrivacyPolicy = new SensorPrivacyPolicy(this);
+ auto audioPolicyEffects = sp<AudioPolicyEffects>::make(effectsFactoryHal);
+ auto uidPolicy = sp<UidPolicy>::make(this);
+ auto sensorPrivacyPolicy = sp<SensorPrivacyPolicy>::make(this);
{
audio_utils::lock_guard _l(mMutex);
mAudioPolicyEffects = audioPolicyEffects;