Add ability to handle data from multiple streams in RateAccCounter.

webrtc/video/stats_counter.cc

Index: webrtc/video/stats_counter.cc
diff --git a/webrtc/video/stats_counter.cc b/webrtc/video/stats_counter.cc
index 42d23adcfc60f71015e690090c8d779e47959421..9a7eccb7fc1150b561c25a48c694a168cf19d8c2 100644
--- a/webrtc/video/stats_counter.cc
+++ b/webrtc/video/stats_counter.cc
@@ -11,6 +11,8 @@
 #include "webrtc/video/stats_counter.h"
 
 #include <algorithm>
+#include <limits>
+#include <map>
 
 #include "webrtc/system_wrappers/include/clock.h"
 
@@ -19,6 +21,9 @@ namespace webrtc {
 namespace {
 // Periodic time interval for processing samples.
 const int64_t kProcessIntervalMs = 2000;
+
+const uint32_t kSsrc0 = 0;
+
 }  // namespace
 
 // Class holding periodically computed metrics.
@@ -54,16 +59,91 @@ class AggregatedCounter {
   AggregatedStats stats_;
 };
 
+// Class holding gathered samples within a process interval.
+class Samples {
+ public:
+  Samples() {}
+  ~Samples() {}
+
+  void Add(int sample, uint32_t ssrc) { samples_[ssrc].Add(sample); }
+  void Set(int sample, uint32_t ssrc) { samples_[ssrc].Set(sample); }
+
+  int64_t Num() const {

[stefan-webrtc, 2016/09/08 07:13:34]

Should we rename Num and num_ Count() and count_?

[åsapersson, 2016/09/19 12:45:40]

Done.

+    int64_t num = 0;
+    for (const auto& it : samples_)
+      num += it.second.num_;
+    return num;
+  }
+
+  int64_t Sum() const {
+    int64_t sum = 0;
+    for (const auto& it : samples_)
+      sum += it.second.sum_;
+    return sum;
+  }
+
+  int Max() const {
+    int max = std::numeric_limits<int>::min();
+    for (const auto& it : samples_)
+      max = std::max(it.second.max_, max);
+    return max;
+  }
+
+  void Reset() {
+    for (auto& it : samples_)
+      it.second.Reset();
+  }
+
+  int64_t Diff() const {
+    int64_t sum_diff = 0;
+    int num = 0;
+    for (const auto& it : samples_) {
+      if (it.second.num_ > 0) {
+        int64_t diff = it.second.sum_ - it.second.last_sum_;
+        if (diff >= 0) {
+          sum_diff += diff;
+          ++num;
+        }
+      }
+    }
+    return (num > 0) ? sum_diff : -1;
+  }
+
+ private:
+  struct Stats {
+    void Add(int sample) {
+      sum_ += sample;
+      ++num_;
+      max_ = std::max(sample, max_);
+    }
+    void Set(int sample) {
+      sum_ = sample;
+      ++num_;
+    }
+    void Reset() {
+      if (num_ > 0)
+        last_sum_ = sum_;
+      sum_ = 0;
+      num_ = 0;
+      max_ = std::numeric_limits<int>::min();
+    }
+
+    int max_ = std::numeric_limits<int>::min();
+    int64_t num_ = 0;
+    int64_t sum_ = 0;
+    int64_t last_sum_ = 0;
+  };
+
+  std::map<uint32_t, Stats> samples_;  // Gathered samples mapped by SSRC.
+};
+
 // StatsCounter class.
 StatsCounter::StatsCounter(Clock* clock,
                            bool include_empty_intervals,
                            StatsCounterObserver* observer)
-    : max_(0),
-      sum_(0),
-      num_samples_(0),
-      last_sum_(0),
-      clock_(clock),
+    : samples_(new Samples()),
       include_empty_intervals_(include_empty_intervals),
+      clock_(clock),
       observer_(observer),
       aggregated_counter_(new AggregatedCounter()),
       last_process_time_ms_(-1) {}
@@ -98,20 +178,14 @@ bool StatsCounter::TimeToProcess() {
   return true;
 }
 
-void StatsCounter::Set(int sample) {
+void StatsCounter::Set(int sample, uint32_t ssrc) {
   TryProcess();
-  ++num_samples_;
-  sum_ = sample;
+  samples_->Set(sample, ssrc);
 }
 
 void StatsCounter::Add(int sample) {

[stefan-webrtc, 2016/09/08 07:15:13]

Do you think it would be better to just expose the SSRC here to keep the class
more general and have the user use ssrc 0 whenever it doesn't have ssrcs?

[åsapersson, 2016/09/19 12:45:40]

It is only one subclass that have use for setting the ssrc, RateAccCounter, so
in most cases it will not be needed (for all Add). Maybe keep it as it is?

   TryProcess();
-  ++num_samples_;
-  sum_ += sample;
-
-  if (num_samples_ == 1)
-    max_ = sample;
-  max_ = std::max(sample, max_);
+  samples_->Add(sample, kSsrc0);
 }
 
 void StatsCounter::TryProcess() {
@@ -124,10 +198,8 @@ void StatsCounter::TryProcess() {
     if (observer_)
       observer_->OnMetricUpdated(metric);
   }
-  last_sum_ = sum_;
-  sum_ = 0;
-  max_ = 0;
-  num_samples_ = 0;
+
+  samples_->Reset();
 }
 
 // StatsCounter sub-classes.
@@ -141,9 +213,11 @@ void AvgCounter::Add(int sample) {
 }
 
 bool AvgCounter::GetMetric(int* metric) const {
-  if (num_samples_ == 0)
+  int64_t num = samples_->Num();
+  if (num == 0)
     return false;

[stefan-webrtc, 2016/09/08 07:13:34]

This is a fairly expensive way of checking for no samples since you loop through
the samples and add them up. What's the expected size of the map? Maybe it's
better to keep a variable around in Samples which keeps track of the count?

[åsapersson, 2016/09/19 12:45:40]

The expected size will be one for all sub-classes except the RateAccCounter.
Added a total_count_.

-  *metric = (sum_ + num_samples_ / 2) / num_samples_;
+
+  *metric = (samples_->Sum() + num / 2) / num;
   return true;
 }
 
@@ -157,9 +231,11 @@ void MaxCounter::Add(int sample) {
 }
 
 bool MaxCounter::GetMetric(int* metric) const {
-  if (num_samples_ == 0)
+  int64_t num = samples_->Num();
+  if (num == 0)
     return false;
-  *metric = max_;
+
+  *metric = samples_->Max();
   return true;
 }
 
@@ -173,9 +249,11 @@ void PercentCounter::Add(bool sample) {
 }
 
 bool PercentCounter::GetMetric(int* metric) const {
-  if (num_samples_ == 0)
+  int64_t num = samples_->Num();
+  if (num == 0)
     return false;
-  *metric = (sum_ * 100 + num_samples_ / 2) / num_samples_;
+
+  *metric = (samples_->Sum() * 100 + num / 2) / num;
   return true;
 }
 
@@ -189,9 +267,11 @@ void PermilleCounter::Add(bool sample) {
 }
 
 bool PermilleCounter::GetMetric(int* metric) const {
-  if (num_samples_ == 0)
+  int64_t num = samples_->Num();
+  if (num == 0)
     return false;
-  *metric = (sum_ * 1000 + num_samples_ / 2) / num_samples_;
+
+  *metric = (samples_->Sum() * 1000 + num / 2) / num;
   return true;
 }
 
@@ -205,9 +285,11 @@ void RateCounter::Add(int sample) {
 }
 
 bool RateCounter::GetMetric(int* metric) const {
-  if (num_samples_ == 0)
+  if (samples_->Num() == 0)
     return false;
-  *metric = (sum_ * 1000 + kProcessIntervalMs / 2) / kProcessIntervalMs;
+
+  *metric =
+      (samples_->Sum() * 1000 + kProcessIntervalMs / 2) / kProcessIntervalMs;
   return true;
 }
 
@@ -216,15 +298,19 @@ RateAccCounter::RateAccCounter(Clock* clock,
                                bool include_empty_intervals)
     : StatsCounter(clock, include_empty_intervals, observer) {}
 
-void RateAccCounter::Set(int sample) {
-  StatsCounter::Set(sample);
+void RateAccCounter::Set(int sample, uint32_t ssrc) {
+  StatsCounter::Set(sample, ssrc);
 }
 
 bool RateAccCounter::GetMetric(int* metric) const {
-  if (num_samples_ == 0 || last_sum_ > sum_)
+  if (samples_->Num() == 0)
     return false;
-  *metric =
-      ((sum_ - last_sum_) * 1000 + kProcessIntervalMs / 2) / kProcessIntervalMs;
+
+  int64_t diff = samples_->Diff();
+  if (diff < 0 || (!include_empty_intervals_ && diff == 0))
+    return false;
+
+  *metric = (diff * 1000 + kProcessIntervalMs / 2) / kProcessIntervalMs;
   return true;
 }