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 <brandtr@webrtc.org>
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38090}
diff --git a/modules/video_coding/timing/BUILD.gn b/modules/video_coding/timing/BUILD.gn
index 985223f..d2479ae 100644
--- a/modules/video_coding/timing/BUILD.gn
+++ b/modules/video_coding/timing/BUILD.gn
@@ -60,6 +60,7 @@
"../../../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 cc8680c..a314abd 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 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<size_t>(
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 d7561c2..dd55907 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 <algorithm>
#include <memory>
#include <queue>
@@ -38,11 +39,8 @@
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<StructParametersParser> 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 f53208c..b018d48 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 @@
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
