Don't overwrite audio stats when they're not available.

talk/app/webrtc/statscollector.cc

Index: talk/app/webrtc/statscollector.cc
diff --git a/talk/app/webrtc/statscollector.cc b/talk/app/webrtc/statscollector.cc
index 28686965e64666f7f225cb03c66c2946491f560e..e1c9adae412eb6a1eaa554a10ea9323993ddc4f7 100644
--- a/talk/app/webrtc/statscollector.cc
+++ b/talk/app/webrtc/statscollector.cc
@@ -115,17 +115,17 @@ void ExtractCommonReceiveProperties(const cricket::MediaReceiverInfo& info,
   report->AddString(StatsReport::kStatsValueNameCodecName, info.codec_name);
 }
 
-void SetAudioProcessingStats(StatsReport* report, int signal_level,
-    bool typing_noise_detected, int echo_return_loss,
-    int echo_return_loss_enhancement, int echo_delay_median_ms,
-    float aec_quality_min, int echo_delay_std_ms) {
+void SetAudioProcessingStats(StatsReport* report,
+                             bool typing_noise_detected,
+                             int echo_return_loss,
+                             int echo_return_loss_enhancement,
+                             int echo_delay_median_ms,
+                             float aec_quality_min,
+                             int echo_delay_std_ms) {
   report->AddBoolean(StatsReport::kStatsValueNameTypingNoiseState,
                      typing_noise_detected);
   report->AddFloat(StatsReport::kStatsValueNameEchoCancellationQualityMin,
                    aec_quality_min);
-  // Don't overwrite the previous signal level if it's not available now.
-  if (signal_level >= 0)
-    report->AddInt(StatsReport::kStatsValueNameAudioInputLevel, signal_level);
   const IntForAdd ints[] = {
     { StatsReport::kStatsValueNameEchoReturnLoss, echo_return_loss },
     { StatsReport::kStatsValueNameEchoReturnLossEnhancement,
@@ -182,11 +182,13 @@ void ExtractStats(const cricket::VoiceReceiverInfo& info, StatsReport* report) {
 void ExtractStats(const cricket::VoiceSenderInfo& info, StatsReport* report) {
   ExtractCommonSendProperties(info, report);
 
-  SetAudioProcessingStats(report, info.audio_level, info.typing_noise_detected,
-      info.echo_return_loss, info.echo_return_loss_enhancement,
-      info.echo_delay_median_ms, info.aec_quality_min, info.echo_delay_std_ms);
+  SetAudioProcessingStats(
+      report, info.typing_noise_detected, info.echo_return_loss,
+      info.echo_return_loss_enhancement, info.echo_delay_median_ms,
+      info.aec_quality_min, info.echo_delay_std_ms);
 
   const IntForAdd ints[] = {
+    { StatsReport::kStatsValueNameAudioInputLevel, info.audio_level},

[tommi, 2015/11/24 20:29:59]

this doesn't handle (or rather, ignore) negative signal values like the current
implementation does. Intentional?

[Andrew MacDonald, 2015/11/24 22:43:54]

Yes. I assumed (perhaps incorrectly) that this was only needed to workaround
GetSignalLevel failing on line 895 in the original code. Effectively, the signal
level stat continued to work outside of Chrome because of this, but the audio
processing stats didn't.

I thought it best to make the behavior consistent. Do you know of any other
reason why we would get a negative signal level here that we shouldn't report? 

[the sun, 2015/11/25 13:02:17]

AFAICT VoiceSenderInfo::audio_level cannot be negative now that we fetch it from
AudioSendStream (which doesn't early-out while collecting stats). The default
AudioSendStream::Stats object sets audio_level to -1, but we should never have a
default object in practice.

[tommi, 2015/11/25 13:11:09]

Thanks Fredrik.  Can we have a DCHECK() here then that info.audio_level >= 0?

[Andrew MacDonald, 2015/11/25 18:50:00]

Sure, done.

     { StatsReport::kStatsValueNameJitterReceived, info.jitter_ms },
     { StatsReport::kStatsValueNamePacketsLost, info.packets_lost },
     { StatsReport::kStatsValueNamePacketsSent, info.packets_sent },
@@ -891,21 +893,22 @@ void StatsCollector::UpdateReportFromAudioTrack(AudioTrackInterface* track,
   RTC_DCHECK(pc_->session()->signaling_thread()->IsCurrent());
   RTC_DCHECK(track != NULL);
 
-  int signal_level = 0;
-  if (!track->GetSignalLevel(&signal_level))
-    signal_level = -1;
+  // Don't overwrite report values if they're not available.
+  int signal_level;
+  if (track->GetSignalLevel(&signal_level))

[the sun, 2015/11/25 13:02:17]

AudioTrackInterface::GetSignalLevel() doesn't have a real ´ implementation (not
in WebRTC, not in chromium/src/chrome). It will always return false.

[tommi, 2015/11/25 13:11:09]

Here it is:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

[the sun, 2015/11/26 09:20:03]

Ah, thanks

+    report->AddInt(StatsReport::kStatsValueNameAudioInputLevel, signal_level);
 
-  rtc::scoped_refptr<AudioProcessorInterface> audio_processor(
-      track->GetAudioProcessor());
+  auto audio_processor(track->GetAudioProcessor());
 
-  AudioProcessorInterface::AudioProcessorStats stats;
-  if (audio_processor.get())
+  if (audio_processor.get()) {
+    AudioProcessorInterface::AudioProcessorStats stats;
     audio_processor->GetStats(&stats);
 
-  SetAudioProcessingStats(report, signal_level, stats.typing_noise_detected,
-      stats.echo_return_loss, stats.echo_return_loss_enhancement,
-      stats.echo_delay_median_ms, stats.aec_quality_min,
-      stats.echo_delay_std_ms);
+    SetAudioProcessingStats(
+        report, stats.typing_noise_detected, stats.echo_return_loss,
+        stats.echo_return_loss_enhancement, stats.echo_delay_median_ms,
+        stats.aec_quality_min, stats.echo_delay_std_ms);
+  }
 }
 
 bool StatsCollector::GetTrackIdBySsrc(uint32_t ssrc,