APM: Make the GetStatistics call independent of the locks in APM

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 <saza@webrtc.org>
Commit-Queue: Per Åhgren <peah@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30131}

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<void(AudioProcessing::Config*)> 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<void>(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<void>(stats_message_passed);
+}
+
 }  // namespace webrtc

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<void(AudioProcessing::Config*)> mutator);
@@ -444,6 +449,25 @@ class AudioProcessingImpl : public AudioProcessing {
     std::unique_ptr<AudioBuffer> 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<AudioProcessingStats> stats_message_queue_;
+  } stats_reporter_;
+
   std::vector<float> aec_render_queue_buffer_ RTC_GUARDED_BY(crit_render_);
   std::vector<float> aec_capture_queue_buffer_ RTC_GUARDED_BY(crit_capture_);
 

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;

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<void>(apm_->GetStatistics(true /* has_remote_tracks */));
+      static_cast<void>(apm_->GetStatistics());
     }
   }
 
@@ -667,8 +667,7 @@ void ApmTest::ProcessDelayVerificationTest(int delay_ms,
       expected_median - rtc::dchecked_cast<int>(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) {

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;

modules/audio_processing/include/mock_audio_processing.h

@@ -129,7 +129,8 @@ class MockAudioProcessing : public ::testing::NiceMock<AudioProcessing> {
   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());
 };

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) << ", ";
   }