From 36b3179312a8aec0ccf02d1eecf60ef1fc4039c6 Mon Sep 17 00:00:00 2001 From: henrika Date: Thu, 13 Sep 2018 13:01:14 +0200 Subject: [PATCH] Removes flaky thread checker in AudioDeviceBuffer. This CL removes a set of DCHECKs in AudioDeviceBuffer (ADB) where the goal has been to ensure that some methods are called on one and the same native I/O thread. The implementation of the ADB is platform independent but the underlying (driving) audio components differ between platforms. This combination has shown to generate complex corner cases such as: - OS dependent I/O-thread(s) changes while audio is active - OS dependent audio device changes and it leads to restart of native I/O threads - Start/Stop of audio has different timing depending on platform and possibly also usage of JNI and/or emulators. To summarize: the gain of maintaining the current strict thread checking (in Debug mode) is not worth all the efforts trying to resolve complex dynamic cases where the native I/O threads changes ID. TBR=glaznev Bug: b/115385789 Change-Id: I681c89adec497a18b97d2a40421c04ea218fd919 Reviewed-on: https://webrtc-review.googlesource.com/100200 Commit-Queue: Henrik Andreassson Reviewed-by: Henrik Andreassson Cr-Commit-Position: refs/heads/master@{#24723} --- modules/audio_device/android/aaudio_player.cc | 1 - .../audio_device/android/aaudio_recorder.cc | 1 - modules/audio_device/audio_device_buffer.cc | 22 -------------- modules/audio_device/audio_device_buffer.h | 29 +++++-------------- modules/audio_device/ios/audio_device_ios.mm | 6 ---- .../audio_device/win/core_audio_base_win.cc | 2 -- .../src/jni/audio_device/aaudio_player.cc | 1 - .../src/jni/audio_device/aaudio_recorder.cc | 1 - sdk/objc/native/src/audio/audio_device_ios.mm | 6 ---- 9 files changed, 8 insertions(+), 61 deletions(-) diff --git a/modules/audio_device/android/aaudio_player.cc b/modules/audio_device/android/aaudio_player.cc index e576a787fa..d4b83725f7 100644 --- a/modules/audio_device/android/aaudio_player.cc +++ b/modules/audio_device/android/aaudio_player.cc @@ -219,7 +219,6 @@ void AAudioPlayer::HandleStreamDisconnected() { } // Perform a restart by first closing the disconnected stream and then start // a new stream; this time using the new (preferred) audio output device. - audio_device_buffer_->NativeAudioPlayoutInterrupted(); StopPlayout(); InitPlayout(); StartPlayout(); diff --git a/modules/audio_device/android/aaudio_recorder.cc b/modules/audio_device/android/aaudio_recorder.cc index 081dda4c64..7eb7b1c59e 100644 --- a/modules/audio_device/android/aaudio_recorder.cc +++ b/modules/audio_device/android/aaudio_recorder.cc @@ -212,7 +212,6 @@ void AAudioRecorder::HandleStreamDisconnected() { // TODO(henrika): resolve issue where a one restart attempt leads to a long // sequence of new calls to OnErrorCallback(). // See b/73148976 for details. - audio_device_buffer_->NativeAudioRecordingInterrupted(); StopRecording(); InitRecording(); StartRecording(); diff --git a/modules/audio_device/audio_device_buffer.cc b/modules/audio_device/audio_device_buffer.cc index e6c2fa2a6b..4462fb3548 100644 --- a/modules/audio_device/audio_device_buffer.cc +++ b/modules/audio_device/audio_device_buffer.cc @@ -67,8 +67,6 @@ AudioDeviceBuffer::AudioDeviceBuffer() RTC_LOG(WARNING) << "AUDIO_DEVICE_PLAYS_SINUS_TONE is defined!"; #endif WebRtcSpl_Init(); - playout_thread_checker_.DetachFromThread(); - recording_thread_checker_.DetachFromThread(); } AudioDeviceBuffer::~AudioDeviceBuffer() { @@ -99,7 +97,6 @@ void AudioDeviceBuffer::StartPlayout() { return; } RTC_LOG(INFO) << __FUNCTION__; - playout_thread_checker_.DetachFromThread(); // Clear members tracking playout stats and do it on the task queue. task_queue_.PostTask([this] { ResetPlayStats(); }); // Start a periodic timer based on task queue if not already done by the @@ -119,7 +116,6 @@ void AudioDeviceBuffer::StartRecording() { return; } RTC_LOG(INFO) << __FUNCTION__; - recording_thread_checker_.DetachFromThread(); // Clear members tracking recording stats and do it on the task queue. task_queue_.PostTask([this] { ResetRecStats(); }); // Start a periodic timer based on task queue if not already done by the @@ -222,30 +218,17 @@ size_t AudioDeviceBuffer::PlayoutChannels() const { } int32_t AudioDeviceBuffer::SetTypingStatus(bool typing_status) { - RTC_DCHECK_RUN_ON(&recording_thread_checker_); typing_status_ = typing_status; return 0; } -void AudioDeviceBuffer::NativeAudioPlayoutInterrupted() { - RTC_DCHECK(main_thread_checker_.CalledOnValidThread()); - playout_thread_checker_.DetachFromThread(); -} - -void AudioDeviceBuffer::NativeAudioRecordingInterrupted() { - RTC_DCHECK(main_thread_checker_.CalledOnValidThread()); - recording_thread_checker_.DetachFromThread(); -} - void AudioDeviceBuffer::SetVQEData(int play_delay_ms, int rec_delay_ms) { - RTC_DCHECK_RUN_ON(&recording_thread_checker_); play_delay_ms_ = play_delay_ms; rec_delay_ms_ = rec_delay_ms; } int32_t AudioDeviceBuffer::SetRecordedBuffer(const void* audio_buffer, size_t samples_per_channel) { - RTC_DCHECK_RUN_ON(&recording_thread_checker_); // Copy the complete input buffer to the local buffer. const size_t old_size = rec_buffer_.size(); rec_buffer_.SetData(static_cast(audio_buffer), @@ -277,7 +260,6 @@ int32_t AudioDeviceBuffer::SetRecordedBuffer(const void* audio_buffer, } int32_t AudioDeviceBuffer::DeliverRecordedData() { - RTC_DCHECK_RUN_ON(&recording_thread_checker_); if (!audio_transport_cb_) { RTC_LOG(LS_WARNING) << "Invalid audio transport"; return 0; @@ -297,7 +279,6 @@ int32_t AudioDeviceBuffer::DeliverRecordedData() { } int32_t AudioDeviceBuffer::RequestPlayoutData(size_t samples_per_channel) { - RTC_DCHECK_RUN_ON(&playout_thread_checker_); // The consumer can change the requested size on the fly and we therefore // resize the buffer accordingly. Also takes place at the first call to this // method. @@ -342,7 +323,6 @@ int32_t AudioDeviceBuffer::RequestPlayoutData(size_t samples_per_channel) { } int32_t AudioDeviceBuffer::GetPlayoutData(void* audio_buffer) { - RTC_DCHECK_RUN_ON(&playout_thread_checker_); RTC_DCHECK_GT(play_buffer_.size(), 0); #ifdef AUDIO_DEVICE_PLAYS_SINUS_TONE const double phase_increment = @@ -484,7 +464,6 @@ void AudioDeviceBuffer::ResetPlayStats() { void AudioDeviceBuffer::UpdateRecStats(int16_t max_abs, size_t samples_per_channel) { - RTC_DCHECK_RUN_ON(&recording_thread_checker_); rtc::CritScope cs(&lock_); ++stats_.rec_callbacks; stats_.rec_samples += samples_per_channel; @@ -495,7 +474,6 @@ void AudioDeviceBuffer::UpdateRecStats(int16_t max_abs, void AudioDeviceBuffer::UpdatePlayStats(int16_t max_abs, size_t samples_per_channel) { - RTC_DCHECK_RUN_ON(&playout_thread_checker_); rtc::CritScope cs(&lock_); ++stats_.play_callbacks; stats_.play_samples += samples_per_channel; diff --git a/modules/audio_device/audio_device_buffer.h b/modules/audio_device/audio_device_buffer.h index 3658be04e4..7f0cf83baf 100644 --- a/modules/audio_device/audio_device_buffer.h +++ b/modules/audio_device/audio_device_buffer.h @@ -104,13 +104,6 @@ class AudioDeviceBuffer { int32_t SetTypingStatus(bool typing_status); - // Called on iOS and Android where the native audio layer can be interrupted - // by other audio applications. These methods can then be used to reset - // internal states and detach thread checkers to allow for new audio sessions - // where native callbacks may come from a new set of I/O threads. - void NativeAudioPlayoutInterrupted(); - void NativeAudioRecordingInterrupted(); - private: // Starts/stops periodic logging of audio stats. void StartPeriodicLogging(); @@ -145,12 +138,6 @@ class AudioDeviceBuffer { // Main thread on which this object is created. rtc::ThreadChecker main_thread_checker_; - // Native (platform specific) audio thread driving the playout side. - rtc::ThreadChecker playout_thread_checker_; - - // Native (platform specific) audio thread driving the recording side. - rtc::ThreadChecker recording_thread_checker_; - rtc::CriticalSection lock_; // Task queue used to invoke LogStats() periodically. Tasks are executed on a @@ -183,18 +170,18 @@ class AudioDeviceBuffer { // Buffer used for audio samples to be played out. Size can be changed // dynamically. The 16-bit samples are interleaved, hence the size is // proportional to the number of channels. - rtc::BufferT play_buffer_ RTC_GUARDED_BY(playout_thread_checker_); + rtc::BufferT play_buffer_; // Byte buffer used for recorded audio samples. Size can be changed // dynamically. - rtc::BufferT rec_buffer_ RTC_GUARDED_BY(recording_thread_checker_); + rtc::BufferT rec_buffer_; // Contains true of a key-press has been detected. - bool typing_status_ RTC_GUARDED_BY(recording_thread_checker_); + bool typing_status_; // Delay values used by the AEC. - int play_delay_ms_ RTC_GUARDED_BY(recording_thread_checker_); - int rec_delay_ms_ RTC_GUARDED_BY(recording_thread_checker_); + int play_delay_ms_; + int rec_delay_ms_; // Counts number of times LogStats() has been called. size_t num_stat_reports_ RTC_GUARDED_BY(task_queue_); @@ -204,8 +191,8 @@ class AudioDeviceBuffer { // Counts number of audio callbacks modulo 50 to create a signal when // a new storage of audio stats shall be done. - int16_t rec_stat_count_ RTC_GUARDED_BY(recording_thread_checker_); - int16_t play_stat_count_ RTC_GUARDED_BY(playout_thread_checker_); + int16_t rec_stat_count_; + int16_t play_stat_count_; // Time stamps of when playout and recording starts. int64_t play_start_time_ RTC_GUARDED_BY(main_thread_checker_); @@ -231,7 +218,7 @@ class AudioDeviceBuffer { // Should *never* be defined in production builds. Only used for testing. // When defined, the output signal will be replaced by a sinus tone at 440Hz. #ifdef AUDIO_DEVICE_PLAYS_SINUS_TONE - double phase_ RTC_GUARDED_BY(playout_thread_checker_); + double phase_; #endif }; diff --git a/modules/audio_device/ios/audio_device_ios.mm b/modules/audio_device/ios/audio_device_ios.mm index 9ca664d119..1aaff1f90c 100644 --- a/modules/audio_device/ios/audio_device_ios.mm +++ b/modules/audio_device/ios/audio_device_ios.mm @@ -903,12 +903,6 @@ void AudioDeviceIOS::PrepareForNewStart() { // which means that we must detach thread checkers here to be prepared for an // upcoming new audio stream. io_thread_checker_.DetachFromThread(); - // The audio device buffer must also be informed about the interrupted - // state so it can detach its thread checkers as well. - if (audio_device_buffer_) { - audio_device_buffer_->NativeAudioPlayoutInterrupted(); - audio_device_buffer_->NativeAudioRecordingInterrupted(); - } } } // namespace webrtc diff --git a/modules/audio_device/win/core_audio_base_win.cc b/modules/audio_device/win/core_audio_base_win.cc index a0164e27cb..2e6e9fa598 100644 --- a/modules/audio_device/win/core_audio_base_win.cc +++ b/modules/audio_device/win/core_audio_base_win.cc @@ -582,8 +582,6 @@ bool CoreAudioBase::Stop() { // error callbacks. if (!IsRestarting()) { thread_checker_audio_.DetachFromThread(); - IsOutput() ? audio_device_buffer_->NativeAudioPlayoutInterrupted() - : audio_device_buffer_->NativeAudioRecordingInterrupted(); } // Release all allocated COM interfaces to allow for a restart without diff --git a/sdk/android/src/jni/audio_device/aaudio_player.cc b/sdk/android/src/jni/audio_device/aaudio_player.cc index ac45e900d8..18e7691d8b 100644 --- a/sdk/android/src/jni/audio_device/aaudio_player.cc +++ b/sdk/android/src/jni/audio_device/aaudio_player.cc @@ -235,7 +235,6 @@ void AAudioPlayer::HandleStreamDisconnected() { } // Perform a restart by first closing the disconnected stream and then start // a new stream; this time using the new (preferred) audio output device. - audio_device_buffer_->NativeAudioPlayoutInterrupted(); StopPlayout(); InitPlayout(); StartPlayout(); diff --git a/sdk/android/src/jni/audio_device/aaudio_recorder.cc b/sdk/android/src/jni/audio_device/aaudio_recorder.cc index 5ba9d3a186..8134d3c0a2 100644 --- a/sdk/android/src/jni/audio_device/aaudio_recorder.cc +++ b/sdk/android/src/jni/audio_device/aaudio_recorder.cc @@ -224,7 +224,6 @@ void AAudioRecorder::HandleStreamDisconnected() { // TODO(henrika): resolve issue where a one restart attempt leads to a long // sequence of new calls to OnErrorCallback(). // See b/73148976 for details. - audio_device_buffer_->NativeAudioRecordingInterrupted(); StopRecording(); InitRecording(); StartRecording(); diff --git a/sdk/objc/native/src/audio/audio_device_ios.mm b/sdk/objc/native/src/audio/audio_device_ios.mm index cd29cf5ba6..ec9a7989e8 100644 --- a/sdk/objc/native/src/audio/audio_device_ios.mm +++ b/sdk/objc/native/src/audio/audio_device_ios.mm @@ -915,12 +915,6 @@ void AudioDeviceIOS::PrepareForNewStart() { // which means that we must detach thread checkers here to be prepared for an // upcoming new audio stream. io_thread_checker_.DetachFromThread(); - // The audio device buffer must also be informed about the interrupted - // state so it can detach its thread checkers as well. - if (audio_device_buffer_) { - audio_device_buffer_->NativeAudioPlayoutInterrupted(); - audio_device_buffer_->NativeAudioRecordingInterrupted(); - } } bool AudioDeviceIOS::IsInterrupted() {