From 8095d0288440866a2d425fedea725934cfa5512b Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Tue, 9 May 2023 09:59:46 +0200 Subject: [PATCH] Add RtpRtcpInterface::LastRtt function to replace RtpRtcpInterface::RTT MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RtpRtcpInterface::RTT follows discouraged style of using return values, uses raw integers to represent time delta, and returns values that no code uses (min, max, average RTT) added LastRtt function addresses all these stylistic issues. Bug: webrtc:13757 Change-Id: Iaf947dd1b7139026f2beb991e69634c606c6b608 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/304520 Commit-Queue: Danil Chapovalov Reviewed-by: Philip Eliasson Reviewed-by: Jakob Ivarsson‎ Cr-Commit-Position: refs/heads/main@{#40028} --- audio/channel_receive.cc | 16 ++++++--------- audio/voip/audio_ingress.cc | 7 +++---- modules/rtp_rtcp/mocks/mock_rtp_rtcp.h | 1 + modules/rtp_rtcp/source/rtcp_receiver.cc | 9 +++++++++ modules/rtp_rtcp/source/rtcp_receiver.h | 2 ++ modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 11 ++++++++++ modules/rtp_rtcp/source/rtp_rtcp_impl.h | 11 +++++----- modules/rtp_rtcp/source/rtp_rtcp_impl2.cc | 11 ++++++++++ modules/rtp_rtcp/source/rtp_rtcp_impl2.h | 11 +++++----- .../source/rtp_rtcp_impl2_unittest.cc | 20 ++++++------------- modules/rtp_rtcp/source/rtp_rtcp_interface.h | 13 +++++++----- video/rtp_video_stream_receiver2.cc | 8 +++----- 12 files changed, 72 insertions(+), 48 deletions(-) diff --git a/audio/channel_receive.cc b/audio/channel_receive.cc index e6435ad1b1..b0560285ed 100644 --- a/audio/channel_receive.cc +++ b/audio/channel_receive.cc @@ -342,11 +342,10 @@ void ChannelReceive::OnReceivedPayloadData( return; } - int64_t round_trip_time = 0; - rtp_rtcp_->RTT(remote_ssrc_, &round_trip_time, /*avg_rtt=*/nullptr, - /*min_rtt=*/nullptr, /*max_rtt=*/nullptr); + TimeDelta round_trip_time = rtp_rtcp_->LastRtt().value_or(TimeDelta::Zero()); - std::vector nack_list = acm_receiver_.GetNackList(round_trip_time); + std::vector nack_list = + acm_receiver_.GetNackList(round_trip_time.ms()); if (!nack_list.empty()) { // Can't use nack_list.data() since it's not supported by all // compilers. @@ -741,10 +740,8 @@ void ChannelReceive::ReceivedRTCPPacket(const uint8_t* data, size_t length) { // Deliver RTCP packet to RTP/RTCP module for parsing rtp_rtcp_->IncomingRtcpPacket(rtc::MakeArrayView(data, length)); - int64_t rtt = 0; - rtp_rtcp_->RTT(remote_ssrc_, &rtt, /*avg_rtt=*/nullptr, /*min_rtt=*/nullptr, - /*max_rtt=*/nullptr); - if (rtt == 0) { + absl::optional rtt = rtp_rtcp_->LastRtt(); + if (!rtt.has_value()) { // Waiting for valid RTT. return; } @@ -758,8 +755,7 @@ void ChannelReceive::ReceivedRTCPPacket(const uint8_t* data, size_t length) { { MutexLock lock(&ts_stats_lock_); - ntp_estimator_.UpdateRtcpTimestamp(TimeDelta::Millis(rtt), - last_sr->last_remote_timestamp, + ntp_estimator_.UpdateRtcpTimestamp(*rtt, last_sr->last_remote_timestamp, last_sr->last_remote_rtp_timestamp); absl::optional remote_to_local_clock_offset = ntp_estimator_.EstimateRemoteToLocalClockOffset(); diff --git a/audio/voip/audio_ingress.cc b/audio/voip/audio_ingress.cc index a00934cb46..80f21152c0 100644 --- a/audio/voip/audio_ingress.cc +++ b/audio/voip/audio_ingress.cc @@ -212,8 +212,8 @@ void AudioIngress::ReceivedRTCPPacket( // Deliver RTCP packet to RTP/RTCP module for parsing and processing. rtp_rtcp_->IncomingRtcpPacket(rtcp_packet); - int64_t rtt = 0; - if (rtp_rtcp_->RTT(remote_ssrc_, &rtt, nullptr, nullptr, nullptr) != 0) { + absl::optional rtt = rtp_rtcp_->LastRtt(); + if (!rtt.has_value()) { // Waiting for valid RTT. return; } @@ -227,8 +227,7 @@ void AudioIngress::ReceivedRTCPPacket( { MutexLock lock(&lock_); - ntp_estimator_.UpdateRtcpTimestamp(TimeDelta::Millis(rtt), - last_sr->last_remote_timestamp, + ntp_estimator_.UpdateRtcpTimestamp(*rtt, last_sr->last_remote_timestamp, last_sr->last_remote_rtp_timestamp); } } diff --git a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index e1e787a284..a6c5cec705 100644 --- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -127,6 +127,7 @@ class MockRtpRtcpInterface : public RtpRtcpInterface { int64_t* min_rtt, int64_t* max_rtt), (const, override)); + MOCK_METHOD(absl::optional, LastRtt, (), (const, override)); MOCK_METHOD(int64_t, ExpectedRetransmissionTimeMs, (), (const, override)); MOCK_METHOD(int32_t, SendRTCP, (RTCPPacketType packet_type), (override)); MOCK_METHOD(void, diff --git a/modules/rtp_rtcp/source/rtcp_receiver.cc b/modules/rtp_rtcp/source/rtcp_receiver.cc index 5c4b2a8341..2039e8bda0 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -293,6 +293,15 @@ int32_t RTCPReceiver::RTT(uint32_t remote_ssrc, return 0; } +absl::optional RTCPReceiver::LastRtt() const { + MutexLock lock(&rtcp_receiver_lock_); + auto it = rtts_.find(remote_ssrc_); + if (it == rtts_.end()) { + return absl::nullopt; + } + return it->second.last_rtt(); +} + RTCPReceiver::NonSenderRttStats RTCPReceiver::GetNonSenderRTT() const { MutexLock lock(&rtcp_receiver_lock_); auto it = non_sender_rtts_.find(remote_ssrc_); diff --git a/modules/rtp_rtcp/source/rtcp_receiver.h b/modules/rtp_rtcp/source/rtcp_receiver.h index 567b5b36f3..2def970c1f 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.h +++ b/modules/rtp_rtcp/source/rtcp_receiver.h @@ -123,6 +123,8 @@ class RTCPReceiver final { int64_t* min_rtt_ms, int64_t* max_rtt_ms) const; + absl::optional LastRtt() const; + // Returns non-sender RTT metrics for the remote SSRC. NonSenderRttStats GetNonSenderRTT() const; diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 913885e557..466503b4a0 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -478,6 +478,17 @@ int32_t ModuleRtpRtcpImpl::RTT(const uint32_t remote_ssrc, return ret; } +absl::optional ModuleRtpRtcpImpl::LastRtt() const { + absl::optional rtt = rtcp_receiver_.LastRtt(); + if (!rtt.has_value()) { + MutexLock lock(&mutex_rtt_); + if (rtt_ms_ > 0) { + rtt = TimeDelta::Millis(rtt_ms_); + } + } + return rtt; +} + int64_t ModuleRtpRtcpImpl::ExpectedRetransmissionTimeMs() const { int64_t expected_retransmission_time_ms = rtt_ms(); if (expected_retransmission_time_ms > 0) { diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index 509036d1f6..491fd2fafb 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -170,11 +170,12 @@ class ABSL_DEPRECATED("") ModuleRtpRtcpImpl int32_t SetCNAME(absl::string_view c_name) override; // Get RoundTripTime. - int32_t RTT(uint32_t remote_ssrc, - int64_t* rtt, - int64_t* avg_rtt, - int64_t* min_rtt, - int64_t* max_rtt) const override; + [[deprecated]] int32_t RTT(uint32_t remote_ssrc, + int64_t* rtt, + int64_t* avg_rtt, + int64_t* min_rtt, + int64_t* max_rtt) const override; + absl::optional LastRtt() const override; int64_t ExpectedRetransmissionTimeMs() const override; diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc index f5627c2fcd..0f64e5463d 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc @@ -463,6 +463,17 @@ int32_t ModuleRtpRtcpImpl2::RTT(const uint32_t remote_ssrc, return ret; } +absl::optional ModuleRtpRtcpImpl2::LastRtt() const { + absl::optional rtt = rtcp_receiver_.LastRtt(); + if (!rtt.has_value()) { + MutexLock lock(&mutex_rtt_); + if (rtt_ms_ > 0) { + rtt = TimeDelta::Millis(rtt_ms_); + } + } + return rtt; +} + int64_t ModuleRtpRtcpImpl2::ExpectedRetransmissionTimeMs() const { int64_t expected_retransmission_time_ms = rtt_ms(); if (expected_retransmission_time_ms > 0) { diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2.h b/modules/rtp_rtcp/source/rtp_rtcp_impl2.h index 147cd33800..033b65d30f 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2.h @@ -181,11 +181,12 @@ class ModuleRtpRtcpImpl2 final : public RtpRtcpInterface, int32_t SetCNAME(absl::string_view c_name) override; // Get RoundTripTime. - int32_t RTT(uint32_t remote_ssrc, - int64_t* rtt, - int64_t* avg_rtt, - int64_t* min_rtt, - int64_t* max_rtt) const override; + [[deprecated]] int32_t RTT(uint32_t remote_ssrc, + int64_t* rtt, + int64_t* avg_rtt, + int64_t* min_rtt, + int64_t* max_rtt) const override; + absl::optional LastRtt() const override; int64_t ExpectedRetransmissionTimeMs() const override; diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc index 009bc3cfcd..950bcb9bc4 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc @@ -69,6 +69,10 @@ enum : int { kTransmissionOffsetExtensionId, }; +MATCHER_P2(Near, value, margin, "") { + return value - margin <= arg && arg <= value + margin; +} + class RtcpRttStatsTestImpl : public RtcpRttStats { public: RtcpRttStatsTestImpl() : rtt_ms_(0) {} @@ -424,20 +428,8 @@ TEST_F(RtpRtcpImpl2Test, Rtt) { AdvanceTime(kOneWayNetworkDelay); // Verify RTT. - int64_t rtt; - int64_t avg_rtt; - int64_t min_rtt; - int64_t max_rtt; - EXPECT_EQ( - 0, sender_.impl_->RTT(kReceiverSsrc, &rtt, &avg_rtt, &min_rtt, &max_rtt)); - EXPECT_NEAR(2 * kOneWayNetworkDelay.ms(), rtt, 1); - EXPECT_NEAR(2 * kOneWayNetworkDelay.ms(), avg_rtt, 1); - EXPECT_NEAR(2 * kOneWayNetworkDelay.ms(), min_rtt, 1); - EXPECT_NEAR(2 * kOneWayNetworkDelay.ms(), max_rtt, 1); - - // No RTT from other ssrc. - EXPECT_EQ(-1, sender_.impl_->RTT(kReceiverSsrc + 1, &rtt, &avg_rtt, &min_rtt, - &max_rtt)); + EXPECT_THAT(sender_.impl_->LastRtt(), + Near(2 * kOneWayNetworkDelay, TimeDelta::Millis(1))); // Verify RTT from rtt_stats config. EXPECT_EQ(0, sender_.rtt_stats_.LastProcessedRtt()); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_interface.h b/modules/rtp_rtcp/source/rtp_rtcp_interface.h index 89c6d4646a..a5c5adfc16 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_interface.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_interface.h @@ -380,11 +380,14 @@ class RtpRtcpInterface : public RtcpFeedbackSenderInterface { // Returns current RTT (round-trip time) estimate. // Returns -1 on failure else 0. - virtual int32_t RTT(uint32_t remote_ssrc, - int64_t* rtt, - int64_t* avg_rtt, - int64_t* min_rtt, - int64_t* max_rtt) const = 0; + [[deprecated]] virtual int32_t RTT(uint32_t remote_ssrc, + int64_t* rtt, + int64_t* avg_rtt, + int64_t* min_rtt, + int64_t* max_rtt) const = 0; + + // Returns current RTT (round-trip time) estimate. + virtual absl::optional LastRtt() const = 0; // Returns the estimated RTT, with fallback to a default value. virtual int64_t ExpectedRetransmissionTimeMs() const = 0; diff --git a/video/rtp_video_stream_receiver2.cc b/video/rtp_video_stream_receiver2.cc index c35be9142d..a01e60f8bb 100644 --- a/video/rtp_video_stream_receiver2.cc +++ b/video/rtp_video_stream_receiver2.cc @@ -1135,9 +1135,8 @@ bool RtpVideoStreamReceiver2::DeliverRtcp(const uint8_t* rtcp_packet, rtp_rtcp_->IncomingRtcpPacket( rtc::MakeArrayView(rtcp_packet, rtcp_packet_length)); - int64_t rtt = 0; - rtp_rtcp_->RTT(config_.rtp.remote_ssrc, &rtt, nullptr, nullptr, nullptr); - if (rtt == 0) { + absl::optional rtt = rtp_rtcp_->LastRtt(); + if (!rtt.has_value()) { // Waiting for valid rtt. return true; } @@ -1152,8 +1151,7 @@ bool RtpVideoStreamReceiver2::DeliverRtcp(const uint8_t* rtcp_packet, last_sr->last_arrival_timestamp.ToMs(); // Don't use old SRs to estimate time. if (time_since_received <= 1) { - ntp_estimator_.UpdateRtcpTimestamp(TimeDelta::Millis(rtt), - last_sr->last_remote_timestamp, + ntp_estimator_.UpdateRtcpTimestamp(*rtt, last_sr->last_remote_timestamp, last_sr->last_remote_rtp_timestamp); absl::optional remote_to_local_clock_offset = ntp_estimator_.EstimateRemoteToLocalClockOffset();