From 7d704739d7ec1407a45b751189b16e4d99649049 Mon Sep 17 00:00:00 2001 From: Rasmus Brandt Date: Thu, 15 Sep 2022 12:36:04 +0200 Subject: [PATCH] JitterEstimator: add input validation to field trials This functionality should have been added as part of the original CLs, but was missed. The purpose of the validation is avoid catastrophic failures due to misconfiguration (such as RTC_CHECK crashes). The purpose is not to always provide practically reasonable values. Bug: webrtc:14151 Change-Id: Icbddade865bd6a868f467a1df7055026935f36f2 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/275560 Commit-Queue: Rasmus Brandt Reviewed-by: Philip Eliasson Cr-Commit-Position: refs/heads/main@{#38090} --- modules/video_coding/timing/BUILD.gn | 1 + .../video_coding/timing/jitter_estimator.cc | 42 ++++++++++++++++++- .../video_coding/timing/jitter_estimator.h | 8 ++-- .../timing/jitter_estimator_unittest.cc | 21 +++++++++- 4 files changed, 65 insertions(+), 7 deletions(-) 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