From cf4c872dbd4e45f194c81aee4a807b7ab6b85218 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Per=20=C3=85hgren?= Date: Mon, 30 Dec 2019 14:32:14 +0100 Subject: [PATCH] APM: Make the GetStatistics call independent of the locks in APM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This CL changes the GetStatistics call in the audio processing module (APM) to not aquire the render or capture locks in APM, while still being thread-safe. This change eliminates the risk of thread-priority inversion due to the GetStatistics call. Apart from the above the CL: -Corrects the GetStatistics to not be const (it was const even though it aquired locks). -Slightly changes the statistics reporting, so that the stats received may be older than the most recent stats reported. Bug: webrtc:11241 Change-Id: I00deb5507e004cbe6e4a19a8bad357491f86f4ab Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/163982 Reviewed-by: Sam Zackrisson Commit-Queue: Per Åhgren Cr-Commit-Position: refs/heads/master@{#30131} --- .../audio_processing/audio_processing_impl.cc | 65 ++++++++++++------- .../audio_processing/audio_processing_impl.h | 26 +++++++- .../audio_processing_impl_locking_unittest.cc | 2 +- .../audio_processing_unittest.cc | 45 ++++--------- .../include/audio_processing.h | 14 ++-- .../include/mock_audio_processing.h | 3 +- .../test/audio_processing_simulator.cc | 2 +- 7 files changed, 90 insertions(+), 67 deletions(-) diff --git a/modules/audio_processing/audio_processing_impl.cc b/modules/audio_processing/audio_processing_impl.cc index cffbfbede3..db9b789629 100644 --- a/modules/audio_processing/audio_processing_impl.cc +++ b/modules/audio_processing/audio_processing_impl.cc @@ -1449,6 +1449,25 @@ int AudioProcessingImpl::ProcessCaptureStreamLocked() { &level); } + // Compute echo-related stats. + if (submodules_.echo_controller) { + auto ec_metrics = submodules_.echo_controller->GetMetrics(); + capture_.stats.echo_return_loss = ec_metrics.echo_return_loss; + capture_.stats.echo_return_loss_enhancement = + ec_metrics.echo_return_loss_enhancement; + capture_.stats.delay_ms = ec_metrics.delay_ms; + } + if (config_.residual_echo_detector.enabled) { + RTC_DCHECK(submodules_.echo_detector); + auto ed_metrics = submodules_.echo_detector->GetMetrics(); + capture_.stats.residual_echo_likelihood = ed_metrics.echo_likelihood; + capture_.stats.residual_echo_likelihood_recent_max = + ed_metrics.echo_likelihood_recent_max; + } + + // Pass stats for reporting. + stats_reporter_.UpdateStatistics(capture_.stats); + capture_.was_stream_delay_set = false; return kNoError; } @@ -1726,30 +1745,6 @@ void AudioProcessingImpl::DetachPlayoutAudioGenerator() { // Delete audio generator, if one is attached. } -AudioProcessingStats AudioProcessingImpl::GetStatistics( - bool has_remote_tracks) const { - rtc::CritScope cs_capture(&crit_capture_); - if (!has_remote_tracks) { - return capture_.stats; - } - AudioProcessingStats stats = capture_.stats; - if (submodules_.echo_controller) { - auto ec_metrics = submodules_.echo_controller->GetMetrics(); - stats.echo_return_loss = ec_metrics.echo_return_loss; - stats.echo_return_loss_enhancement = - ec_metrics.echo_return_loss_enhancement; - stats.delay_ms = ec_metrics.delay_ms; - } - if (config_.residual_echo_detector.enabled) { - RTC_DCHECK(submodules_.echo_detector); - auto ed_metrics = submodules_.echo_detector->GetMetrics(); - stats.residual_echo_likelihood = ed_metrics.echo_likelihood; - stats.residual_echo_likelihood_recent_max = - ed_metrics.echo_likelihood_recent_max; - } - return stats; -} - void AudioProcessingImpl::MutateConfig( rtc::FunctionView mutator) { rtc::CritScope cs_render(&crit_render_); @@ -2120,4 +2115,26 @@ AudioProcessingImpl::ApmRenderState::ApmRenderState() = default; AudioProcessingImpl::ApmRenderState::~ApmRenderState() = default; +AudioProcessingImpl::ApmStatsReporter::ApmStatsReporter() + : stats_message_queue_(1) {} + +AudioProcessingImpl::ApmStatsReporter::~ApmStatsReporter() = default; + +AudioProcessingStats AudioProcessingImpl::ApmStatsReporter::GetStatistics() { + rtc::CritScope cs_stats(&crit_stats_); + bool new_stats_available = stats_message_queue_.Remove(&cached_stats_); + // If the message queue is full, return the cached stats. + static_cast(new_stats_available); + + return cached_stats_; +} + +void AudioProcessingImpl::ApmStatsReporter::UpdateStatistics( + const AudioProcessingStats& new_stats) { + AudioProcessingStats stats_to_queue = new_stats; + bool stats_message_passed = stats_message_queue_.Insert(&stats_to_queue); + // If the message queue is full, discard the new stats. + static_cast(stats_message_passed); +} + } // namespace webrtc diff --git a/modules/audio_processing/audio_processing_impl.h b/modules/audio_processing/audio_processing_impl.h index 29a3c8df4f..bcd115624d 100644 --- a/modules/audio_processing/audio_processing_impl.h +++ b/modules/audio_processing/audio_processing_impl.h @@ -118,7 +118,12 @@ class AudioProcessingImpl : public AudioProcessing { bool was_stream_delay_set() const override RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_capture_); - AudioProcessingStats GetStatistics(bool has_remote_tracks) const override; + AudioProcessingStats GetStatistics(bool has_remote_tracks) override { + return GetStatistics(); + } + AudioProcessingStats GetStatistics() override { + return stats_reporter_.GetStatistics(); + } // TODO(peah): Remove MutateConfig once the new API allows that. void MutateConfig(rtc::FunctionView mutator); @@ -444,6 +449,25 @@ class AudioProcessingImpl : public AudioProcessing { std::unique_ptr render_audio; } render_ RTC_GUARDED_BY(crit_render_); + // Class for statistics reporting. The class is thread-safe and no lock is + // needed when accessing it. + class ApmStatsReporter { + public: + ApmStatsReporter(); + ~ApmStatsReporter(); + + // Returns the most recently reported statistics. + AudioProcessingStats GetStatistics(); + + // Update the cached statistics. + void UpdateStatistics(const AudioProcessingStats& new_stats); + + private: + rtc::CriticalSection crit_stats_; + AudioProcessingStats cached_stats_ RTC_GUARDED_BY(crit_stats_); + SwapQueue stats_message_queue_; + } stats_reporter_; + std::vector aec_render_queue_buffer_ RTC_GUARDED_BY(crit_render_); std::vector aec_capture_queue_buffer_ RTC_GUARDED_BY(crit_capture_); diff --git a/modules/audio_processing/audio_processing_impl_locking_unittest.cc b/modules/audio_processing/audio_processing_impl_locking_unittest.cc index d09e979223..d9a8741bf8 100644 --- a/modules/audio_processing/audio_processing_impl_locking_unittest.cc +++ b/modules/audio_processing/audio_processing_impl_locking_unittest.cc @@ -584,7 +584,7 @@ void StatsProcessor::Process() { EXPECT_TRUE(apm_config.noise_suppression.enabled); // The below return value is not testable. - apm_->GetStatistics(/*has_remote_tracks=*/true); + apm_->GetStatistics(); } const float CaptureProcessor::kCaptureInputFloatLevel = 0.03125f; diff --git a/modules/audio_processing/audio_processing_unittest.cc b/modules/audio_processing/audio_processing_unittest.cc index 9355c11de1..9ba4ee7dfa 100644 --- a/modules/audio_processing/audio_processing_unittest.cc +++ b/modules/audio_processing/audio_processing_unittest.cc @@ -644,7 +644,7 @@ void ApmTest::ProcessDelayVerificationTest(int delay_ms, if (frame_count == 250) { // Discard the first delay metrics to avoid convergence effects. - static_cast(apm_->GetStatistics(true /* has_remote_tracks */)); + static_cast(apm_->GetStatistics()); } } @@ -667,8 +667,7 @@ void ApmTest::ProcessDelayVerificationTest(int delay_ms, expected_median - rtc::dchecked_cast(96 / samples_per_ms), delay_min, delay_max); // Verify delay metrics. - AudioProcessingStats stats = - apm_->GetStatistics(true /* has_remote_tracks */); + AudioProcessingStats stats = apm_->GetStatistics(); ASSERT_TRUE(stats.delay_median_ms.has_value()); int32_t median = *stats.delay_median_ms; EXPECT_GE(expected_median_high, median); @@ -1577,8 +1576,7 @@ TEST_F(ApmTest, Process) { analog_level = apm_->recommended_stream_analog_level(); analog_level_average += analog_level; - AudioProcessingStats stats = - apm_->GetStatistics(/*has_remote_tracks=*/false); + AudioProcessingStats stats = apm_->GetStatistics(); EXPECT_TRUE(stats.voice_detected); EXPECT_TRUE(stats.output_rms_dbfs); has_voice_count += *stats.voice_detected ? 1 : 0; @@ -1597,8 +1595,7 @@ TEST_F(ApmTest, Process) { const int kStatsAggregationFrameNum = 100; // 1 second. if (frame_count % kStatsAggregationFrameNum == 0) { // Get echo and delay metrics. - AudioProcessingStats stats = - apm_->GetStatistics(true /* has_remote_tracks */); + AudioProcessingStats stats = apm_->GetStatistics(); // Echo metrics. const float echo_return_loss = stats.echo_return_loss.value_or(-1.0f); @@ -2517,7 +2514,7 @@ TEST(MAYBE_ApmStatistics, AECEnabledTest) { } // Test statistics interface. - AudioProcessingStats stats = apm->GetStatistics(true); + AudioProcessingStats stats = apm->GetStatistics(); // We expect all statistics to be set and have a sensible value. ASSERT_TRUE(stats.residual_echo_likelihood); EXPECT_GE(*stats.residual_echo_likelihood, 0.0); @@ -2529,17 +2526,6 @@ TEST(MAYBE_ApmStatistics, AECEnabledTest) { EXPECT_NE(*stats.echo_return_loss, -100.0); ASSERT_TRUE(stats.echo_return_loss_enhancement); EXPECT_NE(*stats.echo_return_loss_enhancement, -100.0); - - // If there are no receive streams, we expect the stats not to be set. The - // 'false' argument signals to APM that no receive streams are currently - // active. In that situation the statistics would get stuck at their last - // calculated value (AEC and echo detection need at least one stream in each - // direction), so to avoid that, they should not be set by APM. - stats = apm->GetStatistics(false); - EXPECT_FALSE(stats.residual_echo_likelihood); - EXPECT_FALSE(stats.residual_echo_likelihood_recent_max); - EXPECT_FALSE(stats.echo_return_loss); - EXPECT_FALSE(stats.echo_return_loss_enhancement); } TEST(MAYBE_ApmStatistics, AECMEnabledTest) { @@ -2566,7 +2552,7 @@ TEST(MAYBE_ApmStatistics, AECMEnabledTest) { } // Test statistics interface. - AudioProcessingStats stats = apm->GetStatistics(true); + AudioProcessingStats stats = apm->GetStatistics(); // We expect only the residual echo detector statistics to be set and have a // sensible value. EXPECT_TRUE(stats.residual_echo_likelihood); @@ -2581,13 +2567,6 @@ TEST(MAYBE_ApmStatistics, AECMEnabledTest) { } EXPECT_FALSE(stats.echo_return_loss); EXPECT_FALSE(stats.echo_return_loss_enhancement); - - // If there are no receive streams, we expect the stats not to be set. - stats = apm->GetStatistics(false); - EXPECT_FALSE(stats.residual_echo_likelihood); - EXPECT_FALSE(stats.residual_echo_likelihood_recent_max); - EXPECT_FALSE(stats.echo_return_loss); - EXPECT_FALSE(stats.echo_return_loss_enhancement); } TEST(ApmStatistics, ReportOutputRmsDbfs) { @@ -2611,13 +2590,13 @@ TEST(ApmStatistics, ReportOutputRmsDbfs) { // If not enabled, no metric should be reported. EXPECT_EQ(apm->ProcessStream(&frame), 0); - EXPECT_FALSE(apm->GetStatistics(false).output_rms_dbfs); + EXPECT_FALSE(apm->GetStatistics().output_rms_dbfs); // If enabled, metrics should be reported. config.level_estimation.enabled = true; apm->ApplyConfig(config); EXPECT_EQ(apm->ProcessStream(&frame), 0); - auto stats = apm->GetStatistics(false); + auto stats = apm->GetStatistics(); EXPECT_TRUE(stats.output_rms_dbfs); EXPECT_GE(*stats.output_rms_dbfs, 0); @@ -2625,7 +2604,7 @@ TEST(ApmStatistics, ReportOutputRmsDbfs) { config.level_estimation.enabled = false; apm->ApplyConfig(config); EXPECT_EQ(apm->ProcessStream(&frame), 0); - EXPECT_FALSE(apm->GetStatistics(false).output_rms_dbfs); + EXPECT_FALSE(apm->GetStatistics().output_rms_dbfs); } TEST(ApmStatistics, ReportHasVoice) { @@ -2649,20 +2628,20 @@ TEST(ApmStatistics, ReportHasVoice) { // If not enabled, no metric should be reported. EXPECT_EQ(apm->ProcessStream(&frame), 0); - EXPECT_FALSE(apm->GetStatistics(false).voice_detected); + EXPECT_FALSE(apm->GetStatistics().voice_detected); // If enabled, metrics should be reported. config.voice_detection.enabled = true; apm->ApplyConfig(config); EXPECT_EQ(apm->ProcessStream(&frame), 0); - auto stats = apm->GetStatistics(false); + auto stats = apm->GetStatistics(); EXPECT_TRUE(stats.voice_detected); // If re-disabled, the value is again not reported. config.voice_detection.enabled = false; apm->ApplyConfig(config); EXPECT_EQ(apm->ProcessStream(&frame), 0); - EXPECT_FALSE(apm->GetStatistics(false).voice_detected); + EXPECT_FALSE(apm->GetStatistics().voice_detected); } TEST(ApmConfiguration, HandlingOfRateAndChannelCombinations) { diff --git a/modules/audio_processing/include/audio_processing.h b/modules/audio_processing/include/audio_processing.h index 3d278cde63..c7fc1c43aa 100644 --- a/modules/audio_processing/include/audio_processing.h +++ b/modules/audio_processing/include/audio_processing.h @@ -631,12 +631,14 @@ class RTC_EXPORT AudioProcessing : public rtc::RefCountInterface { // TODO(peah): Remove this method. virtual void UpdateHistogramsOnCallEnd() = 0; - // Get audio processing statistics. The |has_remote_tracks| argument should be - // set if there are active remote tracks (this would usually be true during - // a call). If there are no remote tracks some of the stats will not be set by - // AudioProcessing, because they only make sense if there is at least one - // remote track. - virtual AudioProcessingStats GetStatistics(bool has_remote_tracks) const = 0; + // Get audio processing statistics. + virtual AudioProcessingStats GetStatistics() = 0; + // TODO(webrtc:5298) Deprecated variant. The |has_remote_tracks| argument + // should be set if there are active remote tracks (this would usually be true + // during a call). If there are no remote tracks some of the stats will not be + // set by AudioProcessing, because they only make sense if there is at least + // one remote track. + virtual AudioProcessingStats GetStatistics(bool has_remote_tracks) = 0; // Returns the last applied configuration. virtual AudioProcessing::Config GetConfig() const = 0; diff --git a/modules/audio_processing/include/mock_audio_processing.h b/modules/audio_processing/include/mock_audio_processing.h index 093269605d..b36013a127 100644 --- a/modules/audio_processing/include/mock_audio_processing.h +++ b/modules/audio_processing/include/mock_audio_processing.h @@ -129,7 +129,8 @@ class MockAudioProcessing : public ::testing::NiceMock { MOCK_METHOD0(DetachPlayoutAudioGenerator, void()); MOCK_METHOD0(UpdateHistogramsOnCallEnd, void()); - MOCK_CONST_METHOD1(GetStatistics, AudioProcessingStats(bool)); + MOCK_METHOD0(GetStatistics, AudioProcessingStats()); + MOCK_METHOD1(GetStatistics, AudioProcessingStats(bool)); MOCK_CONST_METHOD0(GetConfig, AudioProcessing::Config()); }; diff --git a/modules/audio_processing/test/audio_processing_simulator.cc b/modules/audio_processing/test/audio_processing_simulator.cc index 02e0867641..1be7f872a3 100644 --- a/modules/audio_processing/test/audio_processing_simulator.cc +++ b/modules/audio_processing/test/audio_processing_simulator.cc @@ -241,7 +241,7 @@ void AudioProcessingSimulator::ProcessStream(bool fixed_interface) { } if (residual_echo_likelihood_graph_writer_.is_open()) { - auto stats = ap_->GetStatistics(true /*has_remote_tracks*/); + auto stats = ap_->GetStatistics(); residual_echo_likelihood_graph_writer_ << stats.residual_echo_likelihood.value_or(-1.f) << ", "; }