AudioFlinger: defer restartIfDisabled()
Defer restartIfDisabled() to the end of
the threadLoop worker thread, allowing execution
without any mutexes held. This prevents mutex
order issues.
To accomplish this, we add a DeferredExecutor object
to ThreadBase, which allows adding functors and dtors
to execute at the end of the worker loop,
where no mutexes are held.
Test: atest AudioPlaybackCaptureTest
Bug: 345400492
Bug: 345676143
Change-Id: I1d7f491fc1289882ce67cb7289b3bc1b58e81d23
diff --git a/services/audioflinger/IAfThread.h b/services/audioflinger/IAfThread.h
index c2a58c6..74fc62f 100644
--- a/services/audioflinger/IAfThread.h
+++ b/services/audioflinger/IAfThread.h
@@ -19,8 +19,9 @@
#include <android/media/IAudioTrackCallback.h>
#include <android/media/IEffectClient.h>
#include <audiomanager/IAudioManager.h>
-#include <audio_utils/mutex.h>
+#include <audio_utils/DeferredExecutor.h>
#include <audio_utils/MelProcessor.h>
+#include <audio_utils/mutex.h>
#include <binder/MemoryDealer.h>
#include <datapath/AudioStreamIn.h>
#include <datapath/AudioStreamOut.h>
@@ -394,6 +395,12 @@
// avoid races.
virtual void waitWhileThreadBusy_l(audio_utils::unique_lock& ul)
REQUIRES(mutex()) = 0;
+
+ // The ThreadloopExecutor is used to defer functors or dtors
+ // to when the Threadloop does not hold any mutexes (at the end of the
+ // processing period cycle).
+ virtual audio_utils::DeferredExecutor& getThreadloopExecutor() = 0;
+
// Dynamic cast to derived interface
virtual sp<IAfDirectOutputThread> asIAfDirectOutputThread() { return nullptr; }
virtual sp<IAfDuplicatingThread> asIAfDuplicatingThread() { return nullptr; }
diff --git a/services/audioflinger/PlaybackTracks.h b/services/audioflinger/PlaybackTracks.h
index 15c786e..767112a 100644
--- a/services/audioflinger/PlaybackTracks.h
+++ b/services/audioflinger/PlaybackTracks.h
@@ -448,7 +448,7 @@
void queueBuffer(Buffer& inBuffer);
void clearBufferQueue();
- void restartIfDisabled();
+ void restartIfDisabled() override;
// Maximum number of pending buffers allocated by OutputTrack::write()
static const uint8_t kMaxOverFlowBuffers = 10;
@@ -512,7 +512,7 @@
void releaseBuffer(Proxy::Buffer* buffer) final;
private:
- void restartIfDisabled();
+ void restartIfDisabled() override;
}; // end of PatchTrack
} // namespace android
diff --git a/services/audioflinger/Threads.cpp b/services/audioflinger/Threads.cpp
index 2b17e92..98faf38 100644
--- a/services/audioflinger/Threads.cpp
+++ b/services/audioflinger/Threads.cpp
@@ -4582,7 +4582,11 @@
// FIXME Note that the above .clear() is no longer necessary since effectChains
// is now local to this block, but will keep it for now (at least until merge done).
+
+ mThreadloopExecutor.process();
}
+ mThreadloopExecutor.process(); // process any remaining deferred actions.
+ // deferred actions after this point are ignored.
threadLoop_exit();
@@ -8732,11 +8736,14 @@
mIoJitterMs.add(jitterMs);
mProcessTimeMs.add(processMs);
}
+ mThreadloopExecutor.process();
// update timing info.
mLastIoBeginNs = lastIoBeginNs;
mLastIoEndNs = lastIoEndNs;
lastLoopCountRead = loopCount;
}
+ mThreadloopExecutor.process(); // process any remaining deferred actions.
+ // deferred actions after this point are ignored.
standbyIfNotAlreadyInStandby();
@@ -10522,7 +10529,10 @@
unlockEffectChains(effectChains);
// Effect chains will be actually deleted here if they were removed from
// mEffectChains list during mixing or effects processing
+ mThreadloopExecutor.process();
}
+ mThreadloopExecutor.process(); // process any remaining deferred actions.
+ // deferred actions after this point are ignored.
threadLoop_exit();
diff --git a/services/audioflinger/Threads.h b/services/audioflinger/Threads.h
index 98e3298..7452ec5 100644
--- a/services/audioflinger/Threads.h
+++ b/services/audioflinger/Threads.h
@@ -569,6 +569,10 @@
void stopMelComputation_l() override
REQUIRES(audio_utils::AudioFlinger_Mutex);
+ audio_utils::DeferredExecutor& getThreadloopExecutor() override {
+ return mThreadloopExecutor;
+ }
+
protected:
// entry describing an effect being suspended in mSuspendedSessions keyed vector
@@ -875,6 +879,14 @@
SimpleLog mLocalLog; // locked internally
+ // mThreadloopExecutor contains deferred functors and object (dtors) to
+ // be executed at the end of the processing period, without any
+ // mutexes held.
+ //
+ // mThreadloopExecutor is locked internally, so its methods are thread-safe
+ // for access.
+ audio_utils::DeferredExecutor mThreadloopExecutor;
+
private:
void dumpBase_l(int fd, const Vector<String16>& args) REQUIRES(mutex());
void dumpEffectChains_l(int fd, const Vector<String16>& args) REQUIRES(mutex());
diff --git a/services/audioflinger/TrackBase.h b/services/audioflinger/TrackBase.h
index 5708c61..a0b85f7 100644
--- a/services/audioflinger/TrackBase.h
+++ b/services/audioflinger/TrackBase.h
@@ -333,6 +333,9 @@
// true for Track, false for RecordTrack,
// this could be a track type if needed later
+ void deferRestartIfDisabled();
+ virtual void restartIfDisabled() {}
+
const wp<IAfThreadBase> mThread;
const alloc_type mAllocType;
/*const*/ sp<Client> mClient; // see explanation at ~TrackBase() why not const
diff --git a/services/audioflinger/Tracks.cpp b/services/audioflinger/Tracks.cpp
index a34f6d7..ce6006c 100644
--- a/services/audioflinger/Tracks.cpp
+++ b/services/audioflinger/Tracks.cpp
@@ -314,6 +314,17 @@
return NO_ERROR;
}
+void TrackBase::deferRestartIfDisabled()
+{
+ const auto thread = mThread.promote();
+ if (thread == nullptr) return;
+ thread->getThreadloopExecutor().defer(
+ [track = wp<TrackBase>::fromExisting(this)] {
+ const auto actual = track.promote();
+ if (actual) actual->restartIfDisabled();
+ });
+}
+
PatchTrackBase::PatchTrackBase(const sp<ClientProxy>& proxy,
IAfThreadBase* thread, const Timeout& timeout)
: mProxy(proxy)
@@ -2303,7 +2314,7 @@
waitTimeLeftMs = 0;
}
if (status == NOT_ENOUGH_DATA) {
- restartIfDisabled();
+ deferRestartIfDisabled();
continue;
}
}
@@ -2315,7 +2326,7 @@
buf.mFrameCount = outFrames;
buf.mRaw = NULL;
mClientProxy->releaseBuffer(&buf);
- restartIfDisabled();
+ deferRestartIfDisabled();
pInBuffer->frameCount -= outFrames;
pInBuffer->raw = (int8_t *)pInBuffer->raw + outFrames * mFrameSize;
mOutBuffer.frameCount -= outFrames;
@@ -2570,7 +2581,7 @@
const size_t originalFrameCount = buffer->mFrameCount;
do {
if (status == NOT_ENOUGH_DATA) {
- restartIfDisabled();
+ deferRestartIfDisabled();
buffer->mFrameCount = originalFrameCount; // cleared on error, must be restored.
}
status = mProxy->obtainBuffer(buffer, timeOut);
@@ -2581,7 +2592,7 @@
void PatchTrack::releaseBuffer(Proxy::Buffer* buffer)
{
mProxy->releaseBuffer(buffer);
- restartIfDisabled();
+ deferRestartIfDisabled();
// Check if the PatchTrack has enough data to write once in releaseBuffer().
// If not, prevent an underrun from occurring by moving the track into FS_FILLING;