From 06e214888939ed129989cd8cc3e04b54d166988c Mon Sep 17 00:00:00 2001
From: Jeremy Leconte <jleconte@google.com>
Date: Fri, 7 Apr 2023 10:09:12 +0200
Subject: [PATCH] Deflake simulcast flow tests: prevent negative Timestamp
 exception.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

These tests often fail in 'ExtrapolateLocalTime' because the result gives a negative Timestamp.

Here is the stack from https://chromium-swarm.appspot.com/task?id=6173230e67897b10:
PC: @     0x7f03afdb8e87  (unknown)  raise
    ...
    @     0x55f4a360ba71        352  webrtc::Timestamp::operator+()
    @     0x55f4a47ecaf3        160  webrtc::TimestampExtrapolator::ExtrapolateLocalTime()

Low-Coverage-Reason: coverage isn't that low.
Change-Id: If3e7cbf31d6c4800727b24352ed2c6edc425fc73
Bug: webrtc:15022
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300600
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Rasmus Brandt <brandtr@webrtc.org>
Commit-Queue: Jeremy Leconte <jleconte@google.com>
Cr-Commit-Position: refs/heads/main@{#39853}
---
 .../timing/timestamp_extrapolator.cc            | 15 +++++++++++++--
 .../timing/timestamp_extrapolator_unittest.cc   | 17 +++++++++++++++--
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/modules/video_coding/timing/timestamp_extrapolator.cc b/modules/video_coding/timing/timestamp_extrapolator.cc
index c91aa1a362..a90df8bf7f 100644
--- a/modules/video_coding/timing/timestamp_extrapolator.cc
+++ b/modules/video_coding/timing/timestamp_extrapolator.cc
@@ -132,13 +132,24 @@ absl::optional<Timestamp> TimestampExtrapolator::ExtrapolateLocalTime(
     constexpr double kRtpTicksPerMs = 90;
     TimeDelta diff = TimeDelta::Millis(
         (unwrapped_ts90khz - *prev_unwrapped_timestamp_) / kRtpTicksPerMs);
+    if (prev_.us() + diff.us() < 0) {
+      // Prevent the construction of a negative Timestamp.
+      // This scenario can occur when the RTP timestamp wraps around.
+      return absl::nullopt;
+    }
     return prev_ + diff;
   } else if (w_[0] < 1e-3) {
     return start_;
   } else {
     double timestampDiff = unwrapped_ts90khz - *first_unwrapped_timestamp_;
-    auto diff_ms = static_cast<int64_t>((timestampDiff - w_[1]) / w_[0] + 0.5);
-    return start_ + TimeDelta::Millis(diff_ms);
+    TimeDelta diff = TimeDelta::Millis(
+        static_cast<int64_t>((timestampDiff - w_[1]) / w_[0] + 0.5));
+    if (start_.us() + diff.us() < 0) {
+      // Prevent the construction of a negative Timestamp.
+      // This scenario can occur when the RTP timestamp wraps around.
+      return absl::nullopt;
+    }
+    return start_ + diff;
   }
 }
 
diff --git a/modules/video_coding/timing/timestamp_extrapolator_unittest.cc b/modules/video_coding/timing/timestamp_extrapolator_unittest.cc
index 0b5fd74a8e..d6c8fa9de1 100644
--- a/modules/video_coding/timing/timestamp_extrapolator_unittest.cc
+++ b/modules/video_coding/timing/timestamp_extrapolator_unittest.cc
@@ -127,12 +127,25 @@ TEST(TimestampExtrapolatorTest, NegativeRtpTimestampWrapAround) {
   ts_extrapolator.Update(clock.CurrentTime(), rtp);
   EXPECT_THAT(ts_extrapolator.ExtrapolateLocalTime(rtp),
               Optional(clock.CurrentTime()));
-  // Go backwards!
-  rtp -= kRtpHz.hertz();
+  // Go backwards! Static cast to avoid undefined behaviour with -=.
+  rtp -= static_cast<uint32_t>(kRtpHz.hertz());
   EXPECT_THAT(ts_extrapolator.ExtrapolateLocalTime(rtp),
               Optional(clock.CurrentTime() - TimeDelta::Seconds(1)));
 }
 
+TEST(TimestampExtrapolatorTest, NegativeRtpTimestampWrapAroundSecondScenario) {
+  SimulatedClock clock(Timestamp::Millis(1337));
+  TimestampExtrapolator ts_extrapolator(clock.CurrentTime());
+  uint32_t rtp = 0;
+  ts_extrapolator.Update(clock.CurrentTime(), rtp);
+  EXPECT_THAT(ts_extrapolator.ExtrapolateLocalTime(rtp),
+              Optional(clock.CurrentTime()));
+  // Go backwards! Static cast to avoid undefined behaviour with -=.
+  rtp -= static_cast<uint32_t>(kRtpHz * TimeDelta::Seconds(10));
+  ts_extrapolator.Update(clock.CurrentTime(), rtp);
+  EXPECT_THAT(ts_extrapolator.ExtrapolateLocalTime(rtp), absl::nullopt);
+}
+
 TEST(TimestampExtrapolatorTest, Slow90KHzClock) {
   // This simulates a slow camera, which produces frames at 24Hz instead of
   // 25Hz. The extrapolator should be able to resolve this with enough data.