From bb7727211c535f8a9dce27891941b52b6ea8e750 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= <sprang@webrtc.org>
Date: Wed, 10 Jul 2019 10:10:37 +0200
Subject: [PATCH] Make new pacer padding more like old one
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The (currently unused) new pacer code path was implemented with what
was intended as a more careful padding strategy.
Unfortunately this doesn't work as well as expected due to the fact
that the padding budget cannot build up underuse.

I made another CL that could fix that, but I think it adds complexity
for dubious gains. It also will make it more difficult to find any
potential regression when switching to the new path, should one occur.
See https://webrtc-review.googlesource.com/c/src/+/144563

Therefore, this CL makes the new code path choose RTX payload in the
same way as is currently done.

Bug: webrtc:10633
Change-Id: If2115d4fa7463add959faa77c63101286c27e0f5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/145202
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28537}
---
 modules/rtp_rtcp/source/rtp_sender.cc          |  5 +----
 modules/rtp_rtcp/source/rtp_sender_unittest.cc | 14 +++++++-------
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc
index 76cc19c2fc..0ab938b610 100644
--- a/modules/rtp_rtcp/source/rtp_sender.cc
+++ b/modules/rtp_rtcp/source/rtp_sender.cc
@@ -1007,14 +1007,11 @@ std::vector<std::unique_ptr<RtpPacketToSend>> RTPSender::GeneratePadding(
   std::vector<std::unique_ptr<RtpPacketToSend>> padding_packets;
   size_t bytes_left = target_size_bytes;
   if ((rtx_ & kRtxRedundantPayloads) != 0) {
-    while (bytes_left >= 0) {
+    while (bytes_left >= kMinPayloadPaddingBytes) {
       std::unique_ptr<RtpPacketToSend> packet =
           packet_history_.GetPayloadPaddingPacket(
               [&](const RtpPacketToSend& packet)
                   -> std::unique_ptr<RtpPacketToSend> {
-                if (packet.payload_size() + kRtxHeaderSize > bytes_left) {
-                  return nullptr;
-                }
                 return BuildRtxPacket(packet);
               });
       if (!packet) {
diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
index d2761ea347..b5703363fb 100644
--- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
@@ -2549,6 +2549,9 @@ TEST_P(RtpSenderTest, TrySendPacketUpdatesStats) {
 }
 
 TEST_P(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_->SetStorePacketsStatus(true, 1);
@@ -2566,7 +2569,7 @@ TEST_P(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) {
   // Generated padding has large enough budget that the video packet should be
   // retransmitted as padding.
   std::vector<std::unique_ptr<RtpPacketToSend>> generated_packets =
-      rtp_sender_->GeneratePadding(kPayloadPacketSize + kRtxHeaderSize);
+      rtp_sender_->GeneratePadding(kMinPaddingSize);
   ASSERT_EQ(generated_packets.size(), 1u);
   auto& padding_packet = generated_packets.front();
   EXPECT_EQ(padding_packet->packet_type(), RtpPacketToSend::Type::kPadding);
@@ -2575,13 +2578,11 @@ TEST_P(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) {
             kPayloadPacketSize + kRtxHeaderSize);
 
   // Not enough budged for payload padding, use plain padding instead.
-  const size_t kPaddingBytesRequested = kPayloadPacketSize + kRtxHeaderSize - 1;
-  const size_t kExpectedNumPaddingPackets =
-      (kPaddingBytesRequested + kMaxPaddingSize - 1) / kMaxPaddingSize;
+  const size_t kPaddingBytesRequested = kMinPaddingSize - 1;
 
   size_t padding_bytes_generated = 0;
   generated_packets = rtp_sender_->GeneratePadding(kPaddingBytesRequested);
-  EXPECT_EQ(generated_packets.size(), kExpectedNumPaddingPackets);
+  EXPECT_EQ(generated_packets.size(), 1u);
   for (auto& packet : generated_packets) {
     EXPECT_EQ(packet->packet_type(), RtpPacketToSend::Type::kPadding);
     EXPECT_EQ(packet->Ssrc(), kRtxSsrc);
@@ -2590,8 +2591,7 @@ TEST_P(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) {
     padding_bytes_generated += packet->padding_size();
   }
 
-  EXPECT_EQ(padding_bytes_generated,
-            kExpectedNumPaddingPackets * kMaxPaddingSize);
+  EXPECT_EQ(padding_bytes_generated, kMaxPaddingSize);
 }
 
 TEST_P(RtpSenderTest, GeneratePaddingCreatesPurePaddingWithoutRtx) {