From c235a8d2bb176061b27876c3520a1afbb048d3a4 Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Fri, 15 Jun 2018 14:46:11 +0200 Subject: [PATCH] Adds trial to always send padding packets when not sending video. This can be used to avoid getting stuck in a state where the encoder is paused due to low bandwidth estimate which means no additional feedback is received to update the bandwidth estimate. This could happen if feedback packets are lost. Bug: webrtc:8415 Change-Id: I59cd60c0277e8b31a6b911b25e8e488af9008fc2 Reviewed-on: https://webrtc-review.googlesource.com/80880 Commit-Queue: Sebastian Jansson Reviewed-by: Christoffer Rodbro Reviewed-by: Stefan Holmer Cr-Commit-Position: refs/heads/master@{#23632} --- modules/pacing/paced_sender.cc | 27 ++++++------ modules/pacing/paced_sender.h | 2 +- modules/pacing/paced_sender_unittest.cc | 58 +++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 14 deletions(-) diff --git a/modules/pacing/paced_sender.cc b/modules/pacing/paced_sender.cc index 036c860cf1..883c33fbbe 100644 --- a/modules/pacing/paced_sender.cc +++ b/modules/pacing/paced_sender.cc @@ -63,6 +63,8 @@ PacedSender::PacedSender(const Clock* clock, : clock_(clock), packet_sender_(packet_sender), alr_detector_(rtc::MakeUnique(event_log)), + send_padding_if_silent_( + field_trial::IsEnabled("WebRTC-Pacer-PadInSilence")), paused_(false), media_budget_(rtc::MakeUnique(0)), padding_budget_(rtc::MakeUnique(0)), @@ -263,11 +265,9 @@ void PacedSender::Process() { << kMaxElapsedTimeMs << " ms"; elapsed_time_ms = kMaxElapsedTimeMs; } - // When congested we send a padding packet every 500 ms to ensure we won't get - // stuck in the congested state due to no feedback being received. - // TODO(srte): Stop sending packet in paused state when pause is no longer - // used for congestion windows. - if (paused_ || Congested()) { + if (send_padding_if_silent_ || paused_ || Congested()) { + // We send a padding packet every 500 ms to ensure we won't get stuck in + // congested state due to no feedback being received. 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 @@ -277,13 +277,13 @@ void PacedSender::Process() { size_t bytes_sent = SendPadding(1, pacing_info); alr_detector_->OnBytesSent(bytes_sent, elapsed_time_ms); } - last_send_time_us_ = clock_->TimeInMicroseconds(); } - return; } + if (paused_) + return; - int target_bitrate_kbps = pacing_bitrate_kbps_; if (elapsed_time_ms > 0) { + int target_bitrate_kbps = pacing_bitrate_kbps_; size_t queue_size_bytes = packets_->SizeInBytes(); if (queue_size_bytes > 0) { // Assuming equal size packets and input/output rate, the average packet @@ -302,8 +302,6 @@ void PacedSender::Process() { UpdateBudgetWithElapsedTime(elapsed_time_ms); } - last_send_time_us_ = clock_->TimeInMicroseconds(); - bool is_probing = prober_->IsProbing(); PacedPacketInfo pacing_info; size_t bytes_sent = 0; @@ -314,7 +312,7 @@ void PacedSender::Process() { } // 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_ && !Congested()) { + 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 // reinsert it if send fails. @@ -362,8 +360,9 @@ void PacedSender::ProcessThreadAttached(ProcessThread* process_thread) { bool PacedSender::SendPacket(const PacketQueueInterface::Packet& packet, const PacedPacketInfo& pacing_info) { RTC_DCHECK(!paused_); - if (media_budget_->bytes_remaining() == 0 && - pacing_info.probe_cluster_id == PacedPacketInfo::kNotAProbe) { + if (Congested() || + (media_budget_->bytes_remaining() == 0 && + pacing_info.probe_cluster_id == PacedPacketInfo::kNotAProbe)) { return false; } @@ -383,6 +382,7 @@ bool PacedSender::SendPacket(const PacketQueueInterface::Packet& packet, // and we probably don't want to update the budget in such cases. // https://bugs.chromium.org/p/webrtc/issues/detail?id=8052 UpdateBudgetWithBytesSent(packet.bytes); + last_send_time_us_ = clock_->TimeInMicroseconds(); } } @@ -400,6 +400,7 @@ size_t PacedSender::SendPadding(size_t padding_needed, if (bytes_sent > 0) { UpdateBudgetWithBytesSent(bytes_sent); } + last_send_time_us_ = clock_->TimeInMicroseconds(); return bytes_sent; } diff --git a/modules/pacing/paced_sender.h b/modules/pacing/paced_sender.h index 5ac9ccf98a..959f6d9606 100644 --- a/modules/pacing/paced_sender.h +++ b/modules/pacing/paced_sender.h @@ -162,7 +162,7 @@ class PacedSender : public Pacer { const Clock* const clock_; PacketSender* const packet_sender_; const std::unique_ptr alr_detector_ RTC_PT_GUARDED_BY(critsect_); - + const bool send_padding_if_silent_; rtc::CriticalSection critsect_; bool paused_ RTC_GUARDED_BY(critsect_); // This is the media budget, keeping track of how many bits of media diff --git a/modules/pacing/paced_sender_unittest.cc b/modules/pacing/paced_sender_unittest.cc index 62e8c5083d..a8b9baed2e 100644 --- a/modules/pacing/paced_sender_unittest.cc +++ b/modules/pacing/paced_sender_unittest.cc @@ -141,6 +141,64 @@ class PacedSenderTest : public testing::TestWithParam { std::unique_ptr send_bucket_; }; +class PacedSenderFieldTrialTest : public testing::Test { + protected: + struct MediaStream { + const RtpPacketSender::Priority priority; + const uint32_t ssrc; + const size_t packet_size; + uint16_t seq_num; + }; + + const int kProcessIntervalsPerSecond = 1000 / 5; + + PacedSenderFieldTrialTest() : clock_(123456) {} + void InsertPacket(PacedSender* pacer, MediaStream* stream) { + pacer->InsertPacket(stream->priority, stream->ssrc, stream->seq_num++, + clock_.TimeInMilliseconds(), stream->packet_size, + false); + } + void ProcessNext(PacedSender* pacer) { + clock_.AdvanceTimeMilliseconds(5); + pacer->Process(); + } + MediaStream audio{/*priority*/ PacedSender::kHighPriority, + /*ssrc*/ 3333, /*packet_size*/ 100, /*seq_num*/ 1000}; + MediaStream video{/*priority*/ PacedSender::kNormalPriority, + /*ssrc*/ 4444, /*packet_size*/ 1000, /*seq_num*/ 1000}; + SimulatedClock clock_; + MockPacedSenderCallback callback_; +}; + +TEST_F(PacedSenderFieldTrialTest, DefaultNoPaddingInSilence) { + PacedSender pacer(&clock_, &callback_, nullptr); + pacer.SetPacingRates(kTargetBitrateBps, 0); + // Video packet to reset last send time and provide padding data. + InsertPacket(&pacer, &video); + EXPECT_CALL(callback_, TimeToSendPacket).WillOnce(Return(true)); + clock_.AdvanceTimeMilliseconds(5); + pacer.Process(); + EXPECT_CALL(callback_, TimeToSendPadding).Times(0); + // Waiting 500 ms should not trigger sending of padding. + clock_.AdvanceTimeMilliseconds(500); + pacer.Process(); +} + +TEST_F(PacedSenderFieldTrialTest, PaddingInSilenceWithTrial) { + ScopedFieldTrials trial("WebRTC-Pacer-PadInSilence/Enabled/"); + PacedSender pacer(&clock_, &callback_, nullptr); + pacer.SetPacingRates(kTargetBitrateBps, 0); + // Video packet to reset last send time and provide padding data. + InsertPacket(&pacer, &video); + EXPECT_CALL(callback_, TimeToSendPacket).WillOnce(Return(true)); + clock_.AdvanceTimeMilliseconds(5); + pacer.Process(); + EXPECT_CALL(callback_, TimeToSendPadding).WillOnce(Return(1000)); + // Waiting 500 ms should trigger sending of padding. + clock_.AdvanceTimeMilliseconds(500); + pacer.Process(); +} + TEST_F(PacedSenderTest, FirstSentPacketTimeIsSet) { uint16_t sequence_number = 1234; const uint32_t kSsrc = 12345;