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

Commit 0b53f60b authored by Atneya Nair's avatar Atneya Nair
Browse files

[audio] Offload thread signal to prevent deadlock 

On an appops changed callback, we need to signal the audio threadloop
for tracks which don't sync write (MMAP and offload). Since this signal
grabs the thread mutex, it isn't legal to do in the callback due to lock
ordering.

Instead, enqueue a task onto an executor per offload/mmap track which
appropriately signals the thread without holding any external locks.

Test: Manual playback muting test for MMAP
Flag: EXEMPT bugfix
Fixes: 391668615
Change-Id: Icfd39957d0667f69bd42eba95dbbaa54136fff4c
parent 41cc06b0

services/audioflinger/IAfTrack.h

+7 −6
Original line number Diff line number Diff line
@@ -22,6 +22,7 @@ 
#include <audiomanager/IAudioManager.h>

#include <binder/IMemory.h>

#include <media/AppOpsSession.h>

#include <mediautils/SingleThreadExecutor.h>

#include <datapath/VolumePortInterface.h>

#include <fastpath/FastMixerDumpState.h>

#include <media/AudioSystem.h>
@@ -268,9 +269,10 @@ class AfPlaybackCommon : public virtual VolumePortInterface { 
    using AppOpsSession = media::permission::AppOpsSession<media::permission::DefaultAppOpsFacade>;



  public:

    AfPlaybackCommon(IAfTrackBase& self, IAfThreadCallback& thread, float volume, bool muted,

    AfPlaybackCommon(IAfTrackBase& self, IAfThreadBase& thread, float volume, bool muted,

                     const audio_attributes_t& attr,

                     const AttributionSourceState& attributionSource,

                     bool isOffloadOrMmap,

                     bool shouldPlaybackHarden = true);



    /**
@@ -312,11 +314,9 @@ class AfPlaybackCommon : public virtual VolumePortInterface { 
    void endPlaybackDelivery();



  private:

    // non-const for signal

    IAfTrackBase& mSelf;

    // TODO: replace PersistableBundle with own struct

    // access these two variables only when holding player thread lock.

    std::unique_ptr<os::PersistableBundle> mMuteEventExtras;

    const IAfTrackBase& mSelf;



    std::optional<mediautils::SingleThreadExecutor> mExecutor;

    // TODO: atomic necessary if underneath thread lock?

    std::atomic<mute_state_t> mMuteState;

    std::atomic<bool> mMutedFromPort;
@@ -328,6 +328,7 @@ class AfPlaybackCommon : public virtual VolumePortInterface { 
    std::atomic<bool> mHasOpControlPartial {true};

    mutable std::atomic<bool> mPlaybackHardeningLogged {false};

    // the ref behind the optional is const

    // this member is last in decl order to ensure it is destroyed first

    std::optional<AppOpsSession> mOpControlSession;

};

services/audioflinger/Tracks.cpp

+29 −12
Original line number Diff line number Diff line
@@ -38,6 +38,7 @@ 
#include <media/RecordBufferConverter.h>

#include <media/nbaio/Pipe.h>

#include <media/nbaio/PipeReader.h>

#include <mediautils/Runnable.h>

#include <mediautils/ServiceUtilities.h>

#include <mediautils/SharedMemoryAllocator.h>

#include <private/media/AudioTrackShared.h>
@@ -879,8 +880,8 @@ Track::Track( 
            float volume,

            bool muted)

    :

    AfPlaybackCommon(*this, *thread->afThreadCallback(), volume, muted,

                     attr, attributionSource, type != TYPE_PATCH),

    AfPlaybackCommon(*this, *thread, volume, muted,

                     attr, attributionSource, thread->isOffloadOrMmap(), type != TYPE_PATCH),

    TrackBase(thread, client, attr, sampleRate, format, channelMask, frameCount,

                  // TODO: Using unsecurePointer() has some associated security pitfalls

                  //       (see declaration for details).
@@ -2322,9 +2323,9 @@ OutputTrack::OutputTrack( 
            size_t frameCount,

            const AttributionSourceState& attributionSource)

    :

    AfPlaybackCommon(*this, *playbackThread->afThreadCallback(), /* volume= */ 0.0f,

    AfPlaybackCommon(*this, *playbackThread, /* volume= */ 0.0f,

                     /* muted= */ false,

                     AUDIO_ATTRIBUTES_INITIALIZER, attributionSource,

                     AUDIO_ATTRIBUTES_INITIALIZER, attributionSource, /* isOffloadOrMmap= */ false,

                     /* shouldPlaybackHarden= */ false),

    Track(playbackThread, NULL, AUDIO_STREAM_PATCH,

              AUDIO_ATTRIBUTES_INITIALIZER ,
@@ -2634,9 +2635,10 @@ PatchTrack::PatchTrack(IAfPlaybackThread* playbackThread, 
                                                     float speed,

                                                     float volume,

                                                     bool muted)

    : AfPlaybackCommon(*this, *playbackThread->afThreadCallback(), volume, muted,

    : AfPlaybackCommon(*this, *playbackThread, volume, muted,

                       AUDIO_ATTRIBUTES_INITIALIZER,

                       audioServerAttributionSource(getpid()),

                       /* isOffloadOrMmap= */ false,

                       /* shouldPlaybackHarden= */ false),

    Track(playbackThread, NULL, streamType,

              AUDIO_ATTRIBUTES_INITIALIZER,
@@ -3680,14 +3682,15 @@ static bool shouldExemptFromOpControl(audio_usage_t usage, IAfThreadCallback& cb 
    }

}



AfPlaybackCommon::AfPlaybackCommon(IAfTrackBase& self, IAfThreadCallback& threadCb, float volume,

AfPlaybackCommon::AfPlaybackCommon(IAfTrackBase& self, IAfThreadBase& thread, float volume,

                                   bool muted, const audio_attributes_t& attr,

                                   const AttributionSourceState& attributionSource,

                                   bool isOffloadOrMmap,

                                   bool shouldPlaybackHarden)

    : mSelf(self),

      mMutedFromPort(muted),

      mVolume(volume),

      mIsExemptedFromOpControl(shouldExemptFromOpControl(attr.usage, threadCb)) {

      mIsExemptedFromOpControl(shouldExemptFromOpControl(attr.usage, *thread.afThreadCallback())) {

    using AppOpsManager::OP_CONTROL_AUDIO_PARTIAL;

    using media::permission::Ops;

    using media::permission::skipOpsForUid;
@@ -3696,12 +3699,26 @@ AfPlaybackCommon::AfPlaybackCommon(IAfTrackBase& self, IAfThreadCallback& thread 
    if (hardening_impl()) {

        // Don't bother for trusted uids

        if (!skipOpsForUid(attributionSource.uid) && shouldPlaybackHarden) {

            if (isOffloadOrMmap) {

                mExecutor.emplace();

            }

            auto thread_wp = wp<IAfThreadBase>::fromExisting(&thread);

            mOpControlSession.emplace(

                    ValidatedAttributionSourceState::createFromTrustedSource(attributionSource),

                    Ops{.attributedOp = OP_CONTROL_AUDIO_PARTIAL}, [this](bool isPermitted) {

                    Ops{.attributedOp = OP_CONTROL_AUDIO_PARTIAL},

                    [this, isOffloadOrMmap, thread_wp](bool isPermitted) {

                        mHasOpControlPartial.store(isPermitted, std::memory_order_release);

                        mSelf.signal();

                    });

                        if (isOffloadOrMmap) {

                            mExecutor->enqueue(mediautils::Runnable{[thread_wp]() {

                                auto thread = thread_wp.promote();

                                if (thread != nullptr) {

                                    audio_utils::lock_guard l {thread->mutex()};

                                    thread->broadcast_l();

                                }

                            }});

                        }

                    }

            );

        }

    }

}
@@ -3799,8 +3816,8 @@ MmapTrack::MmapTrack(IAfThreadBase* thread, 
        audio_port_handle_t portId,

        float volume,

        bool muted)

    :   AfPlaybackCommon(*this, *thread->afThreadCallback(),

                         volume, muted, attr, attributionSource),

    :   AfPlaybackCommon(*this, *thread,

                         volume, muted, attr, attributionSource, /* isOffloadOrMmap */ true),

        TrackBase(thread, NULL, attr, sampleRate, format,

                  channelMask, (size_t)0 /* frameCount */,

                  nullptr /* buffer */, (size_t)0 /* bufferSize */,