StreamHalHidl: Use atomic_wp and make pointers const
Avoid concurrency issues with modification of wp.
Test: basic audio
Bug: 177278988
Change-Id: I5b968a1dad24e1d6c18260884d35c3170661da01
diff --git a/media/libaudiohal/Android.bp b/media/libaudiohal/Android.bp
index 482f40e..d9a7804 100644
--- a/media/libaudiohal/Android.bp
+++ b/media/libaudiohal/Android.bp
@@ -31,6 +31,7 @@
header_libs: [
"libaudiohal_headers",
"libbase_headers",
+ "libmediautils_headers",
]
}
diff --git a/media/libaudiohal/impl/StreamHalHidl.cpp b/media/libaudiohal/impl/StreamHalHidl.cpp
index 09a7c1c..3f9bccb 100644
--- a/media/libaudiohal/impl/StreamHalHidl.cpp
+++ b/media/libaudiohal/impl/StreamHalHidl.cpp
@@ -61,10 +61,6 @@
}
}
-StreamHalHidl::~StreamHalHidl() {
- mStream = nullptr;
-}
-
status_t StreamHalHidl::getSampleRate(uint32_t *rate) {
if (!mStream) return NO_INIT;
return processReturn("getSampleRate", mStream->getSampleRate(), rate);
@@ -286,7 +282,7 @@
}
private:
- wp<StreamOutHalHidl> mStream;
+ const wp<StreamOutHalHidl> mStream;
};
} // namespace
@@ -297,21 +293,19 @@
StreamOutHalHidl::~StreamOutHalHidl() {
if (mStream != 0) {
- if (mCallback.unsafe_get()) {
+ if (mCallback.load().unsafe_get()) {
processReturn("clearCallback", mStream->clearCallback());
}
#if MAJOR_VERSION >= 6
- if (mEventCallback.unsafe_get() != nullptr) {
+ if (mEventCallback.load().unsafe_get() != nullptr) {
processReturn("setEventCallback",
mStream->setEventCallback(nullptr));
}
#endif
processReturn("close", mStream->close());
- mStream.clear();
}
- mCallback.clear();
- mEventCallback.clear();
- hardware::IPCThreadState::self()->flushCommands();
+ mCallback = nullptr;
+ mEventCallback = nullptr;
if (mEfGroup) {
EventFlag::deleteEventFlag(&mEfGroup);
}
@@ -364,7 +358,7 @@
if (bytes == 0 && !mDataMQ) {
// Can't determine the size for the MQ buffer. Wait for a non-empty write request.
- ALOGW_IF(mCallback.unsafe_get(), "First call to async write with 0 bytes");
+ ALOGW_IF(mCallback.load().unsafe_get(), "First call to async write with 0 bytes");
return OK;
}
@@ -680,28 +674,28 @@
#endif
void StreamOutHalHidl::onWriteReady() {
- sp<StreamOutHalInterfaceCallback> callback = mCallback.promote();
+ sp<StreamOutHalInterfaceCallback> callback = mCallback.load().promote();
if (callback == 0) return;
ALOGV("asyncCallback onWriteReady");
callback->onWriteReady();
}
void StreamOutHalHidl::onDrainReady() {
- sp<StreamOutHalInterfaceCallback> callback = mCallback.promote();
+ sp<StreamOutHalInterfaceCallback> callback = mCallback.load().promote();
if (callback == 0) return;
ALOGV("asyncCallback onDrainReady");
callback->onDrainReady();
}
void StreamOutHalHidl::onError() {
- sp<StreamOutHalInterfaceCallback> callback = mCallback.promote();
+ sp<StreamOutHalInterfaceCallback> callback = mCallback.load().promote();
if (callback == 0) return;
ALOGV("asyncCallback onError");
callback->onError();
}
void StreamOutHalHidl::onCodecFormatChanged(const std::basic_string<uint8_t>& metadataBs) {
- sp<StreamOutHalInterfaceEventCallback> callback = mEventCallback.promote();
+ sp<StreamOutHalInterfaceEventCallback> callback = mEventCallback.load().promote();
if (callback == nullptr) return;
ALOGV("asyncCodecFormatCallback %s", __func__);
callback->onCodecFormatChanged(metadataBs);
@@ -715,8 +709,6 @@
StreamInHalHidl::~StreamInHalHidl() {
if (mStream != 0) {
processReturn("close", mStream->close());
- mStream.clear();
- hardware::IPCThreadState::self()->flushCommands();
}
if (mEfGroup) {
EventFlag::deleteEventFlag(&mEfGroup);
diff --git a/media/libaudiohal/impl/StreamHalHidl.h b/media/libaudiohal/impl/StreamHalHidl.h
index 88f8587..8b1c3d3 100644
--- a/media/libaudiohal/impl/StreamHalHidl.h
+++ b/media/libaudiohal/impl/StreamHalHidl.h
@@ -25,6 +25,7 @@
#include <fmq/EventFlag.h>
#include <fmq/MessageQueue.h>
#include <media/audiohal/StreamHalInterface.h>
+#include <mediautils/Synchronization.h>
#include "ConversionHelperHidl.h"
#include "StreamPowerLog.h"
@@ -100,9 +101,6 @@
// Subclasses can not be constructed directly by clients.
explicit StreamHalHidl(IStream *stream);
- // The destructor automatically closes the stream.
- virtual ~StreamHalHidl();
-
status_t getCachedBufferSize(size_t *size);
bool requestHalThreadPriority(pid_t threadPid, pid_t threadId);
@@ -112,7 +110,7 @@
private:
const int HAL_THREAD_PRIORITY_DEFAULT = -1;
- IStream *mStream;
+ IStream * const mStream;
int mHalThreadPriority;
size_t mCachedBufferSize;
};
@@ -184,9 +182,16 @@
typedef MessageQueue<uint8_t, hardware::kSynchronizedReadWrite> DataMQ;
typedef MessageQueue<WriteStatus, hardware::kSynchronizedReadWrite> StatusMQ;
- wp<StreamOutHalInterfaceCallback> mCallback;
- wp<StreamOutHalInterfaceEventCallback> mEventCallback;
- sp<IStreamOut> mStream;
+ // Do not move the Defer. This should be the first member variable in the class;
+ // thus the last member destructor called upon instance destruction.
+ //
+ // The last step is to flush all binder commands so that the AudioFlinger
+ // may recognize the deletion of IStreamOut (mStream) with less delay. See b/35394629.
+ mediautils::Defer mLast{[]() { hardware::IPCThreadState::self()->flushCommands(); }};
+
+ mediautils::atomic_wp<StreamOutHalInterfaceCallback> mCallback;
+ mediautils::atomic_wp<StreamOutHalInterfaceEventCallback> mEventCallback;
+ const sp<IStreamOut> mStream;
std::unique_ptr<CommandMQ> mCommandMQ;
std::unique_ptr<DataMQ> mDataMQ;
std::unique_ptr<StatusMQ> mStatusMQ;
@@ -242,7 +247,14 @@
typedef MessageQueue<uint8_t, hardware::kSynchronizedReadWrite> DataMQ;
typedef MessageQueue<ReadStatus, hardware::kSynchronizedReadWrite> StatusMQ;
- sp<IStreamIn> mStream;
+ // Do not move the Defer. This should be the first member variable in the class;
+ // thus the last member destructor called upon instance destruction.
+ //
+ // The last step is to flush all binder commands so that the AudioFlinger
+ // may recognize the deletion of IStreamIn (mStream) with less delay. See b/35394629.
+ mediautils::Defer mLast{[]() { hardware::IPCThreadState::self()->flushCommands(); }};
+
+ const sp<IStreamIn> mStream;
std::unique_ptr<CommandMQ> mCommandMQ;
std::unique_ptr<DataMQ> mDataMQ;
std::unique_ptr<StatusMQ> mStatusMQ;
diff --git a/media/utils/include/mediautils/Synchronization.h b/media/utils/include/mediautils/Synchronization.h
index 153c5f1..aef4967 100644
--- a/media/utils/include/mediautils/Synchronization.h
+++ b/media/utils/include/mediautils/Synchronization.h
@@ -115,5 +115,19 @@
using atomic_sp = LockItem<
::android::sp<T>, std::mutex, LockItem<::android::sp<T>>::FLAG_DTOR_OUT_OF_LOCK>;
+/**
+ * Defers a function to run in the RAII destructor.
+ * A C++ implementation of Go _defer_ https://golangr.com/defer/.
+ */
+class Defer {
+public:
+ template <typename U>
+ explicit Defer(U &&f) : mThunk(std::forward<U>(f)) {}
+ ~Defer() { mThunk(); }
+
+private:
+ const std::function<void()> mThunk;
+};
+
} // namespace android::mediautils