From 808531653e72bb6b4095a0b33d911edcbbc54f46 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Mon, 21 Feb 2022 13:23:06 +0100 Subject: [PATCH] In RtcpTransceiver implement handling incoming RRTR Bug: webrtc:8239 Change-Id: I4a469b6a0c2e387e35262798f4686fbf310d00cd Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/251902 Reviewed-by: Emil Lundmark Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#36037} --- modules/rtp_rtcp/source/rtcp_packet/dlrr.h | 10 +++++ .../rtcp_packet/extended_reports_unittest.cc | 13 ------ modules/rtp_rtcp/source/rtcp_packet/rrtr.h | 8 ++++ .../rtp_rtcp/source/rtcp_transceiver_config.h | 7 +++ .../rtp_rtcp/source/rtcp_transceiver_impl.cc | 44 ++++++++++++++++--- .../rtp_rtcp/source/rtcp_transceiver_impl.h | 8 ++++ .../source/rtcp_transceiver_impl_unittest.cc | 38 ++++++++++++++++ 7 files changed, 109 insertions(+), 19 deletions(-) diff --git a/modules/rtp_rtcp/source/rtcp_packet/dlrr.h b/modules/rtp_rtcp/source/rtcp_packet/dlrr.h index 6fe2099fd9..ad91dfdcc6 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/dlrr.h +++ b/modules/rtp_rtcp/source/rtcp_packet/dlrr.h @@ -24,11 +24,21 @@ struct ReceiveTimeInfo { ReceiveTimeInfo() : ssrc(0), last_rr(0), delay_since_last_rr(0) {} ReceiveTimeInfo(uint32_t ssrc, uint32_t last_rr, uint32_t delay) : ssrc(ssrc), last_rr(last_rr), delay_since_last_rr(delay) {} + uint32_t ssrc; uint32_t last_rr; uint32_t delay_since_last_rr; }; +inline bool operator==(const ReceiveTimeInfo& lhs, const ReceiveTimeInfo& rhs) { + return lhs.ssrc == rhs.ssrc && lhs.last_rr == rhs.last_rr && + lhs.delay_since_last_rr == rhs.delay_since_last_rr; +} + +inline bool operator!=(const ReceiveTimeInfo& lhs, const ReceiveTimeInfo& rhs) { + return !(lhs == rhs); +} + // DLRR Report Block: Delay since the Last Receiver Report (RFC 3611). class Dlrr { public: diff --git a/modules/rtp_rtcp/source/rtcp_packet/extended_reports_unittest.cc b/modules/rtp_rtcp/source/rtcp_packet/extended_reports_unittest.cc index 7c50c01c43..3d9a2a3408 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/extended_reports_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_packet/extended_reports_unittest.cc @@ -24,19 +24,6 @@ using webrtc::rtcp::ReceiveTimeInfo; using webrtc::rtcp::Rrtr; namespace webrtc { -// Define comparision operators that shouldn't be needed in production, -// but make testing matches more clear. -namespace rtcp { -bool operator==(const Rrtr& rrtr1, const Rrtr& rrtr2) { - return rrtr1.ntp() == rrtr2.ntp(); -} - -bool operator==(const ReceiveTimeInfo& time1, const ReceiveTimeInfo& time2) { - return time1.ssrc == time2.ssrc && time1.last_rr == time2.last_rr && - time1.delay_since_last_rr == time2.delay_since_last_rr; -} -} // namespace rtcp - namespace { constexpr uint32_t kSenderSsrc = 0x12345678; constexpr uint8_t kEmptyPacket[] = {0x80, 207, 0x00, 0x01, diff --git a/modules/rtp_rtcp/source/rtcp_packet/rrtr.h b/modules/rtp_rtcp/source/rtcp_packet/rrtr.h index 8eb4ce62ad..827bd74399 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/rrtr.h +++ b/modules/rtp_rtcp/source/rtcp_packet/rrtr.h @@ -46,6 +46,14 @@ class Rrtr { NtpTime ntp_; }; +inline bool operator==(const Rrtr& rrtr1, const Rrtr& rrtr2) { + return rrtr1.ntp() == rrtr2.ntp(); +} + +inline bool operator!=(const Rrtr& rrtr1, const Rrtr& rrtr2) { + return !(rrtr1 == rrtr2); +} + } // namespace rtcp } // namespace webrtc #endif // MODULES_RTP_RTCP_SOURCE_RTCP_PACKET_RRTR_H_ diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_config.h b/modules/rtp_rtcp/source/rtcp_transceiver_config.h index 0be633fc61..2400f9b908 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_config.h +++ b/modules/rtp_rtcp/source/rtcp_transceiver_config.h @@ -162,6 +162,13 @@ struct RtcpTransceiverConfig { // https://tools.ietf.org/html/rfc3611#section-4.4 and #section-4.5 bool non_sender_rtt_measurement = false; + // Reply to incoming RRTR messages so that remote endpoint may estimate RTT as + // non-sender as described in https://tools.ietf.org/html/rfc3611#section-4.4 + // and #section-4.5 + // TODO(danilchap): Make it true by default after users got enough time to + // turn it off if not needed. + bool reply_to_non_sender_rtt_measurement = false; + // Allows a REMB message to be sent immediately when SetRemb is called without // having to wait for the next compount message to be sent. bool send_remb_on_change = false; diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc b/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc index c075b2f99e..5909af93ce 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc +++ b/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc @@ -370,6 +370,14 @@ void RtcpTransceiverImpl::HandleExtendedReports( if (!extended_reports.Parse(rtcp_packet_header)) return; + if (config_.reply_to_non_sender_rtt_measurement && extended_reports.rrtr()) { + RrtrTimes& rrtr = received_rrtrs_[extended_reports.sender_ssrc()]; + rrtr.received_remote_mid_ntp_time = + CompactNtp(extended_reports.rrtr()->ntp()); + rrtr.local_receive_mid_ntp_time = + CompactNtp(config_.clock->ConvertTimestampToNtpTime(now)); + } + if (extended_reports.dlrr()) HandleDlrr(extended_reports.dlrr(), now); @@ -620,7 +628,29 @@ void RtcpTransceiverImpl::CreateCompoundPacket(Timestamp now, if (remb_.has_value()) { reserved_bytes += remb_->BlockLength(); } + absl::optional xr; + if (!received_rrtrs_.empty()) { + RTC_DCHECK(config_.reply_to_non_sender_rtt_measurement); + xr.emplace(); + uint32_t now_ntp = + CompactNtp(config_.clock->ConvertTimestampToNtpTime(now)); + for (const auto& [ssrc, rrtr_info] : received_rrtrs_) { + rtcp::ReceiveTimeInfo reply; + reply.ssrc = ssrc; + reply.last_rr = rrtr_info.received_remote_mid_ntp_time; + reply.delay_since_last_rr = + now_ntp - rrtr_info.local_receive_mid_ntp_time; + xr->AddDlrrItem(reply); + } + reserved_bytes += xr->BlockLength(); + } if (config_.non_sender_rtt_measurement) { + // It looks like bytes for ExtendedReport header are reserved twice, but in + // practice the same RtcpTransceiver won't both produce RRTR (i.e. it is a + // receiver-only) and reply to RRTR (i.e. remote participant is a receiver + // only). If that happen, then `reserved_bytes` would be slightly larger + // than it should, which is not an issue. + // 4 bytes for common RTCP header + 4 bytes for the ExtenedReports header. reserved_bytes += (4 + 4 + rtcp::Rrtr::kLength); } @@ -635,14 +665,16 @@ void RtcpTransceiverImpl::CreateCompoundPacket(Timestamp now, sender.AppendPacket(*remb_); } if (!result.has_sender_report && config_.non_sender_rtt_measurement) { - rtcp::ExtendedReports xr; - xr.SetSenderSsrc(result.sender_ssrc); - + if (!xr.has_value()) { + xr.emplace(); + } rtcp::Rrtr rrtr; rrtr.SetNtp(config_.clock->ConvertTimestampToNtpTime(now)); - xr.SetRrtr(rrtr); - - sender.AppendPacket(xr); + xr->SetRrtr(rrtr); + } + if (xr.has_value()) { + xr->SetSenderSsrc(result.sender_ssrc); + sender.AppendPacket(*xr); } } diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_impl.h b/modules/rtp_rtcp/source/rtcp_transceiver_impl.h index ce25899bcf..8dc50b89e6 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_impl.h +++ b/modules/rtp_rtcp/source/rtcp_transceiver_impl.h @@ -82,6 +82,13 @@ class RtcpTransceiverImpl { class PacketSender; struct RemoteSenderState; struct LocalSenderState; + struct RrtrTimes { + // Received remote NTP timestamp in compact representation. + uint32_t received_remote_mid_ntp_time; + + // Local NTP time when the report was received in compact representation. + uint32_t local_receive_mid_ntp_time; + }; void HandleReceivedPacket(const rtcp::CommonHeader& rtcp_packet_header, Timestamp now, @@ -144,6 +151,7 @@ class RtcpTransceiverImpl { std::list local_senders_; flat_map::iterator> local_senders_by_ssrc_; + flat_map received_rrtrs_; RepeatingTaskHandle periodic_task_handle_; }; diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc b/modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc index 417dcdd3de..82bfadecef 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc @@ -45,6 +45,7 @@ using ::testing::NiceMock; using ::testing::Return; using ::testing::SizeIs; using ::testing::StrictMock; +using ::testing::UnorderedElementsAre; using ::testing::WithArg; using ::webrtc::rtcp::Bye; using ::webrtc::rtcp::CompoundPacket; @@ -1208,6 +1209,43 @@ TEST(RtcpTransceiverImplTest, SendsXrRrtrWhenEnabled) { EXPECT_EQ(rtcp_parser.xr()->rrtr()->ntp(), ntp_time_now); } +TEST(RtcpTransceiverImplTest, RepliesToRrtrWhenEnabled) { + static constexpr uint32_t kSenderSsrc[] = {4321, 9876}; + SimulatedClock clock(0); + RtcpTransceiverConfig config = DefaultTestConfig(); + config.clock = &clock; + config.reply_to_non_sender_rtt_measurement = true; + RtcpPacketParser rtcp_parser; + RtcpParserTransport transport(&rtcp_parser); + config.outgoing_transport = &transport; + RtcpTransceiverImpl rtcp_transceiver(config); + + rtcp::ExtendedReports xr; + rtcp::Rrtr rrtr; + rrtr.SetNtp(NtpTime(uint64_t{0x1111'2222'3333'4444})); + xr.SetRrtr(rrtr); + xr.SetSenderSsrc(kSenderSsrc[0]); + rtcp_transceiver.ReceivePacket(xr.Build(), clock.CurrentTime()); + clock.AdvanceTime(TimeDelta::Millis(1'500)); + + rrtr.SetNtp(NtpTime(uint64_t{0x4444'5555'6666'7777})); + xr.SetRrtr(rrtr); + xr.SetSenderSsrc(kSenderSsrc[1]); + rtcp_transceiver.ReceivePacket(xr.Build(), clock.CurrentTime()); + clock.AdvanceTime(TimeDelta::Millis(500)); + + rtcp_transceiver.SendCompoundPacket(); + + EXPECT_EQ(rtcp_parser.xr()->num_packets(), 1); + static constexpr uint32_t kComactNtpOneSecond = 0x0001'0000; + EXPECT_THAT(rtcp_parser.xr()->dlrr().sub_blocks(), + UnorderedElementsAre( + rtcp::ReceiveTimeInfo(kSenderSsrc[0], 0x2222'3333, + /*delay=*/2 * kComactNtpOneSecond), + rtcp::ReceiveTimeInfo(kSenderSsrc[1], 0x5555'6666, + /*delay=*/kComactNtpOneSecond / 2))); +} + TEST(RtcpTransceiverImplTest, SendsNoXrRrtrWhenDisabled) { SimulatedClock clock(0); RtcpTransceiverConfig config;