From af785d97599d0a63dc0326154ddb5d7c239656d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Tue, 31 May 2022 10:45:41 +0200 Subject: [PATCH] Deprecate setter RtpRtcpInterface::SetRid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This setter method is replaced by a construction-time config setting. Bug: None Change-Id: Iddffaeeb719a56328bccde3c4a1a0a852d2131b1 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264501 Reviewed-by: Björn Terelius Reviewed-by: Danil Chapovalov Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/main@{#37060} --- call/rtp_video_sender.cc | 21 +++----- call/rtp_video_sender.h | 1 - modules/rtp_rtcp/source/rtp_rtcp_interface.h | 9 +++- modules/rtp_rtcp/source/rtp_sender.cc | 5 +- .../rtp_rtcp/source/rtp_sender_unittest.cc | 51 ++++++------------- 5 files changed, 36 insertions(+), 51 deletions(-) diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index 8dbb2267a5..1930036a9a 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -238,6 +238,12 @@ std::vector CreateRtpStreamSenders( RTC_DCHECK(rtp_config.rtx.ssrcs.empty() || rtp_config.rtx.ssrcs.size() == rtp_config.ssrcs.size()); + + // Some streams could have been disabled, but the rids are still there. + // This will occur when simulcast has been disabled for a codec (e.g. VP9) + RTC_DCHECK(rtp_config.rids.empty() || + rtp_config.rids.size() >= rtp_config.ssrcs.size()); + for (size_t i = 0; i < rtp_config.ssrcs.size(); ++i) { RTPSenderVideo::Config video_config; configuration.local_media_ssrc = rtp_config.ssrcs[i]; @@ -251,6 +257,8 @@ std::vector CreateRtpStreamSenders( RTC_DCHECK_EQ(configuration.rtx_send_ssrc.has_value(), !rtp_config.rtx.ssrcs.empty()); + configuration.rid = (i < rtp_config.rids.size()) ? rtp_config.rids[i] : ""; + configuration.need_rtp_packet_infos = rtp_config.lntf.enabled; std::unique_ptr rtp_rtcp( @@ -425,7 +433,6 @@ RtpVideoSender::RtpVideoSender( } ConfigureSsrcs(suspended_ssrcs); - ConfigureRids(); if (!rtp_config_.mid.empty()) { for (const RtpStreamSender& stream : rtp_streams_) { @@ -760,18 +767,6 @@ void RtpVideoSender::ConfigureSsrcs( } } -void RtpVideoSender::ConfigureRids() { - if (rtp_config_.rids.empty()) - return; - - // Some streams could have been disabled, but the rids are still there. - // This will occur when simulcast has been disabled for a codec (e.g. VP9) - RTC_DCHECK(rtp_config_.rids.size() >= rtp_streams_.size()); - for (size_t i = 0; i < rtp_streams_.size(); ++i) { - rtp_streams_[i].rtp_rtcp->SetRid(rtp_config_.rids[i]); - } -} - void RtpVideoSender::OnNetworkAvailability(bool network_available) { for (const RtpStreamSender& stream : rtp_streams_) { stream.rtp_rtcp->SetRTCPStatus(network_available ? rtp_config_.rtcp_mode diff --git a/call/rtp_video_sender.h b/call/rtp_video_sender.h index c4a2b92011..e177bc4c3a 100644 --- a/call/rtp_video_sender.h +++ b/call/rtp_video_sender.h @@ -159,7 +159,6 @@ class RtpVideoSender : public RtpVideoSenderInterface, void UpdateModuleSendingState() RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_); void ConfigureProtection(); void ConfigureSsrcs(const std::map& suspended_ssrcs); - void ConfigureRids(); bool NackEnabled() const; uint32_t GetPacketizationOverheadRate() const; DataRate CalculateOverheadRate(DataRate data_rate, diff --git a/modules/rtp_rtcp/source/rtp_rtcp_interface.h b/modules/rtp_rtcp/source/rtp_rtcp_interface.h index 2c30318cae..f3a0f39bf8 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_interface.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_interface.h @@ -148,6 +148,12 @@ class RtpRtcpInterface : public RtcpFeedbackSenderInterface { // Estimate RTT as non-sender as described in // https://tools.ietf.org/html/rfc3611#section-4.4 and #section-4.5 bool non_sender_rtt_measurement = false; + + // If non-empty, sets the value for sending in the RID (and Repaired) RTP + // header extension. RIDs are used to identify an RTP stream if SSRCs are + // not negotiated. If the RID and Repaired RID extensions are not + // registered, the RID will not be sent. + std::string rid; }; // Stats for RTCP sender reports (SR) for a specific SSRC. @@ -256,7 +262,8 @@ class RtpRtcpInterface : public RtcpFeedbackSenderInterface { // RIDs are used to identify an RTP stream if SSRCs are not negotiated. // If the RID and Repaired RID extensions are not registered, the RID will // not be sent. - virtual void SetRid(absl::string_view rid) = 0; + [[deprecated("Use the rid member of config struct instead'")]] virtual void + SetRid(absl::string_view rid) = 0; // Sets the value for sending in the MID RTP header extension. // The MID RTP header extension should be registered for this to do anything. diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index d42c7085ab..72e8c4cc10 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -173,6 +173,7 @@ RTPSender::RTPSender(const RtpRtcpInterface::Configuration& config, max_packet_size_(IP_PACKET_SIZE - 28), // Default is IP-v4/UDP. rtp_header_extension_map_(config.extmap_allow_mixed), // RTP variables + rid_(config.rid), always_send_mid_and_rid_(config.always_send_mid_and_rid), ssrc_has_acked_(false), rtx_ssrc_has_acked_(false), @@ -180,12 +181,14 @@ RTPSender::RTPSender(const RtpRtcpInterface::Configuration& config, rtx_(kRtxOff), supports_bwe_extension_(false), retransmission_rate_limiter_(config.retransmission_rate_limiter) { - UpdateHeaderSizes(); // This random initialization is not intended to be cryptographic strong. timestamp_offset_ = random_.Rand(); RTC_DCHECK(paced_sender_); RTC_DCHECK(packet_history_); + RTC_DCHECK_LE(rid_.size(), RtpStreamId::kMaxValueSizeBytes); + + UpdateHeaderSizes(); } RTPSender::~RTPSender() { diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index a9b5dec2c9..0a1e3ebd31 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -72,6 +72,8 @@ const uint8_t kPayloadData[] = {47, 11, 32, 93, 89}; const int64_t kDefaultExpectedRetransmissionTimeMs = 125; const size_t kMaxPaddingLength = 224; // Value taken from rtp_sender.cc. const uint32_t kTimestampTicksPerMs = 90; // 90kHz clock. +constexpr absl::string_view kMid = "mid"; +constexpr absl::string_view kRid = "f"; using ::testing::_; using ::testing::AllOf; @@ -161,6 +163,9 @@ class RtpSenderTest : public ::testing::Test { config.retransmission_rate_limiter = &retransmission_rate_limiter_; config.paced_sender = &mock_paced_sender_; config.field_trials = &field_trials_; + // Configure rid unconditionally, it has effect only if + // corresponding header extension is enabled. + config.rid = std::string(kRid); return config; } @@ -274,12 +279,11 @@ class RtpSenderTest : public ::testing::Test { // Enable sending of the RSID header extension for the primary SSRC and the // RRSID header extension for the RTX SSRC. - void EnableRidSending(absl::string_view rid) { + void EnableRidSending() { rtp_sender_->RegisterRtpHeaderExtension(RtpStreamId::Uri(), kRidExtensionId); rtp_sender_->RegisterRtpHeaderExtension(RepairedRtpStreamId::Uri(), kRepairedRidExtensionId); - rtp_sender_->SetRid(rid); } }; @@ -593,7 +597,6 @@ TEST_F(RtpSenderTest, KeepsTimestampsOnPayloadPadding) { // Test that the MID header extension is included on sent packets when // configured. TEST_F(RtpSenderTest, MidIncludedOnSentPackets) { - const char kMid[] = "mid"; EnableMidSending(kMid); // Send a couple packets, expect both packets to have the MID set. @@ -606,8 +609,7 @@ TEST_F(RtpSenderTest, MidIncludedOnSentPackets) { } TEST_F(RtpSenderTest, RidIncludedOnSentPackets) { - const char kRid[] = "f"; - EnableRidSending(kRid); + EnableRidSending(); EXPECT_CALL(mock_paced_sender_, EnqueuePackets(ElementsAre(Pointee(Property( @@ -616,9 +618,8 @@ TEST_F(RtpSenderTest, RidIncludedOnSentPackets) { } TEST_F(RtpSenderTest, RidIncludedOnRtxSentPackets) { - const char kRid[] = "f"; EnableRtx(); - EnableRidSending(kRid); + EnableRidSending(); EXPECT_CALL(mock_paced_sender_, EnqueuePackets(ElementsAre(Pointee(AllOf( @@ -641,11 +642,8 @@ TEST_F(RtpSenderTest, RidIncludedOnRtxSentPackets) { } TEST_F(RtpSenderTest, MidAndRidNotIncludedOnSentPacketsAfterAck) { - const char kMid[] = "mid"; - const char kRid[] = "f"; - EnableMidSending(kMid); - EnableRidSending(kRid); + EnableRidSending(); // This first packet should include both MID and RID. EXPECT_CALL( @@ -667,10 +665,8 @@ TEST_F(RtpSenderTest, MidAndRidNotIncludedOnSentPacketsAfterAck) { TEST_F(RtpSenderTest, MidAndRidAlwaysIncludedOnSentPacketsWhenConfigured) { SetUpRtpSender(false, /*always_send_mid_and_rid=*/true, nullptr); - const char kMid[] = "mid"; - const char kRid[] = "f"; EnableMidSending(kMid); - EnableRidSending(kRid); + EnableRidSending(); // Send two media packets: one before and one after the ack. // Due to the configuration, both sent packets should contain MID and RID. @@ -690,12 +686,9 @@ TEST_F(RtpSenderTest, MidAndRidAlwaysIncludedOnSentPacketsWhenConfigured) { // the first packets for a given SSRC, and RTX packets are sent on a separate // SSRC. TEST_F(RtpSenderTest, MidAndRidIncludedOnFirstRtxPacket) { - const char kMid[] = "mid"; - const char kRid[] = "f"; - EnableRtx(); EnableMidSending(kMid); - EnableRidSending(kRid); + EnableRidSending(); // This first packet will include both MID and RID. EXPECT_CALL(mock_paced_sender_, EnqueuePackets); @@ -724,12 +717,9 @@ TEST_F(RtpSenderTest, MidAndRidIncludedOnFirstRtxPacket) { // not include either MID or RRID even if the packet being retransmitted did // had a MID or RID. TEST_F(RtpSenderTest, MidAndRidNotIncludedOnRtxPacketsAfterAck) { - const char kMid[] = "mid"; - const char kRid[] = "f"; - EnableRtx(); EnableMidSending(kMid); - EnableRidSending(kRid); + EnableRidSending(); // This first packet will include both MID and RID. auto first_built_packet = SendGenericPacket(); @@ -768,11 +758,9 @@ TEST_F(RtpSenderTest, MidAndRidNotIncludedOnRtxPacketsAfterAck) { TEST_F(RtpSenderTest, MidAndRidAlwaysIncludedOnRtxPacketsWhenConfigured) { SetUpRtpSender(false, /*always_send_mid_and_rid=*/true, nullptr); - const char kMid[] = "mid"; - const char kRid[] = "f"; EnableRtx(); EnableMidSending(kMid); - EnableRidSending(kRid); + EnableRidSending(); // Send two media packets: one before and one after the ack. EXPECT_CALL( @@ -814,11 +802,8 @@ TEST_F(RtpSenderTest, MidAndRidAlwaysIncludedOnRtxPacketsWhenConfigured) { // Test that if the RtpState indicates an ACK has been received on that SSRC // then neither the MID nor RID header extensions will be sent. TEST_F(RtpSenderTest, MidAndRidNotIncludedOnSentPacketsAfterRtpStateRestored) { - const char kMid[] = "mid"; - const char kRid[] = "f"; - EnableMidSending(kMid); - EnableRidSending(kRid); + EnableRidSending(); RtpState state = rtp_sender_->GetRtpState(); EXPECT_FALSE(state.ssrc_has_acked); @@ -837,12 +822,9 @@ TEST_F(RtpSenderTest, MidAndRidNotIncludedOnSentPacketsAfterRtpStateRestored) { // RTX SSRC then neither the MID nor RRID header extensions will be sent on // RTX packets. TEST_F(RtpSenderTest, MidAndRridNotIncludedOnRtxPacketsAfterRtpStateRestored) { - const char kMid[] = "mid"; - const char kRid[] = "f"; - EnableRtx(); EnableMidSending(kMid); - EnableRidSending(kRid); + EnableRidSending(); RtpState rtx_state = rtp_sender_->GetRtxRtpState(); EXPECT_FALSE(rtx_state.ssrc_has_acked); @@ -947,13 +929,12 @@ TEST_F(RtpSenderTest, CountMidOnlyUntilAcked) { EXPECT_EQ(rtp_sender_->ExpectedPerPacketOverhead(), 12u); rtp_sender_->RegisterRtpHeaderExtension(RtpMid::Uri(), kMidExtensionId); - rtp_sender_->RegisterRtpHeaderExtension(RtpStreamId::Uri(), kRidExtensionId); // Counted only if set. EXPECT_EQ(rtp_sender_->ExpectedPerPacketOverhead(), 12u); rtp_sender_->SetMid("foo"); EXPECT_EQ(rtp_sender_->ExpectedPerPacketOverhead(), 36u); - rtp_sender_->SetRid("bar"); + rtp_sender_->RegisterRtpHeaderExtension(RtpStreamId::Uri(), kRidExtensionId); EXPECT_EQ(rtp_sender_->ExpectedPerPacketOverhead(), 52u); // Ack received, mid/rid no longer sent.