diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h index e897718b2d..8cee1bae82 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/modules/rtp_rtcp/include/rtp_rtcp.h @@ -121,6 +121,13 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface { // Corresponds to extmap-allow-mixed in SDP negotiation. bool extmap_allow_mixed = false; + // If true, the RTP sender will always annotate outgoing packets with + // MID and RID header extensions, if provided and negotiated. + // If false, the RTP sender will stop sending MID and RID header extensions, + // when it knows that the receiver is ready to demux based on SSRC. This is + // done by RTCP RR acking. + bool always_send_mid_and_rid = false; + // If set, field trials are read from |field_trials|, otherwise // defaults to webrtc::FieldTrialBasedConfig. const WebRtcKeyValueConfig* field_trials = nullptr; diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index c48a662fc5..584f89c8ce 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -112,6 +112,7 @@ RTPSender::RTPSender(const RtpRtcp::Configuration& config, rtp_header_extension_map_(config.extmap_allow_mixed), // RTP variables sequence_number_forced_(false), + always_send_mid_and_rid_(config.always_send_mid_and_rid), ssrc_has_acked_(false), rtx_ssrc_has_acked_(false), last_rtp_timestamp_(0), @@ -497,13 +498,15 @@ std::unique_ptr RTPSender::AllocatePacket() const { // in the MID and/or (R)RID header extensions if present. Therefore, the // sender can reduce overhead by omitting these header extensions once it // knows that the receiver has "bound" the SSRC. + // This optimization can be configured by setting + // |always_send_mid_and_rid_| appropriately. // // The algorithm here is fairly simple: Always attach a MID and/or RID (if // configured) to the outgoing packets until an RTCP receiver report comes // back for this SSRC. That feedback indicates the receiver must have // received a packet with the SSRC and header extension(s), so the sender // then stops attaching the MID and RID. - if (!ssrc_has_acked_) { + if (always_send_mid_and_rid_ || !ssrc_has_acked_) { // These are no-ops if the corresponding header extension is not registered. if (!mid_.empty()) { packet->SetExtension(mid_); @@ -686,7 +689,7 @@ std::unique_ptr RTPSender::BuildRtxPacket( // Note that RTX packets must used the RepairedRtpStreamId (RRID) header // extension instead of the RtpStreamId (RID) header extension even though // the payload is identical. - if (!rtx_ssrc_has_acked_) { + if (always_send_mid_and_rid_ || !rtx_ssrc_has_acked_) { // These are no-ops if the corresponding header extension is not // registered. if (!mid_.empty()) { diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index 8915e39e9e..4a7550907c 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -179,6 +179,8 @@ class RTPSender { std::string rid_ RTC_GUARDED_BY(send_critsect_); // MID value to send in the MID header extension. std::string mid_ RTC_GUARDED_BY(send_critsect_); + // Should we send MID/RID even when ACKed? (see below). + const bool always_send_mid_and_rid_; // Track if any ACK has been received on the SSRC and RTX SSRC to indicate // when to stop sending the MID and RID header extensions. bool ssrc_has_acked_ RTC_GUARDED_BY(send_critsect_); diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 3b85166e61..d4a7fa9125 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -254,7 +254,7 @@ class RtpSenderTest : public ::testing::TestWithParam { kMarkerBit(true), field_trials_(ToFieldTrialString(GetParam())) {} - void SetUp() override { SetUpRtpSender(true, false); } + void SetUp() override { SetUpRtpSender(true, false, false); } RTPSender* rtp_sender() { RTC_DCHECK(rtp_sender_context_); @@ -266,7 +266,9 @@ class RtpSenderTest : public ::testing::TestWithParam { return &rtp_sender_context_->packet_sender_; } - void SetUpRtpSender(bool pacer, bool populate_network2) { + void SetUpRtpSender(bool pacer, + bool populate_network2, + bool always_send_mid_and_rid) { RtpRtcp::Configuration config; config.clock = &fake_clock_; config.outgoing_transport = &transport_; @@ -279,6 +281,7 @@ class RtpSenderTest : public ::testing::TestWithParam { config.paced_sender = pacer ? &mock_paced_sender_ : nullptr; config.populate_network2_timestamp = populate_network2; config.rtp_stats_callback = &rtp_stats_callback_; + config.always_send_mid_and_rid = always_send_mid_and_rid; rtp_sender_context_ = std::make_unique(config); rtp_sender()->SetSequenceNumber(kSeqNum); rtp_sender()->SetTimestampOffset(0); @@ -378,7 +381,7 @@ class RtpSenderTest : public ::testing::TestWithParam { // default code path. class RtpSenderTestWithoutPacer : public RtpSenderTest { public: - void SetUp() override { SetUpRtpSender(false, false); } + void SetUp() override { SetUpRtpSender(false, false, false); } }; TEST_P(RtpSenderTestWithoutPacer, AllocatePacketSetCsrc) { @@ -603,7 +606,7 @@ TEST_P(RtpSenderTestWithoutPacer, PacketOptionsNoRetransmission) { TEST_P(RtpSenderTestWithoutPacer, SetsIncludedInFeedbackWhenTransportSequenceNumberExtensionIsRegistered) { - SetUpRtpSender(false, false); + SetUpRtpSender(false, false, false); rtp_sender()->RegisterRtpHeaderExtension(kRtpExtensionTransportSequenceNumber, kTransportSequenceNumberExtensionId); EXPECT_CALL(send_packet_observer_, OnSendPacket).Times(1); @@ -614,7 +617,7 @@ TEST_P(RtpSenderTestWithoutPacer, TEST_P( RtpSenderTestWithoutPacer, SetsIncludedInAllocationWhenTransportSequenceNumberExtensionIsRegistered) { - SetUpRtpSender(false, false); + SetUpRtpSender(false, false, false); rtp_sender()->RegisterRtpHeaderExtension(kRtpExtensionTransportSequenceNumber, kTransportSequenceNumberExtensionId); EXPECT_CALL(send_packet_observer_, OnSendPacket).Times(1); @@ -624,7 +627,7 @@ TEST_P( TEST_P(RtpSenderTestWithoutPacer, SetsIncludedInAllocationWhenForcedAsPartOfAllocation) { - SetUpRtpSender(false, false); + SetUpRtpSender(false, false, false); rtp_egress()->ForceIncludeSendPacketsInAllocation(true); SendGenericPacket(); EXPECT_FALSE(transport_.last_options_.included_in_feedback); @@ -632,7 +635,7 @@ TEST_P(RtpSenderTestWithoutPacer, } TEST_P(RtpSenderTestWithoutPacer, DoesnSetIncludedInAllocationByDefault) { - SetUpRtpSender(false, false); + SetUpRtpSender(false, false, false); SendGenericPacket(); EXPECT_FALSE(transport_.last_options_.included_in_feedback); EXPECT_FALSE(transport_.last_options_.included_in_allocation); @@ -813,7 +816,7 @@ TEST_P(RtpSenderTest, WritesPacerExitToTimingExtension) { } TEST_P(RtpSenderTest, WritesNetwork2ToTimingExtensionWithPacer) { - SetUpRtpSender(/*pacer=*/true, /*populate_network2=*/true); + SetUpRtpSender(/*pacer=*/true, /*populate_network2=*/true, false); rtp_sender_context_->packet_history_.SetStorePacketsStatus( RtpPacketHistory::StorageMode::kStoreAndCull, 10); EXPECT_EQ(0, rtp_sender()->RegisterRtpHeaderExtension( @@ -852,7 +855,7 @@ TEST_P(RtpSenderTest, WritesNetwork2ToTimingExtensionWithPacer) { } TEST_P(RtpSenderTest, WritesNetwork2ToTimingExtensionWithoutPacer) { - SetUpRtpSender(/*pacer=*/false, /*populate_network2=*/true); + SetUpRtpSender(/*pacer=*/false, /*populate_network2=*/true, false); EXPECT_EQ(0, rtp_sender()->RegisterRtpHeaderExtension( kRtpExtensionVideoTiming, kVideoTimingExtensionId)); auto packet = rtp_sender()->AllocatePacket(); @@ -1440,6 +1443,27 @@ TEST_P(RtpSenderTestWithoutPacer, MidAndRidNotIncludedOnSentPacketsAfterAck) { EXPECT_FALSE(second_packet.HasExtension()); } +TEST_P(RtpSenderTestWithoutPacer, + MidAndRidAlwaysIncludedOnSentPacketsWhenConfigured) { + SetUpRtpSender(false, false, /*always_send_mid_and_rid=*/true); + const char kMid[] = "mid"; + const char kRid[] = "f"; + EnableMidSending(kMid); + EnableRidSending(kRid); + + // Send two media packets: one before and one after the ack. + auto first_packet = SendGenericPacket(); + rtp_sender()->OnReceivedAckOnSsrc(first_packet->SequenceNumber()); + SendGenericPacket(); + + // Due to the configuration, both sent packets should contain MID and RID. + ASSERT_EQ(2u, transport_.sent_packets_.size()); + for (const RtpPacketReceived& packet : transport_.sent_packets_) { + EXPECT_EQ(packet.GetExtension(), kMid); + EXPECT_EQ(packet.GetExtension(), kRid); + } +} + // Test that the first RTX packet includes both MID and RRID even if the packet // being retransmitted did not have MID or RID. The MID and RID are needed on // the first packets for a given SSRC, and RTX packets are sent on a separate @@ -1517,6 +1541,45 @@ TEST_P(RtpSenderTestWithoutPacer, MidAndRidNotIncludedOnRtxPacketsAfterAck) { EXPECT_FALSE(third_rtx_packet.HasExtension()); } +TEST_P(RtpSenderTestWithoutPacer, + MidAndRidAlwaysIncludedOnRtxPacketsWhenConfigured) { + SetUpRtpSender(false, false, /*always_send_mid_and_rid=*/true); + const char kMid[] = "mid"; + const char kRid[] = "f"; + EnableRtx(); + EnableMidSending(kMid); + EnableRidSending(kRid); + + // Send two media packets: one before and one after the ack. + auto media_packet1 = SendGenericPacket(); + rtp_sender()->OnReceivedAckOnSsrc(media_packet1->SequenceNumber()); + auto media_packet2 = SendGenericPacket(); + + // Send three RTX packets with different combinations of orders w.r.t. the + // media and RTX acks. + ASSERT_LT(0, rtp_sender()->ReSendPacket(media_packet2->SequenceNumber())); + ASSERT_EQ(3u, transport_.sent_packets_.size()); + rtp_sender()->OnReceivedAckOnRtxSsrc( + transport_.sent_packets_[2].SequenceNumber()); + ASSERT_LT(0, rtp_sender()->ReSendPacket(media_packet1->SequenceNumber())); + ASSERT_LT(0, rtp_sender()->ReSendPacket(media_packet2->SequenceNumber())); + + // Due to the configuration, all sent packets should contain MID + // and either RID (media) or RRID (RTX). + ASSERT_EQ(5u, transport_.sent_packets_.size()); + for (const auto& packet : transport_.sent_packets_) { + EXPECT_EQ(packet.GetExtension(), kMid); + } + for (size_t i = 0; i < 2; ++i) { + const RtpPacketReceived& packet = transport_.sent_packets_[i]; + EXPECT_EQ(packet.GetExtension(), kRid); + } + for (size_t i = 2; i < transport_.sent_packets_.size(); ++i) { + const RtpPacketReceived& packet = transport_.sent_packets_[i]; + EXPECT_EQ(packet.GetExtension(), kRid); + } +} + // 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_P(RtpSenderTestWithoutPacer,