Deflake simulcast flow tests: prevent negative Timestamp exception.
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}
diff --git a/modules/video_coding/timing/timestamp_extrapolator.cc b/modules/video_coding/timing/timestamp_extrapolator.cc
index c91aa1a..a90df8b 100644
--- a/modules/video_coding/timing/timestamp_extrapolator.cc
+++ b/modules/video_coding/timing/timestamp_extrapolator.cc
@@ -132,13 +132,24 @@
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 0b5fd74..d6c8fa9 100644
--- a/modules/video_coding/timing/timestamp_extrapolator_unittest.cc
+++ b/modules/video_coding/timing/timestamp_extrapolator_unittest.cc
@@ -127,12 +127,25 @@
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.
