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

talk/app/webrtc/statscollector.cc

 /*
  * libjingle
  * Copyright 2012 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 97 matching lines @@
   report->AddString(StatsReport::kStatsValueNameCodecName, info.codec_name);
   report->AddInt64(StatsReport::kStatsValueNameBytesSent, info.bytes_sent);
   report->AddInt64(StatsReport::kStatsValueNameRtt, info.rtt_ms);
 }
 
 void ExtractCommonReceiveProperties(const cricket::MediaReceiverInfo& info,
                                     StatsReport* report) {
   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,
       echo_return_loss_enhancement },
     { StatsReport::kStatsValueNameEchoDelayMedian, echo_delay_median_ms },
     { StatsReport::kStatsValueNameEchoDelayStdDev, echo_delay_std_ms },
   };
   for (const auto& i : ints)
     report->AddInt(i.name, i.value);
 }
@@ skipping 36 matching lines @@
 
   report->AddInt64(StatsReport::kStatsValueNameBytesReceived,
                    info.bytes_rcvd);
   report->AddInt64(StatsReport::kStatsValueNameCaptureStartNtpTimeMs,
                    info.capture_start_ntp_time_ms);
 }
 
 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 },
   };
 
   for (const auto& i : ints)
     report->AddInt(i.name, i.value);
 }
 
 void ExtractStats(const cricket::VideoReceiverInfo& info, StatsReport* report) {
@@ skipping 684 matching lines @@
     report->set_timestamp(stats_gathering_started_);
     UpdateReportFromAudioTrack(track, report);
   }
 }
 
 void StatsCollector::UpdateReportFromAudioTrack(AudioTrackInterface* track,
                                                 StatsReport* report) {
   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,
                                       std::string* track_id,
                                       StatsReport::Direction direction) {
   RTC_DCHECK(pc_->session()->signaling_thread()->IsCurrent());
   if (direction == StatsReport::kSend) {
     if (!pc_->session()->GetLocalTrackIdBySsrc(ssrc, track_id)) {
       LOG(LS_WARNING) << "The SSRC " << ssrc
                       << " is not associated with a sending track";
@@ skipping 20 matching lines @@
     StatsReport* report = entry.second;
     report->set_timestamp(stats_gathering_started_);
   }
 }
 
 void StatsCollector::ClearUpdateStatsCacheForTest() {
   stats_gathering_started_ = 0;
 }
 
 }  // namespace webrtc