Merge "refactor mutexes for audio effects in audio flinger and audio policy" into qt-dev

media/libaudioclient/AudioSystem.cpp

@@ -1067,6 +1067,13 @@ status_t AudioSystem::setEffectEnabled(int id, bool enabled)
     return aps->setEffectEnabled(id, enabled);
 }
 
+status_t AudioSystem::moveEffectsToIo(const std::vector<int>& ids, audio_io_handle_t io)
+{
+    const sp<IAudioPolicyService>& aps = AudioSystem::get_audio_policy_service();
+    if (aps == 0) return PERMISSION_DENIED;
+    return aps->moveEffectsToIo(ids, io);
+}
+
 status_t AudioSystem::isStreamActive(audio_stream_type_t stream, bool* state, uint32_t inPastMs)
 {
     const sp<IAudioPolicyService>& aps = AudioSystem::get_audio_policy_service();

media/libaudioclient/IAudioPolicyService.cpp

@@ -103,6 +103,7 @@ enum {
     LIST_AUDIO_VOLUME_GROUPS,
     GET_VOLUME_GROUP_FOR_ATTRIBUTES,
     SET_ALLOWED_CAPTURE_POLICY,
+    MOVE_EFFECTS_TO_IO,
 };
 
 #define MAX_ITEMS_PER_LIST 1024
@@ -550,6 +551,22 @@ public:
         return static_cast <status_t> (reply.readInt32());
     }
 
