From 27d5f14cf27b99ff02d0f04d6bc69dd0faeb9dd7 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Tue, 15 Feb 2022 17:28:07 +0100 Subject: [PATCH] in RTPSender disallow enabling misconfigured rtx MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: None Change-Id: Id94771626ef723212e4d92d9093af3ec9e647990 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/251780 Auto-Submit: Danil Chapovalov Reviewed-by: Erik Språng Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#36020} --- modules/rtp_rtcp/source/rtp_sender.cc | 7 ++++ .../rtp_rtcp/source/rtp_sender_unittest.cc | 38 ++++++++++++++++++- 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index feda738d06..c3321d8723 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -258,6 +258,12 @@ size_t RTPSender::MaxRtpPacketSize() const { void RTPSender::SetRtxStatus(int mode) { MutexLock lock(&send_mutex_); + if (mode != kRtxOff && + (!rtx_ssrc_.has_value() || rtx_payload_type_map_.empty())) { + RTC_LOG(LS_ERROR) + << "Failed to enable RTX without RTX SSRC or payload types."; + return; + } rtx_ = mode; } @@ -453,6 +459,7 @@ std::vector> RTPSender::GeneratePadding( } RTC_DCHECK(rtx_ssrc_); + RTC_DCHECK(!rtx_payload_type_map_.empty()); padding_packet->SetSsrc(*rtx_ssrc_); padding_packet->SetPayloadType(rtx_payload_type_map_.begin()->second); } diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 21c32b377c..99c2c8087a 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -443,6 +443,39 @@ TEST_F(RtpSenderTest, NoPaddingAsFirstPacketWithoutBweExtensions) { IsEmpty()); } +TEST_F(RtpSenderTest, RequiresRtxSsrcToEnableRtx) { + RtpRtcpInterface::Configuration config = GetDefaultConfig(); + config.rtx_send_ssrc = absl::nullopt; + RTPSender rtp_sender(config, packet_history_.get(), config.paced_sender); + rtp_sender.SetRtxPayloadType(kRtxPayload, kPayload); + + rtp_sender.SetRtxStatus(kRtxRetransmitted); + + EXPECT_EQ(rtp_sender.RtxStatus(), kRtxOff); +} + +TEST_F(RtpSenderTest, RequiresRtxPayloadTypesToEnableRtx) { + RtpRtcpInterface::Configuration config = GetDefaultConfig(); + config.rtx_send_ssrc = kRtxSsrc; + RTPSender rtp_sender(config, packet_history_.get(), config.paced_sender); + + rtp_sender.SetRtxStatus(kRtxRetransmitted); + + EXPECT_EQ(rtp_sender.RtxStatus(), kRtxOff); +} + +TEST_F(RtpSenderTest, CanEnableRtxWhenRtxSsrcAndPayloadTypeAreConfigured) { + RtpRtcpInterface::Configuration config = GetDefaultConfig(); + config.rtx_send_ssrc = kRtxSsrc; + RTPSender rtp_sender(config, packet_history_.get(), config.paced_sender); + rtp_sender.SetRtxPayloadType(kRtxPayload, kPayload); + + ASSERT_EQ(rtp_sender.RtxStatus(), kRtxOff); + rtp_sender.SetRtxStatus(kRtxRetransmitted); + + EXPECT_EQ(rtp_sender.RtxStatus(), kRtxRetransmitted); +} + TEST_F(RtpSenderTest, AllowPaddingAsFirstPacketOnRtxWithTransportCc) { ASSERT_TRUE(rtp_sender_->RegisterRtpHeaderExtension( TransportSequenceNumber::Uri(), kTransportSequenceNumberExtensionId)); @@ -1049,8 +1082,8 @@ TEST_F(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) { // Min requested size in order to use RTX payload. const size_t kMinPaddingSize = 50; - rtp_sender_->SetRtxStatus(kRtxRetransmitted | kRtxRedundantPayloads); rtp_sender_->SetRtxPayloadType(kRtxPayload, kPayload); + rtp_sender_->SetRtxStatus(kRtxRetransmitted | kRtxRedundantPayloads); packet_history_->SetStorePacketsStatus( RtpPacketHistory::StorageMode::kStoreAndCull, 1); @@ -1099,8 +1132,8 @@ TEST_F(RtpSenderTest, LimitsPayloadPaddingSize) { const double kFactor = 2.0; field_trials_.SetMaxPaddingFactor(kFactor); SetUpRtpSender(false, false, nullptr); - rtp_sender_->SetRtxStatus(kRtxRetransmitted | kRtxRedundantPayloads); rtp_sender_->SetRtxPayloadType(kRtxPayload, kPayload); + rtp_sender_->SetRtxStatus(kRtxRetransmitted | kRtxRedundantPayloads); packet_history_->SetStorePacketsStatus( RtpPacketHistory::StorageMode::kStoreAndCull, 1); @@ -1201,6 +1234,7 @@ TEST_F(RtpSenderTest, SupportsPadding) { if (redundant_payloads) { rtx_mode |= kRtxRedundantPayloads; } + rtp_sender_->SetRtxPayloadType(kRtxPayload, kPayload); rtp_sender_->SetRtxStatus(rtx_mode); for (auto extension_uri : kBweExtensionUris) {