From 074cf23ff62d4922b7381ed18b28185a74208bfc Mon Sep 17 00:00:00 2001 From: Shunkai Yao Date: Fri, 14 Jul 2023 21:08:31 +0000 Subject: [PATCH] Make EffectFactory implementation thread-safe Also adjust some log level as verbos Bug: 271500140 Test: atest --test-mapping hardware/interfaces/audio/aidl/vts:presubmit Change-Id: I04560c62bdbcfb85dbe223bec0149b112205a323 --- audio/aidl/default/EffectFactory.cpp | 36 +++++++++++-------- audio/aidl/default/EffectImpl.cpp | 7 ++-- audio/aidl/default/EffectThread.cpp | 4 +-- .../include/effect-impl/EffectContext.h | 4 +-- .../include/effect-impl/EffectWorker.h | 8 ++--- .../effectFactory-impl/EffectFactory.h | 24 ++++++++----- 6 files changed, 47 insertions(+), 36 deletions(-) diff --git a/audio/aidl/default/EffectFactory.cpp b/audio/aidl/default/EffectFactory.cpp index 62f3c7ee47..96f13ba058 100644 --- a/audio/aidl/default/EffectFactory.cpp +++ b/audio/aidl/default/EffectFactory.cpp @@ -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(entry); RETURN_IF(!libInterface || !libInterface->queryEffectFunc, EX_NULL_POINTER, "dlNullQueryEffectFunc"); @@ -75,6 +75,7 @@ ndk::ScopedAStatus Factory::queryEffects(const std::optional& in_type const std::optional& in_impl_uuid, const std::optional& in_proxy_uuid, std::vector* _aidl_return) { + std::lock_guard lg(mMutex); // get the matching list std::vector idList; std::copy_if(mIdentitySet.begin(), mIdentitySet.end(), std::back_inserter(idList), @@ -88,7 +89,8 @@ ndk::ScopedAStatus Factory::queryEffects(const std::optional& 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& in_type ndk::ScopedAStatus Factory::queryProcessing(const std::optional& in_type, std::vector* _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* _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(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& in_handle) { +ndk::ScopedAStatus Factory::destroyEffectImpl_l(const std::shared_ptr& in_handle) { std::weak_ptr wpHandle(in_handle); // find the effect entry with key (std::weak_ptr) if (auto effectIt = mEffectMap.find(wpHandle); effectIt != mEffectMap.end()) { @@ -177,7 +181,7 @@ ndk::ScopedAStatus Factory::destroyEffectImpl(const std::shared_ptr& 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& 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 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 proxyUuid) { +void Factory::createIdentityWithConfig( + const EffectConfig::Library& configLib, const AudioUuid& typeUuid, + const std::optional 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(entry); RETURN_VALUE_IF(!dlHandle, void(), "dlNullHandle"); // Get the reference of the DL interfaces in library map tuple. diff --git a/audio/aidl/default/EffectImpl.cpp b/audio/aidl/default/EffectImpl.cpp index da1ad111db..c81c731c43 100644 --- a/audio/aidl/default/EffectImpl.cpp +++ b/audio/aidl/default/EffectImpl.cpp @@ -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}; } diff --git a/audio/aidl/default/EffectThread.cpp b/audio/aidl/default/EffectThread.cpp index 574dc69e48..cd2ba5375b 100644 --- a/audio/aidl/default/EffectThread.cpp +++ b/audio/aidl/default/EffectThread.cpp @@ -149,8 +149,8 @@ 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 " - << status.fmqConsumed << " produced " << status.fmqProduced; + LOG(VERBOSE) << mName << __func__ << ": done processing, effect consumed " + << status.fmqConsumed << " produced " << status.fmqProduced; } } diff --git a/audio/aidl/default/include/effect-impl/EffectContext.h b/audio/aidl/default/include/effect-impl/EffectContext.h index 22cdb6be83..698e7a50ce 100644 --- a/audio/aidl/default/include/effect-impl/EffectContext.h +++ b/audio/aidl/default/include/effect-impl/EffectContext.h @@ -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; } diff --git a/audio/aidl/default/include/effect-impl/EffectWorker.h b/audio/aidl/default/include/effect-impl/EffectWorker.h index b456817b45..421429acaf 100644 --- a/audio/aidl/default/include/effect-impl/EffectWorker.h +++ b/audio/aidl/default/include/effect-impl/EffectWorker.h @@ -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,8 +54,8 @@ 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 - << " produced " << status.fmqProduced; + LOG(VERBOSE) << __func__ << " done processing, effect consumed " << status.fmqConsumed + << " produced " << status.fmqProduced; } else { // TODO: maybe add some sleep here to avoid busy waiting } diff --git a/audio/aidl/default/include/effectFactory-impl/EffectFactory.h b/audio/aidl/default/include/effectFactory-impl/EffectFactory.h index 1401db03e5..d0b820434a 100644 --- a/audio/aidl/default/include/effectFactory-impl/EffectFactory.h +++ b/audio/aidl/default/include/effectFactory-impl/EffectFactory.h @@ -24,6 +24,7 @@ #include #include +#include #include "EffectConfig.h" namespace aidl::android::hardware::audio::effect { @@ -82,9 +83,11 @@ class Factory : public BnFactory { private: const EffectConfig mConfig; ~Factory(); + + std::mutex mMutex; // Set of effect descriptors supported by the devices. - std::set mDescSet; - std::set mIdentitySet; + std::set mDescSet GUARDED_BY(mMutex); + std::set mIdentitySet GUARDED_BY(mMutex); static constexpr int kMapEntryHandleIndex = 0; static constexpr int kMapEntryInterfaceIndex = 1; @@ -94,13 +97,15 @@ class Factory : public BnFactory { std::string /* library name */> DlEntry; - std::map mEffectLibMap; + std::map mEffectLibMap + GUARDED_BY(mMutex); typedef std::pair EffectEntry; - std::map, EffectEntry, std::owner_less<>> mEffectMap; + std::map, EffectEntry, std::owner_less<>> mEffectMap GUARDED_BY(mMutex); - ndk::ScopedAStatus destroyEffectImpl(const std::shared_ptr& in_handle); - void cleanupEffectMap(); + ndk::ScopedAStatus destroyEffectImpl_l(const std::shared_ptr& in_handle) + REQUIRES(mMutex); + void cleanupEffectMap_l() REQUIRES(mMutex); bool openEffectLibrary(const ::aidl::android::media::audio::common::AudioUuid& impl, const std::string& path); void createIdentityWithConfig( @@ -108,12 +113,13 @@ class Factory : public BnFactory { const ::aidl::android::media::audio::common::AudioUuid& typeUuidStr, const std::optional<::aidl::android::media::audio::common::AudioUuid> proxyUuid); - ndk::ScopedAStatus getDescriptorWithUuid( - const aidl::android::media::audio::common::AudioUuid& uuid, Descriptor* desc); + ndk::ScopedAStatus getDescriptorWithUuid_l( + const aidl::android::media::audio::common::AudioUuid& uuid, Descriptor* desc) + REQUIRES(mMutex); void loadEffectLibs(); /* Get effect_dl_interface_s from library handle */ - void getDlSyms(DlEntry& entry); + void getDlSyms_l(DlEntry& entry) REQUIRES(mMutex); }; } // namespace aidl::android::hardware::audio::effect