diff --git a/modules/video_coding/timing/BUILD.gn b/modules/video_coding/timing/BUILD.gn index 8dea3deb07..33eb81f45f 100644 --- a/modules/video_coding/timing/BUILD.gn +++ b/modules/video_coding/timing/BUILD.gn @@ -60,9 +60,13 @@ rtc_library("jitter_estimator") { "../../../api/units:timestamp", "../../../rtc_base", "../../../rtc_base:safe_conversions", + "../../../rtc_base/experiments:field_trial_parser", "../../../system_wrappers", ] - absl_deps = [ "//third_party/abseil-cpp/absl/types:optional" ] + absl_deps = [ + "//third_party/abseil-cpp/absl/strings", + "//third_party/abseil-cpp/absl/types:optional", + ] } rtc_library("rtt_filter") { @@ -115,6 +119,7 @@ rtc_library("timing_unittests") { ":rtt_filter", ":timing_module", "../../../api:array_view", + "../../../api:field_trials", "../../../api/units:data_size", "../../../api/units:frequency", "../../../api/units:time_delta", diff --git a/modules/video_coding/timing/jitter_estimator.cc b/modules/video_coding/timing/jitter_estimator.cc index b042245235..ef5faa2781 100644 --- a/modules/video_coding/timing/jitter_estimator.cc +++ b/modules/video_coding/timing/jitter_estimator.cc @@ -79,10 +79,13 @@ constexpr Frequency kMaxFramerateEstimate = Frequency::Hertz(200); } // namespace +constexpr char JitterEstimator::Config::kFieldTrialsKey[]; + JitterEstimator::JitterEstimator(Clock* clock, const FieldTrialsView& field_trials) - : fps_counter_(30), // TODO(sprang): Use an estimator with limit based on - // time, rather than number of samples. + : config_(Config::Parse(field_trials.Lookup(Config::kFieldTrialsKey))), + fps_counter_(30), // TODO(sprang): Use an estimator with limit based + // on time, rather than number of samples. clock_(clock) { Reset(); } @@ -164,12 +167,14 @@ void JitterEstimator::UpdateEstimate(TimeDelta frame_delay, kalman_filter_.GetFrameDelayVariationEstimateTotal(delta_frame_bytes); // Outlier rejection. + double num_stddev_delay_outlier = GetNumStddevDelayOutlier(); bool abs_delay_is_not_outlier = - fabs(delay_deviation_ms) < kNumStdDevDelayOutlier * sqrt(var_noise_ms2_); + fabs(delay_deviation_ms) < + num_stddev_delay_outlier * sqrt(var_noise_ms2_); bool size_is_positive_outlier = frame_size.bytes() > avg_frame_size_bytes_ + - kNumStdDevSizeOutlier * sqrt(var_frame_size_bytes2_); + GetNumStddevSizeOutlier() * sqrt(var_frame_size_bytes2_); // 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 @@ -186,14 +191,14 @@ void JitterEstimator::UpdateEstimate(TimeDelta frame_delay, // will be << 0. This removes all frame samples which arrives after a key // frame. if (delta_frame_bytes > - kCongestionRejectionFactor * max_frame_size_bytes_) { + GetCongestionRejectionFactor() * max_frame_size_bytes_) { // Update the Kalman filter with the new data kalman_filter_.PredictAndUpdate(frame_delay.ms(), delta_frame_bytes, max_frame_size_bytes_, var_noise_ms2_); } } else { - double num_stddev = (delay_deviation_ms >= 0) ? kNumStdDevDelayOutlier - : -kNumStdDevDelayOutlier; + double num_stddev = (delay_deviation_ms >= 0) ? num_stddev_delay_outlier + : -num_stddev_delay_outlier; EstimateRandomJitter(num_stddev * sqrt(var_noise_ms2_)); } // Post process the total estimated jitter @@ -212,6 +217,27 @@ void JitterEstimator::FrameNacked() { latest_nack_ = clock_->CurrentTime(); } +void JitterEstimator::UpdateRtt(TimeDelta rtt) { + rtt_filter_.Update(rtt); +} + +JitterEstimator::Config JitterEstimator::GetConfigForTest() const { + return config_; +} + +double JitterEstimator::GetNumStddevDelayOutlier() const { + return config_.num_stddev_delay_outlier.value_or(kNumStdDevDelayOutlier); +} + +double JitterEstimator::GetNumStddevSizeOutlier() const { + return config_.num_stddev_size_outlier.value_or(kNumStdDevSizeOutlier); +} + +double JitterEstimator::GetCongestionRejectionFactor() const { + return config_.congestion_rejection_factor.value_or( + kCongestionRejectionFactor); +} + // Estimates the random jitter by calculating the variance of the sample // distance from the line given by the Kalman filter. void JitterEstimator::EstimateRandomJitter(double d_dT) { @@ -294,10 +320,6 @@ void JitterEstimator::PostProcessEstimate() { filter_jitter_estimate_ = CalculateEstimate(); } -void JitterEstimator::UpdateRtt(TimeDelta rtt) { - rtt_filter_.Update(rtt); -} - // Returns the current filtered estimate if available, // otherwise tries to calculate an estimate. TimeDelta JitterEstimator::GetJitterEstimate( diff --git a/modules/video_coding/timing/jitter_estimator.h b/modules/video_coding/timing/jitter_estimator.h index d7b29e86ee..6ae4729d3f 100644 --- a/modules/video_coding/timing/jitter_estimator.h +++ b/modules/video_coding/timing/jitter_estimator.h @@ -11,6 +11,9 @@ #ifndef MODULES_VIDEO_CODING_TIMING_JITTER_ESTIMATOR_H_ #define MODULES_VIDEO_CODING_TIMING_JITTER_ESTIMATOR_H_ +#include + +#include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "api/field_trials_view.h" #include "api/units/data_size.h" @@ -19,6 +22,7 @@ #include "api/units/timestamp.h" #include "modules/video_coding/timing/frame_delay_variation_kalman_filter.h" #include "modules/video_coding/timing/rtt_filter.h" +#include "rtc_base/experiments/struct_parameters_parser.h" #include "rtc_base/rolling_accumulator.h" namespace webrtc { @@ -27,10 +31,50 @@ class Clock; class JitterEstimator { public: - explicit JitterEstimator(Clock* clock, const FieldTrialsView& field_trials); - ~JitterEstimator(); + // Configuration struct for statically overriding some constants and + // behaviour, configurable through field trials. + struct Config { + static constexpr char kFieldTrialsKey[] = "WebRTC-JitterEstimatorConfig"; + + static Config Parse(absl::string_view field_trial) { + Config config; + config.Parser()->Parse(field_trial); + return config; + } + + std::unique_ptr Parser() { + return StructParametersParser::Create( + "num_stddev_delay_outlier", &num_stddev_delay_outlier, + "num_stddev_size_outlier", &num_stddev_size_outlier, + "congestion_rejection_factor", &congestion_rejection_factor); + } + + // A (relative) frame delay variation sample is an outlier if its absolute + // deviation from the Kalman filter model falls outside this number of + // sample standard deviations. + // + // Increasing this value rejects fewer samples. + absl::optional num_stddev_delay_outlier; + + // An (absolute) frame size sample is an outlier if its positive deviation + // from the estimated average frame size falls outside this number of sample + // standard deviations. + // + // Increasing this value rejects fewer samples. + absl::optional num_stddev_size_outlier; + + // A (relative) frame size variation sample is deemed "congested", and is + // thus rejected, if its value is less than this factor times the estimated + // max frame size. + // + // Decreasing this value rejects fewer samples. + absl::optional congestion_rejection_factor; + }; + + JitterEstimator(Clock* clock, const FieldTrialsView& field_trials); JitterEstimator(const JitterEstimator&) = delete; JitterEstimator& operator=(const JitterEstimator&) = delete; + ~JitterEstimator(); // Resets the estimate to the initial state. void Reset(); @@ -61,7 +105,15 @@ class JitterEstimator { // - rtt : Round trip time. void UpdateRtt(TimeDelta rtt); + // Returns the configuration. Only to be used by unit tests. + Config GetConfigForTest() const; + private: + // These functions return values that could be overriden through the config. + double GetNumStddevDelayOutlier() const; + double GetNumStddevSizeOutlier() const; + double GetCongestionRejectionFactor() const; + // Updates the random jitter estimate, i.e. the variance of the time // deviations from the line given by the Kalman filter. // @@ -82,6 +134,9 @@ class JitterEstimator { // Returns the estimated incoming frame rate. Frequency GetFrameRate() const; + // Configuration that may override some internals. + const Config config_; + // Filters the {frame_delay_delta, frame_size_delta} measurements through // a linear Kalman filter. FrameDelayVariationKalmanFilter kalman_filter_; diff --git a/modules/video_coding/timing/jitter_estimator_unittest.cc b/modules/video_coding/timing/jitter_estimator_unittest.cc index 565295b20e..612e7d7918 100644 --- a/modules/video_coding/timing/jitter_estimator_unittest.cc +++ b/modules/video_coding/timing/jitter_estimator_unittest.cc @@ -11,11 +11,14 @@ #include +#include +#include #include #include #include "absl/types/optional.h" #include "api/array_view.h" +#include "api/field_trials.h" #include "api/units/data_size.h" #include "api/units/frequency.h" #include "api/units/time_delta.h" @@ -53,10 +56,12 @@ class ValueGenerator { class JitterEstimatorTest : public ::testing::Test { protected: - JitterEstimatorTest() + explicit JitterEstimatorTest(const std::string& field_trials) : fake_clock_(0), - field_trials_(""), - estimator_(&fake_clock_, field_trials_) {} + field_trials_(FieldTrials::CreateNoGlobal(field_trials)), + estimator_(&fake_clock_, *field_trials_) {} + JitterEstimatorTest() : JitterEstimatorTest("") {} + virtual ~JitterEstimatorTest() {} void Run(int duration_s, int framerate_fps, ValueGenerator& gen) { TimeDelta tick = 1 / Frequency::Hertz(framerate_fps); @@ -68,7 +73,7 @@ class JitterEstimatorTest : public ::testing::Test { } SimulatedClock fake_clock_; - test::ScopedKeyValueConfig field_trials_; + std::unique_ptr field_trials_; JitterEstimator estimator_; }; @@ -78,10 +83,6 @@ TEST_F(JitterEstimatorTest, SteadyStateConvergence) { EXPECT_EQ(estimator_.GetJitterEstimate(0, absl::nullopt).ms(), 54); } -// TODO(brandtr): Add test for delay outlier, when the corresponding std dev -// has been configurable. With the current default (n = 15), it seems -// statistically impossible for a delay to be an outlier. - TEST_F(JitterEstimatorTest, SizeOutlierIsNotRejectedAndIncreasesJitterEstimate) { ValueGenerator gen(10); @@ -91,10 +92,11 @@ TEST_F(JitterEstimatorTest, TimeDelta steady_state_jitter = estimator_.GetJitterEstimate(0, absl::nullopt); - // A single outlier frame size. + // A single outlier frame size... estimator_.UpdateEstimate(gen.Delay(), 10 * gen.FrameSize()); TimeDelta outlier_jitter = estimator_.GetJitterEstimate(0, absl::nullopt); + // ...changes the estimate. EXPECT_GT(outlier_jitter.ms(), 1.25 * steady_state_jitter.ms()); } @@ -143,5 +145,47 @@ TEST_F(JitterEstimatorTest, RttMultAddCap) { *jitter_by_rtt_mult_cap[0].second.GetPercentile(1.0) * 1.25); } +TEST_F(JitterEstimatorTest, EmptyFieldTrialsParsesToUnsetConfig) { + JitterEstimator::Config config = estimator_.GetConfigForTest(); + 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()); +} + +class FieldTrialsOverriddenJitterEstimatorTest : public JitterEstimatorTest { + protected: + FieldTrialsOverriddenJitterEstimatorTest() + : JitterEstimatorTest( + "WebRTC-JitterEstimatorConfig/" + "num_stddev_delay_outlier:2," + "num_stddev_size_outlier:3.1," + "congestion_rejection_factor:-1.55/") {} + ~FieldTrialsOverriddenJitterEstimatorTest() {} +}; + +TEST_F(FieldTrialsOverriddenJitterEstimatorTest, FieldTrialsParsesCorrectly) { + JitterEstimator::Config config = estimator_.GetConfigForTest(); + 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); +} + +TEST_F(FieldTrialsOverriddenJitterEstimatorTest, + DelayOutlierIsRejectedAndMaintainsJitterEstimate) { + ValueGenerator gen(10); + + // Steady state. + Run(/*duration_s=*/60, /*framerate_fps=*/30, gen); + TimeDelta steady_state_jitter = + estimator_.GetJitterEstimate(0, absl::nullopt); + + // A single outlier frame size... + estimator_.UpdateEstimate(10 * gen.Delay(), gen.FrameSize()); + TimeDelta outlier_jitter = estimator_.GetJitterEstimate(0, absl::nullopt); + + // ...does not change the estimate. + EXPECT_EQ(outlier_jitter.ms(), steady_state_jitter.ms()); +} + } // namespace } // namespace webrtc