Fix bug in dynamic pacer causing slightly inaccurate pacing rate.
When new packets are enqueued after a dead-period where media debt is
zero, that time slice should not be used to reduce the debt for the
new packet.
Bug: webrtc:10809
Change-Id: Ifb960548e6aa349b79f37743cbfed78a5c937a13
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/234081
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35143}
diff --git a/modules/pacing/pacing_controller.cc b/modules/pacing/pacing_controller.cc
index 27cfb92..896daa0 100644
--- a/modules/pacing/pacing_controller.cc
+++ b/modules/pacing/pacing_controller.cc
@@ -297,10 +297,21 @@
Timestamp now = CurrentTime();
- if (mode_ == ProcessMode::kDynamic && packet_queue_.Empty() &&
- NextSendTime() <= now) {
- TimeDelta elapsed_time = UpdateTimeAndGetElapsed(now);
+ if (mode_ == ProcessMode::kDynamic && packet_queue_.Empty()) {
+ // If queue is empty, we need to "fast-forward" the last process time,
+ // so that we don't use passed time as budget for sending the first new
+ // packet.
+ Timestamp target_process_time = now;
+ Timestamp next_send_time = NextSendTime();
+ if (next_send_time.IsFinite()) {
+ // There was already a valid planned send time, such as a keep-alive.
+ // Use that as last process time only if it's prior to now.
+ target_process_time = std::min(now, next_send_time);
+ }
+
+ TimeDelta elapsed_time = UpdateTimeAndGetElapsed(target_process_time);
UpdateBudgetWithElapsedTime(elapsed_time);
+ last_process_time_ = target_process_time;
}
packet_queue_.Push(priority, now, packet_counter_++, std::move(packet));
}
diff --git a/modules/pacing/pacing_controller_unittest.cc b/modules/pacing/pacing_controller_unittest.cc
index a953d5b..d1f751e 100644
--- a/modules/pacing/pacing_controller_unittest.cc
+++ b/modules/pacing/pacing_controller_unittest.cc
@@ -252,8 +252,7 @@
Send(type, ssrc, sequence_number, capture_time_ms, size);
EXPECT_CALL(callback_,
SendPacket(ssrc, sequence_number, capture_time_ms,
- type == RtpPacketMediaType::kRetransmission, false))
- .Times(1);
+ type == RtpPacketMediaType::kRetransmission, false));
}
std::unique_ptr<RtpPacketToSend> BuildRtpPacket(RtpPacketMediaType type) {
@@ -2135,6 +2134,39 @@
AdvanceTimeAndProcess();
}
+TEST_P(PacingControllerTest, GapInPacingDoesntAccumulateBudget) {
+ if (PeriodicProcess()) {
+ // This test checks behavior when not using interval budget.
+ return;
+ }
+
+ const uint32_t kSsrc = 12345;
+ uint16_t sequence_number = 1234;
+ const DataSize kPackeSize = DataSize::Bytes(250);
+ const TimeDelta kPacketSendTime = TimeDelta::Millis(15);
+
+ pacer_->SetPacingRates(kPackeSize / kPacketSendTime,
+ /*padding_rate=*/DataRate::Zero());
+
+ // Send an initial packet.
+ SendAndExpectPacket(RtpPacketMediaType::kVideo, kSsrc, sequence_number++,
+ clock_.TimeInMilliseconds(), kPackeSize.bytes());
+ pacer_->ProcessPackets();
+ ::testing::Mock::VerifyAndClearExpectations(&callback_);
+
+ // Advance time kPacketSendTime past where the media debt should be 0.
+ clock_.AdvanceTime(2 * kPacketSendTime);
+
+ // Enqueue two new packets. Expect only one to be sent one ProcessPackets().
+ Send(RtpPacketMediaType::kVideo, kSsrc, sequence_number + 1,
+ clock_.TimeInMilliseconds(), kPackeSize.bytes());
+ Send(RtpPacketMediaType::kVideo, kSsrc, sequence_number + 2,
+ clock_.TimeInMilliseconds(), kPackeSize.bytes());
+ EXPECT_CALL(callback_, SendPacket(kSsrc, sequence_number + 1,
+ clock_.TimeInMilliseconds(), false, false));
+ pacer_->ProcessPackets();
+}
+
INSTANTIATE_TEST_SUITE_P(
WithAndWithoutIntervalBudget,
PacingControllerTest,
