Merge "Make EffectFactory implementation thread-safe" into main (cdac9b50) · Commits · e / os / android_hardware_interfaces

audio/aidl/default/EffectFactory.cpp

+21 −15

Original line number	Diff line number	Diff line
		@@ -49,18 +49,18 @@ Factory::~Factory() {
		if (auto spEffect = it.first.lock()) {
		LOG(ERROR) << __func__ << " erase remaining instance UUID "
		<< ::android::audio::utils::toString(it.second.first);
		destroyEffectImpl(spEffect);
		destroyEffectImpl_l(spEffect);
		}
		}
		}
		}

		ndk::ScopedAStatus Factory::getDescriptorWithUuid(const AudioUuid& uuid, Descriptor* desc) {
		ndk::ScopedAStatus Factory::getDescriptorWithUuid_l(const AudioUuid& uuid, Descriptor* desc) {
		RETURN_IF(!desc, EX_NULL_POINTER, "nullDescriptor");

		if (mEffectLibMap.count(uuid)) {
		auto& entry = mEffectLibMap[uuid];
		getDlSyms(entry);
		getDlSyms_l(entry);
		auto& libInterface = std::get<kMapEntryInterfaceIndex>(entry);
		RETURN_IF(!libInterface \|\| !libInterface->queryEffectFunc, EX_NULL_POINTER,
		"dlNullQueryEffectFunc");
		@@ -75,6 +75,7 @@ ndk::ScopedAStatus Factory::queryEffects(const std::optional<AudioUuid>& in_type
		const std::optional<AudioUuid>& in_impl_uuid,
		const std::optional<AudioUuid>& in_proxy_uuid,
		std::vector<Descriptor>* _aidl_return) {
		std::lock_guard lg(mMutex);
		// get the matching list
		std::vector<Descriptor::Identity> idList;
		std::copy_if(mIdentitySet.begin(), mIdentitySet.end(), std::back_inserter(idList),
		@@ -88,7 +89,8 @@ ndk::ScopedAStatus Factory::queryEffects(const std::optional<AudioUuid>& in_type
		for (const auto& id : idList) {
		if (mEffectLibMap.count(id.uuid)) {
		Descriptor desc;
		RETURN_IF_ASTATUS_NOT_OK(getDescriptorWithUuid(id.uuid, &desc), "getDescriptorFailed");
		RETURN_IF_ASTATUS_NOT_OK(getDescriptorWithUuid_l(id.uuid, &desc),
		"getDescriptorFailed");
		// update proxy UUID with information from config xml
		desc.common.id.proxy = id.proxy;
		_aidl_return->emplace_back(std::move(desc));
		@@ -99,6 +101,7 @@ ndk::ScopedAStatus Factory::queryEffects(const std::optional<AudioUuid>& in_type

		ndk::ScopedAStatus Factory::queryProcessing(const std::optional<Processing::Type>& in_type,
		std::vector<Processing>* _aidl_return) {
		std::lock_guard lg(mMutex);
		const auto& processings = mConfig.getProcessingMap();
		// Processing stream type
		for (const auto& procIter : processings) {
		@@ -110,7 +113,7 @@ ndk::ScopedAStatus Factory::queryProcessing(const std::optional<Processing::Type
		if (libs.proxyLibrary.has_value()) {
		desc.common.id.proxy = libs.proxyLibrary.value().uuid;
		}
		RETURN_IF_ASTATUS_NOT_OK(getDescriptorWithUuid(lib.uuid, &desc),
		RETURN_IF_ASTATUS_NOT_OK(getDescriptorWithUuid_l(lib.uuid, &desc),
		"getDescriptorFailed");
		process.ids.emplace_back(desc);
		}
		@@ -125,9 +128,10 @@ ndk::ScopedAStatus Factory::queryProcessing(const std::optional<Processing::Type
		ndk::ScopedAStatus Factory::createEffect(const AudioUuid& in_impl_uuid,
		std::shared_ptr<IEffect>* _aidl_return) {
		LOG(DEBUG) << __func__ << ": UUID " << ::android::audio::utils::toString(in_impl_uuid);
		std::lock_guard lg(mMutex);
		if (mEffectLibMap.count(in_impl_uuid)) {
		auto& entry = mEffectLibMap[in_impl_uuid];
		getDlSyms(entry);
		getDlSyms_l(entry);

		auto& libInterface = std::get<kMapEntryInterfaceIndex>(entry);
		RETURN_IF(!libInterface \|\| !libInterface->createEffectFunc, EX_NULL_POINTER,
		@@ -152,7 +156,7 @@ ndk::ScopedAStatus Factory::createEffect(const AudioUuid& in_impl_uuid,
		return ndk::ScopedAStatus::ok();
		}

		ndk::ScopedAStatus Factory::destroyEffectImpl(const std::shared_ptr<IEffect>& in_handle) {
		ndk::ScopedAStatus Factory::destroyEffectImpl_l(const std::shared_ptr<IEffect>& in_handle) {
		std::weak_ptr<IEffect> wpHandle(in_handle);
		// find the effect entry with key (std::weak_ptr<IEffect>)
		if (auto effectIt = mEffectMap.find(wpHandle); effectIt != mEffectMap.end()) {
		@@ -177,7 +181,7 @@ ndk::ScopedAStatus Factory::destroyEffectImpl(const std::shared_ptr<IEffect>& in
		}

		// go over the map and cleanup all expired weak_ptrs.
		void Factory::cleanupEffectMap() {
		void Factory::cleanupEffectMap_l() {
		for (auto it = mEffectMap.begin(); it != mEffectMap.end();) {
		if (nullptr == it->first.lock()) {
		it = mEffectMap.erase(it);
		@@ -189,13 +193,15 @@ void Factory::cleanupEffectMap() {

		ndk::ScopedAStatus Factory::destroyEffect(const std::shared_ptr<IEffect>& in_handle) {
		LOG(DEBUG) << __func__ << ": instance " << in_handle.get();
		ndk::ScopedAStatus status = destroyEffectImpl(in_handle);
		std::lock_guard lg(mMutex);
		ndk::ScopedAStatus status = destroyEffectImpl_l(in_handle);
		// always do the cleanup
		cleanupEffectMap();
		cleanupEffectMap_l();
		return status;
		}

		bool Factory::openEffectLibrary(const AudioUuid& impl, const std::string& path) {
		bool Factory::openEffectLibrary(const AudioUuid& impl,
		const std::string& path) NO_THREAD_SAFETY_ANALYSIS {
		std::function<void(void)> dlClose = [](void handle) -> void {
		if (handle && dlclose(handle)) {
		LOG(ERROR) << "dlclose failed " << dlerror();
		@@ -219,9 +225,9 @@ bool Factory::openEffectLibrary(const AudioUuid& impl, const std::string& path)
		return true;
		}

		void Factory::createIdentityWithConfig(const EffectConfig::Library& configLib,
		const AudioUuid& typeUuid,
		const std::optional<AudioUuid> proxyUuid) {
		void Factory::createIdentityWithConfig(
		const EffectConfig::Library& configLib, const AudioUuid& typeUuid,
		const std::optional<AudioUuid> proxyUuid) NO_THREAD_SAFETY_ANALYSIS {
		static const auto& libMap = mConfig.getLibraryMap();
		const std::string& libName = configLib.name;
		if (auto path = libMap.find(libName); path != libMap.end()) {
		@@ -263,7 +269,7 @@ void Factory::loadEffectLibs() {
		}
		}

		void Factory::getDlSyms(DlEntry& entry) {
		void Factory::getDlSyms_l(DlEntry& entry) {
		auto& dlHandle = std::get<kMapEntryHandleIndex>(entry);
		RETURN_VALUE_IF(!dlHandle, void(), "dlNullHandle");
		// Get the reference of the DL interfaces in library map tuple.

audio/aidl/default/EffectImpl.cpp

+3 −4

Original line number	Diff line number	Diff line
		@@ -76,7 +76,7 @@ ndk::ScopedAStatus EffectImpl::close() {
		}

		ndk::ScopedAStatus EffectImpl::setParameter(const Parameter& param) {
		LOG(DEBUG) << getEffectName() << __func__ << " with: " << param.toString();
		LOG(VERBOSE) << getEffectName() << __func__ << " with: " << param.toString();

		const auto tag = param.getTag();
		switch (tag) {
		@@ -100,7 +100,6 @@ ndk::ScopedAStatus EffectImpl::setParameter(const Parameter& param) {
		}

		ndk::ScopedAStatus EffectImpl::getParameter(const Parameter::Id& id, Parameter* param) {
		LOG(DEBUG) << getEffectName() << __func__ << id.toString();
		auto tag = id.getTag();
		switch (tag) {
		case Parameter::Id::commonTag: {
		@@ -117,7 +116,7 @@ ndk::ScopedAStatus EffectImpl::getParameter(const Parameter::Id& id, Parameter*
		break;
		}
		}
		LOG(DEBUG) << getEffectName() << __func__ << param->toString();
		LOG(VERBOSE) << getEffectName() << __func__ << id.toString() << param->toString();
		return ndk::ScopedAStatus::ok();
		}

		@@ -254,7 +253,7 @@ IEffect::Status EffectImpl::effectProcessImpl(float* in, float* out, int samples
		for (int i = 0; i < samples; i++) {
		out++ = in++;
		}
		LOG(DEBUG) << getEffectName() << __func__ << " done processing " << samples << " samples";
		LOG(VERBOSE) << getEffectName() << __func__ << " done processing " << samples << " samples";
		return {STATUS_OK, samples, samples};
		}

audio/aidl/default/EffectThread.cpp

+2 −2

Original line number	Diff line number	Diff line
		@@ -149,7 +149,7 @@ void EffectThread::process_l() {
		IEffect::Status status = effectProcessImpl(buffer, buffer, processSamples);
		outputMQ->write(buffer, status.fmqProduced);
		statusMQ->writeBlocking(&status, 1);
		LOG(DEBUG) << mName << __func__ << ": done processing, effect consumed "
		LOG(VERBOSE) << mName << __func__ << ": done processing, effect consumed "
		<< status.fmqConsumed << " produced " << status.fmqProduced;
		}
		}

audio/aidl/default/include/effect-impl/EffectContext.h

+2 −2

Original line number	Diff line number	Diff line
		@@ -124,11 +124,11 @@ class EffectContext {

		virtual RetCode setCommon(const Parameter::Common& common) {
		mCommon = common;
		LOG(INFO) << __func__ << mCommon.toString();
		LOG(VERBOSE) << __func__ << mCommon.toString();
		return RetCode::SUCCESS;
		}
		virtual Parameter::Common getCommon() {
		LOG(DEBUG) << __func__ << mCommon.toString();
		LOG(VERBOSE) << __func__ << mCommon.toString();
		return mCommon;
		}

audio/aidl/default/include/effect-impl/EffectWorker.h

+4 −4

Original line number	Diff line number	Diff line
		@@ -45,8 +45,8 @@ class EffectWorker : public EffectThread {
		auto readSamples = inputMQ->availableToRead(), writeSamples = outputMQ->availableToWrite();
		if (readSamples && writeSamples) {
		auto processSamples = std::min(readSamples, writeSamples);
		LOG(DEBUG) << __func__ << " available to read " << readSamples << " available to write "
		<< writeSamples << " process " << processSamples;
		LOG(VERBOSE) << __func__ << " available to read " << readSamples
		<< " available to write " << writeSamples << " process " << processSamples;

		auto buffer = mContext->getWorkBuffer();
		inputMQ->read(buffer, processSamples);
		@@ -54,7 +54,7 @@ class EffectWorker : public EffectThread {
		IEffect::Status status = effectProcessImpl(buffer, buffer, processSamples);
		outputMQ->write(buffer, status.fmqProduced);
		statusMQ->writeBlocking(&status, 1);
		LOG(DEBUG) << __func__ << " done processing, effect consumed " << status.fmqConsumed
		LOG(VERBOSE) << __func__ << " done processing, effect consumed " << status.fmqConsumed
		<< " produced " << status.fmqProduced;
		} else {
		// TODO: maybe add some sleep here to avoid busy waiting