Use the new interface to get APM stats

content/renderer/media/media_stream_audio_processor_options.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/media/media_stream_audio_processor_options.h"
 
 #include <stddef.h>
 #include <utility>
 
 #include "base/files/file_path.h"
@@ skipping 462 matching lines @@
 #if defined(OS_ANDROID)
   const webrtc::GainControl::Mode mode = webrtc::GainControl::kFixedDigital;
 #else
   const webrtc::GainControl::Mode mode = webrtc::GainControl::kAdaptiveAnalog;
 #endif
   int err = audio_processing->gain_control()->set_mode(mode);
   err |= audio_processing->gain_control()->Enable(true);
   CHECK_EQ(err, 0);
 }
 
-void GetAecStats(webrtc::EchoCancellation* echo_cancellation,
-                 webrtc::AudioProcessorInterface::AudioProcessorStats* stats) {
-  // These values can take on valid negative values, so use the lowest possible
-  // level as default rather than -1.
-  stats->echo_return_loss = -100;
-  stats->echo_return_loss_enhancement = -100;
+void GetAudioProcessingStats(
+    AudioProcessing* audio_processing,
+    webrtc::AudioProcessorInterface::AudioProcessorStats* stats) {
+  auto apm_stats = audio_processing->GetStatistics();
+  stats->echo_return_loss = apm_stats.echo_return_loss.instant();
+  stats->echo_return_loss_enhancement =
+      apm_stats.echo_return_loss_enhancement.instant();

[hbos_chromium, 2016/11/25 10:04:05]

The new stats collector is only concerned with echoReturnLoss and
echoReturnLossEnhancement out of all audio statistics:
https://w3c.github.io/webrtc-stats/#dom-rtcmediastreamtrackstats-echoreturnloss

These are calculated in decibels. Are these values using the same units?

[Ivo-OOO until feb 6, 2016/11/25 10:40:44]

These are in exactly the same units, in fact the
audio_processing->GetStatistics() function calls the old functions internally. 

The reason for this change is that we want to get rid of direct pointers to the
AEC outside of APM, since it makes the APM/AEC locking scheme more complicated
than it needs to be.

[hbos_chromium, 2016/11/25 12:51:39]

Acknowledged.

+  stats->aec_divergent_filter_fraction = apm_stats.divergent_filter_fraction;
 
-  // The median value can also be negative, but in practice -1 is only used to
-  // signal insufficient data, since the resolution is limited to multiples
-  // of 4ms.
-  stats->echo_delay_median_ms = -1;
-  stats->echo_delay_std_ms = -1;
+  stats->echo_delay_median_ms = apm_stats.delay_median;
+  stats->echo_delay_std_ms = apm_stats.delay_standard_deviation;
 
-  // TODO(ajm): Re-enable this metric once we have a reliable implementation.
-  stats->aec_quality_min = -1.0f;

[hbos_chromium, 2016/11/25 10:04:05]

I think that defaulting values to negative, whether here or in [1], is
problematic. If webrtc::AudioProcessorInterface::AudioProcessorStats's stats can
be optional, we should change them to rtc::Optional.

The old stats collector (statscollector.h) just copies values blindly. The new
stats collector (rtcstatscollector.h) supports leaving stats values undefined.
But to do so it needs to know if a value is undefined, and Optional is better
than some arbitrary/undocumented negative value.

Could you do a follow-up WebRTC CL that indicate that optional statistics are
optional? (Might be messy rolling into chromium)

Alternatively, we could make "void MediaStreamAudioProcessor::GetStats"
(overriding webrtc::AudioProcessor::GetStats) return false if stats are not
available, if it's an all-or-nothing OP. Is it?

Or, if undefined values are unexpected, DCHECK.

[1]
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_process...

[Ivo-OOO until feb 6, 2016/11/25 10:40:44]

I agree that this way of using default values is not very nice, and using
rtc::Optional sounds much better. I can do that in a follow-up CL (not sure if I
have time for it right away). 

The reason the stats can be undefined is that the AEC can be disabled, so in
that case an Optional is indeed a much nicer way to signal that.

[hbos_chromium, 2016/11/25 12:51:39]

That's fine, add a TODO comment? :)

[Ivo-OOO until feb 6, 2016/11/25 13:22:19]

Done.

-
-  if (!echo_cancellation->are_metrics_enabled() ||
-      !echo_cancellation->is_delay_logging_enabled() ||
-      !echo_cancellation->is_enabled()) {
-    return;
-  }
-
-  // TODO(ajm): we may want to use VoECallReport::GetEchoMetricsSummary
-  // here, but it appears to be unsuitable currently. Revisit after this is
-  // investigated: http://b/issue?id=5666755
-  webrtc::EchoCancellation::Metrics echo_metrics;
-  if (!echo_cancellation->GetMetrics(&echo_metrics)) {
-    stats->echo_return_loss = echo_metrics.echo_return_loss.instant;
-    stats->echo_return_loss_enhancement =
-        echo_metrics.echo_return_loss_enhancement.instant;
-    stats->aec_divergent_filter_fraction =
-        echo_metrics.divergent_filter_fraction;
-  }
-
-  int median = 0, std = 0;
-  float dummy = 0;
-  if (echo_cancellation->GetDelayMetrics(&median, &std, &dummy) ==
-      webrtc::AudioProcessing::kNoError) {
-    stats->echo_delay_median_ms = median;
-    stats->echo_delay_std_ms = std;
-  }
+  stats->residual_echo_likelihood = apm_stats.residual_echo_likelihood;
 }
 
 std::vector<webrtc::Point> GetArrayGeometryPreferringConstraints(
     const MediaAudioConstraints& audio_constraints,
     const MediaStreamDevice::AudioDeviceParameters& input_params) {
   const std::string constraints_geometry =
       audio_constraints.GetGoogArrayGeometry();
 
   // Give preference to the audio constraint over the device-supplied mic
   // positions. This is mainly for testing purposes.
   return WebrtcPointsFromMediaPoints(
       constraints_geometry.empty()
           ? input_params.mic_positions
           : media::ParsePointsFromString(constraints_geometry));
 }
 
 }  // namespace content