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}

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",

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<size_t>(
           config_.frame_size_window.value_or(kDefaultFrameSizeWindow))),
       max_frame_size_bytes_percentile_(

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 @@ 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<StructParametersParser> Parser() {
       // clang-format off

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