From 516036f945e40f6357f5898e203671db0a65eb47 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Wed, 14 Feb 2018 10:21:27 +0000 Subject: [PATCH] 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 > Commit-Queue: Sebastian Jansson > 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 Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#22014} --- modules/pacing/paced_sender.cc | 35 ++++++++++--------------- modules/pacing/paced_sender.h | 3 +-- modules/pacing/paced_sender_unittest.cc | 20 +++++--------- 3 files changed, 22 insertions(+), 36 deletions(-) diff --git a/modules/pacing/paced_sender.cc b/modules/pacing/paced_sender.cc index c35c50c67b..862f4e189f 100644 --- a/modules/pacing/paced_sender.cc +++ b/modules/pacing/paced_sender.cc @@ -85,8 +85,7 @@ PacedSender::PacedSender(const Clock* clock, prober_(rtc::MakeUnique(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::QueueInMs() const { 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 @@ int64_t PacedSender::TimeUntilNextProcess() { 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 @@ void PacedSender::Process() { 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 @@ void PacedSender::Process() { 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 @@ void PacedSender::Process() { 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 @@ bool PacedSender::SendPacket(const PacketQueue::Packet& packet, 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 @@ size_t PacedSender::SendPadding(size_t padding_needed, } 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 13f3307b4f..0501bbb691 100644 --- a/modules/pacing/paced_sender.h +++ b/modules/pacing/paced_sender.h @@ -156,8 +156,7 @@ class PacedSender : public Pacer { // 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 packets_ diff --git a/modules/pacing/paced_sender_unittest.cc b/modules/pacing/paced_sender_unittest.cc index 4180aa8a8c..263495f64e 100644 --- a/modules/pacing/paced_sender_unittest.cc +++ b/modules/pacing/paced_sender_unittest.cc @@ -599,17 +599,16 @@ TEST_P(PacedSenderTest, Pause) { 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 @@ TEST_P(PacedSenderTest, Pause) { } 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();