JitterEstimator: Remove old LowRate exp and add trial for upper bound.

The frame time deltas are now capped based on the current noise.
This has been tested in various conditions using both screen content
and typical mobile video settings, to produce delays that are not overly
high screen content, and simultaneously not negatively affect mobile
calls on really bad network that may have high natural jitter.

Bug: webrtc:9898
Change-Id: I51ad279af156aba1b5cc75ae203334a34bce9d48
Reviewed-on: https://webrtc-review.googlesource.com/c/107349
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25469}

modules/video_coding/BUILD.gn

@@ -171,6 +171,7 @@ rtc_static_library("video_coding") {
     "../../rtc_base:rtc_task_queue",
     "../../rtc_base:sequenced_task_checker",
     "../../rtc_base/experiments:alr_experiment",
+    "../../rtc_base/experiments:jitter_upper_bound_experiment",
     "../../rtc_base/experiments:rtt_mult_experiment",
     "../../rtc_base/system:fallthrough",
     "../../rtc_base/third_party/base64",
@@ -909,6 +910,7 @@ if (rtc_include_tests) {
       "../../rtc_base:rtc_numerics",
       "../../rtc_base:rtc_task_queue",
       "../../rtc_base:rtc_task_queue_for_test",
+      "../../rtc_base/experiments:jitter_upper_bound_experiment",
       "../../system_wrappers",
       "../../system_wrappers:field_trial",
       "../../system_wrappers:metrics",
@@ -920,6 +922,7 @@ if (rtc_include_tests) {
       "../../test:video_test_support",
       "../rtp_rtcp:rtp_rtcp_format",
       "//third_party/abseil-cpp/absl/memory",
+      "//third_party/abseil-cpp/absl/types:optional",
     ]
     if (rtc_build_libvpx) {
       deps += [ rtc_libvpx_dir ]

modules/video_coding/jitter_estimator.cc

@@ -14,19 +14,24 @@
 #include <math.h>
 #include <stdlib.h>
 #include <string.h>
+#include <algorithm>
 #include <string>
 
+#include "absl/types/optional.h"
 #include "modules/video_coding/internal_defines.h"
 #include "modules/video_coding/rtt_filter.h"
+#include "rtc_base/experiments/jitter_upper_bound_experiment.h"
 #include "system_wrappers/include/clock.h"
 #include "system_wrappers/include/field_trial.h"
 
 namespace webrtc {
-
-enum { kStartupDelaySamples = 30 };
-enum { kFsAccuStartupSamples = 5 };
-enum { kMaxFramerateEstimate = 200 };
-enum { kNackCountTimeoutMs = 60000 };
+namespace {
+static constexpr uint32_t kStartupDelaySamples = 30;
+static constexpr int64_t kFsAccuStartupSamples = 5;
+static constexpr double kMaxFramerateEstimate = 200.0;
+static constexpr int64_t kNackCountTimeoutMs = 60000;
+static constexpr double kDefaultMaxTimestampDeviationInSigmas = 3.5;
+}  // namespace
 
 VCMJitterEstimator::VCMJitterEstimator(const Clock* clock,
                                        int32_t vcmId,
@@ -46,7 +51,9 @@ VCMJitterEstimator::VCMJitterEstimator(const Clock* clock,
       _rttFilter(),
       fps_counter_(30),  // TODO(sprang): Use an estimator with limit based on
                          // time, rather than number of samples.
-      low_rate_experiment_(kInit),
+      time_deviation_upper_bound_(
+          JitterUpperBoundExperiment::GetUpperBoundSigmas().value_or(
+              kDefaultMaxTimestampDeviationInSigmas)),
       clock_(clock) {
   Reset();
 }
@@ -76,6 +83,7 @@ VCMJitterEstimator& VCMJitterEstimator::operator=(
     _latestNackTimestamp = rhs._latestNackTimestamp;
     _nackCount = rhs._nackCount;
     _rttFilter = rhs._rttFilter;
+    clock_ = rhs.clock_;
   }
   return *this;
 }
@@ -157,6 +165,12 @@ void VCMJitterEstimator::UpdateEstimate(int64_t frameDelayMS,
   }
   _prevFrameSize = frameSizeBytes;
 
+  // Cap frameDelayMS based on the current time deviation noise.
+  int64_t max_time_deviation_ms =
+      static_cast<int64_t>(time_deviation_upper_bound_ * sqrt(_varNoise) + 0.5);
+  frameDelayMS = std::max(std::min(frameDelayMS, max_time_deviation_ms),
+                          -max_time_deviation_ms);
+
   // Only update the Kalman filter if the sample is not considered
   // an extreme outlier. Even if it is an extreme outlier from a
   // delay point of view, if the frame size also is large the
@@ -306,22 +320,20 @@ void VCMJitterEstimator::EstimateRandomJitter(double d_dT,
   if (_alphaCount > _alphaCountMax)
     _alphaCount = _alphaCountMax;
 
-  if (LowRateExperimentEnabled()) {
-    // In order to avoid a low frame rate stream to react slower to changes,
-    // scale the alpha weight relative a 30 fps stream.
-    double fps = GetFrameRate();
-    if (fps > 0.0) {
-      double rate_scale = 30.0 / fps;
-      // At startup, there can be a lot of noise in the fps estimate.
-      // Interpolate rate_scale linearly, from 1.0 at sample #1, to 30.0 / fps
-      // at sample #kStartupDelaySamples.
-      if (_alphaCount < kStartupDelaySamples) {
-        rate_scale =
-            (_alphaCount * rate_scale + (kStartupDelaySamples - _alphaCount)) /
-            kStartupDelaySamples;
-      }
-      alpha = pow(alpha, rate_scale);
+  // In order to avoid a low frame rate stream to react slower to changes,
+  // scale the alpha weight relative a 30 fps stream.
+  double fps = GetFrameRate();
+  if (fps > 0.0) {
+    double rate_scale = 30.0 / fps;
+    // At startup, there can be a lot of noise in the fps estimate.
+    // Interpolate rate_scale linearly, from 1.0 at sample #1, to 30.0 / fps
+    // at sample #kStartupDelaySamples.
+    if (_alphaCount < kStartupDelaySamples) {
+      rate_scale =
+          (_alphaCount * rate_scale + (kStartupDelaySamples - _alphaCount)) /
+          kStartupDelaySamples;
     }
+    alpha = pow(alpha, rate_scale);
   }
 
   double avgNoise = alpha * _avgNoise + (1 - alpha) * d_dT;
@@ -393,41 +405,25 @@ int VCMJitterEstimator::GetJitterEstimate(double rttMultiplier) {
   if (_nackCount >= _nackLimit)
     jitterMS += _rttFilter.RttMs() * rttMultiplier;
 
-  if (LowRateExperimentEnabled()) {
-    static const double kJitterScaleLowThreshold = 5.0;
-    static const double kJitterScaleHighThreshold = 10.0;
-    double fps = GetFrameRate();
-    // Ignore jitter for very low fps streams.
-    if (fps < kJitterScaleLowThreshold) {
-      if (fps == 0.0) {
-        return jitterMS;
-      }
-      return 0;
-    }
-
-    // Semi-low frame rate; scale by factor linearly interpolated from 0.0 at
-    // kJitterScaleLowThreshold to 1.0 at kJitterScaleHighThreshold.
-    if (fps < kJitterScaleHighThreshold) {
-      jitterMS =
-          (1.0 / (kJitterScaleHighThreshold - kJitterScaleLowThreshold)) *
-          (fps - kJitterScaleLowThreshold) * jitterMS;
+  static const double kJitterScaleLowThreshold = 5.0;
+  static const double kJitterScaleHighThreshold = 10.0;
+  double fps = GetFrameRate();
+  // Ignore jitter for very low fps streams.
+  if (fps < kJitterScaleLowThreshold) {
+    if (fps == 0.0) {
+      return rtc::checked_cast<int>(std::max(0.0, jitterMS) + 0.5);
     }
+    return 0;
   }
 
-  return static_cast<uint32_t>(jitterMS + 0.5);
-}
-
-bool VCMJitterEstimator::LowRateExperimentEnabled() {
-  if (low_rate_experiment_ == kInit) {
-    std::string group =
-        webrtc::field_trial::FindFullName("WebRTC-ReducedJitterDelay");
-    if (group == "Disabled") {
-      low_rate_experiment_ = kDisabled;
-    } else {
-      low_rate_experiment_ = kEnabled;
-    }
+  // Semi-low frame rate; scale by factor linearly interpolated from 0.0 at
+  // kJitterScaleLowThreshold to 1.0 at kJitterScaleHighThreshold.
+  if (fps < kJitterScaleHighThreshold) {
+    jitterMS = (1.0 / (kJitterScaleHighThreshold - kJitterScaleLowThreshold)) *
+               (fps - kJitterScaleLowThreshold) * jitterMS;
   }
-  return low_rate_experiment_ == kEnabled ? true : false;
+
+  return rtc::checked_cast<int>(std::max(0.0, jitterMS) + 0.5);
 }
 
 double VCMJitterEstimator::GetFrameRate() const {

modules/video_coding/jitter_estimator.h

@@ -72,8 +72,6 @@ class VCMJitterEstimator {
   double _theta[2];  // Estimated line parameters (slope, offset)
   double _varNoise;  // Variance of the time-deviation from the line
 
-  virtual bool LowRateExperimentEnabled();
-
  private:
   // Updates the Kalman filter for the line describing
   // the frame size dependent jitter.
@@ -159,8 +157,7 @@ class VCMJitterEstimator {
   VCMRttFilter _rttFilter;
 
   rtc::RollingAccumulator<uint64_t> fps_counter_;
-  enum ExperimentFlag { kInit, kEnabled, kDisabled };
-  ExperimentFlag low_rate_experiment_;
+  const double time_deviation_upper_bound_;
   const Clock* clock_;
 };
 

modules/video_coding/jitter_estimator_tests.cc

@@ -9,38 +9,30 @@
 
 #include "modules/video_coding/jitter_estimator.h"
 
+#include "rtc_base/experiments/jitter_upper_bound_experiment.h"
+#include "rtc_base/logging.h"
+#include "rtc_base/numerics/histogram_percentile_counter.h"
+#include "rtc_base/timeutils.h"
 #include "system_wrappers/include/clock.h"
+#include "test/field_trial.h"
 #include "test/gtest.h"
 
 namespace webrtc {
 
-class TestEstimator : public VCMJitterEstimator {
- public:
-  explicit TestEstimator(bool exp_enabled)
-      : VCMJitterEstimator(&fake_clock_, 0, 0),
-        fake_clock_(0),
-        exp_enabled_(exp_enabled) {}
+class TestVCMJitterEstimator : public ::testing::Test {
+ protected:
+  TestVCMJitterEstimator() : fake_clock_(0) {}
 
-  virtual bool LowRateExperimentEnabled() { return exp_enabled_; }
+  virtual void SetUp() {
+    estimator_ = absl::make_unique<VCMJitterEstimator>(&fake_clock_, 0, 0);
+  }
 
   void AdvanceClock(int64_t microseconds) {
     fake_clock_.AdvanceTimeMicroseconds(microseconds);
   }
 
- private:
   SimulatedClock fake_clock_;
-  const bool exp_enabled_;
-};
-
-class TestVCMJitterEstimator : public ::testing::Test {
- protected:
-  TestVCMJitterEstimator()
-      : regular_estimator_(false), low_rate_estimator_(true) {}
-
-  virtual void SetUp() { regular_estimator_.Reset(); }
-
-  TestEstimator regular_estimator_;
-  TestEstimator low_rate_estimator_;
+  std::unique_ptr<VCMJitterEstimator> estimator_;
 };
 
 // Generates some simple test data in the form of a sawtooth wave.
@@ -64,98 +56,56 @@ class ValueGenerator {
 // 5 fps, disable jitter delay altogether.
 TEST_F(TestVCMJitterEstimator, TestLowRate) {
   ValueGenerator gen(10);
-  uint64_t time_delta = 1000000 / 5;
+  uint64_t time_delta_us = rtc::kNumMicrosecsPerSec / 5;
   for (int i = 0; i < 60; ++i) {
-    regular_estimator_.UpdateEstimate(gen.Delay(), gen.FrameSize());
-    regular_estimator_.AdvanceClock(time_delta);
-    low_rate_estimator_.UpdateEstimate(gen.Delay(), gen.FrameSize());
-    low_rate_estimator_.AdvanceClock(time_delta);
-    EXPECT_GT(regular_estimator_.GetJitterEstimate(0), 0);
+    estimator_->UpdateEstimate(gen.Delay(), gen.FrameSize());
+    AdvanceClock(time_delta_us);
     if (i > 2)
-      EXPECT_EQ(low_rate_estimator_.GetJitterEstimate(0), 0);
+      EXPECT_EQ(estimator_->GetJitterEstimate(0), 0);
     gen.Advance();
   }
 }
 
-// 8 fps, steady state estimate should be in interpolated interval between 0
-// and value of previous method.
-TEST_F(TestVCMJitterEstimator, TestMidRate) {
-  ValueGenerator gen(10);
-  uint64_t time_delta = 1000000 / 8;
-  for (int i = 0; i < 60; ++i) {
-    regular_estimator_.UpdateEstimate(gen.Delay(), gen.FrameSize());
-    regular_estimator_.AdvanceClock(time_delta);
-    low_rate_estimator_.UpdateEstimate(gen.Delay(), gen.FrameSize());
-    low_rate_estimator_.AdvanceClock(time_delta);
-    EXPECT_GT(regular_estimator_.GetJitterEstimate(0), 0);
-    EXPECT_GT(low_rate_estimator_.GetJitterEstimate(0), 0);
-    EXPECT_GE(regular_estimator_.GetJitterEstimate(0),
-              low_rate_estimator_.GetJitterEstimate(0));
-    gen.Advance();
-  }
-}
+TEST_F(TestVCMJitterEstimator, TestUpperBound) {
+  struct TestContext {
+    TestContext() : upper_bound(0.0), percentiles(1000) {}
+    double upper_bound;
+    rtc::HistogramPercentileCounter percentiles;
+  };
+  std::vector<TestContext> test_cases(2);
 
-// 30 fps, steady state estimate should be same as previous method.
-TEST_F(TestVCMJitterEstimator, TestHighRate) {
-  ValueGenerator gen(10);
-  uint64_t time_delta = 1000000 / 30;
-  for (int i = 0; i < 60; ++i) {
-    regular_estimator_.UpdateEstimate(gen.Delay(), gen.FrameSize());
-    regular_estimator_.AdvanceClock(time_delta);
-    low_rate_estimator_.UpdateEstimate(gen.Delay(), gen.FrameSize());
-    low_rate_estimator_.AdvanceClock(time_delta);
-    EXPECT_EQ(regular_estimator_.GetJitterEstimate(0),
-              low_rate_estimator_.GetJitterEstimate(0));
-    gen.Advance();
-  }
-}
+  test_cases[0].upper_bound = 100.0;  // First use essentially no cap.
+  test_cases[1].upper_bound = 3.5;    // Second, reasonably small cap.
 
-// 10 fps, high jitter then low jitter. Low rate estimator should converge
-// faster to low noise estimate.
-TEST_F(TestVCMJitterEstimator, TestConvergence) {
-  // Reach a steady state with high noise.
-  ValueGenerator gen(50);
-  uint64_t time_delta = 1000000 / 10;
-  for (int i = 0; i < 100; ++i) {
-    regular_estimator_.UpdateEstimate(gen.Delay(), gen.FrameSize());
-    regular_estimator_.AdvanceClock(time_delta * 2);
-    low_rate_estimator_.UpdateEstimate(gen.Delay(), gen.FrameSize());
-    low_rate_estimator_.AdvanceClock(time_delta * 2);
-    gen.Advance();
-  }
+  for (TestContext& context : test_cases) {
+    // Set up field trial and reset jitter estimator.
+    char string_buf[64];
+    rtc::SimpleStringBuilder ssb(string_buf);
+    ssb << JitterUpperBoundExperiment::kJitterUpperBoundExperimentName
+        << "/Enabled-" << context.upper_bound << "/";
+    test::ScopedFieldTrials field_trials(ssb.str());
+    SetUp();
 
-  int threshold = regular_estimator_.GetJitterEstimate(0) / 2;
-
-  // New generator with zero noise.
-  ValueGenerator low_gen(0);
-  int regular_iterations = 0;
-  int low_rate_iterations = 0;
-  for (int i = 0; i < 500; ++i) {
-    if (regular_iterations == 0) {
-      regular_estimator_.UpdateEstimate(low_gen.Delay(), low_gen.FrameSize());
-      regular_estimator_.AdvanceClock(time_delta);
-      if (regular_estimator_.GetJitterEstimate(0) < threshold) {
-        regular_iterations = i;
-      }
+    ValueGenerator gen(50);
+    uint64_t time_delta_us = rtc::kNumMicrosecsPerSec / 30;
+    for (int i = 0; i < 100; ++i) {
+      estimator_->UpdateEstimate(gen.Delay(), gen.FrameSize());
+      AdvanceClock(time_delta_us);
+      context.percentiles.Add(
+          static_cast<uint32_t>(estimator_->GetJitterEstimate(0)));
+      gen.Advance();
     }
-
-    if (low_rate_iterations == 0) {
-      low_rate_estimator_.UpdateEstimate(low_gen.Delay(), low_gen.FrameSize());
-      low_rate_estimator_.AdvanceClock(time_delta);
-      if (low_rate_estimator_.GetJitterEstimate(0) < threshold) {
-        low_rate_iterations = i;
-      }
-    }
-
-    if (regular_iterations != 0 && low_rate_iterations != 0) {
-      break;
-    }
-
-    gen.Advance();
   }
 
-  EXPECT_NE(regular_iterations, 0);
-  EXPECT_NE(low_rate_iterations, 0);
-  EXPECT_LE(low_rate_iterations, regular_iterations);
+  // Median should be similar after three seconds. Allow 5% error margin.
+  uint32_t median_unbound = *test_cases[0].percentiles.GetPercentile(0.5);
+  uint32_t median_bounded = *test_cases[1].percentiles.GetPercentile(0.5);
+  EXPECT_NEAR(median_unbound, median_bounded, (median_unbound * 5) / 100);
+
+  // Max should be lower for the bounded case.
+  uint32_t max_unbound = *test_cases[0].percentiles.GetPercentile(1.0);
+  uint32_t max_bounded = *test_cases[1].percentiles.GetPercentile(1.0);
+  EXPECT_GT(max_unbound, static_cast<uint32_t>(max_bounded * 1.25));
 }
+
 }  // namespace webrtc

rtc_base/experiments/BUILD.gn

@@ -86,6 +86,18 @@ rtc_static_library("rtt_mult_experiment") {
   ]
 }
 
+rtc_static_library("jitter_upper_bound_experiment") {
+  sources = [
+    "jitter_upper_bound_experiment.cc",
+    "jitter_upper_bound_experiment.h",
+  ]
+  deps = [
+    "../:rtc_base_approved",
+    "../../system_wrappers:field_trial",
+    "//third_party/abseil-cpp/absl/types:optional",
+  ]
+}
+
 if (rtc_include_tests) {
   rtc_source_set("experiments_unittests") {
     testonly = true

rtc_base/experiments/jitter_upper_bound_experiment.cc

@@ -0,0 +1,46 @@
+/*
+ *  Copyright (c) 2018 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 "rtc_base/experiments/jitter_upper_bound_experiment.h"
+
+#include <algorithm>
+#include <string>
+
+#include "rtc_base/logging.h"
+#include "system_wrappers/include/field_trial.h"
+
+namespace webrtc {
+
+const char JitterUpperBoundExperiment::kJitterUpperBoundExperimentName[] =
+    "WebRTC-JitterUpperBound";
+
+absl::optional<double> JitterUpperBoundExperiment::GetUpperBoundSigmas() {
+  if (!field_trial::IsEnabled(kJitterUpperBoundExperimentName)) {
+    return absl::nullopt;
+  }
+  const std::string group =
+      webrtc::field_trial::FindFullName(kJitterUpperBoundExperimentName);
+
+  double upper_bound_sigmas;
+  if (sscanf(group.c_str(), "Enabled-%lf", &upper_bound_sigmas) != 1) {
+    RTC_LOG(LS_WARNING) << "Invalid number of parameters provided.";
+    return absl::nullopt;
+  }
+
+  if (upper_bound_sigmas < 0) {
+    RTC_LOG(LS_WARNING) << "Invalid jitter upper bound sigmas, must be >= 0.0: "
+                        << upper_bound_sigmas;
+    return absl::nullopt;
+  }
+
+  return upper_bound_sigmas;
+}
+
+}  // namespace webrtc

rtc_base/experiments/jitter_upper_bound_experiment.h

@@ -0,0 +1,31 @@
+/*
+ *  Copyright (c) 2018 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.
+ */
+
+#ifndef RTC_BASE_EXPERIMENTS_JITTER_UPPER_BOUND_EXPERIMENT_H_
+#define RTC_BASE_EXPERIMENTS_JITTER_UPPER_BOUND_EXPERIMENT_H_
+
+#include "absl/types/optional.h"
+
+namespace webrtc {
+
+class JitterUpperBoundExperiment {
+ public:
+  // Returns nullopt if experiment is not on, otherwise returns the configured
+  // upper bound for frame delay delta used in jitter estimation, expressed as
+  // number of standard deviations of the current deviation from the expected
+  // delay.
+  static absl::optional<double> GetUpperBoundSigmas();
+
+  static const char kJitterUpperBoundExperimentName[];
+};
+
+}  // namespace webrtc
+
+#endif  // RTC_BASE_EXPERIMENTS_JITTER_UPPER_BOUND_EXPERIMENT_H_