From b544f6c2f5d16065a0c7f357102499e5ef8b73a4 Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Mon, 4 Jun 2018 19:02:41 +0200 Subject: [PATCH] Fixing issue where pacer budget increased in congestion. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes an issue where the media budget in the pacer was allowed to increase more than the process interval when congested. Bug: webrtc:8415 Change-Id: I79bf965b6a72ed88313074cdae4746fcaff63340 Reviewed-on: https://webrtc-review.googlesource.com/80121 Reviewed-by: Stefan Holmer Reviewed-by: Björn Terelius Commit-Queue: Sebastian Jansson Cr-Commit-Position: refs/heads/master@{#23531} --- modules/pacing/paced_sender.cc | 5 +-- modules/pacing/paced_sender_unittest.cc | 42 +++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/modules/pacing/paced_sender.cc b/modules/pacing/paced_sender.cc index f8383187a9..036c860cf1 100644 --- a/modules/pacing/paced_sender.cc +++ b/modules/pacing/paced_sender.cc @@ -255,8 +255,8 @@ int64_t PacedSender::TimeUntilNextProcess() { void PacedSender::Process() { int64_t now_us = clock_->TimeInMicroseconds(); rtc::CritScope cs(&critsect_); + int64_t elapsed_time_ms = (now_us - time_last_process_us_ + 500) / 1000; time_last_process_us_ = now_us; - int64_t elapsed_time_ms = (now_us - last_send_time_us_ + 500) / 1000; if (elapsed_time_ms > kMaxElapsedTimeMs) { RTC_LOG(LS_WARNING) << "Elapsed time (" << elapsed_time_ms << " ms) longer than expected, limiting to " @@ -268,7 +268,8 @@ void PacedSender::Process() { // TODO(srte): Stop sending packet in paused state when pause is no longer // used for congestion windows. if (paused_ || Congested()) { - if (elapsed_time_ms >= kCongestedPacketIntervalMs) { + int64_t elapsed_since_last_send_us = now_us - last_send_time_us_; + if (elapsed_since_last_send_us >= kCongestedPacketIntervalMs * 1000) { // We can not send padding unless a normal packet has first been sent. If // we do, timestamps get messed up. if (packet_counter_ > 0) { diff --git a/modules/pacing/paced_sender_unittest.cc b/modules/pacing/paced_sender_unittest.cc index 44914b7c63..62e8c5083d 100644 --- a/modules/pacing/paced_sender_unittest.cc +++ b/modules/pacing/paced_sender_unittest.cc @@ -600,6 +600,48 @@ TEST_F(PacedSenderTest, SendsOnlyPaddingWhenCongested) { EXPECT_EQ(blocked_packets, send_bucket_->QueueSizePackets()); } +TEST_F(PacedSenderTest, DoesNotAllowOveruseAfterCongestion) { + uint32_t ssrc = 202020; + uint16_t seq_num = 1000; + RtpPacketSender::Priority prio = PacedSender::kNormalPriority; + int size = 1000; + auto now_ms = [this] { return clock_.TimeInMilliseconds(); }; + EXPECT_CALL(callback_, TimeToSendPadding).Times(0); + // The pacing rate is low enough that the budget should not allow two packets + // to be sent in a row. + send_bucket_->SetPacingRates(400 * 8 * 1000 / 5, 0); + // The congestion window is small enough to only let one packet through. + send_bucket_->SetCongestionWindow(800); + send_bucket_->UpdateOutstandingData(0); + // Not yet budget limited or congested, packet is sent. + send_bucket_->InsertPacket(prio, ssrc, seq_num++, now_ms(), size, false); + EXPECT_CALL(callback_, TimeToSendPacket).WillOnce(Return(true)); + clock_.AdvanceTimeMilliseconds(5); + send_bucket_->Process(); + // Packet blocked due to congestion. + send_bucket_->InsertPacket(prio, ssrc, seq_num++, now_ms(), size, false); + EXPECT_CALL(callback_, TimeToSendPacket).Times(0); + clock_.AdvanceTimeMilliseconds(5); + send_bucket_->Process(); + // Packet blocked due to congestion. + send_bucket_->InsertPacket(prio, ssrc, seq_num++, now_ms(), size, false); + EXPECT_CALL(callback_, TimeToSendPacket).Times(0); + clock_.AdvanceTimeMilliseconds(5); + send_bucket_->Process(); + send_bucket_->UpdateOutstandingData(0); + // Congestion removed and budget has recovered, packet is sent. + send_bucket_->InsertPacket(prio, ssrc, seq_num++, now_ms(), size, false); + EXPECT_CALL(callback_, TimeToSendPacket).WillOnce(Return(true)); + clock_.AdvanceTimeMilliseconds(5); + send_bucket_->Process(); + send_bucket_->UpdateOutstandingData(0); + // Should be blocked due to budget limitation as congestion has be removed. + send_bucket_->InsertPacket(prio, ssrc, seq_num++, now_ms(), size, false); + EXPECT_CALL(callback_, TimeToSendPacket).Times(0); + clock_.AdvanceTimeMilliseconds(5); + send_bucket_->Process(); +} + TEST_F(PacedSenderTest, ResumesSendingWhenCongestionEnds) { uint32_t ssrc = 202020; uint16_t sequence_number = 1000;