From 2af35ab984e3d62ff7abb215dc293fc082de19f6 Mon Sep 17 00:00:00 2001 From: Markus Handell Date: Mon, 18 May 2020 17:35:44 +0200 Subject: [PATCH] FakeAudioCaptureModule: remove lock recursions. This change removes lock recursions and adds thread annotations. The module had incorrect locking WRT the callback critical section: ProcessFrameP: locks crit_ ReceiveFrameP: locks crit_callback_ ------------- SendFrameP: locks crit_callback_ MicrophoneVolume: locks crit_ Lock crit_callback_ was rolled in under crit_ instead. Bug: webrtc:11567 Change-Id: I974fe91d44de0ddf1a1287fe91db9dfe63a61af9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175662 Reviewed-by: Stefan Holmer Commit-Queue: Markus Handell Cr-Commit-Position: refs/heads/master@{#31313} --- pc/BUILD.gn | 1 + pc/test/fake_audio_capture_module.cc | 72 ++++++++++++++-------------- pc/test/fake_audio_capture_module.h | 53 ++++++++++---------- 3 files changed, 66 insertions(+), 60 deletions(-) diff --git a/pc/BUILD.gn b/pc/BUILD.gn index f553f158d8..c5223b10c4 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -499,6 +499,7 @@ if (rtc_include_tests) { "../rtc_base:rtc_base_approved", "../rtc_base:rtc_task_queue", "../rtc_base:task_queue_for_test", + "../rtc_base/synchronization:sequence_checker", "../rtc_base/task_utils:repeating_task", "../rtc_base/third_party/sigslot", "../test:test_support", diff --git a/pc/test/fake_audio_capture_module.cc b/pc/test/fake_audio_capture_module.cc index db0886ddad..1a7efd4ad1 100644 --- a/pc/test/fake_audio_capture_module.cc +++ b/pc/test/fake_audio_capture_module.cc @@ -47,7 +47,9 @@ FakeAudioCaptureModule::FakeAudioCaptureModule() current_mic_level_(kMaxVolume), started_(false), next_frame_time_(0), - frames_received_(0) {} + frames_received_(0) { + process_thread_checker_.Detach(); +} FakeAudioCaptureModule::~FakeAudioCaptureModule() { if (process_thread_) { @@ -77,7 +79,7 @@ int32_t FakeAudioCaptureModule::ActiveAudioLayer( int32_t FakeAudioCaptureModule::RegisterAudioCallback( webrtc::AudioTransport* audio_callback) { - rtc::CritScope cs(&crit_callback_); + rtc::CritScope cs(&crit_); audio_callback_ = audio_callback; return 0; } @@ -448,29 +450,34 @@ void FakeAudioCaptureModule::UpdateProcessing(bool start) { if (process_thread_) { process_thread_->Stop(); process_thread_.reset(nullptr); + process_thread_checker_.Detach(); } + rtc::CritScope lock(&crit_); started_ = false; } } void FakeAudioCaptureModule::StartProcessP() { - RTC_CHECK(process_thread_->IsCurrent()); - if (started_) { - // Already started. - return; + RTC_DCHECK_RUN_ON(&process_thread_checker_); + { + rtc::CritScope lock(&crit_); + if (started_) { + // Already started. + return; + } } ProcessFrameP(); } void FakeAudioCaptureModule::ProcessFrameP() { - RTC_CHECK(process_thread_->IsCurrent()); - if (!started_) { - next_frame_time_ = rtc::TimeMillis(); - started_ = true; - } - + RTC_DCHECK_RUN_ON(&process_thread_checker_); { rtc::CritScope cs(&crit_); + if (!started_) { + next_frame_time_ = rtc::TimeMillis(); + started_ = true; + } + // Receive and send frames every kTimePerFrameMs. if (playing_) { ReceiveFrameP(); @@ -488,24 +495,22 @@ void FakeAudioCaptureModule::ProcessFrameP() { } void FakeAudioCaptureModule::ReceiveFrameP() { - RTC_CHECK(process_thread_->IsCurrent()); - { - rtc::CritScope cs(&crit_callback_); - if (!audio_callback_) { - return; - } - ResetRecBuffer(); - size_t nSamplesOut = 0; - int64_t elapsed_time_ms = 0; - int64_t ntp_time_ms = 0; - if (audio_callback_->NeedMorePlayData( - kNumberSamples, kNumberBytesPerSample, kNumberOfChannels, - kSamplesPerSecond, rec_buffer_, nSamplesOut, &elapsed_time_ms, - &ntp_time_ms) != 0) { - RTC_NOTREACHED(); - } - RTC_CHECK(nSamplesOut == kNumberSamples); + RTC_DCHECK_RUN_ON(&process_thread_checker_); + if (!audio_callback_) { + return; } + ResetRecBuffer(); + size_t nSamplesOut = 0; + int64_t elapsed_time_ms = 0; + int64_t ntp_time_ms = 0; + if (audio_callback_->NeedMorePlayData(kNumberSamples, kNumberBytesPerSample, + kNumberOfChannels, kSamplesPerSecond, + rec_buffer_, nSamplesOut, + &elapsed_time_ms, &ntp_time_ms) != 0) { + RTC_NOTREACHED(); + } + RTC_CHECK(nSamplesOut == kNumberSamples); + // The SetBuffer() function ensures that after decoding, the audio buffer // should contain samples of similar magnitude (there is likely to be some // distortion due to the audio pipeline). If one sample is detected to @@ -513,25 +518,22 @@ void FakeAudioCaptureModule::ReceiveFrameP() { // has been received from the remote side (i.e. faked frames are not being // pulled). if (CheckRecBuffer(kHighSampleValue)) { - rtc::CritScope cs(&crit_); ++frames_received_; } } void FakeAudioCaptureModule::SendFrameP() { - RTC_CHECK(process_thread_->IsCurrent()); - rtc::CritScope cs(&crit_callback_); + RTC_DCHECK_RUN_ON(&process_thread_checker_); if (!audio_callback_) { return; } bool key_pressed = false; - uint32_t current_mic_level = 0; - MicrophoneVolume(¤t_mic_level); + uint32_t current_mic_level = current_mic_level_; if (audio_callback_->RecordedDataIsAvailable( send_buffer_, kNumberSamples, kNumberBytesPerSample, kNumberOfChannels, kSamplesPerSecond, kTotalDelayMs, kClockDriftMs, current_mic_level, key_pressed, current_mic_level) != 0) { RTC_NOTREACHED(); } - SetMicrophoneVolume(current_mic_level); + current_mic_level_ = current_mic_level; } diff --git a/pc/test/fake_audio_capture_module.h b/pc/test/fake_audio_capture_module.h index 0af3810290..498b6daf61 100644 --- a/pc/test/fake_audio_capture_module.h +++ b/pc/test/fake_audio_capture_module.h @@ -26,6 +26,7 @@ #include "modules/audio_device/include/audio_device.h" #include "rtc_base/critical_section.h" #include "rtc_base/message_handler.h" +#include "rtc_base/synchronization/sequence_checker.h" namespace rtc { class Thread; @@ -47,13 +48,13 @@ class FakeAudioCaptureModule : public webrtc::AudioDeviceModule, // Returns the number of frames that have been successfully pulled by the // instance. Note that correctly detecting success can only be done if the // pulled frame was generated/pushed from a FakeAudioCaptureModule. - int frames_received() const; + int frames_received() const RTC_LOCKS_EXCLUDED(crit_); int32_t ActiveAudioLayer(AudioLayer* audio_layer) const override; // Note: Calling this method from a callback may result in deadlock. - int32_t RegisterAudioCallback( - webrtc::AudioTransport* audio_callback) override; + int32_t RegisterAudioCallback(webrtc::AudioTransport* audio_callback) override + RTC_LOCKS_EXCLUDED(crit_); int32_t Init() override; int32_t Terminate() override; @@ -80,12 +81,12 @@ class FakeAudioCaptureModule : public webrtc::AudioDeviceModule, int32_t InitRecording() override; bool RecordingIsInitialized() const override; - int32_t StartPlayout() override; - int32_t StopPlayout() override; - bool Playing() const override; - int32_t StartRecording() override; - int32_t StopRecording() override; - bool Recording() const override; + int32_t StartPlayout() RTC_LOCKS_EXCLUDED(crit_) override; + int32_t StopPlayout() RTC_LOCKS_EXCLUDED(crit_) override; + bool Playing() const RTC_LOCKS_EXCLUDED(crit_) override; + int32_t StartRecording() RTC_LOCKS_EXCLUDED(crit_) override; + int32_t StopRecording() RTC_LOCKS_EXCLUDED(crit_) override; + bool Recording() const RTC_LOCKS_EXCLUDED(crit_) override; int32_t InitSpeaker() override; bool SpeakerIsInitialized() const override; @@ -99,8 +100,10 @@ class FakeAudioCaptureModule : public webrtc::AudioDeviceModule, int32_t MinSpeakerVolume(uint32_t* min_volume) const override; int32_t MicrophoneVolumeIsAvailable(bool* available) override; - int32_t SetMicrophoneVolume(uint32_t volume) override; - int32_t MicrophoneVolume(uint32_t* volume) const override; + int32_t SetMicrophoneVolume(uint32_t volume) + RTC_LOCKS_EXCLUDED(crit_) override; + int32_t MicrophoneVolume(uint32_t* volume) const + RTC_LOCKS_EXCLUDED(crit_) override; int32_t MaxMicrophoneVolume(uint32_t* max_volume) const override; int32_t MinMicrophoneVolume(uint32_t* min_volume) const override; @@ -170,26 +173,28 @@ class FakeAudioCaptureModule : public webrtc::AudioDeviceModule, // Returns true/false depending on if recording or playback has been // enabled/started. - bool ShouldStartProcessing(); + bool ShouldStartProcessing() RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_); // Starts or stops the pushing and pulling of audio frames. - void UpdateProcessing(bool start); + void UpdateProcessing(bool start) RTC_LOCKS_EXCLUDED(crit_); // Starts the periodic calling of ProcessFrame() in a thread safe way. void StartProcessP(); // Periodcally called function that ensures that frames are pulled and pushed // periodically if enabled/started. - void ProcessFrameP(); + void ProcessFrameP() RTC_LOCKS_EXCLUDED(crit_); // Pulls frames from the registered webrtc::AudioTransport. - void ReceiveFrameP(); + void ReceiveFrameP() RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_); // Pushes frames to the registered webrtc::AudioTransport. - void SendFrameP(); + void SendFrameP() RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_); // Callback for playout and recording. - webrtc::AudioTransport* audio_callback_; + webrtc::AudioTransport* audio_callback_ RTC_GUARDED_BY(crit_); - bool recording_; // True when audio is being pushed from the instance. - bool playing_; // True when audio is being pulled by the instance. + bool recording_ RTC_GUARDED_BY( + crit_); // True when audio is being pushed from the instance. + bool playing_ RTC_GUARDED_BY( + crit_); // True when audio is being pulled by the instance. bool play_is_initialized_; // True when the instance is ready to pull audio. bool rec_is_initialized_; // True when the instance is ready to push audio. @@ -197,13 +202,13 @@ class FakeAudioCaptureModule : public webrtc::AudioDeviceModule, // Input to and output from RecordedDataIsAvailable(..) makes it possible to // modify the current mic level. The implementation does not care about the // mic level so it just feeds back what it receives. - uint32_t current_mic_level_; + uint32_t current_mic_level_ RTC_GUARDED_BY(crit_); // next_frame_time_ is updated in a non-drifting manner to indicate the next // wall clock time the next frame should be generated and received. started_ // ensures that next_frame_time_ can be initialized properly on first call. - bool started_; - int64_t next_frame_time_; + bool started_ RTC_GUARDED_BY(crit_); + int64_t next_frame_time_ RTC_GUARDED_BY(process_thread_checker_); std::unique_ptr process_thread_; @@ -220,9 +225,7 @@ class FakeAudioCaptureModule : public webrtc::AudioDeviceModule, // Protects variables that are accessed from process_thread_ and // the main thread. rtc::CriticalSection crit_; - // Protects |audio_callback_| that is accessed from process_thread_ and - // the main thread. - rtc::CriticalSection crit_callback_; + webrtc::SequenceChecker process_thread_checker_; }; #endif // PC_TEST_FAKE_AUDIO_CAPTURE_MODULE_H_