From 71f80c0bd6003f77f57c7e01ac87d7166e3b1618 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Thu, 11 May 2023 10:35:37 +0200 Subject: [PATCH] Replace RtcpReceiver::RTT function with RtcpReceiver::AverageRtt with cleaner interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:13757 Change-Id: I4320c72628444b88e36ef162cfb34346fcdd967b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/304860 Reviewed-by: Åsa Persson Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#40053} --- modules/rtp_rtcp/source/rtcp_receiver.cc | 36 +++---------------- modules/rtp_rtcp/source/rtcp_receiver.h | 12 +------ .../rtp_rtcp/source/rtcp_receiver_unittest.cc | 24 +++++++------ modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 14 ++++---- modules/rtp_rtcp/source/rtp_rtcp_impl2.cc | 14 ++++---- 5 files changed, 34 insertions(+), 66 deletions(-) diff --git a/modules/rtp_rtcp/source/rtcp_receiver.cc b/modules/rtp_rtcp/source/rtcp_receiver.cc index 5e1e1c6eb1..82612f83d4 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -256,45 +256,17 @@ uint32_t RTCPReceiver::RemoteSSRC() const { void RTCPReceiver::RttStats::AddRtt(TimeDelta rtt) { last_rtt_ = rtt; - if (rtt < min_rtt_) { - min_rtt_ = rtt; - } - if (rtt > max_rtt_) { - max_rtt_ = rtt; - } sum_rtt_ += rtt; ++num_rtts_; } -int32_t RTCPReceiver::RTT(uint32_t remote_ssrc, - int64_t* last_rtt_ms, - int64_t* avg_rtt_ms, - int64_t* min_rtt_ms, - int64_t* max_rtt_ms) const { +absl::optional RTCPReceiver::AverageRtt() const { MutexLock lock(&rtcp_receiver_lock_); - - auto it = rtts_.find(remote_ssrc); + auto it = rtts_.find(remote_ssrc_); if (it == rtts_.end()) { - return -1; + return absl::nullopt; } - - if (last_rtt_ms) { - *last_rtt_ms = it->second.last_rtt().ms(); - } - - if (avg_rtt_ms) { - *avg_rtt_ms = it->second.average_rtt().ms(); - } - - if (min_rtt_ms) { - *min_rtt_ms = it->second.min_rtt().ms(); - } - - if (max_rtt_ms) { - *max_rtt_ms = it->second.max_rtt().ms(); - } - - return 0; + return it->second.average_rtt(); } absl::optional RTCPReceiver::LastRtt() const { diff --git a/modules/rtp_rtcp/source/rtcp_receiver.h b/modules/rtp_rtcp/source/rtcp_receiver.h index c0fcc86fde..00987b3825 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.h +++ b/modules/rtp_rtcp/source/rtcp_receiver.h @@ -116,13 +116,7 @@ class RTCPReceiver final { std::vector ConsumeReceivedXrReferenceTimeInfo(); - // Get rtt. - int32_t RTT(uint32_t remote_ssrc, - int64_t* last_rtt_ms, - int64_t* avg_rtt_ms, - int64_t* min_rtt_ms, - int64_t* max_rtt_ms) const; - + absl::optional AverageRtt() const; absl::optional LastRtt() const; // Returns non-sender RTT metrics for the remote SSRC. @@ -253,14 +247,10 @@ class RTCPReceiver final { void AddRtt(TimeDelta rtt); TimeDelta last_rtt() const { return last_rtt_; } - TimeDelta min_rtt() const { return min_rtt_; } - TimeDelta max_rtt() const { return max_rtt_; } TimeDelta average_rtt() const { return sum_rtt_ / num_rtts_; } private: TimeDelta last_rtt_ = TimeDelta::Zero(); - TimeDelta min_rtt_ = TimeDelta::PlusInfinity(); - TimeDelta max_rtt_ = TimeDelta::MinusInfinity(); TimeDelta sum_rtt_ = TimeDelta::Zero(); size_t num_rtts_ = 0; }; diff --git a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc index 07fc96c910..47bd549639 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc @@ -129,6 +129,10 @@ class MockVideoBitrateAllocationObserver (override)); }; +MATCHER_P2(Near, value, margin, "") { + return value - margin <= arg && arg <= value + margin; +} + // SSRC of remote peer, that sends rtcp packet to the rtcp receiver under test. constexpr uint32_t kSenderSsrc = 0x10203; // SSRCs of local peer, that rtcp packet addressed to. @@ -247,8 +251,7 @@ TEST(RtcpReceiverTest, InjectSrPacketCalculatesRTT) { const uint32_t kDelayNtp = 0x4321; const TimeDelta kDelay = CompactNtpRttToTimeDelta(kDelayNtp); - int64_t rtt_ms = 0; - EXPECT_EQ(-1, receiver.RTT(kSenderSsrc, &rtt_ms, nullptr, nullptr, nullptr)); + EXPECT_EQ(receiver.LastRtt(), absl::nullopt); uint32_t sent_ntp = CompactNtp(mocks.clock.CurrentNtpTime()); mocks.clock.AdvanceTime(kRtt + kDelay); @@ -265,11 +268,10 @@ TEST(RtcpReceiverTest, InjectSrPacketCalculatesRTT) { EXPECT_CALL(mocks.bandwidth_observer, OnReceivedRtcpReceiverReport); receiver.IncomingPacket(sr.Build()); - EXPECT_EQ(0, receiver.RTT(kSenderSsrc, &rtt_ms, nullptr, nullptr, nullptr)); - EXPECT_NEAR(rtt_ms, kRtt.ms(), 1); + EXPECT_THAT(receiver.LastRtt(), Near(kRtt, TimeDelta::Millis(1))); } -TEST(RtcpReceiverTest, InjectSrPacketCalculatesNegativeRTTAsOne) { +TEST(RtcpReceiverTest, InjectSrPacketCalculatesNegativeRTTAsOneMs) { ReceiverMocks mocks; RTCPReceiver receiver(DefaultConfiguration(&mocks), &mocks.rtp_rtcp_impl); receiver.SetRemoteSSRC(kSenderSsrc); @@ -278,8 +280,7 @@ TEST(RtcpReceiverTest, InjectSrPacketCalculatesNegativeRTTAsOne) { const uint32_t kDelayNtp = 0x4321; const TimeDelta kDelay = CompactNtpRttToTimeDelta(kDelayNtp); - int64_t rtt_ms = 0; - EXPECT_EQ(-1, receiver.RTT(kSenderSsrc, &rtt_ms, nullptr, nullptr, nullptr)); + EXPECT_EQ(receiver.LastRtt(), absl::nullopt); uint32_t sent_ntp = CompactNtp(mocks.clock.CurrentNtpTime()); mocks.clock.AdvanceTime(kRtt + kDelay); @@ -297,8 +298,7 @@ TEST(RtcpReceiverTest, InjectSrPacketCalculatesNegativeRTTAsOne) { OnReceivedRtcpReceiverReport(SizeIs(1), _, _)); receiver.IncomingPacket(sr.Build()); - EXPECT_EQ(0, receiver.RTT(kSenderSsrc, &rtt_ms, nullptr, nullptr, nullptr)); - EXPECT_EQ(1, rtt_ms); + EXPECT_EQ(receiver.LastRtt(), TimeDelta::Millis(1)); } TEST(RtcpReceiverTest, @@ -598,7 +598,8 @@ TEST(RtcpReceiverTest, GetRtt) { receiver.SetRemoteSSRC(kSenderSsrc); // No report block received. - EXPECT_EQ(-1, receiver.RTT(kSenderSsrc, nullptr, nullptr, nullptr, nullptr)); + EXPECT_EQ(receiver.LastRtt(), absl::nullopt); + EXPECT_EQ(receiver.AverageRtt(), absl::nullopt); rtcp::ReportBlock rb; rb.SetMediaSsrc(kReceiverMainSsrc); @@ -617,7 +618,8 @@ TEST(RtcpReceiverTest, GetRtt) { receiver.IncomingPacket(rr.Build()); EXPECT_EQ(receiver.LastReceivedReportBlockMs(), now.ms()); - EXPECT_EQ(0, receiver.RTT(kSenderSsrc, nullptr, nullptr, nullptr, nullptr)); + EXPECT_NE(receiver.LastRtt(), absl::nullopt); + EXPECT_NE(receiver.AverageRtt(), absl::nullopt); } // App packets are ignored. diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 689123bbb3..a43f788445 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -482,10 +482,8 @@ int64_t ModuleRtpRtcpImpl::ExpectedRetransmissionTimeMs() const { } // No rtt available (`kRtpRtcpRttProcessTimeMs` not yet passed?), so try to // poll avg_rtt_ms directly from rtcp receiver. - if (rtcp_receiver_.RTT(rtcp_receiver_.RemoteSSRC(), nullptr, - &expected_retransmission_time_ms, nullptr, - nullptr) == 0) { - return expected_retransmission_time_ms; + if (absl::optional rtt = rtcp_receiver_.AverageRtt()) { + return rtt->ms(); } return kDefaultExpectedRetransmissionTimeMs; } @@ -594,7 +592,9 @@ bool ModuleRtpRtcpImpl::TimeToSendFullNackList(int64_t now) const { // Use RTT from RtcpRttStats class if provided. int64_t rtt = rtt_ms(); if (rtt == 0) { - rtcp_receiver_.RTT(rtcp_receiver_.RemoteSSRC(), NULL, &rtt, NULL, NULL); + if (absl::optional average_rtt = rtcp_receiver_.AverageRtt()) { + rtt = average_rtt->ms(); + } } const int64_t kStartUpRttMs = 100; @@ -665,7 +665,9 @@ void ModuleRtpRtcpImpl::OnReceivedNack( // Use RTT from RtcpRttStats class if provided. int64_t rtt = rtt_ms(); if (rtt == 0) { - rtcp_receiver_.RTT(rtcp_receiver_.RemoteSSRC(), NULL, &rtt, NULL, NULL); + if (absl::optional average_rtt = rtcp_receiver_.AverageRtt()) { + rtt = average_rtt->ms(); + } } rtp_sender_->packet_generator.OnReceivedNack(nack_sequence_numbers, rtt); } diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc index f92be76c65..c22479f3c9 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc @@ -464,10 +464,8 @@ int64_t ModuleRtpRtcpImpl2::ExpectedRetransmissionTimeMs() const { } // No rtt available (`kRttUpdateInterval` not yet passed?), so try to // poll avg_rtt_ms directly from rtcp receiver. - if (rtcp_receiver_.RTT(rtcp_receiver_.RemoteSSRC(), nullptr, - &expected_retransmission_time_ms, nullptr, - nullptr) == 0) { - return expected_retransmission_time_ms; + if (absl::optional rtt = rtcp_receiver_.AverageRtt()) { + return rtt->ms(); } return kDefaultExpectedRetransmissionTimeMs; } @@ -581,7 +579,9 @@ bool ModuleRtpRtcpImpl2::TimeToSendFullNackList(int64_t now) const { // Use RTT from RtcpRttStats class if provided. int64_t rtt = rtt_ms(); if (rtt == 0) { - rtcp_receiver_.RTT(rtcp_receiver_.RemoteSSRC(), NULL, &rtt, NULL, NULL); + if (absl::optional average_rtt = rtcp_receiver_.AverageRtt()) { + rtt = average_rtt->ms(); + } } const int64_t kStartUpRttMs = 100; @@ -655,7 +655,9 @@ void ModuleRtpRtcpImpl2::OnReceivedNack( // Use RTT from RtcpRttStats class if provided. int64_t rtt = rtt_ms(); if (rtt == 0) { - rtcp_receiver_.RTT(rtcp_receiver_.RemoteSSRC(), NULL, &rtt, NULL, NULL); + if (absl::optional average_rtt = rtcp_receiver_.AverageRtt()) { + rtt = average_rtt->ms(); + } } rtp_sender_->packet_generator.OnReceivedNack(nack_sequence_numbers, rtt); }