From abdc01542509719821daa122b5e93965f8881a0e Mon Sep 17 00:00:00 2001 From: Phil Burk Date: Wed, 12 Jun 2024 16:01:59 -0700 Subject: [PATCH] Use shared_ptr in FilterAudioStream This is used when Oboe does sample rate conversion. Or format or channel count conversion. It used to use a raw pointer, which could cause crashes. Fixes #2035 --- include/oboe/AudioStream.h | 2 +- src/aaudio/AudioStreamAAudio.cpp | 24 ++++++++++++++++++------ src/common/AudioStream.cpp | 6 ++++++ src/common/AudioStreamBuilder.cpp | 6 ++++-- src/common/FilterAudioStream.h | 10 +++------- 5 files changed, 32 insertions(+), 16 deletions(-) diff --git a/include/oboe/AudioStream.h b/include/oboe/AudioStream.h index 817099d53..d4c2a06e3 100644 --- a/include/oboe/AudioStream.h +++ b/include/oboe/AudioStream.h @@ -54,7 +54,7 @@ class AudioStream : public AudioStreamBase { */ explicit AudioStream(const AudioStreamBuilder &builder); - virtual ~AudioStream() = default; + virtual ~AudioStream(); /** * Open a stream based on the current settings. diff --git a/src/aaudio/AudioStreamAAudio.cpp b/src/aaudio/AudioStreamAAudio.cpp index 421798dfc..7aad6dda3 100644 --- a/src/aaudio/AudioStreamAAudio.cpp +++ b/src/aaudio/AudioStreamAAudio.cpp @@ -60,9 +60,13 @@ static aaudio_data_callback_result_t oboe_aaudio_data_callback_proc( // This runs in its own thread. // Only one of these threads will be launched from internalErrorCallback(). // It calls app error callbacks from a static function in case the stream gets deleted. -static void oboe_aaudio_error_thread_proc(AudioStreamAAudio *oboeStream, +static void oboe_aaudio_error_thread_proc_common(AudioStreamAAudio *oboeStream, Result error) { - LOGD("%s(,%d) - entering >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>", __func__, error); +#if 0 + LOGE("%s() sleep for 5 seconds", __func__); + usleep(5*1000*1000); + LOGD("%s() - woke up -------------------------", __func__); +#endif AudioStreamErrorCallback *errorCallback = oboeStream->getErrorCallback(); if (errorCallback == nullptr) return; // should be impossible bool isErrorHandled = errorCallback->onError(oboeStream, error); @@ -74,16 +78,24 @@ static void oboe_aaudio_error_thread_proc(AudioStreamAAudio *oboeStream, // Warning, oboeStream may get deleted by this callback. errorCallback->onErrorAfterClose(oboeStream, error); } +} + +// Callback thread for raw pointers. +static void oboe_aaudio_error_thread_proc(AudioStreamAAudio *oboeStream, + Result error) { + LOGD("%s(,%d) - entering >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>", __func__, error); + oboe_aaudio_error_thread_proc_common(oboeStream, error); LOGD("%s() - exiting <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<", __func__); } -// This runs in its own thread. -// Only one of these threads will be launched from internalErrorCallback(). -// Prevents deletion of the stream if the app is using AudioStreamBuilder::openSharedStream() +// Callback thread for shared pointers. static void oboe_aaudio_error_thread_proc_shared(std::shared_ptr sharedStream, Result error) { + LOGD("%s(,%d) - entering >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>", __func__, error); + // Hold the shared pointer while we use the raw pointer. AudioStreamAAudio *oboeStream = reinterpret_cast(sharedStream.get()); - oboe_aaudio_error_thread_proc(oboeStream, error); + oboe_aaudio_error_thread_proc_common(oboeStream, error); + LOGD("%s() - exiting <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<", __func__); } namespace oboe { diff --git a/src/common/AudioStream.cpp b/src/common/AudioStream.cpp index 06c01118e..89a8d0198 100644 --- a/src/common/AudioStream.cpp +++ b/src/common/AudioStream.cpp @@ -30,6 +30,12 @@ namespace oboe { */ AudioStream::AudioStream(const AudioStreamBuilder &builder) : AudioStreamBase(builder) { + LOGD("Constructor for AudioStream at %p", this); +} + +AudioStream::~AudioStream() { + // This is to help debug use after free bugs. + LOGD("Destructor for AudioStream at %p", this); } Result AudioStream::close() { diff --git a/src/common/AudioStreamBuilder.cpp b/src/common/AudioStreamBuilder.cpp index b1549b545..7604dc952 100644 --- a/src/common/AudioStreamBuilder.cpp +++ b/src/common/AudioStreamBuilder.cpp @@ -117,7 +117,7 @@ Result AudioStreamBuilder::openStreamInternal(AudioStream **streamPP) { // Do we need to make a child stream and convert. if (conversionNeeded) { AudioStream *tempStream; - result = childBuilder.openStream(&tempStream); + result = childBuilder.openStreamInternal(&tempStream); if (result != Result::OK) { return result; } @@ -144,7 +144,9 @@ Result AudioStreamBuilder::openStreamInternal(AudioStream **streamPP) { // Use childStream in a FilterAudioStream. LOGI("%s() create a FilterAudioStream for data conversion.", __func__); - FilterAudioStream *filterStream = new FilterAudioStream(parentBuilder, tempStream); + std::shared_ptr childStream(tempStream); + FilterAudioStream *filterStream = new FilterAudioStream(parentBuilder, childStream); + childStream->setWeakThis(childStream); result = filterStream->configureFlowGraph(); if (result != Result::OK) { filterStream->close(); diff --git a/src/common/FilterAudioStream.h b/src/common/FilterAudioStream.h index 189074997..99f6f5ac9 100644 --- a/src/common/FilterAudioStream.h +++ b/src/common/FilterAudioStream.h @@ -38,9 +38,9 @@ class FilterAudioStream : public AudioStream, AudioStreamCallback { * * @param builder containing all the stream's attributes */ - FilterAudioStream(const AudioStreamBuilder &builder, AudioStream *childStream) + FilterAudioStream(const AudioStreamBuilder &builder, std::shared_ptr childStream) : AudioStream(builder) - , mChildStream(childStream) { + , mChildStream(childStream) { // Intercept the callback if used. if (builder.isErrorCallbackSpecified()) { mErrorCallback = mChildStream->swapErrorCallback(this); @@ -66,10 +66,6 @@ class FilterAudioStream : public AudioStream, AudioStreamCallback { virtual ~FilterAudioStream() = default; - AudioStream *getChildStream() const { - return mChildStream.get(); - } - Result configureFlowGraph(); // Close child and parent. @@ -216,7 +212,7 @@ class FilterAudioStream : public AudioStream, AudioStreamCallback { private: - std::unique_ptr mChildStream; // this stream wraps the child stream + std::shared_ptr mChildStream; // this stream wraps the child stream std::unique_ptr mFlowGraph; // for converting data std::unique_ptr mBlockingBuffer; // temp buffer for write() double mRateScaler = 1.0; // ratio parent/child sample rates