diff --git a/include/oboe/AudioStream.h b/include/oboe/AudioStream.h index 5a7521ee2..825ffc89b 100644 --- a/include/oboe/AudioStream.h +++ b/include/oboe/AudioStream.h @@ -374,11 +374,6 @@ class AudioStream : public AudioStreamBase { return nullptr; } - /** - * Launch a thread that will stop the stream. - */ - void launchStopThread(); - /** * Update mFramesWritten. * For internal use only. @@ -553,7 +548,6 @@ class AudioStream : public AudioStreamBase { std::atomic mDataCallbackEnabled{false}; std::atomic mErrorCallbackCalled{false}; - }; /** diff --git a/src/aaudio/AudioStreamAAudio.cpp b/src/aaudio/AudioStreamAAudio.cpp index b13ea0cb6..5ce0dc4a8 100644 --- a/src/aaudio/AudioStreamAAudio.cpp +++ b/src/aaudio/AudioStreamAAudio.cpp @@ -353,6 +353,21 @@ Result AudioStreamAAudio::close() { } } +static void oboe_stop_thread_proc(AudioStream *oboeStream) { + if (oboeStream != nullptr) { + oboeStream->requestStop(); + } +} + +void AudioStreamAAudio::launchStopThread() { + // Prevent multiple stop threads from being launched. + if (mStopThreadAllowed.exchange(false)) { + // Stop this stream on a separate thread + std::thread t(oboe_stop_thread_proc, this); + t.detach(); + } +} + DataCallbackResult AudioStreamAAudio::callOnAudioReady(AAudioStream * /*stream*/, void *audioData, int32_t numFrames) { @@ -366,16 +381,12 @@ DataCallbackResult AudioStreamAAudio::callOnAudioReady(AAudioStream * /*stream*/ LOGE("Oboe callback returned unexpected value = %d", result); } - if (getSdkVersion() <= __ANDROID_API_P__) { + // Returning Stop caused various problems before S. See #1230 + if (OboeGlobals::areWorkaroundsEnabled() && getSdkVersion() <= __ANDROID_API_R__) { launchStopThread(); - if (isMMapUsed()) { - return DataCallbackResult::Stop; - } else { - // Legacy stream <= API_P cannot be restarted after returning Stop. - return DataCallbackResult::Continue; - } + return DataCallbackResult::Continue; } else { - return DataCallbackResult::Stop; // OK >= API_Q + return DataCallbackResult::Stop; // OK >= API_S } } } @@ -395,6 +406,7 @@ Result AudioStreamAAudio::requestStart() { if (isDataCallbackSpecified()) { setDataCallbackEnabled(true); } + mStopThreadAllowed = true; return static_cast(mLibLoader->stream_requestStart(stream)); } else { return Result::ErrorClosed; diff --git a/src/aaudio/AudioStreamAAudio.h b/src/aaudio/AudioStreamAAudio.h index fafb5e0d9..e71efe85c 100644 --- a/src/aaudio/AudioStreamAAudio.h +++ b/src/aaudio/AudioStreamAAudio.h @@ -112,11 +112,17 @@ class AudioStreamAAudio : public AudioStream { // Must call under mLock. And stream must NOT be nullptr. Result requestStop_l(AAudioStream *stream); + /** + * Launch a thread that will stop the stream. + */ + void launchStopThread(); + // Time to sleep in order to prevent a race condition with a callback after a close(). // Two milliseconds may be enough but 10 msec is even safer. static constexpr int kDelayBeforeCloseMillis = 10; std::atomic mCallbackThreadEnabled; + std::atomic mStopThreadAllowed{false}; // pointer to the underlying 'C' AAudio stream, valid if open, null if closed std::atomic mAAudioStream{nullptr}; diff --git a/src/common/AudioStream.cpp b/src/common/AudioStream.cpp index 8a9cd452c..66d071172 100644 --- a/src/common/AudioStream.cpp +++ b/src/common/AudioStream.cpp @@ -55,7 +55,7 @@ void AudioStream::checkScheduler() { DataCallbackResult AudioStream::fireDataCallback(void *audioData, int32_t numFrames) { if (!isDataCallbackEnabled()) { LOGW("AudioStream::%s() called with data callback disabled!", __func__); - return DataCallbackResult::Stop; // We should not be getting called any more. + return DataCallbackResult::Stop; // Should not be getting called } DataCallbackResult result; @@ -196,16 +196,4 @@ ResultWithValue AudioStream::getTimestamp(clockid_t clockId) { } } -static void oboe_stop_thread_proc(AudioStream *oboeStream) { - if (oboeStream != nullptr) { - oboeStream->requestStop(); - } -} - -void AudioStream::launchStopThread() { - // Stop this stream on a separate thread - std::thread t(oboe_stop_thread_proc, this); - t.detach(); -} - } // namespace oboe diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 7c96a2687..f73db44e9 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -35,6 +35,7 @@ add_executable( testStreamOpen.cpp testStreamStates.cpp testStreamFramesProcessed.cpp + testReturnStop.cpp ) target_link_libraries(testOboe gtest oboe) diff --git a/tests/run_tests.sh b/tests/run_tests.sh index c012035a5..f4694ff6c 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -122,10 +122,21 @@ cp ${BUILD_DIR}/${TEST_BINARY_FILENAME} ${DESTINATION_DIR} pushd ${TEST_RUNNER_DIR} echo "Building test runner app" ./gradlew assembleDebug + if [ $? -ne 0 ]; then + echo "Building test app FAILED" + exit 1 + fi + echo "Installing to device" ./gradlew installDebug + if [ $? -ne 0 ]; then + echo "Installing tests FAILED" + exit 1 + fi popd +echo "Clear logcat from before the test." +adb logcat -c echo "Starting app - Check your device for test results" adb shell am start ${TEST_RUNNER_PACKAGE_NAME}/.MainActivity diff --git a/tests/testReturnStop.cpp b/tests/testReturnStop.cpp new file mode 100644 index 000000000..326f96f04 --- /dev/null +++ b/tests/testReturnStop.cpp @@ -0,0 +1,131 @@ +/* + * Copyright 2021 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include + +#include +#include + + +// Test returning DataCallbackResult::Stop from a callback. +using namespace oboe; + +static constexpr int kTimeoutInNanos = 500 * kNanosPerMillisecond; + +class ReturnStopCallback : public AudioStreamDataCallback { +public: + DataCallbackResult onAudioReady(AudioStream *oboeStream, void *audioData, int32_t numFrames) override { + callbackCount++; + return (callbackCount < kMaxCallbacks) ? DataCallbackResult::Continue : DataCallbackResult::Stop; + } + + void reset() { + callbackCount = 0; + } + + int getMaxCallbacks() const { return kMaxCallbacks; } + + std::atomic callbackCount{0}; + +private: + // I get strange linker errors with GTest if I try to reference this directly. + static constexpr int kMaxCallbacks = 4; +}; + +using StreamReturnStopParams = std::tuple; + +class StreamReturnStop : public ::testing::Test, + public ::testing::WithParamInterface { + +protected: + void TearDown() override; + + AudioStreamBuilder mBuilder; + AudioStream *mStream = nullptr; +}; + +void StreamReturnStop::TearDown() { + if (mStream != nullptr) { + mStream->close(); + mStream = nullptr; + } +} + +TEST_P(StreamReturnStop, VerifyStreamReturnStop) { + const Direction direction = std::get<0>(GetParam()); + const AudioApi audioApi = std::get<1>(GetParam()); + const PerformanceMode performanceMode = std::get<2>(GetParam()); + const bool useRequestStart = std::get<3>(GetParam()); + + ReturnStopCallback *callback = new ReturnStopCallback(); + mBuilder.setDirection(direction) + ->setAudioApi(audioApi) + ->setFormat(AudioFormat::Float) + ->setPerformanceMode(performanceMode) + ->setDataCallback(callback); + mStream = nullptr; + Result r = mBuilder.openStream(&mStream); + ASSERT_EQ(r, Result::OK) << "Failed to open stream." << convertToText(r); + + // Start and stop several times. + for (int i = 0; i < 3; i++) { + callback->reset(); + // Oboe has two ways to start a stream. + if (useRequestStart) { + r = mStream->requestStart(); + } else { + r = mStream->start(); + } + ASSERT_EQ(r, Result::OK) << "Failed to start stream." << convertToText(r); + + // Wait for callbacks to complete. + const int kMaxCallbackPeriodMillis = 200; + const int kPollPeriodMillis = 20; + int timeout = 2 * callback->getMaxCallbacks() * kMaxCallbackPeriodMillis / kPollPeriodMillis; + do { + usleep(kPollPeriodMillis * 1000); + } while (callback->callbackCount < callback->getMaxCallbacks() && timeout-- > 0); + EXPECT_GT(timeout, 0) << "timed out waiting for enough callbacks"; + + StreamState next = StreamState::Unknown; + r = mStream->waitForStateChange(StreamState::Started, &next, kTimeoutInNanos); + EXPECT_EQ(r, Result::OK) << "waitForStateChange(Started) timed out." << convertToText(r); + r = mStream->waitForStateChange(StreamState::Stopping, &next, kTimeoutInNanos); + EXPECT_EQ(r, Result::OK) << "waitForStateChange(Stopping) timed out." << convertToText(r); + EXPECT_EQ(next, StreamState::Stopped) << "Stream not in state Stopped, was " << convertToText(next); + + EXPECT_EQ(callback->callbackCount, callback->getMaxCallbacks()) << "Too many callbacks = " << callback->callbackCount; + } + + ASSERT_EQ(Result::OK, mStream->close()); +} + +INSTANTIATE_TEST_SUITE_P( + StreamReturnStopTest, + StreamReturnStop, + ::testing::Values( + // Last boolean is true if requestStart() should be called instead of start(). + StreamReturnStopParams({Direction::Output, AudioApi::AAudio, PerformanceMode::LowLatency, true}), + StreamReturnStopParams({Direction::Output, AudioApi::AAudio, PerformanceMode::LowLatency, false}), + StreamReturnStopParams({Direction::Output, AudioApi::AAudio, PerformanceMode::None, true}), + StreamReturnStopParams({Direction::Output, AudioApi::AAudio, PerformanceMode::None, false}), + StreamReturnStopParams({Direction::Output, AudioApi::OpenSLES, PerformanceMode::LowLatency, true}), + StreamReturnStopParams({Direction::Output, AudioApi::OpenSLES, PerformanceMode::LowLatency, false}), + StreamReturnStopParams({Direction::Input, AudioApi::AAudio, PerformanceMode::LowLatency, true}), + StreamReturnStopParams({Direction::Input, AudioApi::AAudio, PerformanceMode::LowLatency, false}) + ) + ); diff --git a/tests/testStreamFramesProcessed.cpp b/tests/testStreamFramesProcessed.cpp index 00bed2c37..352dc0d36 100644 --- a/tests/testStreamFramesProcessed.cpp +++ b/tests/testStreamFramesProcessed.cpp @@ -21,7 +21,7 @@ using namespace oboe; -class MyCallback : public AudioStreamDataCallback { +class FramesProcessedCallback : public AudioStreamDataCallback { public: DataCallbackResult onAudioReady(AudioStream *oboeStream, void *audioData, int32_t numFrames) override { return DataCallbackResult::Continue; @@ -53,7 +53,7 @@ TEST_P(StreamFramesProcessed, VerifyFramesProcessed) { const Direction direction = std::get<0>(GetParam()); const int32_t sampleRate = std::get<1>(GetParam()); - AudioStreamDataCallback *callback = new MyCallback(); + AudioStreamDataCallback *callback = new FramesProcessedCallback(); mBuilder.setDirection(direction) ->setFormat(AudioFormat::Float) ->setSampleRate(sampleRate)