Revert "Base pacer padding in pause state on time since last send."
This reverts commit 18cf4b67ddc66041d6b114ea15d78eea74d0592b.
Reason for revert: Conflicts with reverting https://webrtc-review.googlesource.com/c/src/+/48000
Original change's description:
> Base pacer padding in pause state on time since last send.
>
> This clarifies the logic behind the pacer packet interval
> in paused state and prepares for future congestion window
> functionality.
>
> Bug: None
> Change-Id: Ibf6e23f73523b43742830353915b2b94d09a6fc9
> Reviewed-on: https://webrtc-review.googlesource.com/52060
> Reviewed-by: Stefan Holmer <stefan@webrtc.org>
> Commit-Queue: Sebastian Jansson <srte@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#22004}
TBR=stefan@webrtc.org,srte@webrtc.org
Change-Id: I670d6f24bb600444d1b3d947795c59955d7b2d77
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: None
Reviewed-on: https://webrtc-review.googlesource.com/53061
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22014}diff --git a/modules/pacing/paced_sender.cc b/modules/pacing/paced_sender.cc
index c35c50c..862f4e1 100644
--- a/modules/pacing/paced_sender.cc
+++ b/modules/pacing/paced_sender.cc
@@ -85,8 +85,7 @@
prober_(rtc::MakeUnique<BitrateProber>(event_log)),
probing_send_failure_(false),
pacing_bitrate_kbps_(0),
- time_last_process_us_(clock->TimeInMicroseconds()),
- last_send_time_us_(clock->TimeInMicroseconds()),
+ time_last_update_us_(clock->TimeInMicroseconds()),
first_sent_packet_ms_(-1),
packets_(std::move(packets)),
packet_counter_(0),
@@ -201,8 +200,7 @@
int64_t PacedSender::TimeUntilNextProcess() {
rtc::CritScope cs(&critsect_);
- int64_t elapsed_time_us =
- clock_->TimeInMicroseconds() - time_last_process_us_;
+ int64_t elapsed_time_us = clock_->TimeInMicroseconds() - time_last_update_us_;
int64_t elapsed_time_ms = (elapsed_time_us + 500) / 1000;
// When paused we wake up every 500 ms to send a padding packet to ensure
// we won't get stuck in the paused state due to no feedback being received.
@@ -220,23 +218,21 @@
void PacedSender::Process() {
int64_t now_us = clock_->TimeInMicroseconds();
rtc::CritScope cs(&critsect_);
- time_last_process_us_ = now_us;
- int64_t elapsed_time_ms = (now_us - last_send_time_us_ + 500) / 1000;
+ int64_t elapsed_time_ms = std::min(
+ kMaxIntervalTimeMs, (now_us - time_last_update_us_ + 500) / 1000);
+ int target_bitrate_kbps = pacing_bitrate_kbps_;
- // When paused we send a padding packet every 500 ms to ensure we won't get
- // stuck in the paused state due to no feedback being received.
if (paused_) {
+ PacedPacketInfo pacing_info;
+ time_last_update_us_ = now_us;
// We can not send padding unless a normal packet has first been sent. If we
// do, timestamps get messed up.
- if (elapsed_time_ms >= kPausedPacketIntervalMs && packet_counter_ > 0) {
- PacedPacketInfo pacing_info;
- SendPadding(1, pacing_info);
- last_send_time_us_ = clock_->TimeInMicroseconds();
- }
+ if (packet_counter_ == 0)
+ return;
+ SendPadding(1, pacing_info);
return;
}
- int target_bitrate_kbps = pacing_bitrate_kbps_;
if (elapsed_time_ms > 0) {
size_t queue_size_bytes = packets_->SizeInBytes();
if (queue_size_bytes > 0) {
@@ -256,7 +252,7 @@
UpdateBudgetWithElapsedTime(elapsed_time_ms);
}
- last_send_time_us_ = clock_->TimeInMicroseconds();
+ time_last_update_us_ = now_us;
bool is_probing = prober_->IsProbing();
PacedPacketInfo pacing_info;
@@ -266,8 +262,6 @@
pacing_info = prober_->CurrentCluster();
recommended_probe_size = prober_->RecommendedMinProbeSize();
}
- // The paused state is checked in the loop since SendPacket leaves the
- // critical section allowing the paused state to be changed from other code.
while (!packets_->Empty() && !paused_) {
// Since we need to release the lock in order to send, we first pop the
// element from the priority queue but keep it in storage, so that we can
@@ -275,8 +269,10 @@
const PacketQueue::Packet& packet = packets_->BeginPop();
if (SendPacket(packet, pacing_info)) {
- bytes_sent += packet.bytes;
// Send succeeded, remove it from the queue.
+ if (first_sent_packet_ms_ == -1)
+ first_sent_packet_ms_ = clock_->TimeInMilliseconds();
+ bytes_sent += packet.bytes;
packets_->FinalizePop(packet);
if (is_probing && bytes_sent > recommended_probe_size)
break;
@@ -327,8 +323,6 @@
critsect_.Enter();
if (success) {
- if (first_sent_packet_ms_ == -1)
- first_sent_packet_ms_ = clock_->TimeInMilliseconds();
if (packet.priority != kHighPriority || account_for_audio_) {
// Update media bytes sent.
// TODO(eladalon): TimeToSendPacket() can also return |true| in some
@@ -357,7 +351,6 @@
}
void PacedSender::UpdateBudgetWithElapsedTime(int64_t delta_time_ms) {
- delta_time_ms = std::min(kMaxIntervalTimeMs, delta_time_ms);
media_budget_->IncreaseBudget(delta_time_ms);
padding_budget_->IncreaseBudget(delta_time_ms);
}
diff --git a/modules/pacing/paced_sender.h b/modules/pacing/paced_sender.h
index 13f3307..0501bbb 100644
--- a/modules/pacing/paced_sender.h
+++ b/modules/pacing/paced_sender.h
@@ -156,8 +156,7 @@
// order to meet pace time constraint).
uint32_t pacing_bitrate_kbps_ RTC_GUARDED_BY(critsect_);
- int64_t time_last_process_us_ RTC_GUARDED_BY(critsect_);
- int64_t last_send_time_us_ RTC_GUARDED_BY(critsect_);
+ int64_t time_last_update_us_ RTC_GUARDED_BY(critsect_);
int64_t first_sent_packet_ms_ RTC_GUARDED_BY(critsect_);
const std::unique_ptr<PacketQueueInterface> packets_
diff --git a/modules/pacing/paced_sender_unittest.cc b/modules/pacing/paced_sender_unittest.cc
index 4180aa8..263495f 100644
--- a/modules/pacing/paced_sender_unittest.cc
+++ b/modules/pacing/paced_sender_unittest.cc
@@ -599,17 +599,16 @@
send_bucket_->Process();
int64_t expected_time_until_send = 500;
- EXPECT_CALL(callback_, TimeToSendPadding(_, _)).Times(0);
- while (expected_time_until_send >= 5) {
- send_bucket_->Process();
+ EXPECT_CALL(callback_, TimeToSendPadding(1, _)).Times(1);
+ while (expected_time_until_send >= 0) {
+ // TimeUntilNextProcess must not return 0 when paused. If it does,
+ // we risk running a busy loop, so ideally it should return a large value.
+ EXPECT_EQ(expected_time_until_send, send_bucket_->TimeUntilNextProcess());
+ if (expected_time_until_send == 0)
+ send_bucket_->Process();
clock_.AdvanceTimeMilliseconds(5);
expected_time_until_send -= 5;
}
- testing::Mock::VerifyAndClearExpectations(&callback_);
- EXPECT_CALL(callback_, TimeToSendPadding(1, _)).Times(1);
- clock_.AdvanceTimeMilliseconds(5);
- send_bucket_->Process();
- testing::Mock::VerifyAndClearExpectations(&callback_);
// Expect high prio packets to come out first followed by normal
// prio packets and low prio packets (all in capture order).
@@ -650,11 +649,6 @@
}
send_bucket_->Resume();
- // The pacer was resumed directly after the previous process call finished. It
- // will therefore wait 5 ms until next process.
- EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess());
- clock_.AdvanceTimeMilliseconds(5);
-
for (size_t i = 0; i < 4; i++) {
EXPECT_EQ(0, send_bucket_->TimeUntilNextProcess());
send_bucket_->Process();
