From df5feba1418e460c84f39b4e2882778261e91d3e Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Thu, 15 Dec 2022 00:11:14 +0000 Subject: [PATCH] audio: Retain IBinder for instances with MinSchedulerPolicy The binder passed to AIBinder_setMinSchedulerPolicy must also be returned to the client, otherwise setting the policy for it does not make any sense. However, server side interface instance classes only hold a weak binder reference. It's the caller of the 'asBinder' method who must retain a strong reference. This reference must be retained past exiting from the method which returns the instance to the client. To solve this issue, add storing of binders along with server object references. These binders get released after the client calls a 'close'/'destroy'-type method to release instance resources. Bug: 205884982 Test: run `atest VtsHalAudioCoreTargetTest` and effect VTS, and grep logcat for 'destroyed after setMinSchedulerPolicy before being parceled' Change-Id: I8b905b85cb8263c85edae8839a126ffe4e4d1e69 --- audio/aidl/default/EffectFactory.cpp | 26 ++++++++++--------- audio/aidl/default/Module.cpp | 12 ++++++--- audio/aidl/default/include/core-impl/Module.h | 2 ++ audio/aidl/default/include/core-impl/Stream.h | 8 ++++-- .../effectFactory-impl/EffectFactory.h | 5 ++-- audio/aidl/default/main.cpp | 25 +++++++++--------- 6 files changed, 45 insertions(+), 33 deletions(-) diff --git a/audio/aidl/default/EffectFactory.cpp b/audio/aidl/default/EffectFactory.cpp index 7ae9a66762..3b40ae039b 100644 --- a/audio/aidl/default/EffectFactory.cpp +++ b/audio/aidl/default/EffectFactory.cpp @@ -40,12 +40,13 @@ Factory::Factory(const std::string& file) : mConfig(EffectConfig(file)) { } Factory::~Factory() { - if (auto count = mEffectUuidMap.size()) { + if (auto count = mEffectMap.size()) { LOG(ERROR) << __func__ << " remaining " << count << " effect instances not destroyed indicating resource leak!"; - for (const auto& it : mEffectUuidMap) { + for (const auto& it : mEffectMap) { if (auto spEffect = it.first.lock()) { - LOG(ERROR) << __func__ << " erase remaining instance UUID " << it.second.toString(); + LOG(ERROR) << __func__ << " erase remaining instance UUID " + << it.second.first.toString(); destroyEffectImpl(spEffect); } } @@ -109,9 +110,10 @@ ndk::ScopedAStatus Factory::createEffect(const AudioUuid& in_impl_uuid, return ndk::ScopedAStatus::fromExceptionCode(EX_TRANSACTION_FAILED); } *_aidl_return = effectSp; - AIBinder_setMinSchedulerPolicy(effectSp->asBinder().get(), SCHED_NORMAL, - ANDROID_PRIORITY_AUDIO); - mEffectUuidMap[std::weak_ptr(effectSp)] = in_impl_uuid; + ndk::SpAIBinder effectBinder = effectSp->asBinder(); + AIBinder_setMinSchedulerPolicy(effectBinder.get(), SCHED_NORMAL, ANDROID_PRIORITY_AUDIO); + mEffectMap[std::weak_ptr(effectSp)] = + std::make_pair(in_impl_uuid, std::move(effectBinder)); LOG(DEBUG) << __func__ << ": instance " << effectSp.get() << " created successfully"; return ndk::ScopedAStatus::ok(); } else { @@ -123,9 +125,9 @@ ndk::ScopedAStatus Factory::createEffect(const AudioUuid& in_impl_uuid, ndk::ScopedAStatus Factory::destroyEffectImpl(const std::shared_ptr& in_handle) { std::weak_ptr wpHandle(in_handle); - // find UUID with key (std::weak_ptr) - if (auto uuidIt = mEffectUuidMap.find(wpHandle); uuidIt != mEffectUuidMap.end()) { - auto& uuid = uuidIt->second; + // find the effect entry with key (std::weak_ptr) + if (auto effectIt = mEffectMap.find(wpHandle); effectIt != mEffectMap.end()) { + auto& uuid = effectIt->second.first; // find implementation library with UUID if (auto libIt = mEffectLibMap.find(uuid); libIt != mEffectLibMap.end()) { auto& interface = std::get(libIt->second); @@ -136,7 +138,7 @@ ndk::ScopedAStatus Factory::destroyEffectImpl(const std::shared_ptr& in LOG(ERROR) << __func__ << ": UUID " << uuid.toString() << " does not exist in libMap!"; return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); } - mEffectUuidMap.erase(uuidIt); + mEffectMap.erase(effectIt); return ndk::ScopedAStatus::ok(); } else { LOG(ERROR) << __func__ << ": instance " << in_handle << " does not exist!"; @@ -146,9 +148,9 @@ ndk::ScopedAStatus Factory::destroyEffectImpl(const std::shared_ptr& in // go over the map and cleanup all expired weak_ptrs. void Factory::cleanupEffectMap() { - for (auto it = mEffectUuidMap.begin(); it != mEffectUuidMap.end();) { + for (auto it = mEffectMap.begin(); it != mEffectMap.end();) { if (nullptr == it->first.lock()) { - it = mEffectUuidMap.erase(it); + it = mEffectMap.erase(it); } else { ++it; } diff --git a/audio/aidl/default/Module.cpp b/audio/aidl/default/Module.cpp index 86f0261d2a..38a8cc4b95 100644 --- a/audio/aidl/default/Module.cpp +++ b/audio/aidl/default/Module.cpp @@ -316,7 +316,8 @@ ndk::ScopedAStatus Module::setModuleDebug( ndk::ScopedAStatus Module::getTelephony(std::shared_ptr* _aidl_return) { if (mTelephony == nullptr) { mTelephony = ndk::SharedRefBase::make(); - AIBinder_setMinSchedulerPolicy(mTelephony->asBinder().get(), SCHED_NORMAL, + mTelephonyBinder = mTelephony->asBinder(); + AIBinder_setMinSchedulerPolicy(mTelephonyBinder.get(), SCHED_NORMAL, ANDROID_PRIORITY_AUDIO); } *_aidl_return = mTelephony; @@ -536,8 +537,9 @@ ndk::ScopedAStatus Module::openInputStream(const OpenInputStreamArguments& in_ar if (auto status = stream->init(); !status.isOk()) { return status; } - AIBinder_setMinSchedulerPolicy(stream->asBinder().get(), SCHED_NORMAL, ANDROID_PRIORITY_AUDIO); StreamWrapper streamWrapper(stream); + AIBinder_setMinSchedulerPolicy(streamWrapper.getBinder().get(), SCHED_NORMAL, + ANDROID_PRIORITY_AUDIO); auto patchIt = mPatches.find(in_args.portConfigId); if (patchIt != mPatches.end()) { streamWrapper.setStreamIsConnected(findConnectedDevices(in_args.portConfigId)); @@ -587,8 +589,9 @@ ndk::ScopedAStatus Module::openOutputStream(const OpenOutputStreamArguments& in_ if (auto status = stream->init(); !status.isOk()) { return status; } - AIBinder_setMinSchedulerPolicy(stream->asBinder().get(), SCHED_NORMAL, ANDROID_PRIORITY_AUDIO); StreamWrapper streamWrapper(stream); + AIBinder_setMinSchedulerPolicy(streamWrapper.getBinder().get(), SCHED_NORMAL, + ANDROID_PRIORITY_AUDIO); auto patchIt = mPatches.find(in_args.portConfigId); if (patchIt != mPatches.end()) { streamWrapper.setStreamIsConnected(findConnectedDevices(in_args.portConfigId)); @@ -935,6 +938,9 @@ ndk::ScopedAStatus Module::updateScreenState(bool in_isTurnedOn) { ndk::ScopedAStatus Module::getSoundDose(std::shared_ptr* _aidl_return) { if (mSoundDose == nullptr) { mSoundDose = ndk::SharedRefBase::make(); + mSoundDoseBinder = mSoundDose->asBinder(); + AIBinder_setMinSchedulerPolicy(mSoundDoseBinder.get(), SCHED_NORMAL, + ANDROID_PRIORITY_AUDIO); } *_aidl_return = mSoundDose; LOG(DEBUG) << __func__ << ": returning instance of ISoundDose: " << _aidl_return->get(); diff --git a/audio/aidl/default/include/core-impl/Module.h b/audio/aidl/default/include/core-impl/Module.h index 3cc31c5871..627208336f 100644 --- a/audio/aidl/default/include/core-impl/Module.h +++ b/audio/aidl/default/include/core-impl/Module.h @@ -115,6 +115,7 @@ class Module : public BnModule { // Since it is required to return the same instance of the ITelephony, even // if the client has released it on its side, we need to hold it via a strong pointer. std::shared_ptr mTelephony; + ndk::SpAIBinder mTelephonyBinder; // ids of ports created at runtime via 'connectExternalDevice'. std::set mConnectedDevicePorts; Streams mStreams; @@ -125,6 +126,7 @@ class Module : public BnModule { float mMasterVolume = 1.0f; bool mMicMute = false; std::shared_ptr mSoundDose; + ndk::SpAIBinder mSoundDoseBinder; }; } // namespace aidl::android::hardware::audio::core diff --git a/audio/aidl/default/include/core-impl/Stream.h b/audio/aidl/default/include/core-impl/Stream.h index 7a07eeb4f5..9509a05e7a 100644 --- a/audio/aidl/default/include/core-impl/Stream.h +++ b/audio/aidl/default/include/core-impl/Stream.h @@ -279,8 +279,11 @@ class StreamOut : public StreamCommon<::aidl::android::hardware::audio::common:: class StreamWrapper { public: - explicit StreamWrapper(std::shared_ptr streamIn) : mStream(streamIn) {} - explicit StreamWrapper(std::shared_ptr streamOut) : mStream(streamOut) {} + explicit StreamWrapper(const std::shared_ptr& streamIn) + : mStream(streamIn), mStreamBinder(streamIn->asBinder()) {} + explicit StreamWrapper(const std::shared_ptr& streamOut) + : mStream(streamOut), mStreamBinder(streamOut->asBinder()) {} + ndk::SpAIBinder getBinder() const { return mStreamBinder; } bool isStreamOpen() const { return std::visit( [](auto&& ws) -> bool { @@ -301,6 +304,7 @@ class StreamWrapper { private: std::variant, std::weak_ptr> mStream; + ndk::SpAIBinder mStreamBinder; }; class Streams { diff --git a/audio/aidl/default/include/effectFactory-impl/EffectFactory.h b/audio/aidl/default/include/effectFactory-impl/EffectFactory.h index 5903276066..04bd1bbe8e 100644 --- a/audio/aidl/default/include/effectFactory-impl/EffectFactory.h +++ b/audio/aidl/default/include/effectFactory-impl/EffectFactory.h @@ -96,9 +96,8 @@ class Factory : public BnFactory { std::map mEffectLibMap; - std::map, aidl::android::media::audio::common::AudioUuid, - std::owner_less<>> - mEffectUuidMap; + typedef std::pair EffectEntry; + std::map, EffectEntry, std::owner_less<>> mEffectMap; ndk::ScopedAStatus destroyEffectImpl(const std::shared_ptr& in_handle); void cleanupEffectMap(); diff --git a/audio/aidl/default/main.cpp b/audio/aidl/default/main.cpp index b11af4e34e..b66c13495c 100644 --- a/audio/aidl/default/main.cpp +++ b/audio/aidl/default/main.cpp @@ -16,6 +16,7 @@ #include #include +#include #include #include @@ -44,19 +45,17 @@ int main() { CHECK_EQ(STATUS_OK, status); // Make modules - auto moduleDefault = ndk::SharedRefBase::make(Module::Type::DEFAULT); - const std::string moduleDefaultName = std::string() + Module::descriptor + "/default"; - AIBinder_setMinSchedulerPolicy(moduleDefault->asBinder().get(), SCHED_NORMAL, - ANDROID_PRIORITY_AUDIO); - status = AServiceManager_addService(moduleDefault->asBinder().get(), moduleDefaultName.c_str()); - CHECK_EQ(STATUS_OK, status); - - auto moduleRSubmix = ndk::SharedRefBase::make(Module::Type::R_SUBMIX); - const std::string moduleRSubmixName = std::string() + Module::descriptor + "/r_submix"; - AIBinder_setMinSchedulerPolicy(moduleRSubmix->asBinder().get(), SCHED_NORMAL, - ANDROID_PRIORITY_AUDIO); - status = AServiceManager_addService(moduleRSubmix->asBinder().get(), moduleRSubmixName.c_str()); - CHECK_EQ(STATUS_OK, status); + auto createModule = [](Module::Type type, const std::string& instance) { + auto module = ndk::SharedRefBase::make(type); + ndk::SpAIBinder moduleBinder = module->asBinder(); + const std::string moduleName = std::string(Module::descriptor).append("/").append(instance); + AIBinder_setMinSchedulerPolicy(moduleBinder.get(), SCHED_NORMAL, ANDROID_PRIORITY_AUDIO); + binder_status_t status = AServiceManager_addService(moduleBinder.get(), moduleName.c_str()); + CHECK_EQ(STATUS_OK, status); + return std::make_pair(module, moduleBinder); + }; + auto modules = {createModule(Module::Type::DEFAULT, "default"), + createModule(Module::Type::R_SUBMIX, "r_submix")}; ABinderProcess_joinThreadPool(); return EXIT_FAILURE; // should not reach