From 9acc18d1fe951034cb085df9d56b9c26eb8c679a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Thu, 16 Apr 2020 19:41:07 +0200 Subject: [PATCH] Makes dynamic pacer select paddig target based on rate. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Today when the pacing debt is cleared, we blindly ask for 50 bytes of padding, which is above a static magic number for RTX payload padding. Instead, we should adjust the target size based on the current padding rate. The old pacer sort-of does this, it allows the budget to grow up to one process interval (usually 5ms). This CL makes the dynamic pacer also use a duration as target, by default 5ms to match old pacer but with a trial to allow tweaking it. This will be important for good behavior due to https://bugs.chromium.org/p/webrtc/issues/detail?id=11508 Bug: webrtc:10809 Change-Id: I9c14acc5730c6e2e0d7821adf5fb058b8d5487c6 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/173687 Commit-Queue: Erik Språng Reviewed-by: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/master@{#31091} --- modules/pacing/pacing_controller.cc | 15 +++++-- modules/pacing/pacing_controller.h | 3 ++ modules/pacing/pacing_controller_unittest.cc | 44 +++++++++++++++++++- 3 files changed, 58 insertions(+), 4 deletions(-) diff --git a/modules/pacing/pacing_controller.cc b/modules/pacing/pacing_controller.cc index 14feacf3b8..f21e63733f 100644 --- a/modules/pacing/pacing_controller.cc +++ b/modules/pacing/pacing_controller.cc @@ -20,6 +20,7 @@ #include "modules/pacing/interval_budget.h" #include "modules/utility/include/process_thread.h" #include "rtc_base/checks.h" +#include "rtc_base/experiments/field_trial_parser.h" #include "rtc_base/logging.h" #include "rtc_base/time_utils.h" #include "system_wrappers/include/clock.h" @@ -33,10 +34,9 @@ constexpr TimeDelta kCongestedPacketInterval = TimeDelta::Millis(500); // The maximum debt level, in terms of time, capped when sending packets. constexpr TimeDelta kMaxDebtInTime = TimeDelta::Millis(500); constexpr TimeDelta kMaxElapsedTime = TimeDelta::Seconds(2); -constexpr DataSize kDefaultPaddingTarget = DataSize::Bytes(50); // Upper cap on process interval, in case process has not been called in a long -// time. +// time. Applies only to periodic mode. constexpr TimeDelta kMaxProcessingInterval = TimeDelta::Millis(30); constexpr int kFirstPriority = 0; @@ -51,6 +51,14 @@ bool IsEnabled(const WebRtcKeyValueConfig& field_trials, return absl::StartsWith(field_trials.Lookup(key), "Enabled"); } +TimeDelta GetDynamicPaddingTarget(const WebRtcKeyValueConfig& field_trials) { + FieldTrialParameter padding_target("timedelta", + TimeDelta::Millis(5)); + ParseFieldTrial({&padding_target}, + field_trials.Lookup("WebRTC-Pacer-DynamicPaddingTarget")); + return padding_target.Get(); +} + int GetPriorityForType(RtpPacketMediaType type) { // Lower number takes priority over higher. switch (type) { @@ -102,6 +110,7 @@ PacingController::PacingController(Clock* clock, IsEnabled(*field_trials_, "WebRTC-Pacer-SmallFirstProbePacket")), ignore_transport_overhead_( IsEnabled(*field_trials_, "WebRTC-Pacer-IgnoreTransportOverhead")), + padding_target_duration_(GetDynamicPaddingTarget(*field_trials_)), min_packet_limit_(kDefaultMinPacketLimit), transport_overhead_per_packet_(DataSize::Zero()), last_timestamp_(clock_->CurrentTime()), @@ -605,7 +614,7 @@ DataSize PacingController::PaddingToAdd( return DataSize::Bytes(padding_budget_.bytes_remaining()); } else if (padding_rate_ > DataRate::Zero() && padding_debt_ == DataSize::Zero()) { - return kDefaultPaddingTarget; + return padding_target_duration_ * padding_rate_; } return DataSize::Zero(); } diff --git a/modules/pacing/pacing_controller.h b/modules/pacing/pacing_controller.h index 4ffcbd3afc..27f1614b08 100644 --- a/modules/pacing/pacing_controller.h +++ b/modules/pacing/pacing_controller.h @@ -181,6 +181,9 @@ class PacingController { const bool pace_audio_; const bool small_first_probe_packet_; const bool ignore_transport_overhead_; + // In dynamic mode, indicates the target size when requesting padding, + // expressed as a duration in order to adjust for varying padding rate. + const TimeDelta padding_target_duration_; TimeDelta min_packet_limit_; diff --git a/modules/pacing/pacing_controller_unittest.cc b/modules/pacing/pacing_controller_unittest.cc index 3226c02d8a..fa23da70a0 100644 --- a/modules/pacing/pacing_controller_unittest.cc +++ b/modules/pacing/pacing_controller_unittest.cc @@ -184,7 +184,9 @@ class PacingControllerProbing : public PacingController::PacketSender { class PacingControllerTest : public ::testing::TestWithParam { protected: - PacingControllerTest() : clock_(123456) { + PacingControllerTest() : clock_(123456) {} + + void SetUp() override { srand(0); // Need to initialize PacingController after we initialize clock. pacer_ = std::make_unique(&clock_, &callback_, nullptr, @@ -1983,6 +1985,46 @@ TEST_P(PacingControllerTest, NextSendTimeAccountsForPadding) { EXPECT_EQ(pacer_->NextSendTime() - clock_.CurrentTime(), kPacketPacingTime); } +TEST_P(PacingControllerTest, PaddingTargetAccountsForPaddingRate) { + if (PeriodicProcess()) { + // This test applies only when NOT using interval budget. + return; + } + + // Re-init pacer with an explicitly set padding target of 10ms; + const TimeDelta kPaddingTarget = TimeDelta::Millis(10); + ScopedFieldTrials field_trials( + "WebRTC-Pacer-DynamicPaddingTarget/timedelta:10ms/"); + SetUp(); + + const uint32_t kSsrc = 12345; + const DataRate kPacingDataRate = DataRate::KilobitsPerSec(125); + const DataSize kPacketSize = DataSize::Bytes(130); + + uint32_t sequnce_number = 1; + + // Start with pacing and padding rate equal. + pacer_->SetPacingRates(kPacingDataRate, kPacingDataRate); + + // Send a single packet. + SendAndExpectPacket(RtpPacketMediaType::kVideo, kSsrc, sequnce_number++, + clock_.TimeInMilliseconds(), kPacketSize.bytes()); + AdvanceTimeAndProcess(); + ::testing::Mock::VerifyAndClearExpectations(&callback_); + + size_t expected_padding_target_bytes = + (kPaddingTarget * kPacingDataRate).bytes(); + EXPECT_CALL(callback_, SendPadding(expected_padding_target_bytes)) + .WillOnce(Return(expected_padding_target_bytes)); + AdvanceTimeAndProcess(); + + // Half the padding rate - expect half the padding target. + pacer_->SetPacingRates(kPacingDataRate, kPacingDataRate / 2); + EXPECT_CALL(callback_, SendPadding(expected_padding_target_bytes / 2)) + .WillOnce(Return(expected_padding_target_bytes / 2)); + AdvanceTimeAndProcess(); +} + INSTANTIATE_TEST_SUITE_P( WithAndWithoutIntervalBudget, PacingControllerTest,