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

Commit fce3b6c9 authored by François Gaffie's avatar François Gaffie Committed by Shunkai Yao
Browse files

AudioFlinger: fix regression on device effect introduced locking order 



The lock order has been enforced within AudioFlinger.
Order has been defined. However, the Device Effect does not follow
the same pattern as session effects.

Session:
Client -> EffectHandle -> ThreadBase_Mutex (or AudioFlinger Mutex)
Device:
(Default effect)
AudioFlinger -> DeviceEffectManager -> DeviceEffectProxy -> DeviceEffectHandle
(added by Fx App)
Client -> EffectHandle -> DeviceEffectProxy -> DeviceEffectHandle

-Can be addded automatically by AudioFlinger upon patch creation
-May be Added to Audio IDevice
-Can be enabled / disabled by AudioFx app via DeviceEffectProxy.
-Use internal device effect handle that are now controlled by application
diretly, preventing from risk of concurrent access (e.g. disconnect /
enabled status)

This CL intends to fix the locking order assertions by
-introducing a new lock for internal device effect Handle
-unlocking temporarily to prevent breaking the lock order.

Flag: EXEMPT bugfix
Bug: 329395147
Bug: 348986455
Test: atest CtsMediaAudioTestCases

Change-Id: I6667c3ef1b77708ccb4d644fd1b53a2fe3896b72
Signed-off-by: default avatarFrançois Gaffie <francois.gaffie@renault.com>
parent 6e7873a7

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

        }

        *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?