From c54c85fe8f5ec2677663cb3b0e5bc3b678b5b713 Mon Sep 17 00:00:00 2001
From: Danil Chapovalov <danilchap@webrtc.org>
Date: Mon, 26 Aug 2024 12:24:06 +0200
Subject: [PATCH] Attach Mid/Rid RTP header extension to pure padding packets

same as they attached to other packets.
Otherwise there is risk that ssrc will be acked after few initial pure padding packets are sent, before remote endpoint seen any mid or rid attached.

Bug: b/361257385
Change-Id: I695b379221debe2518ad33d13d65620877f0b2a7
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/360660
Reviewed-by: Per Kjellander <perkj@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42851}
---
 modules/rtp_rtcp/source/rtp_sender.cc         | 32 +++++++++++-----
 .../rtp_rtcp/source/rtp_sender_unittest.cc    | 37 +++++++++++++++++++
 2 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc
index 46157147e2..b64f8b3b4e 100644
--- a/modules/rtp_rtcp/source/rtp_sender.cc
+++ b/modules/rtp_rtcp/source/rtp_sender.cc
@@ -435,6 +435,17 @@ std::vector<std::unique_ptr<RtpPacketToSend>> RTPSender::GeneratePadding(
         break;
       }
       padding_packet->SetSsrc(ssrc_);
+
+      if (always_send_mid_and_rid_ || !ssrc_has_acked_) {
+        // These are no-ops if the corresponding header extension is not
+        // registered.
+        if (!mid_.empty()) {
+          padding_packet->SetExtension<RtpMid>(mid_);
+        }
+        if (!rid_.empty()) {
+          padding_packet->SetExtension<RtpStreamId>(rid_);
+        }
+      }
     } else {
       // Without abs-send-time or transport sequence number a media packet
       // must be sent before padding so that the timestamps used for
@@ -450,17 +461,20 @@ std::vector<std::unique_ptr<RtpPacketToSend>> RTPSender::GeneratePadding(
       RTC_DCHECK(!rtx_payload_type_map_.empty());
       padding_packet->SetSsrc(*rtx_ssrc_);
       padding_packet->SetPayloadType(rtx_payload_type_map_.begin()->second);
+
+      if (always_send_mid_and_rid_ || !rtx_ssrc_has_acked_) {
+        if (!mid_.empty()) {
+          padding_packet->SetExtension<RtpMid>(mid_);
+        }
+        if (!rid_.empty()) {
+          padding_packet->SetExtension<RepairedRtpStreamId>(rid_);
+        }
+      }
     }
 
-    if (rtp_header_extension_map_.IsRegistered(TransportSequenceNumber::kId)) {
-      padding_packet->ReserveExtension<TransportSequenceNumber>();
-    }
-    if (rtp_header_extension_map_.IsRegistered(TransmissionOffset::kId)) {
-      padding_packet->ReserveExtension<TransmissionOffset>();
-    }
-    if (rtp_header_extension_map_.IsRegistered(AbsoluteSendTime::kId)) {
-      padding_packet->ReserveExtension<AbsoluteSendTime>();
-    }
+    padding_packet->ReserveExtension<TransportSequenceNumber>();
+    padding_packet->ReserveExtension<TransmissionOffset>();
+    padding_packet->ReserveExtension<AbsoluteSendTime>();
 
     padding_packet->SetPadding(padding_bytes_in_packet);
     bytes_left -= std::min(bytes_left, padding_bytes_in_packet);
diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
index 80ca3195a2..bd434dd93e 100644
--- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
@@ -1078,6 +1078,43 @@ TEST_F(RtpSenderTest, GeneratedPaddingHasBweExtensions) {
   EXPECT_GT(payload_padding->payload_size(), 0u);
 }
 
+TEST_F(RtpSenderTest, GeneratedPaddingHasMidRidExtensions) {
+  EnableMidSending("mid");
+  EnableRidSending();
+
+  // Send a dummy video packet so it ends up in the packet history. Since we
+  // are not using RTX, it should never be used as padding.
+  packet_history_->SetStorePacketsStatus(
+      RtpPacketHistory::StorageMode::kStoreAndCull, 1);
+  std::unique_ptr<RtpPacketToSend> packet =
+      BuildRtpPacket(kPayload, true, 0, env_.clock().CurrentTime());
+  packet->set_allow_retransmission(true);
+  packet->SetPayloadSize(1234);
+  packet->set_packet_type(RtpPacketMediaType::kVideo);
+  sequencer_->Sequence(*packet);
+  packet_history_->PutRtpPacket(std::move(packet), env_.clock().CurrentTime());
+
+  std::vector<std::unique_ptr<RtpPacketToSend>> padding_packets =
+      GeneratePadding(/*target_size_bytes=*/1);
+  ASSERT_THAT(padding_packets, SizeIs(1));
+
+  EXPECT_TRUE(padding_packets[0]->HasExtension<RtpMid>());
+  EXPECT_TRUE(padding_packets[0]->HasExtension<RtpStreamId>());
+}
+
+TEST_F(RtpSenderTest, GeneratedPaddingOnRtxHasMidRidExtensions) {
+  EnableRtx();
+  EnableMidSending("mid");
+  EnableRidSending();
+
+  std::vector<std::unique_ptr<RtpPacketToSend>> padding_packets =
+      GeneratePadding(/*target_size_bytes=*/1);
+  ASSERT_THAT(padding_packets, SizeIs(1));
+
+  EXPECT_TRUE(padding_packets[0]->HasExtension<RtpMid>());
+  EXPECT_TRUE(padding_packets[0]->HasExtension<RepairedRtpStreamId>());
+}
+
 TEST_F(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) {
   // Min requested size in order to use RTX payload.
   const size_t kMinPaddingSize = 50;