JitterEstimator: add field trial for not estimating noise for congested frames

The reason for rejecting the congested frames in the first place, is
that they do not obey a Gaussian distribution around the line from the
Kalman filter. It therefore also does not make sense to include them
in the noise (*) estimation.

(*) noise = variation around the line from the Kalman filter.

Bug: webrtc:14151
Change-Id: Id8a44ba5f13bf9787ab54848109430ef7657f67a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/275762
Commit-Queue: Rasmus Brandt <brandtr@webrtc.org>
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38100}

modules/video_coding/timing/jitter_estimator.cc

@@ -263,9 +263,6 @@ void JitterEstimator::UpdateEstimate(TimeDelta frame_delay,
   // the frame size also is large the deviation is probably due to an incorrect
   // line slope.
   if (abs_delay_is_not_outlier || size_is_positive_outlier) {
-    // Update the variance of the deviation from the line given by the Kalman
-    // filter.
-    EstimateRandomJitter(delay_deviation_ms);
     // Prevent updating with frames which have been congested by a large frame,
     // and therefore arrives almost at the same time as that frame.
     // This can occur when we receive a large frame (key frame) which has been
@@ -276,14 +273,25 @@ void JitterEstimator::UpdateEstimate(TimeDelta frame_delay,
         config_.MaxFrameSizePercentileEnabled()
             ? max_frame_size_bytes_percentile_.GetFilteredValue()
             : max_frame_size_bytes_;
-    if (delta_frame_bytes >
+    bool is_not_congested =
-        GetCongestionRejectionFactor() * filtered_max_frame_size_bytes) {
+        delta_frame_bytes >
-      // Update the Kalman filter with the new data
+        GetCongestionRejectionFactor() * filtered_max_frame_size_bytes;
+
+    if (is_not_congested || config_.estimate_noise_when_congested) {
+      // Update the variance of the deviation from the line given by the Kalman
+      // filter.
+      EstimateRandomJitter(delay_deviation_ms);
+    }
+    if (is_not_congested) {
+      // Neither a delay outlier nor a congested frame, so we can safely update
+      // the Kalman filter with the sample.
       kalman_filter_.PredictAndUpdate(frame_delay.ms(), delta_frame_bytes,
                                       filtered_max_frame_size_bytes,
                                       var_noise_ms2_);
     }
   } else {
+    // Delay outliers affect the noise estimate through a value equal to the
+    // outlier rejection threshold.
     double num_stddev = (delay_deviation_ms >= 0) ? num_stddev_delay_outlier
                                                   : -num_stddev_delay_outlier;
     EstimateRandomJitter(num_stddev * sqrt(var_noise_ms2_));

modules/video_coding/timing/jitter_estimator.h

@@ -51,7 +51,8 @@ class JitterEstimator {
           "num_stddev_delay_clamp", &num_stddev_delay_clamp,
           "num_stddev_delay_outlier", &num_stddev_delay_outlier,
           "num_stddev_size_outlier", &num_stddev_size_outlier,
-          "congestion_rejection_factor", &congestion_rejection_factor);
+          "congestion_rejection_factor", &congestion_rejection_factor,
+          "estimate_noise_when_congested", &estimate_noise_when_congested);
       // clang-format on
     }
 
@@ -59,7 +60,7 @@ class JitterEstimator {
       return max_frame_size_percentile.has_value();
     }
 
-    // If set, the "avg" frame size is calculated as the median over a window
+    // If true, the "avg" frame size is calculated as the median over a window
     // of recent frame sizes.
     bool avg_frame_size_median = false;
 
@@ -96,6 +97,12 @@ class JitterEstimator {
     //
     // Decreasing this value rejects fewer samples.
     absl::optional<double> congestion_rejection_factor = absl::nullopt;
+
+    // If true, the noise estimate will be updated for congestion rejected
+    // frames. This is currently enabled by default, but that may not be optimal
+    // since congested frames typically are not spread around the line with
+    // Gaussian noise. (This is the whole reason for the congestion rejection!)
+    bool estimate_noise_when_congested = true;
   };
 
   JitterEstimator(Clock* clock, const FieldTrialsView& field_trials);

modules/video_coding/timing/jitter_estimator_unittest.cc

@@ -161,6 +161,24 @@ TEST_F(JitterEstimatorTest, Single2xFrameSizeImpactsJitterEstimate) {
   EXPECT_GT(outlier_jitter.ms(), steady_state_jitter.ms());
 }
 
+// Under the default config, congested frames are used when calculating the
+// noise variance, meaning that they will impact the final jitter estimate.
+TEST_F(JitterEstimatorTest, CongestedFrameImpactsJitterEstimate) {
+  ValueGenerator gen(10);
+
+  // Steady state.
+  Run(/*duration_s=*/10, /*framerate_fps=*/30, gen);
+  TimeDelta steady_state_jitter =
+      estimator_.GetJitterEstimate(0, absl::nullopt);
+
+  // Congested frame...
+  estimator_.UpdateEstimate(-10 * gen.Delay(), 0.1 * gen.FrameSize());
+  TimeDelta outlier_jitter = estimator_.GetJitterEstimate(0, absl::nullopt);
+
+  // ...impacts the estimate.
+  EXPECT_GT(outlier_jitter.ms(), steady_state_jitter.ms());
+}
+
 TEST_F(JitterEstimatorTest, EmptyFieldTrialsParsesToUnsetConfig) {
   JitterEstimator::Config config = estimator_.GetConfigForTest();
   EXPECT_FALSE(config.avg_frame_size_median);
@@ -170,6 +188,7 @@ TEST_F(JitterEstimatorTest, EmptyFieldTrialsParsesToUnsetConfig) {
   EXPECT_FALSE(config.num_stddev_delay_outlier.has_value());
   EXPECT_FALSE(config.num_stddev_size_outlier.has_value());
   EXPECT_FALSE(config.congestion_rejection_factor.has_value());
+  EXPECT_TRUE(config.estimate_noise_when_congested);
 }
 
 class FieldTrialsOverriddenJitterEstimatorTest : public JitterEstimatorTest {
@@ -183,7 +202,8 @@ class FieldTrialsOverriddenJitterEstimatorTest : public JitterEstimatorTest {
             "num_stddev_delay_clamp:1.1,"
             "num_stddev_delay_outlier:2,"
             "num_stddev_size_outlier:3.1,"
-            "congestion_rejection_factor:-1.55/") {}
+            "congestion_rejection_factor:-1.55,"
+            "estimate_noise_when_congested:false/") {}
   ~FieldTrialsOverriddenJitterEstimatorTest() {}
 };
 
@@ -196,6 +216,7 @@ TEST_F(FieldTrialsOverriddenJitterEstimatorTest, FieldTrialsParsesCorrectly) {
   EXPECT_EQ(*config.num_stddev_delay_outlier, 2.0);
   EXPECT_EQ(*config.num_stddev_size_outlier, 3.1);
   EXPECT_EQ(*config.congestion_rejection_factor, -1.55);
+  EXPECT_FALSE(config.estimate_noise_when_congested);
 }
 
 TEST_F(FieldTrialsOverriddenJitterEstimatorTest,
@@ -239,6 +260,25 @@ TEST_F(FieldTrialsOverriddenJitterEstimatorTest,
   EXPECT_GT(outlier_jitter_4x.ms(), outlier_jitter_3x.ms());
 }
 
+// When so configured, congested frames are NOT used when calculating the
+// noise variance, meaning that they will NOT impact the final jitter estimate.
+TEST_F(FieldTrialsOverriddenJitterEstimatorTest,
+       CongestedFrameDoesNotImpactJitterEstimate) {
+  ValueGenerator gen(10);
+
+  // Steady state.
+  Run(/*duration_s=*/10, /*framerate_fps=*/30, gen);
+  TimeDelta steady_state_jitter =
+      estimator_.GetJitterEstimate(0, absl::nullopt);
+
+  // Congested frame...
+  estimator_.UpdateEstimate(-10 * gen.Delay(), 0.1 * gen.FrameSize());
+  TimeDelta outlier_jitter = estimator_.GetJitterEstimate(0, absl::nullopt);
+
+  // ...does not impact the estimate.
+  EXPECT_EQ(outlier_jitter.ms(), steady_state_jitter.ms());
+}
+
 class MisconfiguredFieldTrialsJitterEstimatorTest : public JitterEstimatorTest {
  protected:
   MisconfiguredFieldTrialsJitterEstimatorTest()