Add a new overuse estimator for the delay based BWE behind experiment.

webrtc/modules/congestion_controller/delay_based_bwe.cc

Index: webrtc/modules/congestion_controller/delay_based_bwe.cc
diff --git a/webrtc/modules/congestion_controller/delay_based_bwe.cc b/webrtc/modules/congestion_controller/delay_based_bwe.cc
index af27505268b736a9bf81c5ab912acf477ded65c6..d312cdbc69000ac526b48686058b4649cce905ab 100644
--- a/webrtc/modules/congestion_controller/delay_based_bwe.cc
+++ b/webrtc/modules/congestion_controller/delay_based_bwe.cc
@@ -12,6 +12,7 @@
 
 #include <algorithm>
 #include <cmath>
+#include <string>
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/constructormagic.h"
@@ -38,15 +39,57 @@ constexpr uint32_t kFixedSsrc = 0;
 constexpr int kInitialRateWindowMs = 500;
 constexpr int kRateWindowMs = 150;
 
+constexpr size_t kDefaultTrendlineWindowSize = 20;
+constexpr double kDefaultTrendlineSmoothingCoeff = 0.9;
+constexpr double kDefaultTrendlineThresholdGain = 4.0;
+
 const char kBitrateEstimateExperiment[] = "WebRTC-ImprovedBitrateEstimate";
+const char kBweTrendlineFilterExperiment[] = "WebRTC-BweTrendlineFilter";
 
 bool BitrateEstimateExperimentIsEnabled() {
   return webrtc::field_trial::FindFullName(kBitrateEstimateExperiment) ==
          "Enabled";
 }
