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}
diff --git a/api/transport/network_types.h b/api/transport/network_types.h
index 320a7c0..f45a35d 100644
--- a/api/transport/network_types.h
+++ b/api/transport/network_types.h
@@ -98,6 +98,7 @@
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 {
diff --git a/modules/pacing/bitrate_prober.cc b/modules/pacing/bitrate_prober.cc
index 99041da..4192df9 100644
--- a/modules/pacing/bitrate_prober.cc
+++ b/modules/pacing/bitrate_prober.cc
@@ -146,7 +146,9 @@
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
diff --git a/modules/pacing/pacing_controller.cc b/modules/pacing/pacing_controller.cc
index 1633de9..85b9e05 100644
--- a/modules/pacing/pacing_controller.cc
+++ b/modules/pacing/pacing_controller.cc
@@ -34,6 +34,8 @@
// 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 @@
}
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 @@
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::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());
- packet_queue_.Push(priority, now, packet_counter_++, std::move(packet));
+ // Get priority first and store in temporary, to avoid chance of object being
+ // 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 @@
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 @@
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 @@
// 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.
diff --git a/modules/pacing/pacing_controller.h b/modules/pacing/pacing_controller.h
index 1b05444..d0e68a9 100644
--- a/modules/pacing/pacing_controller.h
+++ b/modules/pacing/pacing_controller.h
@@ -135,6 +135,8 @@
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 @@
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.
diff --git a/modules/pacing/pacing_controller_unittest.cc b/modules/pacing/pacing_controller_unittest.cc
index bcd4384..caec575 100644
--- a/modules/pacing/pacing_controller_unittest.cc
+++ b/modules/pacing/pacing_controller_unittest.cc
@@ -1265,5 +1265,47 @@
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
