Add histogram stats for average QP per frame for VP8 (for sent video streams):

webrtc/video/send_statistics_proxy.cc

Index: webrtc/video/send_statistics_proxy.cc
diff --git a/webrtc/video/send_statistics_proxy.cc b/webrtc/video/send_statistics_proxy.cc
index 6407cdc62ea8e2257994ef90291f60c81ec42294..4a0645afbf9ef8682cb0037d1ba57caef3450065 100644
--- a/webrtc/video/send_statistics_proxy.cc
+++ b/webrtc/video/send_statistics_proxy.cc
@@ -13,6 +13,7 @@
 #include <algorithm>
 #include <cmath>
 #include <map>
+#include <vector>
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
@@ -192,6 +193,25 @@ void SendStatisticsProxy::UmaSamplesContainer::UpdateHistograms(
                                  max_delay_ms);
   }
 
+  for (const auto& it : qp_counters_) {
+    int qp = it.second.vp8.Avg(kMinRequiredSamples);
+    if (qp != -1) {
+      int spatial_idx = it.first;
+      if (spatial_idx == -1) {

[mflodman, 2016/03/23 12:25:20]

I think it would be nice to rather build a string based on 'spatial_idx' and
only have one call to RTC_HISTOGRAMS_COUNT. That would mean less duplicated
code.

wdyt?

[pbos-webrtc, 2016/03/23 12:34:03]

We can't since RTC_HISTOGRAMS_COUNTS requires the string to be unique per macro
for performance reasons.

[åsapersson, 2016/03/23 13:09:26]

Right the name should not vary for a macro.

+        RTC_HISTOGRAMS_COUNTS_200(kIndex, uma_prefix_ + "Encoded.Qp.Vp8", qp);
+      } else if (spatial_idx == 0) {
+        RTC_HISTOGRAMS_COUNTS_200(kIndex, uma_prefix_ + "Encoded.Qp.Vp8.S0",
+                                  qp);
+      } else if (spatial_idx == 1) {
+        RTC_HISTOGRAMS_COUNTS_200(kIndex, uma_prefix_ + "Encoded.Qp.Vp8.S1",
+                                  qp);
+      } else if (spatial_idx == 2) {
+        RTC_HISTOGRAMS_COUNTS_200(kIndex, uma_prefix_ + "Encoded.Qp.Vp8.S2",
+                                  qp);
+      }

[pbos-webrtc, 2016/03/23 12:34:03]

Should we have an else case here that should do something in case spatial_idx
goes above 2 to prevent dropping some unknown stats in the future?

[åsapersson, 2016/03/23 13:09:26]

Added a warning...

+    }
+  }
+
   if (first_rtcp_stats_time_ms_ != -1) {
     int64_t elapsed_sec =
         (clock_->TimeInMilliseconds() - first_rtcp_stats_time_ms_) / 1000;
@@ -426,6 +446,15 @@ void SendStatisticsProxy::OnSendEncodedImage(
     }
   }
 
+  if (encoded_image.qp_ != -1 && rtp_video_header != nullptr) {
+    if (rtp_video_header->codec == kRtpVideoVp8) {

[mflodman, 2016/03/23 12:25:20]

This can be an additional condition in the if above rather than having two ifs.

[pbos-webrtc, 2016/03/23 12:34:03]

I think so as well, if ssrcs.size() is 1 then we ignore what spatial_idx
actually is

[åsapersson, 2016/03/23 13:09:26]

-1 might be used when not using spatial layers for VP9 (therefore used here to
keep it similar..).

[åsapersson, 2016/03/23 13:09:26]

Done.

+      int spatial_idx = (config_.rtp.ssrcs.size() == 1)
+                            ? -1
+                            : static_cast<int>(simulcast_idx);
+      uma_container_->qp_counters_[spatial_idx].vp8.Add(encoded_image.qp_);
+    }
+  }
+
   // TODO(asapersson): This is incorrect if simulcast layers are encoded on
   // different threads and there is no guarantee that one frame of all layers
   // are encoded before the next start.