From 94b3b5e341e811407c6fbf163324ed282b5a09ea Mon Sep 17 00:00:00 2001
From: Rasmus Brandt <brandtr@webrtc.org>
Date: Tue, 16 Aug 2022 09:50:38 +0200
Subject: [PATCH] Improve sanity checks

- Move existing check on `max_frame_size` to the top.
    By doing this early, the filter will not end up in an
    inconsistent state (predicted but not updated) when
    called with a tiny `max_frame_size`.
- Add sanity check on noise variance.
    This will avoid sqrt of a negative number.

Bug: webrtc:14151
Change-Id: I2507a9114655d3c3ae35bf5ed83f3f1154c42ad3
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/271281
Commit-Queue: Rasmus Brandt <brandtr@webrtc.org>
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37798}
---
 .../timing/frame_delay_delta_kalman_filter.cc | 13 +++++----
 ...rame_delay_delta_kalman_filter_unittest.cc | 27 +++++++++++++++++++
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/modules/video_coding/timing/frame_delay_delta_kalman_filter.cc b/modules/video_coding/timing/frame_delay_delta_kalman_filter.cc
index c5081eb16d..69af4e25e4 100644
--- a/modules/video_coding/timing/frame_delay_delta_kalman_filter.cc
+++ b/modules/video_coding/timing/frame_delay_delta_kalman_filter.cc
@@ -40,6 +40,14 @@ void FrameDelayDeltaKalmanFilter::PredictAndUpdate(
     double frame_size_variation_bytes,
     DataSize max_frame_size,
     double var_noise) {
+  // Sanity checks.
+  if (max_frame_size < DataSize::Bytes(1)) {
+    return;
+  }
+  if (var_noise <= 0.0) {
+    return;
+  }
+
   // This member function follows the data flow in
   // https://en.wikipedia.org/wiki/Kalman_filter#Details.
 
@@ -66,11 +74,6 @@ void FrameDelayDeltaKalmanFilter::PredictAndUpdate(
       estimate_cov_[0][0] * frame_size_variation_bytes + estimate_cov_[0][1];
   estim_cov_times_obs[1] =
       estimate_cov_[1][0] * frame_size_variation_bytes + estimate_cov_[1][1];
-  // TODO(brandtr): Why is this check placed in the middle of this function?
-  // Should it be moved to the top?
-  if (max_frame_size < DataSize::Bytes(1)) {
-    return;
-  }
   double observation_noise_stddev =
       (300.0 * exp(-fabs(frame_size_variation_bytes) /
                    (1e0 * max_frame_size.bytes())) +
diff --git a/modules/video_coding/timing/frame_delay_delta_kalman_filter_unittest.cc b/modules/video_coding/timing/frame_delay_delta_kalman_filter_unittest.cc
index 572595b5d4..d4c1fbdba0 100644
--- a/modules/video_coding/timing/frame_delay_delta_kalman_filter_unittest.cc
+++ b/modules/video_coding/timing/frame_delay_delta_kalman_filter_unittest.cc
@@ -58,6 +58,33 @@ TEST(FrameDelayDeltaKalmanFilterTest,
       expected_frame_delay_variation_estimate_ms);
 }
 
+TEST(FrameDelayDeltaKalmanFilterTest,
+     NegativeNoiseVarianceDoesNotUpdateFilter) {
+  FrameDelayDeltaKalmanFilter filter;
+
+  // Negative variance...
+  double var_noise = -0.1;
+  filter.PredictAndUpdate(/*frame_delay_variation=*/TimeDelta::Millis(3),
+                          /*frame_size_variation_bytes=*/200.0,
+                          /*max_frame_size=*/DataSize::Bytes(2000), var_noise);
+
+  // ...does _not_ update the filter.
+  EXPECT_EQ(filter.GetFrameDelayVariationEstimateTotal(
+                /*frame_size_variation_bytes=*/0.0),
+            0.0);
+
+  // Positive variance...
+  var_noise = 0.1;
+  filter.PredictAndUpdate(/*frame_delay_variation=*/TimeDelta::Millis(3),
+                          /*frame_size_variation_bytes=*/200.0,
+                          /*max_frame_size=*/DataSize::Bytes(2000), var_noise);
+
+  // ...does update the filter.
+  EXPECT_GT(filter.GetFrameDelayVariationEstimateTotal(
+                /*frame_size_variation_bytes=*/0.0),
+            0.0);
+}
+
 TEST(FrameDelayDeltaKalmanFilterTest,
      VerifyConvergenceWithAlternatingDeviations) {
   FrameDelayDeltaKalmanFilter filter;