Add GeneratePadding method to replace TimeToSendPadding

Unlike TimeToSendPadding(), the new GeneratePadding() method will
generate RTP packets and put them in the pacer queue, which will be
responsible for actually sending them.

A slight difference from previous logic is that we do not use a lower
bound of 50bytes for getting payload packets, instead we always try and
then abort if the next padding packet is larger than the current
available budget.

Since we're not sending the packets immediately, we don't need to worry
about twcc sequence numbering or updating the stats, that will be
handled by the general SendPacket() codepath. We can also omit the
PacingInfo struct and the return value of bytes sent, as that will
be handled when taking the packets out of the queue.

Bug: webrtc:10633
Change-Id: I066c292805a0bf76c59f68e66c21ea23fdb56c03
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/143794
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28403}

modules/pacing/packet_router.cc

@@ -200,6 +200,31 @@ size_t PacketRouter::TimeToSendPadding(size_t bytes_to_send,
   return total_bytes_sent;
 }
 
+void PacketRouter::GeneratePadding(size_t target_size_bytes) {
+  rtc::CritScope cs(&modules_crit_);
+  // First try on the last rtp module to have sent media. This increases the
+  // the chance that any payload based padding will be useful as it will be
+  // somewhat distributed over modules according the packet rate, even if it
+  // will be more skewed towards the highest bitrate stream. At the very least
+  // this prevents sending payload padding on a disabled stream where it's
+  // guaranteed not to be useful.
+  if (last_send_module_ != nullptr) {
+    RTC_DCHECK(std::find(rtp_send_modules_.begin(), rtp_send_modules_.end(),
+                         last_send_module_) != rtp_send_modules_.end());
+    RTC_DCHECK(last_send_module_->HasBweExtensions());
+    last_send_module_->GeneratePadding(target_size_bytes);
+    return;
+  }
+
+  // Rtp modules are ordered by which stream can most benefit from padding.
+  for (RtpRtcp* rtp_module : rtp_send_modules_) {
+    if (rtp_module->SendingMedia() && rtp_module->HasBweExtensions()) {
+      rtp_module->GeneratePadding(target_size_bytes);
+      return;
+    }
+  }
+}
+
 void PacketRouter::SetTransportWideSequenceNumber(uint16_t sequence_number) {
   rtc::AtomicOps::ReleaseStore(&transport_seq_, sequence_number);
 }

modules/pacing/packet_router.h

@@ -65,6 +65,8 @@ class PacketRouter : public TransportSequenceNumberAllocator,
   virtual size_t TimeToSendPadding(size_t bytes,
                                    const PacedPacketInfo& packet_info);
 
+  virtual void GeneratePadding(size_t target_size_bytes);
+
   void SetTransportWideSequenceNumber(uint16_t sequence_number);
   uint16_t AllocateSequenceNumber() override;
 

modules/pacing/packet_router_unittest.cc

@@ -272,6 +272,38 @@ TEST(PacketRouterTest, TimeToSendPadding) {
   packet_router.RemoveSendRtpModule(&rtp_2);
 }
 