+    status_t moveEffectsToIo(const std::vector<int>& ids, audio_io_handle_t io) override
+    {
+        Parcel data, reply;
+        data.writeInterfaceToken(IAudioPolicyService::getInterfaceDescriptor());
+        data.writeInt32(ids.size());
+        for (auto id : ids) {
+            data.writeInt32(id);
+        }
+        data.writeInt32(io);
+        status_t status = remote()->transact(MOVE_EFFECTS_TO_IO, data, &reply);
+        if (status != NO_ERROR) {
+            return status;
+        }
+        return static_cast <status_t> (reply.readInt32());
+    }
+
     virtual bool isStreamActive(audio_stream_type_t stream, uint32_t inPastMs) const
     {
         Parcel data, reply;
@@ -1284,6 +1301,7 @@ status_t BnAudioPolicyService::onTransact(
         case GET_OUTPUT_FOR_ATTR:
         case ACQUIRE_SOUNDTRIGGER_SESSION:
         case RELEASE_SOUNDTRIGGER_SESSION:
+        case MOVE_EFFECTS_TO_IO:
             ALOGW("%s: transaction %d received from PID %d",
                   __func__, code, IPCThreadState::self()->getCallingPid());
             // return status only for non void methods
@@ -1700,6 +1718,31 @@ status_t BnAudioPolicyService::onTransact(
             return NO_ERROR;
         } break;
 
+        case MOVE_EFFECTS_TO_IO: {
+            CHECK_INTERFACE(IAudioPolicyService, data, reply);
+            std::vector<int> ids;
+            int32_t size;
+            status_t status = data.readInt32(&size);
+            if (status != NO_ERROR) {
+                return status;
+            }
+            if (size > MAX_ITEMS_PER_LIST) {
+                return BAD_VALUE;
+            }
+            for (int32_t i = 0; i < size; i++) {
+                int id;
+                status =  data.readInt32(&id);
+                if (status != NO_ERROR) {
+                    return status;
+                }
+                ids.push_back(id);
+            }
+
+            audio_io_handle_t io = data.readInt32();
+            reply->writeInt32(static_cast <int32_t>(moveEffectsToIo(ids, io)));
+            return NO_ERROR;
+        } break;
+
         case IS_STREAM_ACTIVE: {
             CHECK_INTERFACE(IAudioPolicyService, data, reply);
             audio_stream_type_t stream = (audio_stream_type_t) data.readInt32();

media/libaudioclient/include/media/AudioSystem.h

@@ -286,6 +286,7 @@ public:
                                     int id);
     static status_t unregisterEffect(int id);
     static status_t setEffectEnabled(int id, bool enabled);
+    static status_t moveEffectsToIo(const std::vector<int>& ids, audio_io_handle_t io);
 
     // clear stream to output mapping cache (gStreamOutputMap)
     // and output configuration cache (gOutputs)

media/libaudioclient/include/media/IAudioPolicyService.h

@@ -114,6 +114,7 @@ public:
                                     int id) = 0;
     virtual status_t unregisterEffect(int id) = 0;
     virtual status_t setEffectEnabled(int id, bool enabled) = 0;
+    virtual status_t moveEffectsToIo(const std::vector<int>& ids, audio_io_handle_t io) = 0;
     virtual bool     isStreamActive(audio_stream_type_t stream, uint32_t inPastMs = 0) const = 0;
     virtual bool     isStreamActiveRemotely(audio_stream_type_t stream, uint32_t inPastMs = 0)
                              const = 0;

services/audioflinger/AudioFlinger.cpp

@@ -687,6 +687,10 @@ sp<IAudioTrack> AudioFlinger::createTrack(const CreateTrackInput& input,
     bool updatePid = (input.clientInfo.clientPid == -1);
     const uid_t callingUid = IPCThreadState::self()->getCallingUid();
     uid_t clientUid = input.clientInfo.clientUid;
+    audio_io_handle_t effectThreadId = AUDIO_IO_HANDLE_NONE;
+    std::vector<int> effectIds;
+
+
     if (!isAudioServerOrMediaServerUid(callingUid)) {
         ALOGW_IF(clientUid != callingUid,
                 "%s uid %d tried to pass itself off as %d",
@@ -851,7 +855,10 @@ sp<IAudioTrack> AudioFlinger::createTrack(const CreateTrackInput& input,
             // no risk of deadlock because AudioFlinger::mLock is held
             Mutex::Autolock _dl(thread->mLock);
             Mutex::Autolock _sl(effectThread->mLock);
-            moveEffectChain_l(sessionId, effectThread, thread, true);
+            if (moveEffectChain_l(sessionId, effectThread, thread) == NO_ERROR) {
+                effectThreadId = thread->id();
+                effectIds = thread->getEffectIds_l(sessionId);
+            }
         }
 
         // Look for sync events awaiting for a session to be used.
@@ -885,6 +892,12 @@ sp<IAudioTrack> AudioFlinger::createTrack(const CreateTrackInput& input,
         goto Exit;
     }
 
+    // effectThreadId is not NONE if an effect chain corresponding to the track session
+    // was found on another thread and must be moved on this thread
+    if (effectThreadId != AUDIO_IO_HANDLE_NONE) {
+        AudioSystem::moveEffectsToIo(effectIds, effectThreadId);
+    }
+
     // return handle to client
     trackHandle = new TrackHandle(track);
 
@@ -1225,7 +1238,8 @@ status_t AudioFlinger::setStreamVolume(audio_stream_type_t stream, float value,
     if (output == AUDIO_IO_HANDLE_NONE) {
         return BAD_VALUE;
     }
-    ALOG_ASSERT(stream != AUDIO_STREAM_PATCH, "attempt to change AUDIO_STREAM_PATCH volume");
+    ALOG_ASSERT(stream != AUDIO_STREAM_PATCH || value == 1.0,
+        "attempt to change AUDIO_STREAM_PATCH volume");
 
     AutoMutex lock(mLock);
     VolumeInterface *volumeInterface = getVolumeInterface_l(output);
@@ -1630,30 +1644,36 @@ void AudioFlinger::registerClient(const sp<IAudioFlingerClient>& client)
 
 void AudioFlinger::removeNotificationClient(pid_t pid)
 {
-    Mutex::Autolock _l(mLock);
+    std::vector< sp<AudioFlinger::EffectModule> > removedEffects;
     {
-        Mutex::Autolock _cl(mClientLock);
-        mNotificationClients.removeItem(pid);
-    }
+        Mutex::Autolock _l(mLock);
+        {
+            Mutex::Autolock _cl(mClientLock);
+            mNotificationClients.removeItem(pid);
+        }
 
-    ALOGV("%d died, releasing its sessions", pid);
-    size_t num = mAudioSessionRefs.size();
-    bool removed = false;
-    for (size_t i = 0; i < num; ) {
-        AudioSessionRef *ref = mAudioSessionRefs.itemAt(i);
-        ALOGV(" pid %d @ %zu", ref->mPid, i);
-        if (ref->mPid == pid) {
-            ALOGV(" removing entry for pid %d session %d", pid, ref->mSessionid);
-            mAudioSessionRefs.removeAt(i);
-            delete ref;
-            removed = true;
-            num--;
-        } else {
-            i++;
+        ALOGV("%d died, releasing its sessions", pid);
+        size_t num = mAudioSessionRefs.size();
+        bool removed = false;
+        for (size_t i = 0; i < num; ) {
+            AudioSessionRef *ref = mAudioSessionRefs.itemAt(i);
+            ALOGV(" pid %d @ %zu", ref->mPid, i);
+            if (ref->mPid == pid) {
+                ALOGV(" removing entry for pid %d session %d", pid, ref->mSessionid);
+                mAudioSessionRefs.removeAt(i);
+                delete ref;
+                removed = true;
+                num--;
+            } else {
+                i++;
+            }
+        }
+        if (removed) {
+            removedEffects = purgeStaleEffects_l();
         }
     }
-    if (removed) {
-        purgeStaleEffects_l();
+    for (auto& effect : removedEffects) {
+        effect->updatePolicyState();
     }
 }
 
@@ -2425,7 +2445,7 @@ status_t AudioFlinger::closeOutput_nonvirtual(audio_io_handle_t output)
                     Vector< sp<EffectChain> > effectChains = playbackThread->getEffectChains_l();
                     for (size_t i = 0; i < effectChains.size(); i ++) {
                         moveEffectChain_l(effectChains[i]->sessionId(), playbackThread.get(),
-                                dstThread, true);
+                                dstThread);
                     }
                 }
             }
@@ -2792,31 +2812,40 @@ void AudioFlinger::acquireAudioSessionId(audio_session_t audioSession, pid_t pid
 
 void AudioFlinger::releaseAudioSessionId(audio_session_t audioSession, pid_t pid)
 {
-    Mutex::Autolock _l(mLock);
-    pid_t caller = IPCThreadState::self()->getCallingPid();
-    ALOGV("releasing %d from %d for %d", audioSession, caller, pid);
-    const uid_t callerUid = IPCThreadState::self()->getCallingUid();
-    if (pid != -1 && isAudioServerUid(callerUid)) { // check must match acquireAudioSessionId()
-        caller = pid;
-    }
-    size_t num = mAudioSessionRefs.size();
-    for (size_t i = 0; i < num; i++) {
-        AudioSessionRef *ref = mAudioSessionRefs.itemAt(i);
-        if (ref->mSessionid == audioSession && ref->mPid == caller) {
-            ref->mCnt--;
-            ALOGV(" decremented refcount to %d", ref->mCnt);
-            if (ref->mCnt == 0) {
-                mAudioSessionRefs.removeAt(i);
-                delete ref;
-                purgeStaleEffects_l();
+    std::vector< sp<EffectModule> > removedEffects;
+    {
+        Mutex::Autolock _l(mLock);
+        pid_t caller = IPCThreadState::self()->getCallingPid();
+        ALOGV("releasing %d from %d for %d", audioSession, caller, pid);
+        const uid_t callerUid = IPCThreadState::self()->getCallingUid();
+        if (pid != -1 && isAudioServerUid(callerUid)) { // check must match acquireAudioSessionId()
+            caller = pid;
+        }
+        size_t num = mAudioSessionRefs.size();
+        for (size_t i = 0; i < num; i++) {
+            AudioSessionRef *ref = mAudioSessionRefs.itemAt(i);
+            if (ref->mSessionid == audioSession && ref->mPid == caller) {
+                ref->mCnt--;
+                ALOGV(" decremented refcount to %d", ref->mCnt);
+                if (ref->mCnt == 0) {
+                    mAudioSessionRefs.removeAt(i);
+                    delete ref;
+                    std::vector< sp<EffectModule> > effects = purgeStaleEffects_l();
+                    removedEffects.insert(removedEffects.end(), effects.begin(), effects.end());
+                }
+                goto Exit;
             }
-            return;
         }
+        // If the caller is audioserver it is likely that the session being released was acquired
+        // on behalf of a process not in notification clients and we ignore the warning.
+        ALOGW_IF(!isAudioServerUid(callerUid),
+                 "session id %d not found for pid %d", audioSession, caller);
+    }
+
+Exit:
+    for (auto& effect : removedEffects) {
+        effect->updatePolicyState();
     }
-    // If the caller is audioserver it is likely that the session being released was acquired
-    // on behalf of a process not in notification clients and we ignore the warning.
-    ALOGW_IF(!isAudioServerUid(callerUid),
-            "session id %d not found for pid %d", audioSession, caller);
 }
 
 bool AudioFlinger::isSessionAcquired_l(audio_session_t audioSession)
@@ -2831,11 +2860,12 @@ bool AudioFlinger::isSessionAcquired_l(audio_session_t audioSession)
     return false;
 }
 
-void AudioFlinger::purgeStaleEffects_l() {
+std::vector<sp<AudioFlinger::EffectModule>> AudioFlinger::purgeStaleEffects_l() {
 
     ALOGV("purging stale effects");
 
     Vector< sp<EffectChain> > chains;
+    std::vector< sp<EffectModule> > removedEffects;
 
     for (size_t i = 0; i < mPlaybackThreads.size(); i++) {
         sp<PlaybackThread> t = mPlaybackThreads.valueAt(i);
@@ -2847,6 +2877,7 @@ void AudioFlinger::purgeStaleEffects_l() {
             }
         }
     }
+
     for (size_t i = 0; i < mRecordThreads.size(); i++) {
         sp<RecordThread> t = mRecordThreads.valueAt(i);
         Mutex::Autolock _l(t->mLock);
@@ -2856,6 +2887,15 @@ void AudioFlinger::purgeStaleEffects_l() {
         }
     }
 
+    for (size_t i = 0; i < mMmapThreads.size(); i++) {
+        sp<MmapThread> t = mMmapThreads.valueAt(i);
+        Mutex::Autolock _l(t->mLock);
+        for (size_t j = 0; j < t->mEffectChains.size(); j++) {
+            sp<EffectChain> ec = t->mEffectChains[j];
+            chains.push(ec);
+        }
+    }
+
     for (size_t i = 0; i < chains.size(); i++) {
         sp<EffectChain> ec = chains[i];
         int sessionid = ec->sessionId();
@@ -2884,11 +2924,11 @@ void AudioFlinger::purgeStaleEffects_l() {
                 if (effect->purgeHandles()) {
                     t->checkSuspendOnEffectEnabled_l(effect, false, effect->sessionId());
                 }
-                AudioSystem::unregisterEffect(effect->id());
+                removedEffects.push_back(effect);
             }
         }
     }
-    return;
+    return removedEffects;
 }
 
 // dumpToThreadLog_l() must be called with AudioFlinger::mLock held
@@ -3380,8 +3420,16 @@ sp<IEffect> AudioFlinger::createEffect(
         }
     }
 
-    if (lStatus != NO_ERROR && lStatus != ALREADY_EXISTS) {
-        // handle must be cleared outside lock.
+    if (lStatus == NO_ERROR || lStatus == ALREADY_EXISTS) {
+        // Check CPU and memory usage
+        sp<EffectModule> effect = handle->effect().promote();
+        if (effect != nullptr) {
+            status_t rStatus = effect->updatePolicyState();
+            if (rStatus != NO_ERROR) {
+                lStatus = rStatus;
+            }
+        }
+    } else {
         handle.clear();
     }
 
@@ -3413,14 +3461,13 @@ status_t AudioFlinger::moveEffects(audio_session_t sessionId, audio_io_handle_t
 
     Mutex::Autolock _dl(dstThread->mLock);
     Mutex::Autolock _sl(srcThread->mLock);
-    return moveEffectChain_l(sessionId, srcThread, dstThread, false);
+    return moveEffectChain_l(sessionId, srcThread, dstThread);
 }
 
 // moveEffectChain_l must be called with both srcThread and dstThread mLocks held
 status_t AudioFlinger::moveEffectChain_l(audio_session_t sessionId,
                                    AudioFlinger::PlaybackThread *srcThread,
-                                   AudioFlinger::PlaybackThread *dstThread,
-                                   bool reRegister)
+                                   AudioFlinger::PlaybackThread *dstThread)
 {
     ALOGV("moveEffectChain_l() session %d from thread %p to thread %p",
             sessionId, srcThread, dstThread);
@@ -3476,36 +3523,67 @@ status_t AudioFlinger::moveEffectChain_l(audio_session_t sessionId,
             }
             strategy = dstChain->strategy();
         }
-        if (reRegister) {
-            AudioSystem::unregisterEffect(effect->id());
-            AudioSystem::registerEffect(&effect->desc(),
-                                        dstThread->id(),
-                                        strategy,
-                                        sessionId,
-                                        effect->id());
-            AudioSystem::setEffectEnabled(effect->id(), effect->isEnabled());
-        }
         effect = chain->getEffectFromId_l(0);
     }
 
     if (status != NO_ERROR) {
         for (size_t i = 0; i < removed.size(); i++) {
             srcThread->addEffect_l(removed[i]);
-            if (dstChain != 0 && reRegister) {
-                AudioSystem::unregisterEffect(removed[i]->id());
-                AudioSystem::registerEffect(&removed[i]->desc(),
-                                            srcThread->id(),
-                                            strategy,
-                                            sessionId,
-                                            removed[i]->id());
-                AudioSystem::setEffectEnabled(effect->id(), effect->isEnabled());
-            }
         }
     }
 
     return status;
 }
 
+status_t AudioFlinger::moveAuxEffectToIo(int EffectId,
+                                         const sp<PlaybackThread>& dstThread,
+                                         sp<PlaybackThread> *srcThread)
+{
+    status_t status = NO_ERROR;
+    Mutex::Autolock _l(mLock);
+    sp<PlaybackThread> thread = getEffectThread_l(AUDIO_SESSION_OUTPUT_MIX, EffectId);
+
+    if (EffectId != 0 && thread != 0 && dstThread != thread.get()) {
+        Mutex::Autolock _dl(dstThread->mLock);
+        Mutex::Autolock _sl(thread->mLock);
+        sp<EffectChain> srcChain = thread->getEffectChain_l(AUDIO_SESSION_OUTPUT_MIX);
+        sp<EffectChain> dstChain;
+        if (srcChain == 0) {
+            return INVALID_OPERATION;
+        }
+
+        sp<EffectModule> effect = srcChain->getEffectFromId_l(EffectId);
+        if (effect == 0) {
+            return INVALID_OPERATION;
+        }
+        thread->removeEffect_l(effect);
+        status = dstThread->addEffect_l(effect);
+        if (status != NO_ERROR) {
+            thread->addEffect_l(effect);
+            status = INVALID_OPERATION;
+            goto Exit;
+        }
+
+        dstChain = effect->chain().promote();
+        if (dstChain == 0) {
+            thread->addEffect_l(effect);
+            status = INVALID_OPERATION;
+        }
+
+Exit:
+        // removeEffect_l() has stopped the effect if it was active so it must be restarted
+        if (effect->state() == EffectModule::ACTIVE ||
+            effect->state() == EffectModule::STOPPING) {
+            effect->start();
+        }
+    }
+
+    if (status == NO_ERROR && srcThread != nullptr) {
+        *srcThread = thread;
+    }
+    return status;
+}
+
 bool AudioFlinger::isNonOffloadableGlobalEffectEnabled_l()
 {
     if (mGlobalEffectEnableTime != 0 &&

services/audioflinger/AudioFlinger.h

@@ -369,7 +369,6 @@ private:
 
     AudioHwDevice*          findSuitableHwDev_l(audio_module_handle_t module,
                                                 audio_devices_t devices);
-    void                    purgeStaleEffects_l();
 
     // Set kEnableExtendedChannels to true to enable greater than stereo output
     // for the MixerThread and device sink.  Number of channels allowed is
@@ -696,8 +695,11 @@ using effect_buffer_t = int16_t;
 
               status_t moveEffectChain_l(audio_session_t sessionId,
                                      PlaybackThread *srcThread,
-                                     PlaybackThread *dstThread,
-                                     bool reRegister);
+                                     PlaybackThread *dstThread);
+
+              status_t moveAuxEffectToIo(int EffectId,
+                                         const sp<PlaybackThread>& dstThread,
+                                         sp<PlaybackThread> *srcThread);
 
               // return thread associated with primary hardware device, or NULL
               PlaybackThread *primaryPlaybackThread_l() const;
@@ -732,6 +734,8 @@ using effect_buffer_t = int16_t;
                 // Return true if the effect was found in mOrphanEffectChains, false otherwise.
                 bool            updateOrphanEffectChains(const sp<EffectModule>& effect);
 
+                std::vector< sp<EffectModule> > purgeStaleEffects_l();
+
                 void broacastParametersToRecordThreads_l(const String8& keyValuePairs);
                 void forwardParametersToDownstreamPatches_l(
                         audio_io_handle_t upStream, const String8& keyValuePairs,

services/audioflinger/Effects.cpp

@@ -168,6 +168,68 @@ status_t AudioFlinger::EffectModule::addHandle(EffectHandle *handle)
     return status;
 }
 
+status_t AudioFlinger::EffectModule::updatePolicyState()
+{
+    status_t status = NO_ERROR;
+    bool doRegister = false;
+    bool registered = false;
+    bool doEnable = false;
+    bool enabled = false;
+    audio_io_handle_t io;
+    uint32_t strategy;
+
+    {
+        Mutex::Autolock _l(mLock);
+        // register effect when first handle is attached and unregister when last handle is removed
+        if (mPolicyRegistered != mHandles.size() > 0) {
+            doRegister = true;
+            mPolicyRegistered = mHandles.size() > 0;
+            if (mPolicyRegistered) {
+              sp <EffectChain> chain = mChain.promote();
+              sp <ThreadBase> thread = mThread.promote();
+
+              if (thread == nullptr || chain == nullptr) {
+                    return INVALID_OPERATION;
+                }
+                io = thread->id();
+                strategy = chain->strategy();
+            }
+        }
+        // enable effect when registered according to enable state requested by controlling handle
+        if (mHandles.size() > 0) {
+            EffectHandle *handle = controlHandle_l();
+            if (handle != nullptr && mPolicyEnabled != handle->enabled()) {
+                doEnable = true;
+                mPolicyEnabled = handle->enabled();
+            }
+        }
+        registered = mPolicyRegistered;
+        enabled = mPolicyEnabled;
+        mPolicyLock.lock();
+    }
+    ALOGV("%s name %s id %d session %d doRegister %d registered %d doEnable %d enabled %d",
+        __func__, mDescriptor.name, mId, mSessionId, doRegister, registered, doEnable, enabled);
+    if (doRegister) {
+        if (registered) {
+            status = AudioSystem::registerEffect(
+                &mDescriptor,
+                io,
+                strategy,
+                mSessionId,
+                mId);
+        } else {
+            status = AudioSystem::unregisterEffect(mId);
+        }
+    }
+    if (registered && doEnable) {
+        status = AudioSystem::setEffectEnabled(mId, enabled);
+    }
+    mPolicyLock.unlock();
+
+    return status;
+}
+
+
 ssize_t AudioFlinger::EffectModule::removeHandle(EffectHandle *handle)
 {
     Mutex::Autolock _l(mLock);
@@ -230,7 +292,6 @@ ssize_t AudioFlinger::EffectModule::disconnectHandle(EffectHandle *handle, bool
     Mutex::Autolock _l(mLock);
     ssize_t numHandles = removeHandle_l(handle);
     if ((numHandles == 0) && (!mPinned || unpinIfLast)) {
-        AudioSystem::unregisterEffect(mId);
         sp<AudioFlinger> af = mAudioFlinger.promote();
         if (af != 0) {
             mLock.unlock();
@@ -943,11 +1004,6 @@ status_t AudioFlinger::EffectModule::setEnabled_l(bool enabled)
     ALOGV("setEnabled %p enabled %d", this, enabled);
 
     if (enabled != isEnabled()) {
-        status_t status = AudioSystem::setEffectEnabled(mId, enabled);
-        if (enabled && status != NO_ERROR) {
-            return status;
-        }
-
         switch (mState) {
         // going from disabled to enabled
         case IDLE:
@@ -1253,14 +1309,11 @@ bool AudioFlinger::EffectModule::purgeHandles()
 {
     bool enabled = false;
     Mutex::Autolock _l(mLock);
-    for (size_t i = 0; i < mHandles.size(); i++) {
-        EffectHandle *handle = mHandles[i];
-        if (handle != NULL && !handle->disconnected()) {
-            if (handle->hasControl()) {
-                enabled = handle->enabled();
-            }
-        }
+    EffectHandle *handle = controlHandle_l();
+    if (handle != NULL) {
+        enabled = handle->enabled();
     }
+    mHandles.clear();
     return enabled;
 }
 
@@ -1572,6 +1625,12 @@ status_t AudioFlinger::EffectHandle::enable()
 
     mEnabled = true;
 
+    status_t status = effect->updatePolicyState();
+    if (status != NO_ERROR) {
+        mEnabled = false;
+        return status;
+    }
+
     sp<ThreadBase> thread = effect->thread().promote();
     if (thread != 0) {
         thread->checkSuspendOnEffectEnabled(effect, true, effect->sessionId());
@@ -1582,7 +1641,7 @@ status_t AudioFlinger::EffectHandle::enable()
         return NO_ERROR;
     }
 
-    status_t status = effect->setEnabled(true);
+    status = effect->setEnabled(true);
     if (status != NO_ERROR) {
         if (thread != 0) {
             thread->checkSuspendOnEffectEnabled(effect, false, effect->sessionId());
@@ -1625,6 +1684,8 @@ status_t AudioFlinger::EffectHandle::disable()
     }
     mEnabled = false;
 
+    effect->updatePolicyState();
+
     if (effect->suspended()) {
         return NO_ERROR;
     }
@@ -1660,20 +1721,17 @@ void AudioFlinger::EffectHandle::disconnect(bool unpinIfLast)
         return;
     }
     mDisconnected = true;
-    sp<ThreadBase> thread;
     {
         sp<EffectModule> effect = mEffect.promote();
         if (effect != 0) {
-            thread = effect->thread().promote();
-        }
-    }
-    if (thread != 0) {
-        thread->disconnectEffectHandle(this, unpinIfLast);
-    } else {
-        // try to cleanup as much as we can
-        sp<EffectModule> effect = mEffect.promote();
-        if (effect != 0 && effect->disconnectHandle(this, unpinIfLast) > 0) {
-            ALOGW("%s Effect handle %p disconnected after thread destruction", __FUNCTION__, this);
+            sp<ThreadBase> thread = effect->thread().promote();
+            if (thread != 0) {
+                thread->disconnectEffectHandle(this, unpinIfLast);
+            } else if (effect->disconnectHandle(this, unpinIfLast) > 0) {
+                ALOGW("%s Effect handle %p disconnected after thread destruction",
+                    __func__, this);
+            }
+            effect->updatePolicyState();
         }
     }
 
@@ -1947,6 +2005,16 @@ sp<AudioFlinger::EffectModule> AudioFlinger::EffectChain::getEffectFromType_l(
     return 0;
 }
 
+std::vector<int> AudioFlinger::EffectChain::getEffectIds()
+{
+    std::vector<int> ids;
+    Mutex::Autolock _l(mLock);
+    for (size_t i = 0; i < mEffects.size(); i++) {
+        ids.push_back(mEffects[i]->id());
+    }
+    return ids;
+}
+
 void AudioFlinger::EffectChain::clearInputBuffer()
 {
     Mutex::Autolock _l(mLock);

services/audioflinger/Effects.h

@@ -142,6 +142,8 @@ public:
     void             addEffectToHal_l();
     void             release_l();
 
+    status_t         updatePolicyState();
+
     void             dump(int fd, const Vector<String16>& args);
 
 private:
@@ -204,6 +206,16 @@ mutable Mutex               mLock;      // mutex for process, commands and handl
     static constexpr pid_t INVALID_PID = (pid_t)-1;
     // this tid is allowed to call setVolume() without acquiring the mutex.
     pid_t mSetVolumeReentrantTid = INVALID_PID;
+
+    // Audio policy effect state management
+    // Mutex protecting transactions with audio policy manager as mLock cannot
+    // be held to avoid cross deadlocks with audio policy mutex
+    Mutex   mPolicyLock;
+    // Effect is registered in APM or not
+    bool    mPolicyRegistered = false;
+    // Effect enabled state communicated to APM. Enabled state corresponds to
+    // state requested by the EffectHandle with control
+    bool    mPolicyEnabled = false;
 };
 
 // The EffectHandle class implements the IEffect interface. It provides resources
@@ -334,6 +346,7 @@ public:
     sp<EffectModule> getEffectFromDesc_l(effect_descriptor_t *descriptor);
     sp<EffectModule> getEffectFromId_l(int id);
     sp<EffectModule> getEffectFromType_l(const effect_uuid_t *type);
+    std::vector<int> getEffectIds();
     // FIXME use float to improve the dynamic range
     bool setVolume_l(uint32_t *left, uint32_t *right, bool force = false);
     void resetVolume_l();

services/audioflinger/Threads.cpp

@@ -1306,7 +1306,6 @@ sp<AudioFlinger::EffectHandle> AudioFlinger::ThreadBase::createEffect_l(
     sp<EffectChain> chain;
     bool chainCreated = false;
     bool effectCreated = false;
-    bool effectRegistered = false;
     audio_unique_id_t effectId = AUDIO_UNIQUE_ID_USE_UNSPECIFIED;
 
     lStatus = initCheck();
@@ -1342,13 +1341,6 @@ sp<AudioFlinger::EffectHandle> AudioFlinger::ThreadBase::createEffect_l(
 
         if (effect == 0) {
             effectId = mAudioFlinger->nextUniqueId(AUDIO_UNIQUE_ID_USE_EFFECT);
-            // Check CPU and memory usage
-            lStatus = AudioSystem::registerEffect(
-                    desc, mId, chain->strategy(), sessionId, effectId);
-            if (lStatus != NO_ERROR) {
-                goto Exit;
-            }
-            effectRegistered = true;
             // create a new effect module if none present in the chain
             lStatus = chain->createEffect_l(effect, this, desc, effectId, sessionId, pinned);
             if (lStatus != NO_ERROR) {
@@ -1378,9 +1370,6 @@ Exit:
         if (effectCreated) {
             chain->removeEffect_l(effect);
         }
-        if (effectRegistered) {
-            AudioSystem::unregisterEffect(effectId);
-        }
         if (chainCreated) {
             removeEffectChain_l(chain);
         }
@@ -1411,7 +1400,6 @@ void AudioFlinger::ThreadBase::disconnectEffectHandle(EffectHandle *handle,
     }
     if (remove) {
         mAudioFlinger->updateOrphanEffectChains(effect);
-        AudioSystem::unregisterEffect(effect->id());
         if (handle->enabled()) {
             checkSuspendOnEffectEnabled(effect, false, effect->sessionId());
         }
@@ -1432,6 +1420,12 @@ sp<AudioFlinger::EffectModule> AudioFlinger::ThreadBase::getEffect_l(audio_sessi
     return chain != 0 ? chain->getEffectFromId_l(effectId) : 0;
 }
 
+std::vector<int> AudioFlinger::ThreadBase::getEffectIds_l(audio_session_t sessionId)
+{
+    sp<EffectChain> chain = getEffectChain_l(sessionId);
+    return chain != nullptr ? chain->getEffectIds() : std::vector<int>{};
+}
+
 // PlaybackThread::addEffect_l() must be called with AudioFlinger::mLock and
 // PlaybackThread::mLock held
 status_t AudioFlinger::ThreadBase::addEffect_l(const sp<EffectModule>& effect)
@@ -2732,7 +2726,8 @@ void AudioFlinger::PlaybackThread::readOutputParameters_l()
     // create a copy of mEffectChains as calling moveEffectChain_l() can reorder some effect chains
     Vector< sp<EffectChain> > effectChains = mEffectChains;
     for (size_t i = 0; i < effectChains.size(); i ++) {
-        mAudioFlinger->moveEffectChain_l(effectChains[i]->sessionId(), this, this, false);
+        mAudioFlinger->moveEffectChain_l(effectChains[i]->sessionId(),
+            this/* srcThread */, this/* dstThread */);
     }
 }
 

services/audioflinger/Threads.h

@@ -315,6 +315,7 @@ public:
                 sp<EffectChain> getEffectChain(audio_session_t sessionId);
                 // same as getEffectChain() but must be called with ThreadBase mutex locked
                 sp<EffectChain> getEffectChain_l(audio_session_t sessionId) const;
+                std::vector<int> getEffectIds_l(audio_session_t sessionId);
                 // add an effect chain to the chain list (mEffectChains)
     virtual     status_t addEffectChain_l(const sp<EffectChain>& chain) = 0;
                 // remove an effect chain from the chain list (mEffectChains)
@@ -334,7 +335,8 @@ public:
                 sp<AudioFlinger::EffectModule> getEffect(audio_session_t sessionId, int effectId);
                 sp<AudioFlinger::EffectModule> getEffect_l(audio_session_t sessionId, int effectId);
                 // add and effect module. Also creates the effect chain is none exists for
-                // the effects audio session
+                // the effects audio session. Only called in a context of moving an effect
+                // from one thread to another
                 status_t addEffect_l(const sp< EffectModule>& effect);
                 // remove and effect module. Also removes the effect chain is this was the last
                 // effect

services/audioflinger/Tracks.cpp

@@ -1244,54 +1244,25 @@ status_t AudioFlinger::PlaybackThread::Track::getTimestamp(AudioTimestamp& times
 
 status_t AudioFlinger::PlaybackThread::Track::attachAuxEffect(int EffectId)
 {
-    status_t status = DEAD_OBJECT;
     sp<ThreadBase> thread = mThread.promote();
-    if (thread != 0) {
-        PlaybackThread *playbackThread = (PlaybackThread *)thread.get();
-        sp<AudioFlinger> af = mClient->audioFlinger();
-
-        Mutex::Autolock _l(af->mLock);
-
-        sp<PlaybackThread> srcThread = af->getEffectThread_l(AUDIO_SESSION_OUTPUT_MIX, EffectId);
+    if (thread == nullptr) {
+        return DEAD_OBJECT;
+    }
 
-        if (EffectId != 0 && srcThread != 0 && playbackThread != srcThread.get()) {
-            Mutex::Autolock _dl(playbackThread->mLock);
-            Mutex::Autolock _sl(srcThread->mLock);
-            sp<EffectChain> chain = srcThread->getEffectChain_l(AUDIO_SESSION_OUTPUT_MIX);
-            if (chain == 0) {
-                return INVALID_OPERATION;
-            }
+    sp<PlaybackThread> dstThread = (PlaybackThread *)thread.get();
+    sp<PlaybackThread> srcThread; // srcThread is initialized by call to moveAuxEffectToIo()
+    sp<AudioFlinger> af = mClient->audioFlinger();
+    status_t status = af->moveAuxEffectToIo(EffectId, dstThread, &srcThread);
 
-            sp<EffectModule> effect = chain->getEffectFromId_l(EffectId);
-            if (effect == 0) {
-                return INVALID_OPERATION;
-            }
-            srcThread->removeEffect_l(effect);
-            status = playbackThread->addEffect_l(effect);
-            if (status != NO_ERROR) {
-                srcThread->addEffect_l(effect);
-                return INVALID_OPERATION;
-            }
-            // removeEffect_l() has stopped the effect if it was active so it must be restarted
-            if (effect->state() == EffectModule::ACTIVE ||
-                    effect->state() == EffectModule::STOPPING) {
-                effect->start();
-            }
-
-            sp<EffectChain> dstChain = effect->chain().promote();
-            if (dstChain == 0) {
-                srcThread->addEffect_l(effect);
-                return INVALID_OPERATION;
-            }
-            AudioSystem::unregisterEffect(effect->id());
-            AudioSystem::registerEffect(&effect->desc(),
-                                        srcThread->id(),
-                                        dstChain->strategy(),
-                                        AUDIO_SESSION_OUTPUT_MIX,
-                                        effect->id());
-            AudioSystem::setEffectEnabled(effect->id(), effect->isEnabled());
+    if (EffectId != 0 && status == NO_ERROR) {
+        status = dstThread->attachAuxEffect(this, EffectId);
+        if (status == NO_ERROR) {
+            AudioSystem::moveEffectsToIo(std::vector<int>(EffectId), dstThread->id());
         }
-        status = playbackThread->attachAuxEffect(this, EffectId);
+    }
+
+    if (status != NO_ERROR && srcThread != nullptr) {
+        af->moveAuxEffectToIo(EffectId, srcThread, &dstThread);
     }
     return status;
 }

services/audiopolicy/AudioPolicyInterface.h

@@ -189,6 +189,7 @@ public:
                                     int id) = 0;
     virtual status_t unregisterEffect(int id) = 0;
     virtual status_t setEffectEnabled(int id, bool enabled) = 0;
+    virtual status_t moveEffectsToIo(const std::vector<int>& ids, audio_io_handle_t io) = 0;
 
     virtual bool isStreamActive(audio_stream_type_t stream, uint32_t inPastMs = 0) const = 0;
     virtual bool isStreamActiveRemotely(audio_stream_type_t stream,

services/audiopolicy/common/managerdefinitions/include/EffectDescriptor.h

@@ -65,6 +65,11 @@ public:
     uint32_t getMaxEffectsMemory() const;
     bool isNonOffloadableEffectEnabled() const;
 
+    void moveEffects(audio_session_t session,
+                     audio_io_handle_t srcOutput,
+                     audio_io_handle_t dstOutput);
+    void moveEffects(const std::vector<int>& ids, audio_io_handle_t dstOutput);
+
     void dump(String8 *dst, int spaces = 0, bool verbose = true) const;
 
 private:

services/audiopolicy/common/managerdefinitions/src/EffectDescriptor.cpp

@@ -174,6 +174,32 @@ uint32_t EffectDescriptorCollection::getMaxEffectsMemory() const
     return MAX_EFFECTS_MEMORY;
 }
 
+void EffectDescriptorCollection::moveEffects(audio_session_t session,
+                                             audio_io_handle_t srcOutput,
+                                             audio_io_handle_t dstOutput)
+{
+    ALOGV("%s session %d srcOutput %d dstOutput %d", __func__, session, srcOutput, dstOutput);
+    for (size_t i = 0; i < size(); i++) {
+        sp<EffectDescriptor> effect = valueAt(i);
+        if (effect->mSession == session && effect->mIo == srcOutput) {
+            effect->mIo = dstOutput;
+        }
+    }
+}
+
+void EffectDescriptorCollection::moveEffects(const std::vector<int>& ids,
+                                             audio_io_handle_t dstOutput)
+{
+    ALOGV("%s num effects %zu, first ID %d, dstOutput %d",
+        __func__, ids.size(), ids.size() ? ids[0] : 0, dstOutput);
+    for (size_t i = 0; i < size(); i++) {
+        sp<EffectDescriptor> effect = valueAt(i);
+        if (std::find(begin(ids), end(ids), effect->mId) != end(ids)) {
+            effect->mIo = dstOutput;
+        }
+    }
+}
+
 void EffectDescriptorCollection::dump(String8 *dst, int spaces, bool verbose) const
 {
     if (verbose) {

services/audiopolicy/managerdefault/AudioPolicyManager.cpp

@@ -2675,6 +2675,7 @@ audio_io_handle_t AudioPolicyManager::selectOutputForMusicEffects()
     }
 
     if (output != mMusicEffectOutput) {
+        mEffects.moveEffects(AUDIO_SESSION_OUTPUT_MIX, mMusicEffectOutput, output);
         mpClientInterface->moveEffects(AUDIO_SESSION_OUTPUT_MIX, mMusicEffectOutput, output);
         mMusicEffectOutput = output;
     }
@@ -2734,6 +2735,13 @@ status_t AudioPolicyManager::setEffectEnabled(int id, bool enabled)
     return status;
 }
 
+
+status_t AudioPolicyManager::moveEffectsToIo(const std::vector<int>& ids, audio_io_handle_t io)
+{
+   mEffects.moveEffects(ids, io);
+   return NO_ERROR;
+}
+
 bool AudioPolicyManager::isStreamActive(audio_stream_type_t stream, uint32_t inPastMs) const
 {
     return mOutputs.isActive(toVolumeSource(stream), inPastMs);

services/audiopolicy/managerdefault/AudioPolicyManager.h

@@ -200,6 +200,7 @@ public:
                                         int id);
         virtual status_t unregisterEffect(int id);
         virtual status_t setEffectEnabled(int id, bool enabled);
+        status_t moveEffectsToIo(const std::vector<int>& ids, audio_io_handle_t io) override;
 
         virtual bool isStreamActive(audio_stream_type_t stream, uint32_t inPastMs = 0) const;
         // return whether a stream is playing remotely, override to change the definition of

services/audiopolicy/service/AudioPolicyEffects.h

@@ -225,6 +225,8 @@ private:
                          size_t *totSize);
 
     // protects access to mInputSources, mInputSessions, mOutputStreams, mOutputSessions
+    // never hold AudioPolicyService::mLock when calling AudioPolicyEffects methods as
+    // those can call back into AudioPolicyService methods and try to acquire the mutex
     Mutex mLock;
     // Automatic input effects are configured per audio_source_t
     KeyedVector< audio_source_t, EffectDescVector* > mInputSources;

services/audiopolicy/service/AudioPolicyInterfaceImpl.cpp

@@ -797,7 +797,7 @@ status_t AudioPolicyService::registerEffect(const effect_descriptor_t *desc,
     if (mAudioPolicyManager == NULL) {
         return NO_INIT;
     }
-    Mutex::Autolock _l(mEffectsLock);
+    Mutex::Autolock _l(mLock);
     AutoCallerClear acc;
     return mAudioPolicyManager->registerEffect(desc, io, strategy, session, id);
 }
@@ -807,7 +807,7 @@ status_t AudioPolicyService::unregisterEffect(int id)
     if (mAudioPolicyManager == NULL) {
         return NO_INIT;
     }
-    Mutex::Autolock _l(mEffectsLock);
+    Mutex::Autolock _l(mLock);
     AutoCallerClear acc;
     return mAudioPolicyManager->unregisterEffect(id);
 }
@@ -817,11 +817,21 @@ status_t AudioPolicyService::setEffectEnabled(int id, bool enabled)
     if (mAudioPolicyManager == NULL) {
         return NO_INIT;
     }
-    Mutex::Autolock _l(mEffectsLock);
+    Mutex::Autolock _l(mLock);
     AutoCallerClear acc;
     return mAudioPolicyManager->setEffectEnabled(id, enabled);
 }
 
+status_t AudioPolicyService::moveEffectsToIo(const std::vector<int>& ids, audio_io_handle_t io)
+{
+    if (mAudioPolicyManager == NULL) {
+        return NO_INIT;
+    }
+    Mutex::Autolock _l(mLock);
+    AutoCallerClear acc;
+    return mAudioPolicyManager->moveEffectsToIo(ids, io);
+}
+
 bool AudioPolicyService::isStreamActive(audio_stream_type_t stream, uint32_t inPastMs) const
 {
     if (uint32_t(stream) >= AUDIO_STREAM_PUBLIC_CNT) {
@@ -973,8 +983,6 @@ bool AudioPolicyService::isOffloadSupported(const audio_offload_info_t& info)
         return false;
     }
     Mutex::Autolock _l(mLock);
-    Mutex::Autolock _le(mEffectsLock); // isOffloadSupported queries for
-                                      // non-offloadable effects
     AutoCallerClear acc;
     return mAudioPolicyManager->isOffloadSupported(info);
 }

services/audiopolicy/service/AudioPolicyService.h

@@ -134,6 +134,7 @@ public:
                                     int id);
     virtual status_t unregisterEffect(int id);
     virtual status_t setEffectEnabled(int id, bool enabled);
+    status_t moveEffectsToIo(const std::vector<int>& ids, audio_io_handle_t io) override;
     virtual bool isStreamActive(audio_stream_type_t stream, uint32_t inPastMs = 0) const;
     virtual bool isStreamActiveRemotely(audio_stream_type_t stream, uint32_t inPastMs = 0) const;
     virtual bool isSourceActive(audio_source_t source) const;
@@ -810,7 +811,6 @@ private:
 
     mutable Mutex mLock;    // prevents concurrent access to AudioPolicy manager functions changing
                             // device connection state  or routing
-    mutable Mutex mEffectsLock; // serialize access to Effect state within APM.
     // Note: lock acquisition order is always mLock > mEffectsLock:
     // mLock protects AudioPolicyManager methods that can call into audio flinger
     // and possibly back in to audio policy service and acquire mEffectsLock.
@@ -824,6 +824,8 @@ private:
     DefaultKeyedVector< int64_t, sp<NotificationClient> >    mNotificationClients;
     Mutex mNotificationClientsLock;  // protects mNotificationClients
     // Manage all effects configured in audio_effects.conf
+    // never hold AudioPolicyService::mLock when calling AudioPolicyEffects methods as
+    // those can call back into AudioPolicyService methods and try to acquire the mutex
     sp<AudioPolicyEffects> mAudioPolicyEffects;
     audio_mode_t mPhoneState;