From e4bc975a8ac85ac51b8df237356271fb09717cfa Mon Sep 17 00:00:00 2001 From: Rasmus Brandt Date: Fri, 16 Sep 2022 13:27:47 +0200 Subject: [PATCH] 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 Reviewed-by: Philip Eliasson Cr-Commit-Position: refs/heads/main@{#38100} --- .../video_coding/timing/jitter_estimator.cc | 20 ++++++--- .../video_coding/timing/jitter_estimator.h | 11 ++++- .../timing/jitter_estimator_unittest.cc | 42 ++++++++++++++++++- 3 files changed, 64 insertions(+), 9 deletions(-) diff --git a/modules/video_coding/timing/jitter_estimator.cc b/modules/video_coding/timing/jitter_estimator.cc index 22fdcacf03..2416ed02ed 100644 --- a/modules/video_coding/timing/jitter_estimator.cc +++ b/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 > - GetCongestionRejectionFactor() * filtered_max_frame_size_bytes) { - // Update the Kalman filter with the new data + bool is_not_congested = + delta_frame_bytes > + 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_)); diff --git a/modules/video_coding/timing/jitter_estimator.h b/modules/video_coding/timing/jitter_estimator.h index c615be10e4..769bb12a0c 100644 --- a/modules/video_coding/timing/jitter_estimator.h +++ b/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 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); diff --git a/modules/video_coding/timing/jitter_estimator_unittest.cc b/modules/video_coding/timing/jitter_estimator_unittest.cc index 2602b0d4de..8e0c01587f 100644 --- a/modules/video_coding/timing/jitter_estimator_unittest.cc +++ b/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()