Make the BWE threshold adaptive.

webrtc/modules/remote_bitrate_estimator/overuse_detector.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/modules/remote_bitrate_estimator/overuse_detector.h"
 
 #include <algorithm>
+#include <sstream>
 #include <math.h>
 #include <stdlib.h>
 
+#include "webrtc/base/common.h"
 #include "webrtc/modules/remote_bitrate_estimator/include/bwe_defines.h"
 #include "webrtc/modules/remote_bitrate_estimator/test/bwe_test_logging.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h"
+#include "webrtc/system_wrappers/interface/field_trial.h"
 #include "webrtc/system_wrappers/interface/trace.h"
 
 namespace webrtc {
 
-enum { kOverUsingTimeThreshold = 100 };
+enum { kOverUsingTimeThreshold = 10 };
+const char* kAdaptiveThresholdExperiment = "WebRTC-AdaptiveBweThreshold";
+const char* kEnabledPrefix = "Enabled";
+const size_t kEnabledPrefixLength = sizeof(kEnabledPrefix) - 1;
+const size_t kMinExperimentLength = kEnabledPrefixLength + 3;
+
+const double kMaxAdaptOffsetMs = 15.0;
+
+bool AdaptiveThresholdExperimentIsEnabled() {
+  std::string experiment_string =
+      webrtc::field_trial::FindFullName(kAdaptiveThresholdExperiment);
+  if (experiment_string.length() < kMinExperimentLength)
+    return false;
+  return experiment_string.substr(0, kEnabledPrefixLength) == kEnabledPrefix;
+}
 
 OveruseDetector::OveruseDetector(const OverUseDetectorOptions& options)
-    : options_(options),
-      threshold_(options_.initial_threshold),
+    : threshold_(12.5),
+      k_up_(0.01),
+      k_down_(0.00018),
+      experiment_set_(false),
+      in_experiment_(false),

[pbos-webrtc, 2015/07/06 10:01:40]

Can you make a struct of experiment settings (if it's not just in_experiment_)?
Also, is the base class what needs to know about the experiment? (Does it apply
to all subclasses?)

[stefan-webrtc, 2015/07/06 10:54:26]

There are only sublcasses in tests. Don't think it's worth making a struct.

+      options_(options),
+      last_update_ms_(-1),
       prev_offset_(0.0),
       time_over_using_(-1),
       overuse_counter_(0),
-      hypothesis_(kBwNormal) {}
+      hypothesis_(kBwNormal) {
+}
 
 OveruseDetector::~OveruseDetector() {}
 
 BandwidthUsage OveruseDetector::State() const {
   return hypothesis_;
 }
 
-
-void OveruseDetector::SetRateControlRegion(RateControlRegion region) {
-  switch (region) {
-    case kRcMaxUnknown: {
-      threshold_ = options_.initial_threshold;
-      break;
-    }
-    case kRcAboveMax:
-    case kRcNearMax: {
-      threshold_ = options_.initial_threshold / 2;
-      break;
-    }
-  }
-}
-
 BandwidthUsage OveruseDetector::Detect(double offset,
                                        double ts_delta,
                                        int num_of_deltas,
                                        int64_t now_ms) {
   if (num_of_deltas < 2) {
     return kBwNormal;
   }
   const double prev_offset = prev_offset_;
   prev_offset_ = offset;
   const double T = std::min(num_of_deltas, 60) * offset;
   BWE_TEST_LOGGING_PLOT(1, "offset", now_ms, T);
   BWE_TEST_LOGGING_PLOT(1, "threshold", now_ms, threshold_);
-
   if (T > threshold_) {
     if (time_over_using_ == -1) {
       // Initialize the timer. Assume that we've been
       // over-using half of the time since the previous
       // sample.
       time_over_using_ = ts_delta / 2;
     } else {
       // Increment timer
       time_over_using_ += ts_delta;
     }
     overuse_counter_++;
     if (time_over_using_ > kOverUsingTimeThreshold
         && overuse_counter_ > 1) {
       if (offset >= prev_offset) {
         time_over_using_ = 0;
         overuse_counter_ = 0;
         hypothesis_ = kBwOverusing;
       }
     }
   } else if (T < -threshold_) {
     time_over_using_ = -1;
     overuse_counter_ = 0;
     hypothesis_ = kBwUnderusing;
   } else {
     time_over_using_ = -1;
     overuse_counter_ = 0;
     hypothesis_ = kBwNormal;
   }
+
+  UpdateThreshold(T, now_ms);
+
   return hypothesis_;
 }
+
+void OveruseDetector::UpdateThreshold(double modified_offset, int64_t now_ms) {
+  if (!AdaptiveThresholdExperimentIsEnabled())

[pbos-webrtc, 2015/07/06 10:01:40]

Call in constructor instead, make in_experiment_ const.

[stefan-webrtc, 2015/07/06 10:54:26]

Fine with me. The reason for doing it this way was to make sure that the logic
with calling the AdaptiveThresholdExperimentIsEnabled() actually worked, but now
that most of it is overridden anyway it doesn't add much value.

in_experiment_ can't easily be made const for testing purposes though. The
subclass has to be able to change it.

+    return;
+
+  if (last_update_ms_ == -1)
+    last_update_ms_ = now_ms;
+
+  if (fabs(modified_offset) > threshold_ + kMaxAdaptOffsetMs) {
+    // Avoid adapting the threshold to big latency spikes, caused e.g.,
+    // by a sudden capacity drop.
+    last_update_ms_ = now_ms;
+    return;
+  }
+
+  const double k = fabs(modified_offset) < threshold_ ? k_down_ : k_up_;
+  threshold_ +=
+      k * (fabs(modified_offset) - threshold_) * (now_ms - last_update_ms_);
+
+  const double kMinThreshold = 6;
+  const double kMaxThreshold = 600;
+  threshold_ = std::min(std::max(threshold_, kMinThreshold), kMaxThreshold);
+
+  last_update_ms_ = now_ms;
+}
+
+bool OveruseDetector::AdaptiveThresholdExperimentIsEnabled() {
+  InitializeExperiment();
+  return in_experiment_;
+}
+
+// Gets thresholds from the experiment name following the format
+// "WebRTC-AdaptiveBweThreshold/Enabled-0.5,0.002/".
+void OveruseDetector::InitializeExperiment() {
+  if (!experiment_set_) {

[pbos-webrtc, 2015/07/06 10:01:40]

remove experiment_set_, this should not be lazy.

[stefan-webrtc, 2015/07/06 10:54:26]

Done.

+    experiment_set_ = true;
+    in_experiment_ = webrtc::AdaptiveThresholdExperimentIsEnabled();
+    if (in_experiment_) {
+      double k_up = 0.0;
+      double k_down = 0.0;
+      if (ReadExperimentConstants(&k_up, &k_down)) {
+        k_up_ = k_up;
+        k_down_ = k_down;
+      }
+    }
+  }
+}
+
+bool OveruseDetector::ReadExperimentConstants(double* k_up,
+                                              double* k_down) const {
+  std::string experiment_string =
+      webrtc::field_trial::FindFullName(kAdaptiveThresholdExperiment);
+  std::stringstream ss(experiment_string.substr(kEnabledPrefixLength + 1));
+  ss >> *k_up;
+  if (ss.peek() != ',')
+    return false;
+  ss.ignore();
+  ss >> *k_down;
+  if (ss.rdbuf()->in_avail() > 0)
+    return false;
+  return true;
+}
 }  // namespace webrtc