+
+bool TrendlineFilterExperimentIsEnabled() {
+  std::string experiment_string =
+      webrtc::field_trial::FindFullName(kBweTrendlineFilterExperiment);
+  // The experiment is enabled iff the field trial string begins with "Enabled".
+  return experiment_string.find("Enabled") == 0;
+}
+
+bool ReadTrendlineFilterExperimentParameters(size_t* window_points,
+                                             double* smoothing_coef,
+                                             double* threshold_gain) {
+  RTC_DCHECK(TrendlineFilterExperimentIsEnabled());
+  std::string experiment_string =
+      webrtc::field_trial::FindFullName(kBweTrendlineFilterExperiment);
+  int parsed_values = sscanf(experiment_string.c_str(), "Enabled-%zu,%lf,%lf",
+                             window_points, smoothing_coef, threshold_gain);
+  if (parsed_values == 3) {
+    if (2 <= *window_points && 0 <= *smoothing_coef && *smoothing_coef <= 1 &&

[stefan-webrtc, 2016/11/13 13:09:53]

I personally prefer having variables first and constants second, e.g.,
if (*window_points > 2 ...

[terelius, 2016/11/14 13:21:39]

In general I agree, but for the smoothing_coef I wanted to highlight that it is
in the interval [0,1], so I wanted to write something similar to the
mathematical relation 0 <= *smoothing_coef <= 1.

[stefan-webrtc, 2016/11/14 13:30:58]

Sounds good for smoothing_coef, but the other two doesn't really follow the same
logic?

[terelius, 2016/11/15 17:35:46]

The if statement is redundant if we CHECK anyway. Removed.

[stefan-webrtc, 2016/11/16 15:11:20]

Acknowledged.

+        0 <= *threshold_gain)
+      return true;
+    if (*window_points < 2)
+      LOG(LS_ERROR) << "Needs at least 2 points to fit a trend line.";

[stefan-webrtc, 2016/11/13 13:09:53]

Should we perhaps DCHECK on all broken configurations instead?

[terelius, 2016/11/14 13:21:39]

It would not give us any alert if we misconfigure the parameters in release
builds. Would CHECK be appropriate?

[stefan-webrtc, 2016/11/14 13:30:58]

Yes, then I think we should CHECK as it would be bad if we didn't catch those
issues.

[terelius, 2016/11/15 17:35:46]

Done.

+    if (*smoothing_coef < 0 || 1 < *smoothing_coef)
+      LOG(LS_ERROR)
+          << "Coefficient needs to be between 0 and 1 for weighted average.";
+    if (*threshold_gain < 0)
+      LOG(LS_ERROR) << "Threshold gain needs to be positive.";
+  }
+  LOG(LS_ERROR) << "Failed to parse parameters for BweTrendlineFilter "
+                   "experiment from field trial string.";
+  *window_points = kDefaultTrendlineWindowSize;
+  *smoothing_coef = kDefaultTrendlineSmoothingCoeff;
+  *threshold_gain = kDefaultTrendlineThresholdGain;
+  return false;
+}
+
 }  // namespace
 
 namespace webrtc {
+
 DelayBasedBwe::BitrateEstimator::BitrateEstimator()
     : sum_(0),
       current_win_ms_(0),
@@ -132,12 +175,22 @@ rtc::Optional<uint32_t> DelayBasedBwe::BitrateEstimator::bitrate_bps() const {
 DelayBasedBwe::DelayBasedBwe(Clock* clock)
     : clock_(clock),
       inter_arrival_(),
-      estimator_(),
+      kalman_estimator_(),
+      trendline_estimator_(),
       detector_(OverUseDetectorOptions()),
       receiver_incoming_bitrate_(),
       last_update_ms_(-1),
       last_seen_packet_ms_(-1),
-      uma_recorded_(false) {
+      uma_recorded_(false),
+      trendline_window_size_(kDefaultTrendlineWindowSize),
+      trendline_smoothing_coeff_(kDefaultTrendlineSmoothingCoeff),
+      trendline_threshold_gain_(kDefaultTrendlineThresholdGain),
+      in_trendline_experiment_(TrendlineFilterExperimentIsEnabled()) {
+  if (in_trendline_experiment_) {
+    ReadTrendlineFilterExperimentParameters(&trendline_window_size_,
+                                            &trendline_smoothing_coeff_,
+                                            &trendline_threshold_gain_);
+  }
   network_thread_.DetachFromThread();
 }
 
@@ -171,7 +224,10 @@ DelayBasedBwe::Result DelayBasedBwe::IncomingPacketInfo(
     inter_arrival_.reset(
         new InterArrival((kTimestampGroupLengthMs << kInterArrivalShift) / 1000,
                          kTimestampToMs, true));
-    estimator_.reset(new OveruseEstimator(OverUseDetectorOptions()));
+    kalman_estimator_.reset(new OveruseEstimator(OverUseDetectorOptions()));
+    trendline_estimator_.reset(new TrendlineEstimator(
+        trendline_window_size_, trendline_smoothing_coeff_,
+        trendline_threshold_gain_));
   }
   last_seen_packet_ms_ = now_ms;
 
@@ -192,10 +248,19 @@ DelayBasedBwe::Result DelayBasedBwe::IncomingPacketInfo(
                                     info.payload_size, &ts_delta, &t_delta,
                                     &size_delta)) {
     double ts_delta_ms = (1000.0 * ts_delta) / (1 << kInterArrivalShift);
-    estimator_->Update(t_delta, ts_delta_ms, size_delta, detector_.State(),
+    if (in_trendline_experiment_) {
+      trendline_estimator_->Update(t_delta, ts_delta_ms, info.arrival_time_ms);
+      detector_.Detect(trendline_estimator_->trendline_slope(), ts_delta_ms,
+                       trendline_estimator_->num_of_deltas(),
                        info.arrival_time_ms);
-    detector_.Detect(estimator_->offset(), ts_delta_ms,
-                     estimator_->num_of_deltas(), info.arrival_time_ms);
+
+    } else {
+      kalman_estimator_->Update(t_delta, ts_delta_ms, size_delta,
+                                detector_.State(), info.arrival_time_ms);
+      detector_.Detect(kalman_estimator_->offset(), ts_delta_ms,
+                       kalman_estimator_->num_of_deltas(),
+                       info.arrival_time_ms);
+    }
   }
 
   int probing_bps = 0;
@@ -237,8 +302,9 @@ bool DelayBasedBwe::UpdateEstimate(int64_t arrival_time_ms,
                                    int64_t now_ms,
                                    rtc::Optional<uint32_t> acked_bitrate_bps,
                                    uint32_t* target_bitrate_bps) {
-  const RateControlInput input(detector_.State(), acked_bitrate_bps,
-                               estimator_->var_noise());
+  // TODO(terelius): RateControlInput::noise_var is deprecated and will be
+  // removed. In the meantime, we set it to zero.
+  const RateControlInput input(detector_.State(), acked_bitrate_bps, 0);
   rate_control_.Update(&input, now_ms);
   *target_bitrate_bps = rate_control_.UpdateBandwidthEstimate(now_ms);
   return rate_control_.ValidEstimate();