diff --git a/modules/video_coding/timing/BUILD.gn b/modules/video_coding/timing/BUILD.gn index 985223fac6..d2479aec8d 100644 --- a/modules/video_coding/timing/BUILD.gn +++ b/modules/video_coding/timing/BUILD.gn @@ -60,6 +60,7 @@ rtc_library("jitter_estimator") { "../../../api/units:timestamp", "../../../rtc_base", "../../../rtc_base:checks", + "../../../rtc_base:logging", "../../../rtc_base:rtc_numerics", "../../../rtc_base:safe_conversions", "../../../rtc_base/experiments:field_trial_parser", diff --git a/modules/video_coding/timing/jitter_estimator.cc b/modules/video_coding/timing/jitter_estimator.cc index cc8680ce64..a314abd3fc 100644 --- a/modules/video_coding/timing/jitter_estimator.cc +++ b/modules/video_coding/timing/jitter_estimator.cc @@ -24,6 +24,7 @@ #include "api/units/timestamp.h" #include "modules/video_coding/timing/rtt_filter.h" #include "rtc_base/checks.h" +#include "rtc_base/logging.h" #include "rtc_base/numerics/safe_conversions.h" #include "system_wrappers/include/clock.h" @@ -85,9 +86,48 @@ constexpr Frequency kMaxFramerateEstimate = Frequency::Hertz(200); constexpr char JitterEstimator::Config::kFieldTrialsKey[]; +JitterEstimator::Config JitterEstimator::Config::ParseAndValidate( + absl::string_view field_trial) { + Config config; + config.Parser()->Parse(field_trial); + + // The `MovingPercentileFilter` RTC_CHECKs on the validity of the + // percentile and window length, so we'd better validate the field trial + // provided values here. + if (config.max_frame_size_percentile) { + double original = *config.max_frame_size_percentile; + config.max_frame_size_percentile = std::min(std::max(0.0, original), 1.0); + if (config.max_frame_size_percentile != original) { + RTC_LOG(LS_ERROR) << "Skipping invalid max_frame_size_percentile=" + << original; + } + } + if (config.frame_size_window && config.frame_size_window < 1) { + RTC_LOG(LS_ERROR) << "Skipping invalid frame_size_window=" + << *config.frame_size_window; + config.frame_size_window = 1; + } + + // General sanity checks. + if (config.num_stddev_delay_outlier && + config.num_stddev_delay_outlier < 0.0) { + RTC_LOG(LS_ERROR) << "Skipping invalid num_stddev_delay_outlier=" + << *config.num_stddev_delay_outlier; + config.num_stddev_delay_outlier = 0.0; + } + if (config.num_stddev_size_outlier && config.num_stddev_size_outlier < 0.0) { + RTC_LOG(LS_ERROR) << "Skipping invalid num_stddev_size_outlier=" + << *config.num_stddev_size_outlier; + config.num_stddev_size_outlier = 0.0; + } + + return config; +} + JitterEstimator::JitterEstimator(Clock* clock, const FieldTrialsView& field_trials) - : config_(Config::Parse(field_trials.Lookup(Config::kFieldTrialsKey))), + : config_(Config::ParseAndValidate( + field_trials.Lookup(Config::kFieldTrialsKey))), avg_frame_size_median_bytes_(static_cast( config_.frame_size_window.value_or(kDefaultFrameSizeWindow))), max_frame_size_bytes_percentile_( diff --git a/modules/video_coding/timing/jitter_estimator.h b/modules/video_coding/timing/jitter_estimator.h index d7561c2211..dd55907421 100644 --- a/modules/video_coding/timing/jitter_estimator.h +++ b/modules/video_coding/timing/jitter_estimator.h @@ -11,6 +11,7 @@ #ifndef MODULES_VIDEO_CODING_TIMING_JITTER_ESTIMATOR_H_ #define MODULES_VIDEO_CODING_TIMING_JITTER_ESTIMATOR_H_ +#include #include #include @@ -38,11 +39,8 @@ class JitterEstimator { 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; - } + // Parses a field trial string and validates the values. + static Config ParseAndValidate(absl::string_view field_trial); std::unique_ptr Parser() { // clang-format off diff --git a/modules/video_coding/timing/jitter_estimator_unittest.cc b/modules/video_coding/timing/jitter_estimator_unittest.cc index f53208c4a9..b018d48055 100644 --- a/modules/video_coding/timing/jitter_estimator_unittest.cc +++ b/modules/video_coding/timing/jitter_estimator_unittest.cc @@ -26,7 +26,6 @@ #include "rtc_base/time_utils.h" #include "system_wrappers/include/clock.h" #include "test/gtest.h" -#include "test/scoped_key_value_config.h" namespace webrtc { namespace { @@ -237,5 +236,25 @@ TEST_F(FieldTrialsOverriddenJitterEstimatorTest, EXPECT_GT(outlier_jitter_4x.ms(), outlier_jitter_3x.ms()); } +class MisconfiguredFieldTrialsJitterEstimatorTest : public JitterEstimatorTest { + protected: + MisconfiguredFieldTrialsJitterEstimatorTest() + : JitterEstimatorTest( + "WebRTC-JitterEstimatorConfig/" + "max_frame_size_percentile:-0.9," + "frame_size_window:-1," + "num_stddev_delay_outlier:-2," + "num_stddev_size_outlier:-23.1/") {} + ~MisconfiguredFieldTrialsJitterEstimatorTest() {} +}; + +TEST_F(MisconfiguredFieldTrialsJitterEstimatorTest, FieldTrialsAreValidated) { + JitterEstimator::Config config = estimator_.GetConfigForTest(); + EXPECT_EQ(*config.max_frame_size_percentile, 0.0); + EXPECT_EQ(*config.frame_size_window, 1); + EXPECT_EQ(*config.num_stddev_delay_outlier, 0.0); + EXPECT_EQ(*config.num_stddev_size_outlier, 0.0); +} + } // namespace } // namespace webrtc