diff --git a/modules/rtp_rtcp/source/rtcp_receiver.cc b/modules/rtp_rtcp/source/rtcp_receiver.cc index c4371388be..dd9d2664ed 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -154,7 +154,6 @@ RTCPReceiver::RTCPReceiver(const RtpRtcpInterface::Configuration& config, // TODO(bugs.webrtc.org/10774): Remove fallback. remote_ssrc_(0), xr_rrtr_status_(config.non_sender_rtt_measurement), - xr_rr_rtt_ms_(0), oldest_tmmbr_info_ms_(0), cname_callback_(config.rtcp_cname_callback), report_block_data_observer_(config.report_block_data_observer), @@ -182,7 +181,6 @@ RTCPReceiver::RTCPReceiver(const RtpRtcpInterface::Configuration& config, // TODO(bugs.webrtc.org/10774): Remove fallback. remote_ssrc_(0), xr_rrtr_status_(config.non_sender_rtt_measurement), - xr_rr_rtt_ms_(0), oldest_tmmbr_info_ms_(0), cname_callback_(config.rtcp_cname_callback), report_block_data_observer_(config.report_block_data_observer), @@ -284,15 +282,11 @@ void RTCPReceiver::SetNonSenderRttMeasurement(bool enabled) { xr_rrtr_status_ = enabled; } -bool RTCPReceiver::GetAndResetXrRrRtt(int64_t* rtt_ms) { - RTC_DCHECK(rtt_ms); +absl::optional RTCPReceiver::GetAndResetXrRrRtt() { MutexLock lock(&rtcp_receiver_lock_); - if (xr_rr_rtt_ms_ == 0) { - return false; - } - *rtt_ms = xr_rr_rtt_ms_; - xr_rr_rtt_ms_ = 0; - return true; + absl::optional rtt = xr_rr_rtt_; + xr_rr_rtt_ = absl::nullopt; + return rtt; } // Called regularly (1/sec) on the worker thread to do rtt calculations. @@ -328,10 +322,7 @@ absl::optional RTCPReceiver::OnPeriodicRttUpdate( } } else { // Report rtt from receiver. - int64_t rtt_ms; - if (GetAndResetXrRrRtt(&rtt_ms)) { - rtt.emplace(TimeDelta::Millis(rtt_ms)); - } + rtt = GetAndResetXrRrRtt(); } return rtt; @@ -794,7 +785,7 @@ bool RTCPReceiver::HandleBye(const CommonHeader& rtcp_block) { received_rrtrs_.erase(it->second); received_rrtrs_ssrc_it_.erase(it); } - xr_rr_rtt_ms_ = 0; + xr_rr_rtt_ = absl::nullopt; return true; } @@ -869,7 +860,7 @@ void RTCPReceiver::HandleXrDlrrReportBlock(uint32_t sender_ssrc, uint32_t rtt_ntp = now_ntp - delay_ntp - send_time_ntp; TimeDelta rtt = CompactNtpRttToTimeDelta(rtt_ntp); - xr_rr_rtt_ms_ = rtt.ms(); + xr_rr_rtt_ = rtt; non_sender_rtts_[sender_ssrc].Update(rtt); } diff --git a/modules/rtp_rtcp/source/rtcp_receiver.h b/modules/rtp_rtcp/source/rtcp_receiver.h index e5e0ab1692..b0978690ab 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.h +++ b/modules/rtp_rtcp/source/rtcp_receiver.h @@ -123,7 +123,7 @@ class RTCPReceiver final { NonSenderRttStats GetNonSenderRTT() const; void SetNonSenderRttMeasurement(bool enabled); - bool GetAndResetXrRrRtt(int64_t* rtt_ms); + absl::optional GetAndResetXrRrRtt(); // Called once per second on the worker thread to do rtt calculations. // Returns an optional rtt value if one is available. @@ -376,9 +376,9 @@ class RTCPReceiver final { flat_map::iterator> received_rrtrs_ssrc_it_ RTC_GUARDED_BY(rtcp_receiver_lock_); - // Estimated rtt, zero when there is no valid estimate. + // Estimated rtt, nullopt when there is no valid estimate. bool xr_rrtr_status_ RTC_GUARDED_BY(rtcp_receiver_lock_); - int64_t xr_rr_rtt_ms_; + absl::optional xr_rr_rtt_; int64_t oldest_tmmbr_info_ms_ RTC_GUARDED_BY(rtcp_receiver_lock_); // Mapped by remote ssrc. diff --git a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc index 159086bf68..1f5f138b83 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc @@ -135,6 +135,7 @@ constexpr uint32_t kNotToUsSsrc = 0x654321; constexpr uint32_t kUnknownSenderSsrc = 0x54321; constexpr int64_t kRtcpIntervalMs = 1000; +constexpr TimeDelta kEpsilon = TimeDelta::Millis(1); } // namespace @@ -780,8 +781,7 @@ TEST(RtcpReceiverTest, ExtendedReportsDlrrPacketNotToUsIgnored) { receiver.IncomingPacket(xr.Build()); - int64_t rtt_ms = 0; - EXPECT_FALSE(receiver.GetAndResetXrRrRtt(&rtt_ms)); + EXPECT_FALSE(receiver.GetAndResetXrRrRtt()); RTCPReceiver::NonSenderRttStats non_sender_rtt_stats = receiver.GetNonSenderRTT(); EXPECT_FALSE(non_sender_rtt_stats.round_trip_time().has_value()); @@ -798,8 +798,7 @@ TEST(RtcpReceiverTest, InjectExtendedReportsDlrrPacketWithSubBlock) { const uint32_t kLastRR = 0x12345; const uint32_t kDelay = 0x23456; - int64_t rtt_ms = 0; - EXPECT_FALSE(receiver.GetAndResetXrRrRtt(&rtt_ms)); + EXPECT_FALSE(receiver.GetAndResetXrRrRtt()); rtcp::ExtendedReports xr; xr.SetSenderSsrc(kSenderSsrc); @@ -808,9 +807,9 @@ TEST(RtcpReceiverTest, InjectExtendedReportsDlrrPacketWithSubBlock) { receiver.IncomingPacket(xr.Build()); uint32_t compact_ntp_now = CompactNtp(mocks.clock.CurrentNtpTime()); - EXPECT_TRUE(receiver.GetAndResetXrRrRtt(&rtt_ms)); uint32_t rtt_ntp = compact_ntp_now - kDelay - kLastRR; - EXPECT_NEAR(CompactNtpRttToTimeDelta(rtt_ntp).ms(), rtt_ms, 1); + EXPECT_THAT(receiver.GetAndResetXrRrRtt(), + Near(CompactNtpRttToTimeDelta(rtt_ntp), kEpsilon)); RTCPReceiver::NonSenderRttStats non_sender_rtt_stats = receiver.GetNonSenderRTT(); EXPECT_GT(non_sender_rtt_stats.round_trip_time(), TimeDelta::Zero()); @@ -837,10 +836,9 @@ TEST(RtcpReceiverTest, InjectExtendedReportsDlrrPacketWithMultipleSubBlocks) { receiver.IncomingPacket(xr.Build()); uint32_t compact_ntp_now = CompactNtp(mocks.clock.CurrentNtpTime()); - int64_t rtt_ms = 0; - EXPECT_TRUE(receiver.GetAndResetXrRrRtt(&rtt_ms)); uint32_t rtt_ntp = compact_ntp_now - kDelay - kLastRR; - EXPECT_NEAR(CompactNtpRttToTimeDelta(rtt_ntp).ms(), rtt_ms, 1); + EXPECT_THAT(receiver.GetAndResetXrRrRtt(), + Near(CompactNtpRttToTimeDelta(rtt_ntp), kEpsilon)); RTCPReceiver::NonSenderRttStats non_sender_rtt_stats = receiver.GetNonSenderRTT(); EXPECT_GT(non_sender_rtt_stats.round_trip_time(), TimeDelta::Zero()); @@ -866,8 +864,7 @@ TEST(RtcpReceiverTest, InjectExtendedReportsPacketWithMultipleReportBlocks) { std::vector last_xr_rtis = receiver.ConsumeReceivedXrReferenceTimeInfo(); EXPECT_THAT(last_xr_rtis, SizeIs(1)); - int64_t rtt_ms = 0; - EXPECT_TRUE(receiver.GetAndResetXrRrRtt(&rtt_ms)); + EXPECT_TRUE(receiver.GetAndResetXrRrRtt()); } TEST(RtcpReceiverTest, InjectExtendedReportsPacketWithUnknownReportBlock) { @@ -894,8 +891,7 @@ TEST(RtcpReceiverTest, InjectExtendedReportsPacketWithUnknownReportBlock) { receiver.ConsumeReceivedXrReferenceTimeInfo(); EXPECT_THAT(last_xr_rtis, SizeIs(1)); // Validate Dlrr report wasn't processed. - int64_t rtt_ms = 0; - EXPECT_FALSE(receiver.GetAndResetXrRrRtt(&rtt_ms)); + EXPECT_FALSE(receiver.GetAndResetXrRrRtt()); RTCPReceiver::NonSenderRttStats non_sender_rtt_stats = receiver.GetNonSenderRTT(); EXPECT_FALSE(non_sender_rtt_stats.round_trip_time().has_value()); @@ -910,8 +906,7 @@ TEST(RtcpReceiverTest, TestExtendedReportsRrRttInitiallyFalse) { RTCPReceiver receiver(config, &mocks.rtp_rtcp_impl); receiver.SetRemoteSSRC(kSenderSsrc); - int64_t rtt_ms; - EXPECT_FALSE(receiver.GetAndResetXrRrRtt(&rtt_ms)); + EXPECT_FALSE(receiver.GetAndResetXrRrRtt()); RTCPReceiver::NonSenderRttStats non_sender_rtt_stats = receiver.GetNonSenderRTT(); EXPECT_FALSE(non_sender_rtt_stats.round_trip_time().has_value()); @@ -940,9 +935,7 @@ TEST(RtcpReceiverTest, RttCalculatedAfterExtendedReportsDlrr) { receiver.IncomingPacket(xr.Build()); - int64_t rtt_ms = 0; - EXPECT_TRUE(receiver.GetAndResetXrRrRtt(&rtt_ms)); - EXPECT_NEAR(kRtt.ms(), rtt_ms, 1); + EXPECT_THAT(receiver.GetAndResetXrRrRtt(), Near(kRtt, kEpsilon)); RTCPReceiver::NonSenderRttStats non_sender_rtt_stats = receiver.GetNonSenderRTT(); EXPECT_TRUE(non_sender_rtt_stats.round_trip_time().has_value()); @@ -975,9 +968,7 @@ TEST(RtcpReceiverTest, SetterEnablesReceiverRtt) { receiver.IncomingPacket(xr.Build()); - int64_t rtt_ms = 0; - EXPECT_TRUE(receiver.GetAndResetXrRrRtt(&rtt_ms)); - EXPECT_NEAR(rtt_ms, kRtt.ms(), 1); + EXPECT_THAT(receiver.GetAndResetXrRrRtt(), Near(kRtt, kEpsilon)); RTCPReceiver::NonSenderRttStats non_sender_rtt_stats = receiver.GetNonSenderRTT(); EXPECT_TRUE(non_sender_rtt_stats.round_trip_time().has_value()); @@ -1011,8 +1002,7 @@ TEST(RtcpReceiverTest, DoesntCalculateRttOnReceivedDlrr) { receiver.IncomingPacket(xr.Build()); // We expect that no RTT is available (because receive-side RTT was disabled). - int64_t rtt_ms = 0; - EXPECT_FALSE(receiver.GetAndResetXrRrRtt(&rtt_ms)); + EXPECT_FALSE(receiver.GetAndResetXrRrRtt()); RTCPReceiver::NonSenderRttStats non_sender_rtt_stats = receiver.GetNonSenderRTT(); EXPECT_FALSE(non_sender_rtt_stats.round_trip_time().has_value()); @@ -1020,7 +1010,7 @@ TEST(RtcpReceiverTest, DoesntCalculateRttOnReceivedDlrr) { EXPECT_EQ(non_sender_rtt_stats.round_trip_time_measurements(), 0); } -TEST(RtcpReceiverTest, XrDlrrCalculatesNegativeRttAsOne) { +TEST(RtcpReceiverTest, XrDlrrCalculatesNegativeRttAsOneMillisecond) { ReceiverMocks mocks; auto config = DefaultConfiguration(&mocks); config.non_sender_rtt_measurement = true; @@ -1041,9 +1031,7 @@ TEST(RtcpReceiverTest, XrDlrrCalculatesNegativeRttAsOne) { receiver.IncomingPacket(xr.Build()); - int64_t rtt_ms = 0; - EXPECT_TRUE(receiver.GetAndResetXrRrRtt(&rtt_ms)); - EXPECT_EQ(1, rtt_ms); + EXPECT_EQ(receiver.GetAndResetXrRrRtt(), TimeDelta::Millis(1)); RTCPReceiver::NonSenderRttStats non_sender_rtt_stats = receiver.GetNonSenderRTT(); EXPECT_TRUE(non_sender_rtt_stats.round_trip_time().has_value()); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index f2cb9db6cc..ab5d419648 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -141,10 +141,10 @@ void ModuleRtpRtcpImpl::Process() { } } else { // Report rtt from receiver. - if (process_rtt) { - int64_t rtt_ms; - if (rtt_stats_ && rtcp_receiver_.GetAndResetXrRrRtt(&rtt_ms)) { - rtt_stats_->OnRttUpdate(rtt_ms); + if (process_rtt && rtt_stats_ != nullptr) { + absl::optional rtt = rtcp_receiver_.GetAndResetXrRrRtt(); + if (rtt.has_value()) { + rtt_stats_->OnRttUpdate(rtt->ms()); } } }