Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit df5feba1 authored by Mikhail Naganov's avatar Mikhail Naganov Committed by Shunkai Yao
Browse files

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
parent 2100e632

audio/aidl/default/EffectFactory.cpp

+14 −12
Original line number Diff line number Diff line
@@ -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<IEffect>(effectSp)] = in_impl_uuid;

        ndk::SpAIBinder effectBinder = effectSp->asBinder();

        AIBinder_setMinSchedulerPolicy(effectBinder.get(), SCHED_NORMAL, ANDROID_PRIORITY_AUDIO);

        mEffectMap[std::weak_ptr<IEffect>(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<IEffect>& in_handle) {

    std::weak_ptr<IEffect> wpHandle(in_handle);

    // find UUID with key (std::weak_ptr<IEffect>)

    if (auto uuidIt = mEffectUuidMap.find(wpHandle); uuidIt != mEffectUuidMap.end()) {

        auto& uuid = uuidIt->second;

    // find the effect entry with key (std::weak_ptr<IEffect>)

    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<kMapEntryInterfaceIndex>(libIt->second);
@@ -136,7 +138,7 @@ ndk::ScopedAStatus Factory::destroyEffectImpl(const std::shared_ptr<IEffect>& 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<IEffect>& 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;

        }

audio/aidl/default/Module.cpp

+9 −3
Original line number Diff line number Diff line
@@ -316,7 +316,8 @@ ndk::ScopedAStatus Module::setModuleDebug( 
ndk::ScopedAStatus Module::getTelephony(std::shared_ptr<ITelephony>* _aidl_return) {

    if (mTelephony == nullptr) {

        mTelephony = ndk::SharedRefBase::make<Telephony>();

        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<ISoundDose>* _aidl_return) {

    if (mSoundDose == nullptr) {

        mSoundDose = ndk::SharedRefBase::make<SoundDose>();

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

audio/aidl/default/include/core-impl/Module.h

+2 −0
Original line number Diff line number Diff line
@@ -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<ITelephony> mTelephony;

    ndk::SpAIBinder mTelephonyBinder;

    // ids of ports created at runtime via 'connectExternalDevice'.

    std::set<int32_t> mConnectedDevicePorts;

    Streams mStreams;
@@ -125,6 +126,7 @@ class Module : public BnModule { 
    float mMasterVolume = 1.0f;

    bool mMicMute = false;

    std::shared_ptr<ISoundDose> mSoundDose;

    ndk::SpAIBinder mSoundDoseBinder;

};



}  // namespace aidl::android::hardware::audio::core

audio/aidl/default/include/core-impl/Stream.h

+6 −2
Original line number Diff line number Diff line
@@ -279,8 +279,11 @@ class StreamOut : public StreamCommon<::aidl::android::hardware::audio::common:: 


class StreamWrapper {

  public:

    explicit StreamWrapper(std::shared_ptr<StreamIn> streamIn) : mStream(streamIn) {}

    explicit StreamWrapper(std::shared_ptr<StreamOut> streamOut) : mStream(streamOut) {}

    explicit StreamWrapper(const std::shared_ptr<StreamIn>& streamIn)

        : mStream(streamIn), mStreamBinder(streamIn->asBinder()) {}

    explicit StreamWrapper(const std::shared_ptr<StreamOut>& 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<StreamIn>, std::weak_ptr<StreamOut>> mStream;

    ndk::SpAIBinder mStreamBinder;

};



class Streams {

audio/aidl/default/include/effectFactory-impl/EffectFactory.h

+2 −3
Original line number Diff line number Diff line
@@ -96,9 +96,8 @@ class Factory : public BnFactory { 


    std::map<aidl::android::media::audio::common::AudioUuid /* implUUID */, DlEntry> mEffectLibMap;



    std::map<std::weak_ptr<IEffect>, aidl::android::media::audio::common::AudioUuid,

             std::owner_less<>>

            mEffectUuidMap;

    typedef std::pair<aidl::android::media::audio::common::AudioUuid, ndk::SpAIBinder> EffectEntry;

    std::map<std::weak_ptr<IEffect>, EffectEntry, std::owner_less<>> mEffectMap;



    ndk::ScopedAStatus destroyEffectImpl(const std::shared_ptr<IEffect>& in_handle);

    void cleanupEffectMap();