diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h index 81c1a981b6..dd2ddae163 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -159,13 +159,15 @@ struct RtpState { timestamp(0), capture_time_ms(-1), last_timestamp_time_ms(-1), - media_has_been_sent(false) {} + media_has_been_sent(false), + ssrc_has_acked(false) {} uint16_t sequence_number; uint32_t start_timestamp; uint32_t timestamp; int64_t capture_time_ms; int64_t last_timestamp_time_ms; bool media_has_been_sent; + bool ssrc_has_acked; }; // Callback interface for packets recovered by FlexFEC or ULPFEC. In diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 9252de385c..2aed84e71a 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -733,11 +733,20 @@ void ModuleRtpRtcpImpl::OnReceivedRtcpReportBlocks( const ReportBlockList& report_blocks) { if (ack_observer_) { uint32_t ssrc = SSRC(); + absl::optional rtx_ssrc; + if (rtp_sender_->RtxStatus() != kRtxOff) { + rtx_ssrc = rtp_sender_->RtxSsrc(); + } for (const RTCPReportBlock& report_block : report_blocks) { if (ssrc == report_block.source_ssrc) { + rtp_sender_->OnReceivedAckOnSsrc( + report_block.extended_highest_sequence_number); ack_observer_->OnReceivedAck( report_block.extended_highest_sequence_number); + } else if (rtx_ssrc && *rtx_ssrc == report_block.source_ssrc) { + rtp_sender_->OnReceivedAckOnRtxSsrc( + report_block.extended_highest_sequence_number); } } } diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 62fe25c3a7..4ba645fea2 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -176,6 +176,8 @@ RTPSender::RTPSender(const RtpRtcp::Configuration& config) // RTP variables sequence_number_forced_(false), ssrc_(config.media_send_ssrc), + ssrc_has_acked_(false), + rtx_ssrc_has_acked_(false), last_rtp_timestamp_(0), capture_time_ms_(0), last_timestamp_time_ms_(0), @@ -258,6 +260,8 @@ RTPSender::RTPSender( bitrate_callback_(bitrate_callback), // RTP variables sequence_number_forced_(false), + ssrc_has_acked_(false), + rtx_ssrc_has_acked_(false), last_rtp_timestamp_(0), capture_time_ms_(0), last_timestamp_time_ms_(0), @@ -651,6 +655,17 @@ int32_t RTPSender::ReSendPacket(uint16_t packet_id) { return packet_size; } +void RTPSender::OnReceivedAckOnSsrc(int64_t extended_highest_sequence_number) { + rtc::CritScope lock(&send_critsect_); + ssrc_has_acked_ = true; +} + +void RTPSender::OnReceivedAckOnRtxSsrc( + int64_t extended_highest_sequence_number) { + rtc::CritScope lock(&send_critsect_); + rtx_ssrc_has_acked_ = true; +} + bool RTPSender::SendPacketToNetwork(const RtpPacketToSend& packet, const PacketOptions& options, const PacedPacketInfo& pacing_info) { @@ -1336,13 +1351,24 @@ std::unique_ptr RTPSender::AllocatePacket() const { packet->ReserveExtension(); packet->ReserveExtension(); - if (!mid_.empty()) { - // This is a no-op if the MID header extension is not registered. - packet->SetExtension(mid_); - } - if (!rid_.empty()) { - // This is a no-op if the RID header extension is not registered. - packet->SetExtension(rid_); + // BUNDLE requires that the receiver "bind" the received SSRC to the values + // 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. + // + // 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_) { + // These are no-ops if the corresponding header extension is not registered. + if (!mid_.empty()) { + packet->SetExtension(mid_); + } + if (!rid_.empty()) { + packet->SetExtension(rid_); + } } return packet; } @@ -1443,6 +1469,7 @@ void RTPSender::SetRid(const std::string& rid) { void RTPSender::SetMid(const std::string& mid) { // This is configured via the API. rtc::CritScope lock(&send_critsect_); + RTC_DCHECK_LE(mid.length(), RtpMid::kMaxValueSizeBytes); mid_ = mid; } @@ -1493,26 +1520,27 @@ static void CopyHeaderAndExtensionsToRtxPacket(const RtpPacketToSend& packet, // * Header extensions - replace Rid header with RepairedRid header. const std::vector csrcs = packet.Csrcs(); rtx_packet->SetCsrcs(csrcs); - for (int extension = kRtpExtensionNone + 1; - extension < kRtpExtensionNumberOfExtensions; ++extension) { - RTPExtensionType source_extension = - static_cast(extension); - // Rid header should be replaced with RepairedRid header - RTPExtensionType destination_extension = - source_extension == kRtpExtensionRtpStreamId - ? kRtpExtensionRepairedRtpStreamId - : source_extension; + for (int extension_num = kRtpExtensionNone + 1; + extension_num < kRtpExtensionNumberOfExtensions; ++extension_num) { + auto extension = static_cast(extension_num); - // Empty extensions should be supported, so not checking |source.empty()|. - if (!packet.HasExtension(source_extension)) { + // Stream ID header extensions (MID, RSID) are sent per-SSRC. Since RTX + // operates on a different SSRC, the presence and values of these header + // extensions should be determined separately and not blindly copied. + if (extension == kRtpExtensionMid || + extension == kRtpExtensionRtpStreamId) { continue; } - rtc::ArrayView source = - packet.FindExtension(source_extension); + // Empty extensions should be supported, so not checking |source.empty()|. + if (!packet.HasExtension(extension)) { + continue; + } + + rtc::ArrayView source = packet.FindExtension(extension); rtc::ArrayView destination = - rtx_packet->AllocateExtension(destination_extension, source.size()); + rtx_packet->AllocateExtension(extension, source.size()); // Could happen if any: // 1. Extension has 0 length. @@ -1556,19 +1584,22 @@ std::unique_ptr RTPSender::BuildRtxPacket( CopyHeaderAndExtensionsToRtxPacket(packet, rtx_packet.get()); - // The spec indicates that it is possible for a sender to stop sending mids - // once the SSRCs have been bound on the receiver. As a result the source - // rtp packet might not have the MID header extension set. - // However, the SSRC of the RTX stream might not have been bound on the - // receiver. This means that we should include it here. - // The same argument goes for the Repaired RID extension. - if (!mid_.empty()) { - // This is a no-op if the MID header extension is not registered. - rtx_packet->SetExtension(mid_); - } - if (!rid_.empty()) { - // This is a no-op if the Repaired-RID header extension is not registered. - // rtx_packet->SetExtension(rid_); + // RTX packets are sent on an SSRC different from the main media, so the + // decision to attach MID and/or RRID header extensions is completely + // separate from that of the main media SSRC. + // + // 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_) { + // These are no-ops if the corresponding header extension is not + // registered. + if (!mid_.empty()) { + rtx_packet->SetExtension(mid_); + } + if (!rid_.empty()) { + rtx_packet->SetExtension(rid_); + } } } RTC_DCHECK(rtx_packet); @@ -1619,6 +1650,7 @@ void RTPSender::SetRtpState(const RtpState& rtp_state) { capture_time_ms_ = rtp_state.capture_time_ms; last_timestamp_time_ms_ = rtp_state.last_timestamp_time_ms; media_has_been_sent_ = rtp_state.media_has_been_sent; + ssrc_has_acked_ = rtp_state.ssrc_has_acked; } RtpState RTPSender::GetRtpState() const { @@ -1631,6 +1663,7 @@ RtpState RTPSender::GetRtpState() const { state.capture_time_ms = capture_time_ms_; state.last_timestamp_time_ms = last_timestamp_time_ms_; state.media_has_been_sent = media_has_been_sent_; + state.ssrc_has_acked = ssrc_has_acked_; return state; } @@ -1638,6 +1671,7 @@ RtpState RTPSender::GetRtpState() const { void RTPSender::SetRtxRtpState(const RtpState& rtp_state) { rtc::CritScope lock(&send_critsect_); sequence_number_rtx_ = rtp_state.sequence_number; + rtx_ssrc_has_acked_ = rtp_state.ssrc_has_acked; } RtpState RTPSender::GetRtxRtpState() const { @@ -1646,6 +1680,7 @@ RtpState RTPSender::GetRtxRtpState() const { RtpState state; state.sequence_number = sequence_number_rtx_; state.start_timestamp = timestamp_offset_; + state.ssrc_has_acked = rtx_ssrc_has_acked_; return state; } diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index 50bbd304d3..08e8f42528 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -134,6 +134,10 @@ class RTPSender { int32_t ReSendPacket(uint16_t packet_id); + // ACK. + void OnReceivedAckOnSsrc(int64_t extended_highest_sequence_number); + void OnReceivedAckOnRtxSsrc(int64_t extended_highest_sequence_number); + // RTX. void SetRtxStatus(int mode); int RtxStatus() const; @@ -300,6 +304,10 @@ 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_); + // 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_); + bool rtx_ssrc_has_acked_ RTC_GUARDED_BY(send_critsect_); uint32_t last_rtp_timestamp_ RTC_GUARDED_BY(send_critsect_); int64_t capture_time_ms_ RTC_GUARDED_BY(send_critsect_); int64_t last_timestamp_time_ms_ 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 c33825529f..7d856fdf1b 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -303,6 +303,36 @@ class RtpSenderTest : public ::testing::TestWithParam { const int64_t kCaptureTimeMs = fake_clock_.TimeInMilliseconds(); return SendPacket(kCaptureTimeMs, sizeof(kPayloadData)); } + + // The following are helpers for configuring the RTPSender. They must be + // called before sending any packets. + + // Enable the retransmission stream with sizable packet storage. + void EnableRtx() { + // RTX needs to be able to read the source packets from the packet store. + // Pick a number of packets to store big enough for any unit test. + constexpr uint16_t kNumberOfPacketsToStore = 100; + rtp_sender_->SetStorePacketsStatus(true, kNumberOfPacketsToStore); + rtp_sender_->SetRtxPayloadType(kRtxPayload, kPayload); + rtp_sender_->SetRtxStatus(kRtxRetransmitted | kRtxRedundantPayloads); + } + + // Enable sending of the MID header extension for both the primary SSRC and + // the RTX SSRC. + void EnableMidSending(const std::string& mid) { + rtp_sender_->RegisterRtpHeaderExtension(kRtpExtensionMid, kMidExtensionId); + rtp_sender_->SetMid(mid); + } + + // Enable sending of the RSID header extension for the primary SSRC and the + // RRSID header extension for the RTX SSRC. + void EnableRidSending(const std::string& rid) { + rtp_sender_->RegisterRtpHeaderExtension(kRtpExtensionRtpStreamId, + kRidExtensionId); + rtp_sender_->RegisterRtpHeaderExtension(kRtpExtensionRepairedRtpStreamId, + kRepairedRidExtensionId); + rtp_sender_->SetRid(rid); + } }; // TODO(pbos): Move tests over from WithoutPacer to RtpSenderTest as this is our @@ -1736,11 +1766,7 @@ TEST_P(RtpSenderTestWithoutPacer, SendFlexfecPackets) { TEST_P(RtpSenderTestWithoutPacer, MidIncludedOnSentPackets) { const char kMid[] = "mid"; - // Register MID header extension and set the MID for the RTPSender. - rtp_sender_->SetSendingMediaStatus(false); - rtp_sender_->RegisterRtpHeaderExtension(kRtpExtensionMid, kMidExtensionId); - rtp_sender_->SetMid(kMid); - rtp_sender_->SetSendingMediaStatus(true); + EnableMidSending(kMid); // Send a couple packets. SendGenericPacket(); @@ -1758,11 +1784,7 @@ TEST_P(RtpSenderTestWithoutPacer, MidIncludedOnSentPackets) { TEST_P(RtpSenderTestWithoutPacer, RidIncludedOnSentPackets) { const char kRid[] = "f"; - rtp_sender_->SetSendingMediaStatus(false); - rtp_sender_->RegisterRtpHeaderExtension(kRtpExtensionRtpStreamId, - kRidExtensionId); - rtp_sender_->SetRid(kRid); - rtp_sender_->SetSendingMediaStatus(true); + EnableRidSending(kRid); SendGenericPacket(); @@ -1776,18 +1798,8 @@ TEST_P(RtpSenderTestWithoutPacer, RidIncludedOnSentPackets) { TEST_P(RtpSenderTestWithoutPacer, RidIncludedOnRtxSentPackets) { const char kRid[] = "f"; - rtp_sender_->SetSendingMediaStatus(false); - rtp_sender_->RegisterRtpHeaderExtension(kRtpExtensionRtpStreamId, - kRidExtensionId); - rtp_sender_->RegisterRtpHeaderExtension(kRtpExtensionRepairedRtpStreamId, - kRepairedRidExtensionId); - rtp_sender_->SetRid(kRid); - rtp_sender_->SetSendingMediaStatus(true); - - rtp_sender_->SetRtxStatus(kRtxRetransmitted | kRtxRedundantPayloads); - rtp_sender_->SetRtxPayloadType(kRtxPayload, kPayload); - - rtp_sender_->SetStorePacketsStatus(true, 10); + EnableRtx(); + EnableRidSending(kRid); SendGenericPacket(); ASSERT_EQ(1u, transport_.sent_packets_.size()); @@ -1796,7 +1808,7 @@ TEST_P(RtpSenderTestWithoutPacer, RidIncludedOnRtxSentPackets) { ASSERT_TRUE(packet.GetExtension(&rid)); EXPECT_EQ(kRid, rid); rid = kNoRid; - EXPECT_FALSE(packet.GetExtension(&rid)); + EXPECT_FALSE(packet.HasExtension()); uint16_t packet_id = packet.SequenceNumber(); rtp_sender_->ReSendPacket(packet_id); @@ -1807,6 +1819,160 @@ TEST_P(RtpSenderTestWithoutPacer, RidIncludedOnRtxSentPackets) { EXPECT_FALSE(rtx_packet.HasExtension()); } +TEST_P(RtpSenderTestWithoutPacer, MidAndRidNotIncludedOnSentPacketsAfterAck) { + const char kMid[] = "mid"; + const char kRid[] = "f"; + + EnableMidSending(kMid); + EnableRidSending(kRid); + + // This first packet should include both MID and RID. + auto first_built_packet = SendGenericPacket(); + + rtp_sender_->OnReceivedAckOnSsrc(first_built_packet->SequenceNumber()); + + // The second packet should include neither since an ack was received. + SendGenericPacket(); + + ASSERT_EQ(2u, transport_.sent_packets_.size()); + + const RtpPacketReceived& first_packet = transport_.sent_packets_[0]; + std::string mid, rid; + ASSERT_TRUE(first_packet.GetExtension(&mid)); + EXPECT_EQ(kMid, mid); + ASSERT_TRUE(first_packet.GetExtension(&rid)); + EXPECT_EQ(kRid, rid); + + const RtpPacketReceived& second_packet = transport_.sent_packets_[1]; + EXPECT_FALSE(second_packet.HasExtension()); + EXPECT_FALSE(second_packet.HasExtension()); +} + +// 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 +// SSRC. +TEST_P(RtpSenderTestWithoutPacer, MidAndRidIncludedOnFirstRtxPacket) { + const char kMid[] = "mid"; + const char kRid[] = "f"; + + EnableRtx(); + EnableMidSending(kMid); + EnableRidSending(kRid); + + // This first packet will include both MID and RID. + auto first_built_packet = SendGenericPacket(); + rtp_sender_->OnReceivedAckOnSsrc(first_built_packet->SequenceNumber()); + + // The second packet will include neither since an ack was received. + auto second_built_packet = SendGenericPacket(); + + // The first RTX packet should include MID and RRID. + ASSERT_LT(0, + rtp_sender_->ReSendPacket(second_built_packet->SequenceNumber())); + + ASSERT_EQ(3u, transport_.sent_packets_.size()); + + const RtpPacketReceived& rtx_packet = transport_.sent_packets_[2]; + std::string mid, rrid; + ASSERT_TRUE(rtx_packet.GetExtension(&mid)); + EXPECT_EQ(kMid, mid); + ASSERT_TRUE(rtx_packet.GetExtension(&rrid)); + EXPECT_EQ(kRid, rrid); +} + +// Test that the RTX packets sent after receving an ACK on the RTX SSRC does +// not include either MID or RRID even if the packet being retransmitted did +// had a MID or RID. +TEST_P(RtpSenderTestWithoutPacer, MidAndRidNotIncludedOnRtxPacketsAfterAck) { + const char kMid[] = "mid"; + const char kRid[] = "f"; + + EnableRtx(); + EnableMidSending(kMid); + EnableRidSending(kRid); + + // This first packet will include both MID and RID. + auto first_built_packet = SendGenericPacket(); + rtp_sender_->OnReceivedAckOnSsrc(first_built_packet->SequenceNumber()); + + // The second packet will include neither since an ack was received. + auto second_built_packet = SendGenericPacket(); + + // The first RTX packet will include MID and RRID. + ASSERT_LT(0, + rtp_sender_->ReSendPacket(second_built_packet->SequenceNumber())); + + ASSERT_EQ(3u, transport_.sent_packets_.size()); + const RtpPacketReceived& first_rtx_packet = transport_.sent_packets_[2]; + + rtp_sender_->OnReceivedAckOnRtxSsrc(first_rtx_packet.SequenceNumber()); + + // The second and third RTX packets should not include MID nor RRID. + ASSERT_LT(0, rtp_sender_->ReSendPacket(first_built_packet->SequenceNumber())); + ASSERT_LT(0, + rtp_sender_->ReSendPacket(second_built_packet->SequenceNumber())); + + ASSERT_EQ(5u, transport_.sent_packets_.size()); + + const RtpPacketReceived& second_rtx_packet = transport_.sent_packets_[3]; + EXPECT_FALSE(second_rtx_packet.HasExtension()); + EXPECT_FALSE(second_rtx_packet.HasExtension()); + + const RtpPacketReceived& third_rtx_packet = transport_.sent_packets_[4]; + EXPECT_FALSE(third_rtx_packet.HasExtension()); + EXPECT_FALSE(third_rtx_packet.HasExtension()); +} + +// 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, + MidAndRidNotIncludedOnSentPacketsAfterRtpStateRestored) { + const char kMid[] = "mid"; + const char kRid[] = "f"; + + EnableMidSending(kMid); + EnableRidSending(kRid); + + RtpState state = rtp_sender_->GetRtpState(); + EXPECT_FALSE(state.ssrc_has_acked); + state.ssrc_has_acked = true; + rtp_sender_->SetRtpState(state); + + SendGenericPacket(); + + ASSERT_EQ(1u, transport_.sent_packets_.size()); + const RtpPacketReceived& packet = transport_.sent_packets_[0]; + EXPECT_FALSE(packet.HasExtension()); + EXPECT_FALSE(packet.HasExtension()); +} + +// Test that if the RTX RtpState indicates an ACK has been received on that +// RTX SSRC then neither the MID nor RRID header extensions will be sent on +// RTX packets. +TEST_P(RtpSenderTestWithoutPacer, + MidAndRridNotIncludedOnRtxPacketsAfterRtpStateRestored) { + const char kMid[] = "mid"; + const char kRid[] = "f"; + + EnableRtx(); + EnableMidSending(kMid); + EnableRidSending(kRid); + + RtpState rtx_state = rtp_sender_->GetRtxRtpState(); + EXPECT_FALSE(rtx_state.ssrc_has_acked); + rtx_state.ssrc_has_acked = true; + rtp_sender_->SetRtxRtpState(rtx_state); + + auto built_packet = SendGenericPacket(); + ASSERT_LT(0, rtp_sender_->ReSendPacket(built_packet->SequenceNumber())); + + ASSERT_EQ(2u, transport_.sent_packets_.size()); + const RtpPacketReceived& rtx_packet = transport_.sent_packets_[1]; + EXPECT_FALSE(rtx_packet.HasExtension()); + EXPECT_FALSE(rtx_packet.HasExtension()); +} + TEST_P(RtpSenderTest, FecOverheadRate) { constexpr uint32_t kTimestamp = 1234; constexpr int kMediaPayloadType = 127;