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;