From 1009cfca47a2e2765c375c4f8591125c1c222f23 Mon Sep 17 00:00:00 2001 From: stefan Date: Fri, 30 Jun 2017 06:28:13 -0700 Subject: [PATCH] More gracefully handle rtp timestamp jumps in the rtp to ntp estimator. BUG=webrtc:7905 Review-Url: https://codereview.webrtc.org/2963133003 Cr-Commit-Position: refs/heads/master@{#18860} --- .../include/rtp_to_ntp_estimator.h | 3 + .../source/rtp_to_ntp_estimator.cc | 66 ++++++++++--------- .../source/rtp_to_ntp_estimator_unittest.cc | 42 ++++++++++++ 3 files changed, 81 insertions(+), 30 deletions(-) diff --git a/webrtc/system_wrappers/include/rtp_to_ntp_estimator.h b/webrtc/system_wrappers/include/rtp_to_ntp_estimator.h index a17428bf27..9d6da051d5 100644 --- a/webrtc/system_wrappers/include/rtp_to_ntp_estimator.h +++ b/webrtc/system_wrappers/include/rtp_to_ntp_estimator.h @@ -54,9 +54,12 @@ class RtpToNtpEstimator { const Parameters& params() const { return params_; } + static const int kMaxInvalidSamples = 3; + private: void UpdateParameters(); + int consecutive_invalid_samples_; std::list measurements_; Parameters params_; }; diff --git a/webrtc/system_wrappers/source/rtp_to_ntp_estimator.cc b/webrtc/system_wrappers/source/rtp_to_ntp_estimator.cc index 339635e88d..baa218d4ae 100644 --- a/webrtc/system_wrappers/source/rtp_to_ntp_estimator.cc +++ b/webrtc/system_wrappers/source/rtp_to_ntp_estimator.cc @@ -54,30 +54,6 @@ bool Contains(const std::list& measurements, } return false; } - -bool IsValid(const std::list& measurements, - const RtpToNtpEstimator::RtcpMeasurement& other) { - if (!other.ntp_time.Valid()) - return false; - - int64_t ntp_ms_new = other.ntp_time.ToMs(); - for (const auto& measurement : measurements) { - if (ntp_ms_new <= measurement.ntp_time.ToMs()) { - // Old report. - return false; - } - int64_t timestamp_new = other.rtp_timestamp; - if (!CompensateForWrapAround(timestamp_new, measurement.rtp_timestamp, - ×tamp_new)) { - return false; - } - if (timestamp_new <= measurement.rtp_timestamp) { - LOG(LS_WARNING) << "Newer RTCP SR report with older RTP timestamp."; - return false; - } - } - return true; -} } // namespace RtpToNtpEstimator::RtcpMeasurement::RtcpMeasurement(uint32_t ntp_secs, @@ -93,7 +69,7 @@ bool RtpToNtpEstimator::RtcpMeasurement::IsEqual( } // Class for converting an RTP timestamp to the NTP domain. -RtpToNtpEstimator::RtpToNtpEstimator() {} +RtpToNtpEstimator::RtpToNtpEstimator() : consecutive_invalid_samples_(0) {} RtpToNtpEstimator::~RtpToNtpEstimator() {} void RtpToNtpEstimator::UpdateParameters() { @@ -122,21 +98,51 @@ bool RtpToNtpEstimator::UpdateMeasurements(uint32_t ntp_secs, bool* new_rtcp_sr) { *new_rtcp_sr = false; - RtcpMeasurement measurement(ntp_secs, ntp_frac, rtp_timestamp); - if (Contains(measurements_, measurement)) { + RtcpMeasurement new_measurement(ntp_secs, ntp_frac, rtp_timestamp); + if (Contains(measurements_, new_measurement)) { // RTCP SR report already added. return true; } - if (!IsValid(measurements_, measurement)) { - // Old report or invalid parameters. + if (!new_measurement.ntp_time.Valid()) return false; + + int64_t ntp_ms_new = new_measurement.ntp_time.ToMs(); + bool invalid_sample = false; + for (const auto& measurement : measurements_) { + if (ntp_ms_new <= measurement.ntp_time.ToMs()) { + // Old report. + invalid_sample = true; + break; + } + int64_t timestamp_new = new_measurement.rtp_timestamp; + if (!CompensateForWrapAround(timestamp_new, measurement.rtp_timestamp, + ×tamp_new)) { + invalid_sample = true; + break; + } + if (timestamp_new <= measurement.rtp_timestamp) { + LOG(LS_WARNING) + << "Newer RTCP SR report with older RTP timestamp, dropping"; + invalid_sample = true; + break; + } } + if (invalid_sample) { + ++consecutive_invalid_samples_; + if (consecutive_invalid_samples_ < kMaxInvalidSamples) { + return false; + } + LOG(LS_WARNING) << "Multiple consecutively invalid RTCP SR reports, " + "clearing measurements."; + measurements_.clear(); + } + consecutive_invalid_samples_ = 0; // Insert new RTCP SR report. if (measurements_.size() == kNumRtcpReportsToUse) measurements_.pop_back(); - measurements_.push_front(measurement); + measurements_.push_front(new_measurement); *new_rtcp_sr = true; // List updated, calculate new parameters. diff --git a/webrtc/system_wrappers/source/rtp_to_ntp_estimator_unittest.cc b/webrtc/system_wrappers/source/rtp_to_ntp_estimator_unittest.cc index 09b47b4873..cfcb13bf8d 100644 --- a/webrtc/system_wrappers/source/rtp_to_ntp_estimator_unittest.cc +++ b/webrtc/system_wrappers/source/rtp_to_ntp_estimator_unittest.cc @@ -138,6 +138,48 @@ TEST(WrapAroundTests, OldRtp_NewRtcpWrapped) { EXPECT_EQ(0, timestamp_ms); } +TEST(WrapAroundTests, GracefullyHandleRtpJump) { + RtpToNtpEstimator estimator; + bool new_sr; + uint32_t ntp_sec = 0; + uint32_t ntp_frac = 1; + uint32_t timestamp = 0; + EXPECT_TRUE( + estimator.UpdateMeasurements(ntp_sec, ntp_frac, timestamp, &new_sr)); + ntp_frac += kOneMsInNtpFrac; + timestamp += kTimestampTicksPerMs; + EXPECT_TRUE( + estimator.UpdateMeasurements(ntp_sec, ntp_frac, timestamp, &new_sr)); + ntp_frac += kOneMsInNtpFrac; + timestamp -= kTimestampTicksPerMs; + int64_t timestamp_ms = -1; + EXPECT_TRUE(estimator.Estimate(timestamp, ×tamp_ms)); + // Constructed at the same time as the first RTCP and should therefore be + // mapped to zero. + EXPECT_EQ(0, timestamp_ms); + + timestamp -= 0xFFFFF; + for (int i = 0; i < RtpToNtpEstimator::kMaxInvalidSamples - 1; ++i) { + EXPECT_FALSE( + estimator.UpdateMeasurements(ntp_sec, ntp_frac, timestamp, &new_sr)); + ntp_frac += kOneMsInNtpFrac; + timestamp += kTimestampTicksPerMs; + } + EXPECT_TRUE( + estimator.UpdateMeasurements(ntp_sec, ntp_frac, timestamp, &new_sr)); + ntp_frac += kOneMsInNtpFrac; + timestamp += kTimestampTicksPerMs; + EXPECT_TRUE( + estimator.UpdateMeasurements(ntp_sec, ntp_frac, timestamp, &new_sr)); + ntp_frac += kOneMsInNtpFrac; + timestamp += kTimestampTicksPerMs; + + timestamp_ms = -1; + EXPECT_TRUE(estimator.Estimate(timestamp, ×tamp_ms)); + // 6 milliseconds has passed since the start of the test. + EXPECT_EQ(6, timestamp_ms); +} + TEST(UpdateRtcpMeasurementTests, FailsForZeroNtp) { RtpToNtpEstimator estimator; uint32_t ntp_sec = 0;