rtt calculation handles time go backwards

CompactNtpIntervalToMs renamed to CompactNtpRttToMs and handle special cases:
large values consider negative/invalid and result in value of 1.
0 result consider too small and increases to 1.

BUG=590996
R=asapersson@webrtc.org, stefan@webrtc.org

Review URL: https://codereview.webrtc.org/1763823003 .

Cr-Commit-Position: refs/heads/master@{#11928}

webrtc/modules/rtp_rtcp/BUILD.gn

@@ -141,6 +141,8 @@ source_set("rtp_rtcp") {
     "source/rtp_utility.h",
     "source/ssrc_database.cc",
     "source/ssrc_database.h",
+    "source/time_util.cc",
+    "source/time_util.h",
     "source/tmmbr_help.cc",
     "source/tmmbr_help.h",
     "source/video_codec_information.h",

webrtc/modules/rtp_rtcp/rtp_rtcp.gypi

@@ -109,6 +109,8 @@
         'source/rtp_utility.h',
         'source/ssrc_database.cc',
         'source/ssrc_database.h',
+        'source/time_util.cc',
+        'source/time_util.h',
         'source/tmmbr_help.cc',
         'source/tmmbr_help.h',
         # Audio Files

webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc

@@ -13,8 +13,6 @@
 #include <assert.h>
 #include <string.h>
 
-#include <algorithm>
-
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/trace_event.h"
@@ -66,6 +64,7 @@ RTCPReceiver::RTCPReceiver(
       _lastReceivedSRNTPfrac(0),
       _lastReceivedXRNTPsecs(0),
       _lastReceivedXRNTPfrac(0),
+      xr_rrtr_status_(false),
       xr_rr_rtt_ms_(0),
       _receivedInfoMap(),
       _lastReceivedRrMs(0),
@@ -191,6 +190,11 @@ int32_t RTCPReceiver::RTT(uint32_t remoteSSRC,
   return 0;
 }
 
+void RTCPReceiver::SetRtcpXrRrtrStatus(bool enable) {
+  CriticalSectionScoped lock(_criticalSectionRTCPReceiver);
+  xr_rrtr_status_ = enable;
+}
+
 bool RTCPReceiver::GetAndResetXrRrRtt(int64_t* rtt_ms) {
   assert(rtt_ms);
   CriticalSectionScoped lock(_criticalSectionRTCPReceiver);
@@ -520,10 +524,13 @@ void RTCPReceiver::HandleReportBlock(
     reportBlock->remoteMaxJitter = rtcpPacket.ReportBlockItem.Jitter;
   }
 
+  int64_t rtt = 0;
   uint32_t send_time = rtcpPacket.ReportBlockItem.LastSR;
-  uint32_t rtt = 0;
+  // RFC3550, section 6.4.1, LSR field discription states:
-
+  // If no SR has been received yet, the field is set to zero.
-  if (send_time > 0) {
+  // Receiver rtp_rtcp module is not expected to calculate rtt using
+  // Sender Reports even if it accidentally can.
+  if (!receiver_only_ && send_time != 0) {
     uint32_t delay = rtcpPacket.ReportBlockItem.DelayLastSR;
     // Local NTP time.
     uint32_t receive_time = CompactNtp(NtpTime(*_clock));
@@ -531,8 +538,7 @@ void RTCPReceiver::HandleReportBlock(
     // RTT in 1/(2^16) seconds.
     uint32_t rtt_ntp = receive_time - delay - send_time;
     // Convert to 1/1000 seconds (milliseconds).
-    uint32_t rtt_ms = CompactNtpIntervalToMs(rtt_ntp);
+    rtt = CompactNtpRttToMs(rtt_ntp);
-    rtt = std::max<uint32_t>(rtt_ms, 1);
     if (rtt > reportBlock->maxRTT) {
       // Store max RTT.
       reportBlock->maxRTT = rtt;
@@ -916,9 +922,13 @@ void RTCPReceiver::HandleXrDlrrReportBlockItem(
 
   rtcpPacketInformation.xr_dlrr_item = true;
 
+  // Caller should explicitly enable rtt calculation using extended reports.
+  if (!xr_rrtr_status_)
+    return;
+
   // The send_time and delay_rr fields are in units of 1/2^16 sec.
   uint32_t send_time = packet.XRDLRRReportBlockItem.LastRR;
-  // RFC3411, section 4.5, LRR field discription states:
+  // RFC3611, section 4.5, LRR field discription states:
   // If no such block has been received, the field is set to zero.
   if (send_time == 0)
     return;
@@ -927,8 +937,7 @@ void RTCPReceiver::HandleXrDlrrReportBlockItem(
   uint32_t now = CompactNtp(NtpTime(*_clock));
 
   uint32_t rtt_ntp = now - delay_rr - send_time;
-  uint32_t rtt_ms = CompactNtpIntervalToMs(rtt_ntp);
+  xr_rr_rtt_ms_ = CompactNtpRttToMs(rtt_ntp);
-  xr_rr_rtt_ms_ = std::max<uint32_t>(rtt_ms, 1);
 
   rtcpPacketInformation.rtcpPacketTypeFlags |= kRtcpXrDlrrReportBlock;
 }

webrtc/modules/rtp_rtcp/source/rtcp_receiver.h

@@ -77,6 +77,7 @@ public:
 
     int32_t SenderInfoReceived(RTCPSenderInfo* senderInfo) const;
 
+    void SetRtcpXrRrtrStatus(bool enable);
     bool GetAndResetXrRrRtt(int64_t* rtt_ms);
 
     // get statistics
@@ -292,6 +293,7 @@ protected:
   uint32_t _lastReceivedXRNTPsecs;
   uint32_t _lastReceivedXRNTPfrac;
   // Estimated rtt, zero when there is no valid estimate.
+  bool xr_rrtr_status_ GUARDED_BY(_criticalSectionRTCPReceiver);
   int64_t xr_rr_rtt_ms_;
 
   // Received report blocks.

webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc

@@ -184,9 +184,9 @@ TEST_F(RtcpReceiverTest, InjectSrPacketCalculatesRTT) {
   Random r(0x0123456789abcdef);
   const uint32_t kSenderSsrc = r.Rand(0x00000001u, 0xfffffffeu);
   const uint32_t kRemoteSsrc = r.Rand(0x00000001u, 0xfffffffeu);
-  const int64_t kRttMs = r.Rand(1, 18 * 3600 * 1000);
+  const int64_t kRttMs = r.Rand(1, 9 * 3600 * 1000);
-  const uint32_t kDelayNtp = r.Rand<uint32_t>();
+  const uint32_t kDelayNtp = r.Rand(0, 0x7fffffff);
-  const uint32_t kDelayMs = CompactNtpIntervalToMs(kDelayNtp);
+  const int64_t kDelayMs = CompactNtpRttToMs(kDelayNtp);
 
   rtcp_receiver_->SetRemoteSSRC(kSenderSsrc);
   std::set<uint32_t> ssrcs;
@@ -216,6 +216,42 @@ TEST_F(RtcpReceiverTest, InjectSrPacketCalculatesRTT) {
   EXPECT_NEAR(kRttMs, rtt_ms, 1);
 }
 
+TEST_F(RtcpReceiverTest, InjectSrPacketCalculatesNegativeRTTAsOne) {
+  Random r(0x0123456789abcdef);
+  const uint32_t kSenderSsrc = r.Rand(0x00000001u, 0xfffffffeu);
+  const uint32_t kRemoteSsrc = r.Rand(0x00000001u, 0xfffffffeu);
+  const int64_t kRttMs = r.Rand(-3600 * 1000, -1);
+  const uint32_t kDelayNtp = r.Rand(0, 0x7fffffff);
+  const int64_t kDelayMs = CompactNtpRttToMs(kDelayNtp);
+
+  rtcp_receiver_->SetRemoteSSRC(kSenderSsrc);
+  std::set<uint32_t> ssrcs;
+  ssrcs.insert(kRemoteSsrc);
+  rtcp_receiver_->SetSsrcs(kRemoteSsrc, ssrcs);
+
+  int64_t rtt_ms = 0;
+  EXPECT_EQ(
+      -1, rtcp_receiver_->RTT(kSenderSsrc, &rtt_ms, nullptr, nullptr, nullptr));
+
+  uint32_t sent_ntp = CompactNtp(NtpTime(system_clock_));
+  system_clock_.AdvanceTimeMilliseconds(kRttMs + kDelayMs);
+
+  rtcp::SenderReport sr;
+  sr.From(kSenderSsrc);
+  rtcp::ReportBlock block;
+  block.To(kRemoteSsrc);
+  block.WithLastSr(sent_ntp);
+  block.WithDelayLastSr(kDelayNtp);
+  sr.WithReportBlock(block);
+
+  rtc::Buffer packet = sr.Build();
+  EXPECT_EQ(0, InjectRtcpPacket(packet.data(), packet.size()));
+
+  EXPECT_EQ(
+      0, rtcp_receiver_->RTT(kSenderSsrc, &rtt_ms, nullptr, nullptr, nullptr));
+  EXPECT_EQ(1, rtt_ms);
+}
+
 TEST_F(RtcpReceiverTest, InjectRrPacket) {
   const uint32_t kSenderSsrc = 0x10203;
   rtcp::ReceiverReport rr;
@@ -790,12 +826,13 @@ TEST_F(RtcpReceiverTest, TestXrRrRttInitiallyFalse) {
   EXPECT_FALSE(rtcp_receiver_->GetAndResetXrRrRtt(&rtt_ms));
 }
 
-TEST_F(RtcpReceiverTest, RttCalculatedAfterXrDlrr) {
+TEST_F(RtcpReceiverTest, XrDlrrCalculatesRtt) {
   Random rand(0x0123456789abcdef);
   const uint32_t kSourceSsrc = rand.Rand(0x00000001u, 0xfffffffeu);
-  const uint32_t kRttMs = rand.Rand(1, 18 * 3600 * 1000);
+  const int64_t kRttMs = rand.Rand(1, 9 * 3600 * 1000);
-  const uint32_t kDelayNtp = rand.Rand<uint32_t>();
+  const uint32_t kDelayNtp = rand.Rand(0, 0x7fffffff);
-  const uint32_t kDelayMs = CompactNtpIntervalToMs(kDelayNtp);
+  const int64_t kDelayMs = CompactNtpRttToMs(kDelayNtp);
+  rtcp_receiver_->SetRtcpXrRrtrStatus(true);
   std::set<uint32_t> ssrcs;
   ssrcs.insert(kSourceSsrc);
   rtcp_receiver_->SetSsrcs(kSourceSsrc, ssrcs);
@@ -816,6 +853,33 @@ TEST_F(RtcpReceiverTest, RttCalculatedAfterXrDlrr) {
   EXPECT_NEAR(kRttMs, rtt_ms, 1);
 }
 
+TEST_F(RtcpReceiverTest, XrDlrrCalculatesNegativeRttAsOne) {
+  Random rand(0x0123456789abcdef);
+  const uint32_t kSourceSsrc = rand.Rand(0x00000001u, 0xfffffffeu);
+  const int64_t kRttMs = rand.Rand(-3600 * 1000, -1);
+  const uint32_t kDelayNtp = rand.Rand(0, 0x7fffffff);
+  const int64_t kDelayMs = CompactNtpRttToMs(kDelayNtp);
+  rtcp_receiver_->SetRtcpXrRrtrStatus(true);
+  std::set<uint32_t> ssrcs;
+  ssrcs.insert(kSourceSsrc);
+  rtcp_receiver_->SetSsrcs(kSourceSsrc, ssrcs);
+  NtpTime now(system_clock_);
+  uint32_t sent_ntp = CompactNtp(now);
+  system_clock_.AdvanceTimeMilliseconds(kRttMs + kDelayMs);
+
+  rtcp::Dlrr dlrr;
+  dlrr.WithDlrrItem(kSourceSsrc, sent_ntp, kDelayNtp);
+  rtcp::ExtendedReports xr;
+  xr.From(0x2345);
+  xr.WithDlrr(dlrr);
+  rtc::Buffer packet = xr.Build();
+  EXPECT_EQ(0, InjectRtcpPacket(packet.data(), packet.size()));
+
+  int64_t rtt_ms = 0;
+  EXPECT_TRUE(rtcp_receiver_->GetAndResetXrRrRtt(&rtt_ms));
+  EXPECT_EQ(1, rtt_ms);
+}
+
 TEST_F(RtcpReceiverTest, LastReceivedXrReferenceTimeInfoInitiallyFalse) {
   RtcpReceiveTimeInfo info;
   EXPECT_FALSE(rtcp_receiver_->LastReceivedXrReferenceTimeInfo(&info));

webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc

@@ -586,7 +586,8 @@ int32_t ModuleRtpRtcpImpl::SetRTCPVoIPMetrics(
 }
 
 void ModuleRtpRtcpImpl::SetRtcpXrRrtrStatus(bool enable) {
-  return rtcp_sender_.SendRtcpXrReceiverReferenceTime(enable);
+  rtcp_receiver_.SetRtcpXrRrtrStatus(enable);
+  rtcp_sender_.SendRtcpXrReceiverReferenceTime(enable);
 }
 
 bool ModuleRtpRtcpImpl::RtcpXrRrtrStatus() const {

webrtc/modules/rtp_rtcp/source/time_util.cc

@@ -0,0 +1,42 @@
+/*
+ *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "webrtc/modules/rtp_rtcp/source/time_util.h"
+
+#include <algorithm>
+
+namespace webrtc {
+namespace {
+// TODO(danilchap): Make generic, optimize and move to base.
+inline int64_t DivideRoundToNearest(int64_t x, uint32_t y) {
+  // Caller ensure x is positive by converting unsigned value into it.
+  // So this Divide doesn't need to handle negative argument case.
+  return (x + y / 2) / y;
+}
+}  // namespace
+
+int64_t CompactNtpRttToMs(uint32_t compact_ntp_interval) {
+  // Interval to convert expected to be positive, e.g. rtt or delay.
+  // Because interval can be derived from non-monotonic ntp clock,
+  // it might become negative that is indistinguishable from very large values.
+  // Since very large rtt/delay are less likely than non-monotonic ntp clock,
+  // those values consider to be negative and convert to minimum value of 1ms.
+  if (compact_ntp_interval > 0x80000000)
+    return 1;
+  // Convert to 64bit value to avoid multiplication overflow.
+  int64_t value = static_cast<int64_t>(compact_ntp_interval);
+  // To convert to milliseconds need to divide by 2^16 to get seconds,
+  // then multiply by 1000 to get milliseconds. To avoid float operations,
+  // multiplication and division swapped.
+  int64_t ms = DivideRoundToNearest(value * 1000, 1 << 16);
+  // Rtt value 0 considered too good to be true and increases to 1.
+  return std::max<int64_t>(ms, 1);
+}
+}  // namespace webrtc

webrtc/modules/rtp_rtcp/source/time_util.h

@@ -39,11 +39,9 @@ inline uint32_t CompactNtp(NtpTime ntp) {
   return (ntp.seconds() << 16) | (ntp.fractions() >> 16);
 }
 // Converts interval between compact ntp timestamps to milliseconds.
-// This interval can be upto ~18.2 hours (2^16 seconds).
+// This interval can be up to ~9.1 hours (2^15 seconds).
-inline uint32_t CompactNtpIntervalToMs(uint32_t compact_ntp_interval) {
+// Values close to 2^16 seconds consider negative and result in minimum rtt = 1.
-  uint64_t value = static_cast<uint64_t>(compact_ntp_interval);
+int64_t CompactNtpRttToMs(uint32_t compact_ntp_interval);
-  return (value * 1000 + (1 << 15)) >> 16;
-}
 
 }  // namespace webrtc
 #endif  // WEBRTC_MODULES_RTP_RTCP_SOURCE_TIME_UTIL_H_

webrtc/modules/rtp_rtcp/source/time_util_unittest.cc

@@ -21,21 +21,21 @@ TEST(TimeUtilTest, CompactNtp) {
   EXPECT_EQ(kNtpMid, CompactNtp(kNtp));
 }
 
-TEST(TimeUtilTest, CompactNtpToMs) {
+TEST(TimeUtilTest, CompactNtpRttToMs) {
   const NtpTime ntp1(0x12345, 0x23456);
   const NtpTime ntp2(0x12654, 0x64335);
-  uint32_t ms_diff = ntp2.ToMs() - ntp1.ToMs();
+  int64_t ms_diff = ntp2.ToMs() - ntp1.ToMs();
   uint32_t ntp_diff = CompactNtp(ntp2) - CompactNtp(ntp1);
 
-  uint32_t ntp_to_ms_diff = CompactNtpIntervalToMs(ntp_diff);
+  int64_t ntp_to_ms_diff = CompactNtpRttToMs(ntp_diff);
 
   EXPECT_NEAR(ms_diff, ntp_to_ms_diff, 1);
 }
 
-TEST(TimeUtilTest, CompactNtpToMsWithWrap) {
+TEST(TimeUtilTest, CompactNtpRttToMsWithWrap) {
   const NtpTime ntp1(0x1ffff, 0x23456);
   const NtpTime ntp2(0x20000, 0x64335);
-  uint32_t ms_diff = ntp2.ToMs() - ntp1.ToMs();
+  int64_t ms_diff = ntp2.ToMs() - ntp1.ToMs();
 
   // While ntp2 > ntp1, there compact ntp presentation happen to be opposite.
   // That shouldn't be a problem as long as unsigned arithmetic is used.
@@ -43,20 +43,31 @@ TEST(TimeUtilTest, CompactNtpToMsWithWrap) {
   ASSERT_LT(CompactNtp(ntp2), CompactNtp(ntp1));
 
   uint32_t ntp_diff = CompactNtp(ntp2) - CompactNtp(ntp1);
-  uint32_t ntp_to_ms_diff = CompactNtpIntervalToMs(ntp_diff);
+  int64_t ntp_to_ms_diff = CompactNtpRttToMs(ntp_diff);
 
   EXPECT_NEAR(ms_diff, ntp_to_ms_diff, 1);
 }
 
-TEST(TimeUtilTest, CompactNtpToMsLarge) {
+TEST(TimeUtilTest, CompactNtpRttToMsLarge) {
-  const NtpTime ntp1(0x10000, 0x23456);
+  const NtpTime ntp1(0x10000, 0x00006);
-  const NtpTime ntp2(0x1ffff, 0x64335);
+  const NtpTime ntp2(0x17fff, 0xffff5);
-  uint32_t ms_diff = ntp2.ToMs() - ntp1.ToMs();
+  int64_t ms_diff = ntp2.ToMs() - ntp1.ToMs();
-  // Ntp difference close to maximum of ~18 hours should convert correctly too.
+  // Ntp difference close to 2^15 seconds should convert correctly too.
-  ASSERT_GT(ms_diff, 18u * 3600 * 1000);
+  ASSERT_NEAR(ms_diff, ((1 << 15) - 1) * 1000, 1);
   uint32_t ntp_diff = CompactNtp(ntp2) - CompactNtp(ntp1);
-  uint32_t ntp_to_ms_diff = CompactNtpIntervalToMs(ntp_diff);
+  int64_t ntp_to_ms_diff = CompactNtpRttToMs(ntp_diff);
 
   EXPECT_NEAR(ms_diff, ntp_to_ms_diff, 1);
 }
+
+TEST(TimeUtilTest, CompactNtpRttToMsNegative) {
+  const NtpTime ntp1(0x20000, 0x23456);
+  const NtpTime ntp2(0x1ffff, 0x64335);
+  int64_t ms_diff = ntp2.ToMs() - ntp1.ToMs();
+  ASSERT_GT(0, ms_diff);
+  // Ntp difference close to 2^16 seconds should be treated as negative.
+  uint32_t ntp_diff = CompactNtp(ntp2) - CompactNtp(ntp1);
+  int64_t ntp_to_ms_diff = CompactNtpRttToMs(ntp_diff);
+  EXPECT_EQ(1, ntp_to_ms_diff);
+}
 }  // namespace webrtc