Adds trial to always start probes with a small padding packet.

This will reduce bias caused by uncertainty in averaging window.

Bug: None
Change-Id: I5c4fe39ffe69fb4af87d86995196a54115d3e0b2
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/144720
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#29374}

api/transport/network_types.h

@@ -98,6 +98,7 @@ struct PacedPacketInfo {
   int probe_cluster_id = kNotAProbe;
   int probe_cluster_min_probes = -1;
   int probe_cluster_min_bytes = -1;
+  int probe_cluster_bytes_sent = 0;
 };
 
 struct SentPacket {

modules/pacing/bitrate_prober.cc

@@ -146,7 +146,9 @@ int BitrateProber::TimeUntilNextProbe(int64_t now_ms) {
 PacedPacketInfo BitrateProber::CurrentCluster() const {
   RTC_DCHECK(!clusters_.empty());
   RTC_DCHECK(probing_state_ == ProbingState::kActive);
-  return clusters_.front().pace_info;
+  PacedPacketInfo info = clusters_.front().pace_info;
+  info.probe_cluster_bytes_sent = clusters_.front().sent_bytes;
+  return info;
 }
 
 // Probe size is recommended based on the probe bitrate required. We choose

modules/pacing/pacing_controller.cc

@@ -34,6 +34,8 @@ constexpr TimeDelta kMaxElapsedTime = TimeDelta::Seconds<2>();
 // time.
 constexpr TimeDelta kMaxProcessingInterval = TimeDelta::Millis<30>();
 
+constexpr int kFirstPriority = 0;
+
 bool IsDisabled(const WebRtcKeyValueConfig& field_trials,
                 absl::string_view key) {
   return field_trials.Lookup(key).find("Disabled") == 0;
@@ -45,24 +47,24 @@ bool IsEnabled(const WebRtcKeyValueConfig& field_trials,
 }
 
 int GetPriorityForType(RtpPacketToSend::Type type) {
+  // Lower number takes priority over higher.
   switch (type) {
     case RtpPacketToSend::Type::kAudio:
       // Audio is always prioritized over other packet types.
-      return 0;
+      return kFirstPriority + 1;
     case RtpPacketToSend::Type::kRetransmission:
       // Send retransmissions before new media.
-      return 1;
+      return kFirstPriority + 2;
     case RtpPacketToSend::Type::kVideo:
-      // Video has "normal" priority, in the old speak.
-      return 2;
     case RtpPacketToSend::Type::kForwardErrorCorrection:
+      // Video has "normal" priority, in the old speak.
       // Send redundancy concurrently to video. If it is delayed it might have a
       // lower chance of being useful.
-      return 2;
+      return kFirstPriority + 3;
     case RtpPacketToSend::Type::kPadding:
       // Packets that are in themselves likely useless, only sent to keep the
       // BWE high.
-      return 3;
+      return kFirstPriority + 4;
   }
 }
 
@@ -88,6 +90,8 @@ PacingController::PacingController(Clock* clock,
       send_padding_if_silent_(
           IsEnabled(*field_trials_, "WebRTC-Pacer-PadInSilence")),
       pace_audio_(!IsDisabled(*field_trials_, "WebRTC-Pacer-BlockAudio")),
+      small_first_probe_packet_(
+          IsEnabled(*field_trials_, "WebRTC-Pacer-SmallFirstProbePacket")),
       min_packet_limit_(kDefaultMinPacketLimit),
       last_timestamp_(clock_->CurrentTime()),
       paused_(false),
@@ -187,17 +191,11 @@ void PacingController::SetPacingRates(DataRate pacing_rate,
 void PacingController::EnqueuePacket(std::unique_ptr<RtpPacketToSend> packet) {
   RTC_DCHECK(pacing_bitrate_ > DataRate::Zero())
       << "SetPacingRate must be called before InsertPacket.";
-
-  Timestamp now = CurrentTime();
-  prober_.OnIncomingPacket(packet->payload_size());
-
-  if (packet->capture_time_ms() < 0) {
-    packet->set_capture_time_ms(now.ms());
-  }
-
   RTC_CHECK(packet->packet_type());
-  int priority = GetPriorityForType(*packet->packet_type());
+  // Get priority first and store in temporary, to avoid chance of object being
-  packet_queue_.Push(priority, now, packet_counter_++, std::move(packet));
+  // moved before GetPriorityForType() being called.
+  const int priority = GetPriorityForType(*packet->packet_type());
+  EnqueuePacketInternal(std::move(packet), priority);
 }
 
 void PacingController::SetAccountForAudioPackets(bool account_for_audio) {
@@ -232,6 +230,22 @@ TimeDelta PacingController::OldestPacketWaitTime() const {
   return CurrentTime() - oldest_packet;
 }
 
+void PacingController::EnqueuePacketInternal(
+    std::unique_ptr<RtpPacketToSend> packet,
+    int priority) {
+  prober_.OnIncomingPacket(packet->payload_size());
+
+  Timestamp now = CurrentTime();
+  prober_.OnIncomingPacket(packet->payload_size());
+
+  // TODO(sprang): Make sure tests respect this, replace with DCHECK.
+  if (packet->capture_time_ms() < 0) {
+    packet->set_capture_time_ms(now.ms());
+  }
+
+  packet_queue_.Push(priority, now, packet_counter_++, std::move(packet));
+}
+
 TimeDelta PacingController::UpdateTimeAndGetElapsed(Timestamp now) {
   TimeDelta elapsed_time = now - time_last_process_;
   time_last_process_ = now;
@@ -322,11 +336,13 @@ void PacingController::ProcessPackets() {
     UpdateBudgetWithElapsedTime(elapsed_time);
   }
 
+  bool first_packet_in_probe = false;
   bool is_probing = prober_.IsProbing();
   PacedPacketInfo pacing_info;
   absl::optional<DataSize> recommended_probe_size;
   if (is_probing) {
     pacing_info = prober_.CurrentCluster();
+    first_packet_in_probe = pacing_info.probe_cluster_bytes_sent == 0;
     recommended_probe_size = DataSize::bytes(prober_.RecommendedMinProbeSize());
   }
 
@@ -334,6 +350,22 @@ void PacingController::ProcessPackets() {
   // The paused state is checked in the loop since it leaves the critical
   // section allowing the paused state to be changed from other code.
   while (!paused_) {
+    if (small_first_probe_packet_ && first_packet_in_probe) {
+      // If first packet in probe, insert a small padding packet so we have a
+      // more reliable start window for the rate estimation.
+      auto padding = packet_sender_->GeneratePadding(DataSize::bytes(1));
+      // If no RTP modules sending media are registered, we may not get a
+      // padding packet back.
+      if (!padding.empty()) {
+        // Insert with high priority so larger media packets don't preempt it.
+        EnqueuePacketInternal(std::move(padding[0]), kFirstPriority);
+        // We should never get more than one padding packets with a requested
+        // size of 1 byte.
+        RTC_DCHECK_EQ(padding.size(), 1u);
+      }
+      first_packet_in_probe = false;
+    }
+
     auto* packet = GetPendingPacket(pacing_info);
     if (packet == nullptr) {
       // No packet available to send, check if we should send padding.

modules/pacing/pacing_controller.h

@@ -135,6 +135,8 @@ class PacingController {
   bool Congested() const;
 
  private:
+  void EnqueuePacketInternal(std::unique_ptr<RtpPacketToSend> packet,
+                             int priority);
   TimeDelta UpdateTimeAndGetElapsed(Timestamp now);
   bool ShouldSendKeepalive(Timestamp now) const;
 
@@ -160,6 +162,7 @@ class PacingController {
   const bool drain_large_queues_;
   const bool send_padding_if_silent_;
   const bool pace_audio_;
+  const bool small_first_probe_packet_;
   TimeDelta min_packet_limit_;
 
   // TODO(webrtc:9716): Remove this when we are certain clocks are monotonic.

modules/pacing/pacing_controller_unittest.cc

@@ -1265,5 +1265,47 @@ TEST_F(PacingControllerTest, OwnedPacketPrioritizedOnType) {
   clock_.AdvanceTimeMilliseconds(200);
   pacer_->ProcessPackets();
 }
+
+TEST_F(PacingControllerTest, SmallFirstProbePacket) {
+  ScopedFieldTrials trial("WebRTC-Pacer-SmallFirstProbePacket/Enabled/");
+  MockPacketSender callback;
+  pacer_ =
+      std::make_unique<PacingController>(&clock_, &callback, nullptr, nullptr);
+  pacer_->CreateProbeCluster(kFirstClusterRate, /*cluster_id=*/0);
+  pacer_->SetPacingRates(kTargetRate * kPaceMultiplier, DataRate::Zero());
+
+  // Add high prio media.
+  pacer_->EnqueuePacket(BuildRtpPacket(RtpPacketToSend::Type::kAudio));
+
+  // Expect small padding packet to be requested.
+  EXPECT_CALL(callback, GeneratePadding(DataSize::bytes(1)))
+      .WillOnce([&](DataSize padding_size) {
+        std::vector<std::unique_ptr<RtpPacketToSend>> padding_packets;
+        padding_packets.emplace_back(
+            BuildPacket(RtpPacketToSend::Type::kPadding, kAudioSsrc, 1,
+                        clock_.TimeInMilliseconds(), 1));
+        return padding_packets;
+      });
+
+  size_t packets_sent = 0;
+  bool media_seen = false;
+  EXPECT_CALL(callback, SendRtpPacket)
+      .Times(::testing::AnyNumber())
+      .WillRepeatedly([&](std::unique_ptr<RtpPacketToSend> packet,
+                          const PacedPacketInfo& cluster_info) {
+        if (packets_sent == 0) {
+          EXPECT_EQ(packet->packet_type(), RtpPacketToSend::Type::kPadding);
+        } else {
+          if (packet->packet_type() == RtpPacketToSend::Type::kAudio) {
+            media_seen = true;
+          }
+        }
+        packets_sent++;
+      });
+  while (!media_seen) {
+    pacer_->ProcessPackets();
+    clock_.AdvanceTimeMilliseconds(5);
+  }
+}
 }  // namespace test
 }  // namespace webrtc