Merge "AudioFlinger: fix regression on device effect introduced locking order" into main (a8e0b582) · Commits · e / os / android_frameworks_av

services/audioflinger/DeviceEffectManager.cpp

+6 −1

Original line number	Diff line number	Diff line
		@@ -71,10 +71,15 @@ void DeviceEffectManager::onCreateAudioPatch(audio_patch_handle_t handle,

		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));
		}
		}
		}
		}

services/audioflinger/Effects.cpp

+22 −9

Original line number	Diff line number	Diff line
		@@ -1760,6 +1760,9 @@ sp<IAfEffectHandle> IAfEffectHandle::create(
		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 @@ sp<IAfEffectHandle> IAfEffectHandle::create(
		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 @@ Status EffectHandle::disconnect()

		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 @@ void EffectHandle::disconnect(bool unpinIfLast)
		{
		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 @@ NO_THREAD_SAFETY_ANALYSIS
		mHalEffect->setDevices({mDevice});
		}
		}
		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 @@ NO_THREAD_SAFETY_ANALYSIS
		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;
		}

services/audioflinger/Effects.h

+36 −5

Original line number	Diff line number	Diff line
		@@ -343,7 +343,8 @@ public:
		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 @@ public:
		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 @@ private:

		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 @@ private:
		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 @@ public:
		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;

services/audioflinger/IAfEffect.h

+8 −1

Original line number	Diff line number	Diff line
		@@ -400,7 +400,14 @@ public:
		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?