+TEST(PacketRouterTest, GeneratePaddingPicksCorrectModule) {
+  PacketRouter packet_router;
+
+  // Two RTP modules. The first (prioritized due to rtx) isn't sending media so
+  // should not be called.
+  const uint16_t kSsrc1 = 1234;
+  const uint16_t kSsrc2 = 4567;
+
+  NiceMock<MockRtpRtcp> rtp_1;
+  ON_CALL(rtp_1, RtxSendStatus()).WillByDefault(Return(kRtxRedundantPayloads));
+  ON_CALL(rtp_1, SSRC()).WillByDefault(Return(kSsrc1));
+  ON_CALL(rtp_1, SendingMedia()).WillByDefault(Return(false));
+  ON_CALL(rtp_1, HasBweExtensions()).WillByDefault(Return(false));
+
+  NiceMock<MockRtpRtcp> rtp_2;
+  ON_CALL(rtp_2, RtxSendStatus()).WillByDefault(Return(kRtxOff));
+  ON_CALL(rtp_2, SSRC()).WillByDefault(Return(kSsrc2));
+  ON_CALL(rtp_2, SendingMedia()).WillByDefault(Return(true));
+  ON_CALL(rtp_2, HasBweExtensions()).WillByDefault(Return(true));
+
+  packet_router.AddSendRtpModule(&rtp_1, false);
+  packet_router.AddSendRtpModule(&rtp_2, false);
+
+  const size_t kPaddingSize = 123;
+  EXPECT_CALL(rtp_1, GeneratePadding(_)).Times(0);
+  EXPECT_CALL(rtp_2, GeneratePadding(kPaddingSize)).Times(1);
+  packet_router.GeneratePadding(kPaddingSize);
+
+  packet_router.RemoveSendRtpModule(&rtp_1);
+  packet_router.RemoveSendRtpModule(&rtp_2);
+}
+
 TEST(PacketRouterTest, PadsOnLastActiveMediaStream) {
   PacketRouter packet_router;
 

modules/pacing/round_robin_packet_queue.cc

@@ -98,7 +98,7 @@ void RoundRobinPacketQueue::Push(int priority,
   uint32_t ssrc = packet->Ssrc();
   uint16_t sequence_number = packet->SequenceNumber();
   int64_t capture_time_ms = packet->capture_time_ms();
-  size_t size_bytes = packet->payload_size();
+  size_t size_bytes = packet->payload_size() + packet->padding_size();
   auto type = packet->packet_type();
   RTC_DCHECK(type.has_value());
 

modules/rtp_rtcp/include/rtp_rtcp.h

@@ -279,6 +279,8 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface {
   virtual size_t TimeToSendPadding(size_t bytes,
                                    const PacedPacketInfo& pacing_info) = 0;
 
+  virtual void GeneratePadding(size_t target_size_bytes) = 0;
+
   // Called on generation of new statistics after an RTP send.
   virtual void RegisterSendChannelRtpStatisticsCallback(
       StreamDataCountersCallback* callback) = 0;

modules/rtp_rtcp/mocks/mock_rtp_rtcp.h

@@ -95,6 +95,7 @@ class MockRtpRtcp : public RtpRtcp {
                     const PacedPacketInfo& pacing_info));
   MOCK_METHOD2(TimeToSendPadding,
                size_t(size_t bytes, const PacedPacketInfo& pacing_info));
+  MOCK_METHOD1(GeneratePadding, void(size_t target_size_bytes));
   MOCK_METHOD2(RegisterRtcpObservers,
                void(RtcpIntraFrameObserver* intra_frame_callback,
                     RtcpBandwidthObserver* bandwidth_callback));

modules/rtp_rtcp/source/rtp_packet_history.cc

@@ -346,6 +346,15 @@ std::unique_ptr<RtpPacketToSend> RtpPacketHistory::GetBestFittingPacket(
 }
 
 std::unique_ptr<RtpPacketToSend> RtpPacketHistory::GetPayloadPaddingPacket() {
+  // Default implementation always just returns a copy of the packet.
+  return GetPayloadPaddingPacket([](const RtpPacketToSend& packet) {
+    return absl::make_unique<RtpPacketToSend>(packet);
+  });
+}
+
+std::unique_ptr<RtpPacketToSend> RtpPacketHistory::GetPayloadPaddingPacket(
+    rtc::FunctionView<std::unique_ptr<RtpPacketToSend>(const RtpPacketToSend&)>
+        encapsulate) {
   rtc::CritScope cs(&lock_);
   if (mode_ == StorageMode::kDisabled || padding_priority_.empty()) {
     return nullptr;
@@ -362,11 +371,15 @@ std::unique_ptr<RtpPacketToSend> RtpPacketHistory::GetPayloadPaddingPacket() {
     return nullptr;
   }
 
+  auto padding_packet = encapsulate(*best_packet->packet_);
+  if (!padding_packet) {
+    return nullptr;
+  }
+
   best_packet->send_time_ms_ = clock_->TimeInMilliseconds();
   best_packet->IncrementTimesRetransmitted(&padding_priority_);
 
-  // Return a copy of the packet.
-  return absl::make_unique<RtpPacketToSend>(*best_packet->packet_);
+  return padding_packet;
 }
 
 void RtpPacketHistory::CullAcknowledgedPackets(

modules/rtp_rtcp/source/rtp_packet_history.h

@@ -119,6 +119,14 @@ class RtpPacketHistory {
   // a const method.
   std::unique_ptr<RtpPacketToSend> GetPayloadPaddingPacket();
 
+  // Same as GetPayloadPaddingPacket(void), but adds an encapsulation
+  // that can be used for instance to encapsulate the packet in an RTX
+  // container, or to abort getting the packet if the function returns
+  // nullptr.
+  std::unique_ptr<RtpPacketToSend> GetPayloadPaddingPacket(
+      rtc::FunctionView<std::unique_ptr<RtpPacketToSend>(
+          const RtpPacketToSend&)> encapsulate);
+
   // Cull packets that have been acknowledged as received by the remote end.
   void CullAcknowledgedPackets(rtc::ArrayView<const uint16_t> sequence_numbers);
 

modules/rtp_rtcp/source/rtp_packet_history_unittest.cc

@@ -839,4 +839,27 @@ TEST_F(RtpPacketHistoryTest, NoPendingPacketAsPadding) {
   EXPECT_EQ(hist_.GetPayloadPaddingPacket()->SequenceNumber(), kStartSeqNum);
 }
 
+TEST_F(RtpPacketHistoryTest, PayloadPaddingWithEncapsulation) {
+  hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1);
+
+  hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), kAllowRetransmission,
+                     fake_clock_.TimeInMilliseconds());
+  fake_clock_.AdvanceTimeMilliseconds(1);
+
+  // Aborted padding.
+  EXPECT_EQ(nullptr,
+            hist_.GetPayloadPaddingPacket(
+                [](const RtpPacketToSend& packet) { return nullptr; }));
+
+  // Get copy of packet, but with sequence number modified.
+  auto padding_packet =
+      hist_.GetPayloadPaddingPacket([&](const RtpPacketToSend& packet) {
+        auto encapsulated_packet = absl::make_unique<RtpPacketToSend>(packet);
+        encapsulated_packet->SetSequenceNumber(kStartSeqNum + 1);
+        return encapsulated_packet;
+      });
+  ASSERT_TRUE(padding_packet);
+  EXPECT_EQ(padding_packet->SequenceNumber(), kStartSeqNum + 1);
+}
+
 }  // namespace webrtc

modules/rtp_rtcp/source/rtp_rtcp_impl.cc

@@ -427,6 +427,10 @@ size_t ModuleRtpRtcpImpl::TimeToSendPadding(
   return rtp_sender_->TimeToSendPadding(bytes, pacing_info);
 }
 
+void ModuleRtpRtcpImpl::GeneratePadding(size_t target_size_bytes) {
+  rtp_sender_->GeneratePadding(target_size_bytes);
+}
+
 size_t ModuleRtpRtcpImpl::MaxRtpPacketSize() const {
   return rtp_sender_->MaxRtpPacketSize();
 }

modules/rtp_rtcp/source/rtp_rtcp_impl.h

@@ -148,6 +148,8 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp {
   size_t TimeToSendPadding(size_t bytes,
                            const PacedPacketInfo& pacing_info) override;
 
+  void GeneratePadding(size_t target_size_bytes) override;
+
   // RTCP part.
 
   // Get RTCP status.

modules/rtp_rtcp/source/rtp_sender.cc

@@ -354,16 +354,15 @@ size_t RTPSender::SendPadData(size_t bytes,
 
   if (audio_configured_) {
     // Allow smaller padding packets for audio.
-    padding_bytes_in_packet = rtc::SafeClamp<size_t>(
-        bytes, kMinAudioPaddingLength,
-        rtc::SafeMin(max_payload_size, kMaxPaddingLength));
+    padding_bytes_in_packet =
+        rtc::SafeClamp(bytes, kMinAudioPaddingLength,
+                       rtc::SafeMin(max_payload_size, kMaxPaddingLength));
   } else {
     // Always send full padding packets. This is accounted for by the
     // RtpPacketSender, which will make sure we don't send too much padding even
     // if a single packet is larger than requested.
     // We do this to avoid frequently sending small packets on higher bitrates.
-    padding_bytes_in_packet =
-        rtc::SafeMin<size_t>(max_payload_size, kMaxPaddingLength);
+    padding_bytes_in_packet = rtc::SafeMin(max_payload_size, kMaxPaddingLength);
   }
   size_t bytes_sent = 0;
   while (bytes_sent < bytes) {
@@ -837,6 +836,108 @@ size_t RTPSender::TimeToSendPadding(size_t bytes,
   return bytes_sent;
 }
 
+void RTPSender::GeneratePadding(size_t target_size_bytes) {
+  // This method does not actually send packets, it just generates
+  // them and puts them in the pacer queue. Since this should incur
+  // low overhead, keep the lock for the scope of the method in order
+  // to make the code more readable.
+  rtc::CritScope lock(&send_critsect_);
+  if (!sending_media_)
+    return;
+
+  size_t bytes_left = target_size_bytes;
+  if ((rtx_ & kRtxRedundantPayloads) != 0) {
+    while (bytes_left >= 0) {
+      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) {
+        break;
+      }
+
+      bytes_left -= std::min(bytes_left, packet->payload_size());
+      packet->set_packet_type(RtpPacketToSend::Type::kPadding);
+      paced_sender_->EnqueuePacket(std::move(packet));
+    }
+  }
+
+  size_t padding_bytes_in_packet;
+  const size_t max_payload_size = max_packet_size_ - RtpHeaderLength();
+  if (audio_configured_) {
+    // Allow smaller padding packets for audio.
+    padding_bytes_in_packet = rtc::SafeClamp<size_t>(
+        bytes_left, kMinAudioPaddingLength,
+        rtc::SafeMin(max_payload_size, kMaxPaddingLength));
+  } else {
+    // Always send full padding packets. This is accounted for by the
+    // RtpPacketSender, which will make sure we don't send too much padding even
+    // if a single packet is larger than requested.
+    // We do this to avoid frequently sending small packets on higher bitrates.
+    padding_bytes_in_packet = rtc::SafeMin(max_payload_size, kMaxPaddingLength);
+  }
+
+  while (bytes_left > 0) {
+    auto padding_packet =
+        absl::make_unique<RtpPacketToSend>(&rtp_header_extension_map_);
+    padding_packet->set_packet_type(RtpPacketToSend::Type::kPadding);
+    padding_packet->SetMarker(false);
+    padding_packet->SetTimestamp(last_rtp_timestamp_);
+    padding_packet->set_capture_time_ms(capture_time_ms_);
+    if (rtx_ == kRtxOff) {
+      if (last_payload_type_ == -1) {
+        break;
+      }
+      // Without RTX we can't send padding in the middle of frames.
+      // For audio marker bits doesn't mark the end of a frame and frames
+      // are usually a single packet, so for now we don't apply this rule
+      // for audio.
+      if (!audio_configured_ && !last_packet_marker_bit_) {
+        break;
+      }
+
+      RTC_DCHECK(ssrc_);
+      padding_packet->SetSsrc(*ssrc_);
+      padding_packet->SetPayloadType(last_payload_type_);
+      padding_packet->SetSequenceNumber(sequence_number_++);
+    } else {
+      // Without abs-send-time or transport sequence number a media packet
+      // must be sent before padding so that the timestamps used for
+      // estimation are correct.
+      if (!media_has_been_sent_ &&
+          !(rtp_header_extension_map_.IsRegistered(AbsoluteSendTime::kId) ||
+            rtp_header_extension_map_.IsRegistered(
+                TransportSequenceNumber::kId))) {
+        break;
+      }
+      // Only change the timestamp of padding packets sent over RTX.
+      // Padding only packets over RTP has to be sent as part of a media
+      // frame (and therefore the same timestamp).
+      int64_t now_ms = clock_->TimeInMilliseconds();
+      if (last_timestamp_time_ms_ > 0) {
+        padding_packet->SetTimestamp(padding_packet->Timestamp() +
+                                     (now_ms - last_timestamp_time_ms_) *
+                                         kTimestampTicksPerMs);
+        padding_packet->set_capture_time_ms(padding_packet->capture_time_ms() +
+                                            (now_ms - last_timestamp_time_ms_));
+      }
+      RTC_DCHECK(ssrc_rtx_);
+      padding_packet->SetSsrc(*ssrc_rtx_);
+      padding_packet->SetSequenceNumber(sequence_number_rtx_++);
+      padding_packet->SetPayloadType(rtx_payload_type_map_.begin()->second);
+    }
+
+    padding_packet->SetPadding(padding_bytes_in_packet);
+    bytes_left -= std::min(bytes_left, padding_bytes_in_packet);
+    paced_sender_->EnqueuePacket(std::move(padding_packet));
+  }
+}
+
 bool RTPSender::SendToNetwork(std::unique_ptr<RtpPacketToSend> packet,
                               StorageType storage) {
   RTC_DCHECK(packet);

modules/rtp_rtcp/source/rtp_sender.h

@@ -114,6 +114,7 @@ class RTPSender {
   bool TrySendPacket(RtpPacketToSend* packet,
                      const PacedPacketInfo& pacing_info);
   size_t TimeToSendPadding(size_t bytes, const PacedPacketInfo& pacing_info);
+  void GeneratePadding(size_t target_size_bytes);
 
   // NACK.
   void OnReceivedNack(const std::vector<uint16_t>& nack_sequence_numbers,

modules/rtp_rtcp/source/rtp_sender_unittest.cc

@@ -78,8 +78,11 @@ using ::testing::DoAll;
 using ::testing::ElementsAre;
 using ::testing::ElementsAreArray;
 using ::testing::Field;
+using ::testing::Gt;
 using ::testing::Invoke;
 using ::testing::NiceMock;
+using ::testing::Pointee;
+using ::testing::Property;
 using ::testing::Return;
 using ::testing::SaveArg;
 using ::testing::SizeIs;
@@ -2085,6 +2088,79 @@ TEST_P(RtpSenderTest, TrySendPacketUpdatesStats) {
   EXPECT_EQ(rtx_stats.retransmitted.packets, 1u);
 }
 
+TEST_P(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) {
+  rtp_sender_->SetRtxStatus(kRtxRetransmitted | kRtxRedundantPayloads);
+  rtp_sender_->SetRtxPayloadType(kRtxPayload, kPayload);
+  rtp_sender_->SetStorePacketsStatus(true, 1);
+
+  const size_t kPayloadPacketSize = 1234;
+  std::unique_ptr<RtpPacketToSend> packet =
+      BuildRtpPacket(kPayload, true, 0, fake_clock_.TimeInMilliseconds());
+  packet->set_allow_retransmission(true);
+  packet->SetPayloadSize(kPayloadPacketSize);
+  packet->set_packet_type(RtpPacketToSend::Type::kVideo);
+
+  // Send a dummy video packet so it ends up in the packet history.
+  EXPECT_TRUE(rtp_sender_->TrySendPacket(packet.get(), PacedPacketInfo()));
+
+  // Generated padding has large enough budget that the video packet should be
+  // retransmitted as padding.
+  EXPECT_CALL(mock_paced_sender_,
+              EnqueuePacket(AllOf(
+                  Pointee(Property(&RtpPacketToSend::packet_type,
+                                   RtpPacketToSend::Type::kPadding)),
+                  Pointee(Property(&RtpPacketToSend::Ssrc, kRtxSsrc)),
+                  Pointee(Property(&RtpPacketToSend::payload_size,
+                                   kPayloadPacketSize + kRtxHeaderSize)))))
+      .Times(1);
+  rtp_sender_->GeneratePadding(kPayloadPacketSize + kRtxHeaderSize);
+
+  // Not enough budged for payload padding, use plain padding instead.
+  EXPECT_CALL(mock_paced_sender_,
+              EnqueuePacket(AllOf(
+                  Pointee(Property(&RtpPacketToSend::packet_type,
+                                   RtpPacketToSend::Type::kPadding)),
+                  Pointee(Property(&RtpPacketToSend::Ssrc, kRtxSsrc)),
+                  Pointee(Property(&RtpPacketToSend::payload_size, 0)),
+                  Pointee(Property(&RtpPacketToSend::padding_size, Gt(0u))))))
+      .Times((kPayloadPacketSize + kMaxPaddingSize - 1) / kMaxPaddingSize);
+  rtp_sender_->GeneratePadding(kPayloadPacketSize + kRtxHeaderSize - 1);
+}
+
+TEST_P(RtpSenderTest, GeneratePaddingCreatesPurePaddingWithoutRtx) {
+  rtp_sender_->SetStorePacketsStatus(true, 1);
+
+  const size_t kPayloadPacketSize = 1234;
+  // 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.
+  std::unique_ptr<RtpPacketToSend> packet =
+      BuildRtpPacket(kPayload, true, 0, fake_clock_.TimeInMilliseconds());
+  packet->set_allow_retransmission(true);
+  packet->SetPayloadSize(kPayloadPacketSize);
+  packet->set_packet_type(RtpPacketToSend::Type::kVideo);
+  EXPECT_TRUE(rtp_sender_->TrySendPacket(packet.get(), PacedPacketInfo()));
+
+  // Payload padding not available without RTX, only generate plain padding on
+  // the media SSRC.
+  // Number of padding packets is the requested padding size divided by max
+  // padding packet size, rounded up. Pure padding packets are always of the
+  // maximum size.
+  const size_t kPaddingBytesRequested = kPayloadPacketSize + kRtxHeaderSize;
+  const size_t kExpectedNumPaddingPackets =
+      (kPaddingBytesRequested + kMaxPaddingSize - 1) / kMaxPaddingSize;
+  size_t padding_bytes_sent = 0;
+  EXPECT_CALL(mock_paced_sender_, EnqueuePacket)
+      .WillRepeatedly([&](std::unique_ptr<RtpPacketToSend> packet) {
+        EXPECT_EQ(packet->packet_type(), RtpPacketToSend::Type::kPadding);
+        EXPECT_EQ(packet->Ssrc(), kSsrc);
+        EXPECT_EQ(packet->payload_size(), 0u);
+        EXPECT_GT(packet->padding_size(), 0u);
+        padding_bytes_sent += packet->padding_size();
+      });
+  rtp_sender_->GeneratePadding(kPaddingBytesRequested);
+  EXPECT_EQ(padding_bytes_sent, kExpectedNumPaddingPackets * kMaxPaddingSize);
+}
+
 INSTANTIATE_TEST_SUITE_P(WithAndWithoutOverhead,
                          RtpSenderTest,
                          ::testing::Bool());