Reland "Fixes dynamic mode pacing issues."
This is a reland of 72e6cb0b3f548900fd3b548b4b6966e3f5ee854f
Was not the cause of perf alert, relanding.
TBR=ilnik@webrtc.org
Original change's description:
> Fixes dynamic mode pacing issues.
>
> This CL fixes a few issues in the (default-disabled) dynamic pacing
> mode:
> * Slight update to sleep timing to avoid short spin loops
> * Removed support for early execution as that lead to time-travel
> contradictions that were difficult to solve.
> * Makes sure we schedule a process call when a packet is due to be
> drained even if the queue is empty, so that padding will start at
> the correct time.
> * While paused or empty, sleep relative last send time if we send
> padding while silent - otherwise just relative to last process
> time.
> * If target send time shifts so far back that packet should have
> been sent prior to the last process, make sure we don't let the
> buffer level remain.
> * Update the PacedSender test to _actually_ use dynamic processing
> when the param says so.
>
> Bug: webrtc:10809
> Change-Id: Iebfde9769647d2390fd192a40bbe2d5bf1f6cc62
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/160407
> Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
> Commit-Queue: Erik Språng <sprang@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#29911}
Bug: webrtc:10809
Change-Id: Ie7b307e574c2057bb05af87b6718a132d639a416
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/160786
Reviewed-by: Erik Språng <sprang@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#29928}
diff --git a/modules/pacing/pacing_controller_unittest.cc b/modules/pacing/pacing_controller_unittest.cc
index 5b5f6e7..9337ad2 100644
--- a/modules/pacing/pacing_controller_unittest.cc
+++ b/modules/pacing/pacing_controller_unittest.cc
@@ -732,33 +732,48 @@
EXPECT_LE((actual_pace_time - expected_pace_time).Abs(),
PacingController::kMinSleepTime);
- // Pacing media happens 2.5x factor, but padding was configured with 1.0x
+ // Pacing media happens at 2.5x, but padding was configured with 1.0x
// factor. We have to wait until the padding debt is gone before we start
// sending padding.
const TimeDelta time_to_padding_debt_free =
(expected_pace_time * kPaceMultiplier) - actual_pace_time;
- TimeDelta time_to_next = pacer_->NextSendTime() - clock_.CurrentTime();
- EXPECT_EQ(time_to_next, time_to_padding_debt_free);
- clock_.AdvanceTime(time_to_next);
+ clock_.AdvanceTime(time_to_padding_debt_free -
+ PacingController::kMinSleepTime);
+ pacer_->ProcessPackets();
// Send 10 padding packets.
const size_t kPaddingPacketsToSend = 10;
DataSize padding_sent = DataSize::Zero();
+ size_t packets_sent = 0;
+ Timestamp first_send_time = Timestamp::MinusInfinity();
+ Timestamp last_send_time = Timestamp::MinusInfinity();
+
EXPECT_CALL(callback_, SendPadding)
.Times(kPaddingPacketsToSend)
.WillRepeatedly([&](size_t target_size) {
- padding_sent += DataSize::bytes(target_size);
+ ++packets_sent;
+ if (packets_sent < kPaddingPacketsToSend) {
+ // Don't count bytes of last packet, instead just
+ // use this as the time the last packet finished
+ // sending.
+ padding_sent += DataSize::bytes(target_size);
+ }
+ if (first_send_time.IsInfinite()) {
+ first_send_time = clock_.CurrentTime();
+ } else {
+ last_send_time = clock_.CurrentTime();
+ }
return target_size;
});
EXPECT_CALL(callback_, SendPacket(_, _, _, false, true))
.Times(kPaddingPacketsToSend);
- const Timestamp padding_start_time = clock_.CurrentTime();
- for (size_t i = 0; i < kPaddingPacketsToSend; ++i) {
+
+ while (packets_sent < kPaddingPacketsToSend) {
AdvanceTimeAndProcess();
}
// Verify rate of sent padding.
- TimeDelta padding_duration = pacer_->NextSendTime() - padding_start_time;
+ TimeDelta padding_duration = last_send_time - first_send_time;
DataRate padding_rate = padding_sent / padding_duration;
EXPECT_EQ(padding_rate, kTargetRate);
}
@@ -781,15 +796,18 @@
SendAndExpectPacket(RtpPacketToSend::Type::kVideo, ssrc, sequence_number++,
capture_time_ms, 250);
- EXPECT_CALL(callback_, SendPadding).WillOnce([](size_t padding) {
+ bool padding_sent = false;
+ EXPECT_CALL(callback_, SendPadding).WillOnce([&](size_t padding) {
+ padding_sent = true;
return padding;
});
EXPECT_CALL(callback_, SendPacket(_, _, _, _, true)).Times(1);
if (PeriodicProcess()) {
pacer_->ProcessPackets();
} else {
- AdvanceTimeAndProcess(); // Media.
- AdvanceTimeAndProcess(); // Padding.
+ while (!padding_sent) {
+ AdvanceTimeAndProcess();
+ }
}
}
@@ -1677,47 +1695,6 @@
}
}
-TEST_P(PacingControllerTest, TaskEarly) {
- if (PeriodicProcess()) {
- // This test applies only when NOT using interval budget.
- return;
- }
-
- // Set a low send rate to more easily test timing issues.
- DataRate kSendRate = DataRate::kbps(30);
- pacer_->SetPacingRates(kSendRate, DataRate::Zero());
-
- // Add two packets.
- pacer_->EnqueuePacket(BuildRtpPacket(RtpPacketToSend::Type::kVideo));
- pacer_->EnqueuePacket(BuildRtpPacket(RtpPacketToSend::Type::kVideo));
-
- // Process packets, only first should be sent.
- EXPECT_CALL(callback_, SendPacket).Times(1);
- pacer_->ProcessPackets();
-
- Timestamp next_send_time = pacer_->NextSendTime();
-
- // Packets won't be sent if we try process more than one sleep time early.
- ASSERT_GT(next_send_time - clock_.CurrentTime(),
- PacingController::kMinSleepTime);
- clock_.AdvanceTime(next_send_time - clock_.CurrentTime() -
- (PacingController::kMinSleepTime + TimeDelta::ms(1)));
-
- EXPECT_CALL(callback_, SendPacket).Times(0);
- pacer_->ProcessPackets();
-
- // Assume timing is accurate within +-100us due to rounding.
- const TimeDelta kErrorMargin = TimeDelta::us(100);
-
- // Check that next scheduled send time is still the same (within margin).
- EXPECT_LT((pacer_->NextSendTime() - next_send_time).Abs(), kErrorMargin);
-
- // Advance to within error margin for execution.
- clock_.AdvanceTime(TimeDelta::ms(1) + kErrorMargin);
- EXPECT_CALL(callback_, SendPacket).Times(1);
- pacer_->ProcessPackets();
-}
-
TEST_P(PacingControllerTest, TaskLate) {
if (PeriodicProcess()) {
// This test applies only when NOT using interval budget.
