aaudio: add thread safety annotation
Also improve naming to clarify lock requirements.
And call some locked methods that were not called before.
Although they were protected with a different lock so
it should have no effect.
Bug: 171296283
Test: adb logcat *:F
Test: In another window:
Test: atest AAudioTestCases
Change-Id: I6e863cbdea9250188e3f4b8f8654ef71c8951e74
diff --git a/media/libaaudio/src/Android.bp b/media/libaaudio/src/Android.bp
index 328ceda..aafcccc 100644
--- a/media/libaaudio/src/Android.bp
+++ b/media/libaaudio/src/Android.bp
@@ -21,6 +21,7 @@
],
cflags: [
+ "-Wthread-safety",
"-Wno-unused-parameter",
"-Wall",
"-Werror",
diff --git a/media/libaaudio/src/client/AudioStreamInternal.cpp b/media/libaaudio/src/client/AudioStreamInternal.cpp
index c2dcd35..809c76e 100644
--- a/media/libaaudio/src/client/AudioStreamInternal.cpp
+++ b/media/libaaudio/src/client/AudioStreamInternal.cpp
@@ -418,7 +418,6 @@
}
}
-// This must be called under mStreamLock.
aaudio_result_t AudioStreamInternal::requestStop_l() {
aaudio_result_t result = stopCallback_l();
if (result != AAUDIO_OK) {
@@ -428,7 +427,7 @@
// The stream may have been unlocked temporarily to let a callback finish
// and the callback may have stopped the stream.
// Check to make sure the stream still needs to be stopped.
- // See also AudioStream::safeStop().
+ // See also AudioStream::safeStop_l().
if (!(isActive() || getState() == AAUDIO_STREAM_STATE_DISCONNECTED)) {
ALOGD("%s() returning early, not active or disconnected", __func__);
return AAUDIO_OK;
@@ -810,15 +809,6 @@
return mBufferCapacityInFrames;
}
-aaudio_result_t AudioStreamInternal::joinThread(void** returnArg) {
- return AudioStream::joinThread(returnArg, calculateReasonableTimeout(getFramesPerBurst()));
-}
-
-// This must be called under mStreamLock.
-aaudio_result_t AudioStreamInternal::joinThread_l(void** returnArg) {
- return AudioStream::joinThread_l(returnArg, calculateReasonableTimeout(getFramesPerBurst()));
-}
-
bool AudioStreamInternal::isClockModelInControl() const {
return isActive() && mAudioEndpoint->isFreeRunning() && mClockModel.isRunning();
}
diff --git a/media/libaaudio/src/client/AudioStreamInternal.h b/media/libaaudio/src/client/AudioStreamInternal.h
index 1838b53..fbe4c13 100644
--- a/media/libaaudio/src/client/AudioStreamInternal.h
+++ b/media/libaaudio/src/client/AudioStreamInternal.h
@@ -44,10 +44,6 @@
AudioStreamInternal(AAudioServiceInterface &serviceInterface, bool inService);
virtual ~AudioStreamInternal();
- aaudio_result_t requestStart_l() override;
-
- aaudio_result_t requestStop_l() override;
-
aaudio_result_t getTimestamp(clockid_t clockId,
int64_t *framePosition,
int64_t *timeNanoseconds) override;
@@ -56,8 +52,6 @@
aaudio_result_t open(const AudioStreamBuilder &builder) override;
- aaudio_result_t release_l() override;
-
aaudio_result_t setBufferSize(int32_t requestedFrames) override;
int32_t getBufferSize() const override;
@@ -72,12 +66,9 @@
aaudio_result_t unregisterThread() override;
- aaudio_result_t joinThread(void** returnArg);
-
// Called internally from 'C'
virtual void *callbackLoop() = 0;
-
bool isMMap() override {
return true;
}
@@ -96,6 +87,10 @@
}
protected:
+ aaudio_result_t requestStart_l() REQUIRES(mStreamLock) override;
+ aaudio_result_t requestStop_l() REQUIRES(mStreamLock) override;
+
+ aaudio_result_t release_l() REQUIRES(mStreamLock) override;
aaudio_result_t processData(void *buffer,
int32_t numFrames,
@@ -119,8 +114,6 @@
aaudio_result_t stopCallback_l();
- aaudio_result_t joinThread_l(void** returnArg);
-
virtual void prepareBuffersForStart() {}
virtual void advanceClientToMatchServerPosition(int32_t serverMargin = 0) = 0;
diff --git a/media/libaaudio/src/core/AudioStream.cpp b/media/libaaudio/src/core/AudioStream.cpp
index ba86170..57c4c16 100644
--- a/media/libaaudio/src/core/AudioStream.cpp
+++ b/media/libaaudio/src/core/AudioStream.cpp
@@ -248,7 +248,7 @@
aaudio_result_t AudioStream::systemStopFromCallback() {
std::lock_guard<std::mutex> lock(mStreamLock);
- aaudio_result_t result = safeStop();
+ aaudio_result_t result = safeStop_l();
if (result == AAUDIO_OK) {
// We only call this for logging in "dumpsys audio". So ignore return code.
(void) mPlayerBase->stop();
@@ -262,7 +262,7 @@
ALOGE("stream cannot be stopped by calling from a callback!");
return AAUDIO_ERROR_INVALID_STATE;
}
- aaudio_result_t result = safeStop();
+ aaudio_result_t result = safeStop_l();
if (result == AAUDIO_OK) {
// We only call this for logging in "dumpsys audio". So ignore return code.
(void) mPlayerBase->stop();
@@ -270,8 +270,7 @@
return result;
}
-// This must be called under mStreamLock.
-aaudio_result_t AudioStream::safeStop() {
+aaudio_result_t AudioStream::safeStop_l() {
switch (getState()) {
// Proceed with stopping.
@@ -472,15 +471,14 @@
}
}
-aaudio_result_t AudioStream::joinThread(void** returnArg, int64_t timeoutNanoseconds) {
+aaudio_result_t AudioStream::joinThread(void** returnArg) {
// This may get temporarily unlocked in the MMAP release() when joining callback threads.
std::lock_guard<std::mutex> lock(mStreamLock);
- return joinThread_l(returnArg, timeoutNanoseconds);
+ return joinThread_l(returnArg);
}
// This must be called under mStreamLock.
-aaudio_result_t AudioStream::joinThread_l(void** returnArg, int64_t /* timeoutNanoseconds */)
-{
+aaudio_result_t AudioStream::joinThread_l(void** returnArg) {
if (!mHasThread) {
ALOGD("joinThread() - but has no thread");
return AAUDIO_ERROR_INVALID_STATE;
@@ -492,13 +490,7 @@
// Called from an app thread. Not the callback.
// Unlock because the callback may be trying to stop the stream but is blocked.
mStreamLock.unlock();
-#if 0
- // TODO implement equivalent of pthread_timedjoin_np()
- struct timespec abstime;
- int err = pthread_timedjoin_np(mThread, returnArg, &abstime);
-#else
int err = pthread_join(mThread, returnArg);
-#endif
mStreamLock.lock();
if (err) {
ALOGE("%s() pthread_join() returns err = %d", __func__, err);
@@ -614,7 +606,7 @@
}
if (audioStream) {
// No pan and only left volume is taken into account from IPLayer interface
- audioStream->setDuckAndMuteVolume(mVolumeMultiplierL /* * mPanMultiplierL */);
+ audioStream->setDuckAndMuteVolume(mVolumeMultiplierL /* mPanMultiplierL */);
}
return android::NO_ERROR;
}
diff --git a/media/libaaudio/src/core/AudioStream.h b/media/libaaudio/src/core/AudioStream.h
index d9a9d8e..510ead8 100644
--- a/media/libaaudio/src/core/AudioStream.h
+++ b/media/libaaudio/src/core/AudioStream.h
@@ -20,11 +20,13 @@
#include <atomic>
#include <mutex>
#include <stdint.h>
-#include <aaudio/AAudio.h>
+
+#include <android-base/thread_annotations.h>
#include <binder/IServiceManager.h>
#include <binder/Status.h>
#include <utils/StrongPointer.h>
+#include <aaudio/AAudio.h>
#include <media/AudioSystem.h>
#include <media/PlayerBase.h>
#include <media/VolumeShaper.h>
@@ -57,11 +59,6 @@
protected:
- /* Asynchronous requests.
- * Use waitForStateChange() to wait for completion.
- */
- virtual aaudio_result_t requestStart_l() = 0;
-
/**
* Check the state to see if Pause is currently legal.
*
@@ -80,17 +77,22 @@
return false;
}
- virtual aaudio_result_t requestPause_l() {
+ /* Asynchronous requests.
+ * Use waitForStateChange() to wait for completion.
+ */
+ virtual aaudio_result_t requestStart_l() REQUIRES(mStreamLock) = 0;
+
+ virtual aaudio_result_t requestPause_l() REQUIRES(mStreamLock) {
// Only implement this for OUTPUT streams.
return AAUDIO_ERROR_UNIMPLEMENTED;
}
- virtual aaudio_result_t requestFlush_l() {
+ virtual aaudio_result_t requestFlush_l() REQUIRES(mStreamLock) {
// Only implement this for OUTPUT streams.
return AAUDIO_ERROR_UNIMPLEMENTED;
}
- virtual aaudio_result_t requestStop_l() = 0;
+ virtual aaudio_result_t requestStop_l() REQUIRES(mStreamLock) = 0;
public:
virtual aaudio_result_t getTimestamp(clockid_t clockId,
@@ -130,11 +132,12 @@
* The AAudioStream_close() method releases if needed and then closes.
*/
+protected:
/**
* Free any hardware or system resources from the open() call.
* It is safe to call release_l() multiple times.
*/
- virtual aaudio_result_t release_l() {
+ virtual aaudio_result_t release_l() REQUIRES(mStreamLock) {
setState(AAUDIO_STREAM_STATE_CLOSING);
return AAUDIO_OK;
}
@@ -143,7 +146,7 @@
* Free any resources not already freed by release_l().
* Assume release_l() already called.
*/
- virtual void close_l() {
+ virtual void close_l() REQUIRES(mStreamLock) {
// Releasing the stream will set the state to CLOSING.
assert(getState() == AAUDIO_STREAM_STATE_CLOSING);
// setState() prevents a transition from CLOSING to any state other than CLOSED.
@@ -151,6 +154,7 @@
setState(AAUDIO_STREAM_STATE_CLOSED);
}
+public:
// This is only used to identify a stream in the logs without
// revealing any pointers.
aaudio_stream_id_t getId() {
@@ -163,7 +167,7 @@
aaudio_audio_thread_proc_t threadProc,
void *threadArg);
- aaudio_result_t joinThread(void **returnArg, int64_t timeoutNanoseconds);
+ aaudio_result_t joinThread(void **returnArg);
virtual aaudio_result_t registerThread() {
return AAUDIO_OK;
@@ -473,9 +477,8 @@
private:
// Use a weak pointer so the AudioStream can be deleted.
-
std::mutex mParentLock;
- android::wp<AudioStream> mParent;
+ android::wp<AudioStream> mParent GUARDED_BY(mParentLock);
aaudio_result_t mResult = AAUDIO_OK;
bool mRegistered = false;
};
@@ -533,7 +536,7 @@
mSessionId = sessionId;
}
- aaudio_result_t joinThread_l(void **returnArg, int64_t timeoutNanoseconds);
+ aaudio_result_t joinThread_l(void **returnArg) REQUIRES(mStreamLock);
std::atomic<bool> mCallbackEnabled{false};
@@ -601,14 +604,16 @@
std::string mMetricsId; // set once during open()
+ std::mutex mStreamLock;
+
private:
- aaudio_result_t safeStop();
+ aaudio_result_t safeStop_l() REQUIRES(mStreamLock);
/**
* Release then close the stream.
*/
- void releaseCloseFinal_l() {
+ void releaseCloseFinal_l() REQUIRES(mStreamLock) {
if (getState() != AAUDIO_STREAM_STATE_CLOSING) { // not already released?
// Ignore result and keep closing.
(void) release_l();
@@ -616,8 +621,6 @@
close_l();
}
- std::mutex mStreamLock;
-
const android::sp<MyPlayerBase> mPlayerBase;
// These do not change after open().
@@ -656,8 +659,8 @@
std::atomic<pid_t> mErrorCallbackThread{CALLBACK_THREAD_NONE};
// background thread ----------------------------------
- bool mHasThread = false;
- pthread_t mThread = {};
+ bool mHasThread GUARDED_BY(mStreamLock) = false;
+ pthread_t mThread GUARDED_BY(mStreamLock) = {};
// These are set by the application thread and then read by the audio pthread.
std::atomic<int64_t> mPeriodNanoseconds; // for tuning SCHED_FIFO threads
diff --git a/media/libaaudio/src/legacy/AudioStreamRecord.h b/media/libaaudio/src/legacy/AudioStreamRecord.h
index fe9689f..b2f8ba5 100644
--- a/media/libaaudio/src/legacy/AudioStreamRecord.h
+++ b/media/libaaudio/src/legacy/AudioStreamRecord.h
@@ -41,9 +41,6 @@
aaudio_result_t release_l() override;
void close_l() override;
- aaudio_result_t requestStart_l() override;
- aaudio_result_t requestStop_l() override;
-
virtual aaudio_result_t getTimestamp(clockid_t clockId,
int64_t *framePosition,
int64_t *timeNanoseconds) override;
@@ -77,6 +74,9 @@
protected:
+ aaudio_result_t requestStart_l() REQUIRES(mStreamLock) override;
+ aaudio_result_t requestStop_l() REQUIRES(mStreamLock) override;
+
int32_t getFramesPerBurstFromDevice() const override;
int32_t getBufferCapacityFromDevice() const override;
diff --git a/media/libaaudio/src/legacy/AudioStreamTrack.h b/media/libaaudio/src/legacy/AudioStreamTrack.h
index 654ea9b..f604871 100644
--- a/media/libaaudio/src/legacy/AudioStreamTrack.h
+++ b/media/libaaudio/src/legacy/AudioStreamTrack.h
@@ -44,11 +44,13 @@
aaudio_result_t release_l() override;
void close_l() override;
- aaudio_result_t requestStart_l() override;
- aaudio_result_t requestPause_l() override;
- aaudio_result_t requestFlush_l() override;
- aaudio_result_t requestStop_l() override;
+protected:
+ aaudio_result_t requestStart_l() REQUIRES(mStreamLock) override;
+ aaudio_result_t requestPause_l() REQUIRES(mStreamLock) override;
+ aaudio_result_t requestFlush_l() REQUIRES(mStreamLock) override;
+ aaudio_result_t requestStop_l() REQUIRES(mStreamLock) override;
+public:
bool isFlushSupported() const override {
// Only implement FLUSH for OUTPUT streams.
return true;
diff --git a/services/oboeservice/AAudioClientTracker.cpp b/services/oboeservice/AAudioClientTracker.cpp
index 3ec8dea..054a896 100644
--- a/services/oboeservice/AAudioClientTracker.cpp
+++ b/services/oboeservice/AAudioClientTracker.cpp
@@ -41,7 +41,7 @@
: Singleton<AAudioClientTracker>() {
}
-std::string AAudioClientTracker::dump() const {
+std::string AAudioClientTracker::dump() const NO_THREAD_SAFETY_ANALYSIS {
std::stringstream result;
const bool isLocked = AAudio_tryUntilTrue(
[this]()->bool { return mLock.try_lock(); } /* f */,
@@ -207,7 +207,7 @@
}
-std::string AAudioClientTracker::NotificationClient::dump() const {
+std::string AAudioClientTracker::NotificationClient::dump() const NO_THREAD_SAFETY_ANALYSIS {
std::stringstream result;
const bool isLocked = AAudio_tryUntilTrue(
[this]()->bool { return mLock.try_lock(); } /* f */,
diff --git a/services/oboeservice/AAudioClientTracker.h b/services/oboeservice/AAudioClientTracker.h
index facfc3b..2b38621 100644
--- a/services/oboeservice/AAudioClientTracker.h
+++ b/services/oboeservice/AAudioClientTracker.h
@@ -21,6 +21,7 @@
#include <mutex>
#include <set>
+#include <android-base/thread_annotations.h>
#include <utils/Singleton.h>
#include <aaudio/AAudio.h>
@@ -114,10 +115,12 @@
};
// This must be called under mLock
- android::sp<NotificationClient> getNotificationClient_l(pid_t pid);
+ android::sp<NotificationClient> getNotificationClient_l(pid_t pid)
+ REQUIRES(mLock);
mutable std::mutex mLock;
- std::map<pid_t, android::sp<NotificationClient>> mNotificationClients;
+ std::map<pid_t, android::sp<NotificationClient>> mNotificationClients
+ GUARDED_BY(mLock);
android::AAudioService *mAAudioService = nullptr;
};
diff --git a/services/oboeservice/AAudioEndpointManager.cpp b/services/oboeservice/AAudioEndpointManager.cpp
index 483a264..407f6d5 100644
--- a/services/oboeservice/AAudioEndpointManager.cpp
+++ b/services/oboeservice/AAudioEndpointManager.cpp
@@ -43,7 +43,7 @@
, mExclusiveStreams() {
}
-std::string AAudioEndpointManager::dump() const {
+std::string AAudioEndpointManager::dump() const NO_THREAD_SAFETY_ANALYSIS {
std::stringstream result;
int index = 0;
diff --git a/services/oboeservice/AAudioEndpointManager.h b/services/oboeservice/AAudioEndpointManager.h
index ae776b1..b07bcef 100644
--- a/services/oboeservice/AAudioEndpointManager.h
+++ b/services/oboeservice/AAudioEndpointManager.h
@@ -20,6 +20,8 @@
#include <map>
#include <mutex>
#include <sys/types.h>
+
+#include <android-base/thread_annotations.h>
#include <utils/Singleton.h>
#include "binding/AAudioServiceMessage.h"
@@ -70,10 +72,12 @@
const aaudio::AAudioStreamRequest &request);
android::sp<AAudioServiceEndpoint> findExclusiveEndpoint_l(
- const AAudioStreamConfiguration& configuration);
+ const AAudioStreamConfiguration& configuration)
+ REQUIRES(mExclusiveLock);
android::sp<AAudioServiceEndpointShared> findSharedEndpoint_l(
- const AAudioStreamConfiguration& configuration);
+ const AAudioStreamConfiguration& configuration)
+ REQUIRES(mSharedLock);
void closeExclusiveEndpoint(android::sp<AAudioServiceEndpoint> serviceEndpoint);
void closeSharedEndpoint(android::sp<AAudioServiceEndpoint> serviceEndpoint);
@@ -83,23 +87,25 @@
// Lock mSharedLock before mExclusiveLock.
// it is OK to only lock mExclusiveLock.
mutable std::mutex mSharedLock;
- std::vector<android::sp<AAudioServiceEndpointShared>> mSharedStreams;
+ std::vector<android::sp<AAudioServiceEndpointShared>> mSharedStreams
+ GUARDED_BY(mSharedLock);
mutable std::mutex mExclusiveLock;
- std::vector<android::sp<AAudioServiceEndpointMMAP>> mExclusiveStreams;
+ std::vector<android::sp<AAudioServiceEndpointMMAP>> mExclusiveStreams
+ GUARDED_BY(mExclusiveLock);
- // Modified under a lock.
- int32_t mExclusiveSearchCount = 0; // number of times we SEARCHED for an exclusive endpoint
- int32_t mExclusiveFoundCount = 0; // number of times we FOUND an exclusive endpoint
- int32_t mExclusiveOpenCount = 0; // number of times we OPENED an exclusive endpoint
- int32_t mExclusiveCloseCount = 0; // number of times we CLOSED an exclusive endpoint
- int32_t mExclusiveStolenCount = 0; // number of times we STOLE an exclusive endpoint
+ // Counts related to an exclusive endpoint.
+ int32_t mExclusiveSearchCount GUARDED_BY(mExclusiveLock) = 0; // # SEARCHED
+ int32_t mExclusiveFoundCount GUARDED_BY(mExclusiveLock) = 0; // # FOUND
+ int32_t mExclusiveOpenCount GUARDED_BY(mExclusiveLock) = 0; // # OPENED
+ int32_t mExclusiveCloseCount GUARDED_BY(mExclusiveLock) = 0; // # CLOSED
+ int32_t mExclusiveStolenCount GUARDED_BY(mExclusiveLock) = 0; // # STOLEN
// Same as above but for SHARED endpoints.
- int32_t mSharedSearchCount = 0;
- int32_t mSharedFoundCount = 0;
- int32_t mSharedOpenCount = 0;
- int32_t mSharedCloseCount = 0;
+ int32_t mSharedSearchCount GUARDED_BY(mSharedLock) = 0;
+ int32_t mSharedFoundCount GUARDED_BY(mSharedLock) = 0;
+ int32_t mSharedOpenCount GUARDED_BY(mSharedLock) = 0;
+ int32_t mSharedCloseCount GUARDED_BY(mSharedLock) = 0;
// For easily disabling the stealing of exclusive streams.
static constexpr bool kStealingEnabled = true;
diff --git a/services/oboeservice/AAudioServiceEndpoint.cpp b/services/oboeservice/AAudioServiceEndpoint.cpp
index b139be1..faea58f 100644
--- a/services/oboeservice/AAudioServiceEndpoint.cpp
+++ b/services/oboeservice/AAudioServiceEndpoint.cpp
@@ -38,7 +38,7 @@
using namespace android; // TODO just import names needed
using namespace aaudio; // TODO just import names needed
-std::string AAudioServiceEndpoint::dump() const {
+std::string AAudioServiceEndpoint::dump() const NO_THREAD_SAFETY_ANALYSIS {
std::stringstream result;
const bool isLocked = AAudio_tryUntilTrue(
diff --git a/services/oboeservice/AAudioServiceEndpoint.h b/services/oboeservice/AAudioServiceEndpoint.h
index 04b906a..72090c2 100644
--- a/services/oboeservice/AAudioServiceEndpoint.h
+++ b/services/oboeservice/AAudioServiceEndpoint.h
@@ -22,6 +22,8 @@
#include <mutex>
#include <vector>
+#include <android-base/thread_annotations.h>
+
#include "client/AudioStreamInternal.h"
#include "client/AudioStreamInternalPlay.h"
#include "core/AAudioStreamParameters.h"
@@ -141,7 +143,8 @@
std::vector<android::sp<AAudioServiceStreamBase>> disconnectRegisteredStreams();
mutable std::mutex mLockStreams;
- std::vector<android::sp<AAudioServiceStreamBase>> mRegisteredStreams;
+ std::vector<android::sp<AAudioServiceStreamBase>> mRegisteredStreams
+ GUARDED_BY(mLockStreams);
SimpleDoubleBuffer<Timestamp> mAtomicEndpointTimestamp;
diff --git a/services/oboeservice/AAudioServiceEndpointPlay.cpp b/services/oboeservice/AAudioServiceEndpointPlay.cpp
index 6ddc30b..4e46033 100644
--- a/services/oboeservice/AAudioServiceEndpointPlay.cpp
+++ b/services/oboeservice/AAudioServiceEndpointPlay.cpp
@@ -98,7 +98,7 @@
{
// Lock the AudioFifo to protect against close.
- std::lock_guard <std::mutex> lock(streamShared->getAudioDataQueueLock());
+ std::lock_guard <std::mutex> lock(streamShared->audioDataQueueLock);
std::shared_ptr<SharedRingBuffer> audioDataQueue
= streamShared->getAudioDataQueue_l();
std::shared_ptr<FifoBuffer> fifo;
diff --git a/services/oboeservice/AAudioServiceEndpointShared.cpp b/services/oboeservice/AAudioServiceEndpointShared.cpp
index caf6139..501e8c0 100644
--- a/services/oboeservice/AAudioServiceEndpointShared.cpp
+++ b/services/oboeservice/AAudioServiceEndpointShared.cpp
@@ -137,24 +137,25 @@
aaudio_result_t aaudio::AAudioServiceEndpointShared::stopSharingThread() {
mCallbackEnabled.store(false);
- aaudio_result_t result = getStreamInternal()->joinThread(NULL);
- return result;
+ return getStreamInternal()->joinThread(NULL);
}
-aaudio_result_t AAudioServiceEndpointShared::startStream(sp<AAudioServiceStreamBase> sharedStream,
- audio_port_handle_t *clientHandle) {
+aaudio_result_t AAudioServiceEndpointShared::startStream(
+ sp<AAudioServiceStreamBase> sharedStream,
+ audio_port_handle_t *clientHandle)
+ NO_THREAD_SAFETY_ANALYSIS {
aaudio_result_t result = AAUDIO_OK;
{
std::lock_guard<std::mutex> lock(mLockStreams);
if (++mRunningStreamCount == 1) { // atomic
- result = getStreamInternal()->requestStart_l();
+ result = getStreamInternal()->systemStart();
if (result != AAUDIO_OK) {
--mRunningStreamCount;
} else {
result = startSharingThread_l();
if (result != AAUDIO_OK) {
- getStreamInternal()->requestStop_l();
+ getStreamInternal()->systemStopFromApp();
--mRunningStreamCount;
}
}
@@ -168,7 +169,7 @@
if (result != AAUDIO_OK) {
if (--mRunningStreamCount == 0) { // atomic
stopSharingThread();
- getStreamInternal()->requestStop_l();
+ getStreamInternal()->systemStopFromApp();
}
}
}
@@ -183,7 +184,7 @@
if (--mRunningStreamCount == 0) { // atomic
stopSharingThread(); // the sharing thread locks mLockStreams
- getStreamInternal()->requestStop_l();
+ getStreamInternal()->systemStopFromApp();
}
return AAUDIO_OK;
}
diff --git a/services/oboeservice/AAudioServiceEndpointShared.h b/services/oboeservice/AAudioServiceEndpointShared.h
index 91a86c1..8357567 100644
--- a/services/oboeservice/AAudioServiceEndpointShared.h
+++ b/services/oboeservice/AAudioServiceEndpointShared.h
@@ -20,6 +20,8 @@
#include <atomic>
#include <mutex>
+#include <android-base/thread_annotations.h>
+
#include "AAudioServiceEndpoint.h"
#include "client/AudioStreamInternal.h"
#include "client/AudioStreamInternalPlay.h"
@@ -63,7 +65,7 @@
protected:
- aaudio_result_t startSharingThread_l();
+ aaudio_result_t startSharingThread_l() REQUIRES(mLockStreams);
aaudio_result_t stopSharingThread();
diff --git a/services/oboeservice/AAudioServiceStreamBase.cpp b/services/oboeservice/AAudioServiceStreamBase.cpp
index 9736091..7edc25c 100644
--- a/services/oboeservice/AAudioServiceStreamBase.cpp
+++ b/services/oboeservice/AAudioServiceStreamBase.cpp
@@ -595,14 +595,3 @@
void AAudioServiceStreamBase::onVolumeChanged(float volume) {
sendServiceEvent(AAUDIO_SERVICE_EVENT_VOLUME, volume);
}
-
-int32_t AAudioServiceStreamBase::incrementServiceReferenceCount_l() {
- return ++mCallingCount;
-}
-
-int32_t AAudioServiceStreamBase::decrementServiceReferenceCount_l() {
- int32_t count = --mCallingCount;
- // Each call to increment should be balanced with one call to decrement.
- assert(count >= 0);
- return count;
-}
diff --git a/services/oboeservice/AAudioServiceStreamBase.h b/services/oboeservice/AAudioServiceStreamBase.h
index f9efc2a..0f752b7 100644
--- a/services/oboeservice/AAudioServiceStreamBase.h
+++ b/services/oboeservice/AAudioServiceStreamBase.h
@@ -20,6 +20,7 @@
#include <assert.h>
#include <mutex>
+#include <android-base/thread_annotations.h>
#include <media/AudioClient.h>
#include <utils/RefBase.h>
@@ -209,25 +210,6 @@
return mSuspended;
}
- /**
- * Atomically increment the number of active references to the stream by AAudioService.
- *
- * This is called under a global lock in AAudioStreamTracker.
- *
- * @return value after the increment
- */
- int32_t incrementServiceReferenceCount_l();
-
- /**
- * Atomically decrement the number of active references to the stream by AAudioService.
- * This should only be called after incrementServiceReferenceCount_l().
- *
- * This is called under a global lock in AAudioStreamTracker.
- *
- * @return value after the decrement
- */
- int32_t decrementServiceReferenceCount_l();
-
bool isCloseNeeded() const {
return mCloseNeeded.load();
}
@@ -250,11 +232,10 @@
aaudio_result_t open(const aaudio::AAudioStreamRequest &request,
aaudio_sharing_mode_t sharingMode);
- // These must be called under mLock
- virtual aaudio_result_t close_l();
- virtual aaudio_result_t pause_l();
- virtual aaudio_result_t stop_l();
- void disconnect_l();
+ virtual aaudio_result_t close_l() REQUIRES(mLock);
+ virtual aaudio_result_t pause_l() REQUIRES(mLock);
+ virtual aaudio_result_t stop_l() REQUIRES(mLock);
+ void disconnect_l() REQUIRES(mLock);
void setState(aaudio_stream_state_t state);
@@ -332,18 +313,17 @@
aaudio_handle_t mHandle = -1;
bool mFlowing = false;
- // This is modified under a global lock in AAudioStreamTracker.
- int32_t mCallingCount = 0;
-
- // This indicates that a stream that is being referenced by a binder call needs to closed.
- std::atomic<bool> mCloseNeeded{false};
+ // This indicates that a stream that is being referenced by a binder call
+ // and needs to closed.
+ std::atomic<bool> mCloseNeeded{false}; // TODO remove
// This indicate that a running stream should not be processed because of an error,
// for example a full message queue. Note that this atomic is unrelated to mCloseNeeded.
std::atomic<bool> mSuspended{false};
+protected:
// Locking order is important.
- // Always acquire mLock before acquiring AAudioServiceEndpoint::mLockStreams
+ // Acquire mLock before acquiring AAudioServiceEndpoint::mLockStreams
std::mutex mLock; // Prevent start/stop/close etcetera from colliding
};
diff --git a/services/oboeservice/AAudioServiceStreamMMAP.h b/services/oboeservice/AAudioServiceStreamMMAP.h
index 5902613..6ba1725 100644
--- a/services/oboeservice/AAudioServiceStreamMMAP.h
+++ b/services/oboeservice/AAudioServiceStreamMMAP.h
@@ -19,6 +19,7 @@
#include <atomic>
+#include <android-base/thread_annotations.h>
#include <android-base/unique_fd.h>
#include <media/audiohal/StreamHalInterface.h>
#include <media/MmapStreamCallback.h>
@@ -34,10 +35,8 @@
#include "TimestampScheduler.h"
#include "utility/MonotonicCounter.h"
-
namespace aaudio {
-
/**
* These corresponds to an EXCLUSIVE mode MMAP client stream.
* It has exclusive use of one AAudioServiceEndpointMMAP to communicate with the underlying
@@ -68,9 +67,9 @@
* This is not guaranteed to be synchronous but it currently is.
* An AAUDIO_SERVICE_EVENT_PAUSED will be sent to the client when complete.
*/
- aaudio_result_t pause_l() override;
+ aaudio_result_t pause_l() REQUIRES(mLock) override;
- aaudio_result_t stop_l() override;
+ aaudio_result_t stop_l() REQUIRES(mLock) override;
aaudio_result_t getAudioDataDescription(AudioEndpointParcelable &parcelable) override;
diff --git a/services/oboeservice/AAudioServiceStreamShared.cpp b/services/oboeservice/AAudioServiceStreamShared.cpp
index 031468e..c665cda 100644
--- a/services/oboeservice/AAudioServiceStreamShared.cpp
+++ b/services/oboeservice/AAudioServiceStreamShared.cpp
@@ -52,14 +52,26 @@
return result.str();
}
-std::string AAudioServiceStreamShared::dump() const {
+std::string AAudioServiceStreamShared::dump() const NO_THREAD_SAFETY_ANALYSIS {
std::stringstream result;
+ const bool isLocked = AAudio_tryUntilTrue(
+ [this]()->bool { return audioDataQueueLock.try_lock(); } /* f */,
+ 50 /* times */,
+ 20 /* sleepMs */);
+ if (!isLocked) {
+ result << "AAudioServiceStreamShared may be deadlocked\n";
+ }
+
result << AAudioServiceStreamBase::dump();
result << mAudioDataQueue->dump();
result << std::setw(8) << getXRunCount();
+ if (isLocked) {
+ audioDataQueueLock.unlock();
+ }
+
return result.str();
}
@@ -171,7 +183,7 @@
}
{
- std::lock_guard<std::mutex> lock(mAudioDataQueueLock);
+ std::lock_guard<std::mutex> lock(audioDataQueueLock);
// Create audio data shared memory buffer for client.
mAudioDataQueue = std::make_shared<SharedRingBuffer>();
result = mAudioDataQueue->allocate(calculateBytesPerFrame(), getBufferCapacity());
@@ -202,7 +214,7 @@
aaudio_result_t AAudioServiceStreamShared::getAudioDataDescription(
AudioEndpointParcelable &parcelable)
{
- std::lock_guard<std::mutex> lock(mAudioDataQueueLock);
+ std::lock_guard<std::mutex> lock(audioDataQueueLock);
if (mAudioDataQueue == nullptr) {
ALOGW("%s(): mUpMessageQueue null! - stream not open", __func__);
return AAUDIO_ERROR_NULL;
@@ -260,7 +272,7 @@
int64_t clientFramesWritten = 0;
// Lock the AudioFifo to protect against close.
- std::lock_guard <std::mutex> lock(mAudioDataQueueLock);
+ std::lock_guard <std::mutex> lock(audioDataQueueLock);
if (mAudioDataQueue != nullptr) {
std::shared_ptr<FifoBuffer> fifo = mAudioDataQueue->getFifoBuffer();
diff --git a/services/oboeservice/AAudioServiceStreamShared.h b/services/oboeservice/AAudioServiceStreamShared.h
index 5b1f8da..4fae5b4 100644
--- a/services/oboeservice/AAudioServiceStreamShared.h
+++ b/services/oboeservice/AAudioServiceStreamShared.h
@@ -52,22 +52,15 @@
aaudio_result_t open(const aaudio::AAudioStreamRequest &request) override;
- /**
- * This must be locked when calling getAudioDataQueue_l() and while
- * using the FifoBuffer it contains.
- */
- std::mutex &getAudioDataQueueLock() {
- return mAudioDataQueueLock;
- }
-
void writeDataIfRoom(int64_t mmapFramesRead, const void *buffer, int32_t numFrames);
/**
* This must only be called under getAudioDataQueueLock().
* @return
*/
- std::shared_ptr<SharedRingBuffer> getAudioDataQueue_l() {
- return mAudioDataQueue;
+ std::shared_ptr<SharedRingBuffer> getAudioDataQueue_l()
+ REQUIRES(audioDataQueueLock) {
+ return mAudioDataQueue;
}
/* Keep a record of when a buffer transfer completed.
@@ -89,6 +82,10 @@
const char *getTypeText() const override { return "Shared"; }
+ // This is public so that the thread safety annotation, GUARDED_BY(),
+ // Can work when another object takes the lock.
+ mutable std::mutex audioDataQueueLock;
+
protected:
aaudio_result_t getAudioDataDescription(AudioEndpointParcelable &parcelable) override;
@@ -107,8 +104,7 @@
private:
- std::shared_ptr<SharedRingBuffer> mAudioDataQueue; // protected by mAudioDataQueueLock
- std::mutex mAudioDataQueueLock;
+ std::shared_ptr<SharedRingBuffer> mAudioDataQueue GUARDED_BY(audioDataQueueLock);
std::atomic<int64_t> mTimestampPositionOffset;
std::atomic<int32_t> mXRunCount;
diff --git a/services/oboeservice/AAudioStreamTracker.cpp b/services/oboeservice/AAudioStreamTracker.cpp
index 8e66b94..9bbbc73 100644
--- a/services/oboeservice/AAudioStreamTracker.cpp
+++ b/services/oboeservice/AAudioStreamTracker.cpp
@@ -96,7 +96,7 @@
return handle;
}
-std::string AAudioStreamTracker::dump() const {
+std::string AAudioStreamTracker::dump() const NO_THREAD_SAFETY_ANALYSIS {
std::stringstream result;
const bool isLocked = AAudio_tryUntilTrue(
[this]()->bool { return mHandleLock.try_lock(); } /* f */,
diff --git a/services/oboeservice/AAudioStreamTracker.h b/services/oboeservice/AAudioStreamTracker.h
index d1301a2..43870fc 100644
--- a/services/oboeservice/AAudioStreamTracker.h
+++ b/services/oboeservice/AAudioStreamTracker.h
@@ -17,13 +17,13 @@
#ifndef AAUDIO_AAUDIO_STREAM_TRACKER_H
#define AAUDIO_AAUDIO_STREAM_TRACKER_H
+#include <mutex>
#include <time.h>
-#include <pthread.h>
+#include <android-base/thread_annotations.h>
#include <aaudio/AAudio.h>
#include "binding/AAudioCommon.h"
-
#include "AAudioServiceStreamBase.h"
namespace aaudio {
@@ -75,11 +75,10 @@
static aaudio_handle_t bumpHandle(aaudio_handle_t handle);
// Track stream using a unique handle that wraps. Only use positive half.
- mutable std::mutex mHandleLock;
- // protected by mHandleLock
- aaudio_handle_t mPreviousHandle = 0;
- // protected by mHandleLock
- std::map<aaudio_handle_t, android::sp<aaudio::AAudioServiceStreamBase>> mStreamsByHandle;
+ mutable std::mutex mHandleLock;
+ aaudio_handle_t mPreviousHandle GUARDED_BY(mHandleLock) = 0;
+ std::map<aaudio_handle_t, android::sp<aaudio::AAudioServiceStreamBase>>
+ mStreamsByHandle GUARDED_BY(mHandleLock);
};
diff --git a/services/oboeservice/Android.bp b/services/oboeservice/Android.bp
index 31e590e..80f17f4 100644
--- a/services/oboeservice/Android.bp
+++ b/services/oboeservice/Android.bp
@@ -37,6 +37,7 @@
],
cflags: [
+ "-Wthread-safety",
"-Wno-unused-parameter",
"-Wall",
"-Werror",