AudioFlinger: fix regression on device effect introduced locking order
The lock order has been enforced within AudioFlinger.
Order has been defined. However, the Device Effect does not follow
the same pattern as session effects.
Session:
Client -> EffectHandle -> ThreadBase_Mutex (or AudioFlinger Mutex)
Device:
(Default effect)
AudioFlinger -> DeviceEffectManager -> DeviceEffectProxy -> DeviceEffectHandle
(added by Fx App)
Client -> EffectHandle -> DeviceEffectProxy -> DeviceEffectHandle
-Can be addded automatically by AudioFlinger upon patch creation
-May be Added to Audio IDevice
-Can be enabled / disabled by AudioFx app via DeviceEffectProxy.
-Use internal device effect handle that are now controlled by application
diretly, preventing from risk of concurrent access (e.g. disconnect /
enabled status)
This CL intends to fix the locking order assertions by
-introducing a new lock for internal device effect Handle
-unlocking temporarily to prevent breaking the lock order.
Flag: EXEMPT bugfix
Bug: 329395147
Bug: 348986455
Test: atest CtsMediaAudioTestCases
Change-Id: I6667c3ef1b77708ccb4d644fd1b53a2fe3896b72
Signed-off-by: François Gaffie <francois.gaffie@renault.com>
diff --git a/services/audioflinger/DeviceEffectManager.cpp b/services/audioflinger/DeviceEffectManager.cpp
index feae97e..7cb9329 100644
--- a/services/audioflinger/DeviceEffectManager.cpp
+++ b/services/audioflinger/DeviceEffectManager.cpp
@@ -71,10 +71,15 @@
void DeviceEffectManager::onReleaseAudioPatch(audio_patch_handle_t handle) {
ALOGV("%s", __func__);
+ // Keep a reference on disconnected handle to delay destruction without lock held.
+ std::vector<sp<IAfEffectHandle>> disconnectedHandles{};
audio_utils::lock_guard _l(mutex());
for (auto& effectProxies : mDeviceEffects) {
for (auto& effect : effectProxies.second) {
- effect->onReleasePatch(handle);
+ sp<IAfEffectHandle> disconnectedHandle = effect->onReleasePatch(handle);
+ if (disconnectedHandle != nullptr) {
+ disconnectedHandles.push_back(std::move(disconnectedHandle));
+ }
}
}
}
diff --git a/services/audioflinger/Effects.cpp b/services/audioflinger/Effects.cpp
index 973b2ab..0405035 100644
--- a/services/audioflinger/Effects.cpp
+++ b/services/audioflinger/Effects.cpp
@@ -1760,6 +1760,9 @@
const sp<media::IEffectClient>& effectClient,
int32_t priority, bool notifyFramesProcessed)
{
+ if (client == nullptr && effectClient == nullptr) {
+ return sp<InternalEffectHandle>::make(effect, notifyFramesProcessed);
+ }
return sp<EffectHandle>::make(
effect, client, effectClient, priority, notifyFramesProcessed);
}
@@ -1767,12 +1770,14 @@
EffectHandle::EffectHandle(const sp<IAfEffectBase>& effect,
const sp<Client>& client,
const sp<media::IEffectClient>& effectClient,
- int32_t priority, bool notifyFramesProcessed)
- : BnEffect(),
+ int32_t priority, bool notifyFramesProcessed,
+ bool isInternal,
+ audio_utils::MutexOrder mutexOrder)
+ : BnEffect(), mMutex(mutexOrder),
mEffect(effect), mEffectClient(media::EffectClientAsyncProxy::makeIfNeeded(effectClient)),
mClient(client), mCblk(nullptr),
mPriority(priority), mHasControl(false), mEnabled(false), mDisconnected(false),
- mNotifyFramesProcessed(notifyFramesProcessed)
+ mNotifyFramesProcessed(notifyFramesProcessed), mIsInternal(isInternal)
{
ALOGV("constructor %p client %p", this, client.get());
setMinSchedulerPolicy(SCHED_NORMAL, ANDROID_PRIORITY_AUDIO);
@@ -1939,7 +1944,7 @@
void EffectHandle::disconnect(bool unpinIfLast)
{
- audio_utils::lock_guard _l(mutex());
+ audio_utils::unique_lock _l(mutex());
ALOGV("disconnect(%s) %p", unpinIfLast ? "true" : "false", this);
if (mDisconnected) {
if (unpinIfLast) {
@@ -1951,11 +1956,19 @@
{
sp<IAfEffectBase> effect = mEffect.promote();
if (effect != 0) {
+ // Unlock e.g. for device effect: may need to acquire AudioFlinger lock
+ // Also Internal Effect Handle would require Proxy lock (and vice versa).
+ if (isInternal()) {
+ _l.unlock();
+ }
if (effect->disconnectHandle(this, unpinIfLast) > 0) {
ALOGW("%s Effect handle %p disconnected after thread destruction",
__func__, this);
}
effect->updatePolicyState();
+ if (isInternal()) {
+ _l.lock();
+ }
}
}
@@ -3544,8 +3557,7 @@
}
mHalEffect->configure_l();
}
- *handle = new EffectHandle(mHalEffect, nullptr, nullptr, 0 /*priority*/,
- mNotifyFramesProcessed);
+ *handle = sp<InternalEffectHandle>::make(mHalEffect, mNotifyFramesProcessed);
status = (*handle)->initCheck();
if (status == OK) {
status = mHalEffect->addHandle((*handle).get());
@@ -3592,15 +3604,16 @@
return status;
}
-void DeviceEffectProxy::onReleasePatch(audio_patch_handle_t patchHandle) {
- sp<IAfEffectHandle> effect;
+sp<IAfEffectHandle> DeviceEffectProxy::onReleasePatch(audio_patch_handle_t patchHandle) {
+ sp<IAfEffectHandle> disconnectedHandle;
{
audio_utils::lock_guard _l(proxyMutex());
if (mEffectHandles.find(patchHandle) != mEffectHandles.end()) {
- effect = mEffectHandles.at(patchHandle);
+ disconnectedHandle = std::move(mEffectHandles.at(patchHandle));
mEffectHandles.erase(patchHandle);
}
}
+ return disconnectedHandle;
}
diff --git a/services/audioflinger/Effects.h b/services/audioflinger/Effects.h
index 830fe81..35e18d7 100644
--- a/services/audioflinger/Effects.h
+++ b/services/audioflinger/Effects.h
@@ -343,7 +343,8 @@
EffectHandle(const sp<IAfEffectBase>& effect,
const sp<Client>& client,
const sp<media::IEffectClient>& effectClient,
- int32_t priority, bool notifyFramesProcessed);
+ int32_t priority, bool notifyFramesProcessed, bool isInternal = false,
+ audio_utils::MutexOrder mutexOrder = audio_utils::MutexOrder::kEffectHandle_Mutex);
~EffectHandle() override;
status_t onTransact(
uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) final;
@@ -363,6 +364,11 @@
int32_t* _aidl_return) final;
const sp<Client>& client() const final { return mClient; }
+ /**
+ * Checks if the handle is internal, aka created by AudioFlinger for internal needs (e.g.
+ * device effect HAL handle or device effect thread handle).
+ */
+ virtual bool isInternal() const { return mIsInternal; }
sp<android::media::IEffect> asIEffect() final {
return sp<android::media::IEffect>::fromExisting(this);
@@ -400,15 +406,18 @@
void dumpToBuffer(char* buffer, size_t size) const final;
+protected:
+ // protects IEffect method calls
+ mutable audio_utils::mutex mMutex;
private:
DISALLOW_COPY_AND_ASSIGN(EffectHandle);
- audio_utils::mutex& mutex() const RETURN_CAPABILITY(android::audio_utils::EffectHandle_Mutex) {
+ virtual audio_utils::mutex& mutex() const
+ RETURN_CAPABILITY(android::audio_utils::EffectHandle_Mutex) {
return mMutex;
}
- // protects IEffect method calls
- mutable audio_utils::mutex mMutex{audio_utils::MutexOrder::kEffectHandle_Mutex};
+
const wp<IAfEffectBase> mEffect; // pointer to controlled EffectModule
const sp<media::IEffectClient> mEffectClient; // callback interface for client notifications
/*const*/ sp<Client> mClient; // client for shared memory allocation, see
@@ -424,6 +433,28 @@
bool mDisconnected; // Set to true by disconnect()
const bool mNotifyFramesProcessed; // true if the client callback event
// EVENT_FRAMES_PROCESSED must be generated
+ const bool mIsInternal;
+};
+
+/**
+ * There are 2 types of effects:
+ * -Session Effect: handle is directly called from the client, without AudioFlinger lock.
+ * -Device Effect: a device effect proxy is aggregating a collection of internal effect handles that
+ * controls the same effect added on all audio patches involving the device effect selected port
+ * requested either by a client or by AudioPolicyEffects. These internal effect handles do not have
+ * client. Sequence flow implies a different locking order, hence the lock is specialied.
+ */
+class InternalEffectHandle : public EffectHandle {
+public:
+ InternalEffectHandle(const sp<IAfEffectBase>& effect, bool notifyFramesProcessed) :
+ EffectHandle(effect, /* client= */ nullptr, /* effectClient= */ nullptr,
+ /* priority= */ 0, notifyFramesProcessed, /* isInternal */ true,
+ audio_utils::MutexOrder::kDeviceEffectHandle_Mutex) {}
+
+ virtual audio_utils::mutex& mutex() const
+ RETURN_CAPABILITY(android::audio_utils::DeviceEffectHandle_Mutex) {
+ return mMutex;
+ }
};
// the EffectChain class represents a group of effects associated to one audio session.
@@ -761,7 +792,7 @@
status_t onUpdatePatch(audio_patch_handle_t oldPatchHandle, audio_patch_handle_t newPatchHandle,
const IAfPatchPanel::Patch& patch) final;
- void onReleasePatch(audio_patch_handle_t patchHandle) final;
+ sp<IAfEffectHandle> onReleasePatch(audio_patch_handle_t patchHandle) final;
size_t removeEffect(const sp<IAfEffectModule>& effect) final;
diff --git a/services/audioflinger/IAfEffect.h b/services/audioflinger/IAfEffect.h
index f1edcfe..3452e94 100644
--- a/services/audioflinger/IAfEffect.h
+++ b/services/audioflinger/IAfEffect.h
@@ -400,7 +400,14 @@
virtual status_t onUpdatePatch(audio_patch_handle_t oldPatchHandle,
audio_patch_handle_t newPatchHandle,
const IAfPatchPanel::Patch& patch) = 0;
- virtual void onReleasePatch(audio_patch_handle_t patchHandle) = 0;
+ /**
+ * Checks (and release) of the effect handle is linked with the given release patch handle.
+ *
+ * @param patchHandle handle of the released patch
+ * @return a reference on the effect handle released if any, nullptr otherwise.
+ * It allows to delay the destruction of the handle.
+ */
+ virtual sp<IAfEffectHandle> onReleasePatch(audio_patch_handle_t patchHandle) = 0;
virtual void dump2(int fd, int spaces) const = 0; // TODO(b/291319101) naming?