From 28bc2ca92c3510791cbb842f8a73b08e3ad37e31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Thu, 7 Jul 2022 14:49:18 +0200 Subject: [PATCH] Remove unused WebRTC-LimitPaddingSize field trial MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:11508 Change-Id: Ib7d48e23bd44e2f948d51800090fc14b873d11eb Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/268122 Auto-Submit: Erik Språng Reviewed-by: Danil Chapovalov Commit-Queue: Erik Språng Cr-Commit-Position: refs/heads/main@{#37485} --- modules/rtp_rtcp/BUILD.gn | 1 + .../source/rtp_rtcp_impl2_unittest.cc | 11 +----- modules/rtp_rtcp/source/rtp_sender.cc | 27 ++++--------- modules/rtp_rtcp/source/rtp_sender.h | 3 -- .../rtp_rtcp/source/rtp_sender_unittest.cc | 39 ++++--------------- 5 files changed, 18 insertions(+), 63 deletions(-) diff --git a/modules/rtp_rtcp/BUILD.gn b/modules/rtp_rtcp/BUILD.gn index 649c1efd51..b2a01aafac 100644 --- a/modules/rtp_rtcp/BUILD.gn +++ b/modules/rtp_rtcp/BUILD.gn @@ -656,6 +656,7 @@ if (rtc_include_tests) { "../../test:mock_transport", "../../test:rtp_test_utils", "../../test:run_loop", + "../../test:scoped_key_value_config", "../../test:test_support", "../../test/time_controller:time_controller", "../video_coding:codec_globals_headers", diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc index 664fbbadc9..27e34cd22e 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc @@ -165,19 +165,13 @@ class FieldTrialConfig : public FieldTrialsView { return trials; } - FieldTrialConfig() : overhead_enabled_(false), max_padding_factor_(1200) {} + FieldTrialConfig() : overhead_enabled_(false) {} ~FieldTrialConfig() override {} void SetOverHeadEnabled(bool enabled) { overhead_enabled_ = enabled; } - void SetMaxPaddingFactor(double factor) { max_padding_factor_ = factor; } std::string Lookup(absl::string_view key) const override { - if (key == "WebRTC-LimitPaddingSize") { - char string_buf[32]; - rtc::SimpleStringBuilder ssb(string_buf); - ssb << "factor:" << max_padding_factor_; - return ssb.str(); - } else if (key == "WebRTC-SendSideBwe-WithOverhead") { + if (key == "WebRTC-SendSideBwe-WithOverhead") { return overhead_enabled_ ? "Enabled" : "Disabled"; } return ""; @@ -185,7 +179,6 @@ class FieldTrialConfig : public FieldTrialsView { private: bool overhead_enabled_; - double max_padding_factor_; }; class RtpRtcpModule : public RtcpPacketTypeCounterObserver, diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 5aed22d3b8..adf7384ff7 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -46,6 +46,10 @@ constexpr size_t kRtpHeaderLength = 12; // Min size needed to get payload padding from packet history. constexpr int kMinPayloadPaddingBytes = 50; +// Determines how much larger a payload padding packet may be, compared to the +// requested padding size. +constexpr double kMaxPaddingSizeFactor = 3.0; + template constexpr RtpExtensionSize CreateExtensionSize() { return {Extension::kId, Extension::kValueSizeBytes}; @@ -139,21 +143,6 @@ bool HasBweExtension(const RtpHeaderExtensionMap& extensions_map) { extensions_map.IsRegistered(kRtpExtensionTransmissionTimeOffset); } -double GetMaxPaddingSizeFactor(const FieldTrialsView* field_trials) { - // Too low factor means RTX payload padding is rarely used and ineffective. - // Too high means we risk interrupting regular media packets. - // In practice, 3x seems to yield reasonable results. - constexpr double kDefaultFactor = 3.0; - if (!field_trials) { - return kDefaultFactor; - } - - FieldTrialOptional factor("factor", kDefaultFactor); - ParseFieldTrial({&factor}, field_trials->Lookup("WebRTC-LimitPaddingSize")); - RTC_CHECK_GE(factor.Value(), 0.0); - return factor.Value(); -} - } // namespace RTPSender::RTPSender(const RtpRtcpInterface::Configuration& config, @@ -166,7 +155,6 @@ RTPSender::RTPSender(const RtpRtcpInterface::Configuration& config, rtx_ssrc_(config.rtx_send_ssrc), flexfec_ssrc_(config.fec_generator ? config.fec_generator->FecSsrc() : absl::nullopt), - max_padding_size_factor_(GetMaxPaddingSizeFactor(config.field_trials)), packet_history_(packet_history), paced_sender_(packet_sender), sending_media_(true), // Default to sending media. @@ -392,11 +380,10 @@ std::vector> RTPSender::GeneratePadding( packet_history_->GetPayloadPaddingPacket( [&](const RtpPacketToSend& packet) -> std::unique_ptr { - // Limit overshoot, generate <= `max_padding_size_factor_` * - // target_size_bytes. + // Limit overshoot, generate <= `kMaxPaddingSizeFactor` * + // `target_size_bytes`. const size_t max_overshoot_bytes = static_cast( - ((max_padding_size_factor_ - 1.0) * target_size_bytes) + - 0.5); + ((kMaxPaddingSizeFactor - 1.0) * target_size_bytes) + 0.5); if (packet.payload_size() + kRtxHeaderSize > max_overshoot_bytes + bytes_left) { return nullptr; diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index 48e55eb0c3..55dee7f219 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -175,9 +175,6 @@ class RTPSender { const uint32_t ssrc_; const absl::optional rtx_ssrc_; const absl::optional flexfec_ssrc_; - // Limits GeneratePadding() outcome to <= - // `max_padding_size_factor_` * `target_size_bytes` - const double max_padding_size_factor_; RtpPacketHistory* const packet_history_; RtpPacketSender* const paced_sender_; diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index bd3094b609..ea9277f612 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -40,6 +40,7 @@ #include "test/gmock.h" #include "test/gtest.h" #include "test/mock_transport.h" +#include "test/scoped_key_value_config.h" #include "test/time_controller/simulated_time_controller.h" namespace webrtc { @@ -73,6 +74,7 @@ const size_t kMaxPaddingLength = 224; // Value taken from rtp_sender.cc. const uint32_t kTimestampTicksPerMs = 90; // 90kHz clock. constexpr absl::string_view kMid = "mid"; constexpr absl::string_view kRid = "f"; +constexpr bool kMarkerBit = true; using ::testing::_; using ::testing::AllOf; @@ -102,27 +104,6 @@ class MockRtpPacketPacer : public RtpPacketSender { (override)); }; -class FieldTrialConfig : public FieldTrialsView { - public: - FieldTrialConfig() : max_padding_factor_(1200) {} - ~FieldTrialConfig() override {} - - void SetMaxPaddingFactor(double factor) { max_padding_factor_ = factor; } - - std::string Lookup(absl::string_view key) const override { - if (key == "WebRTC-LimitPaddingSize") { - char string_buf[32]; - rtc::SimpleStringBuilder ssb(string_buf); - ssb << "factor:" << max_padding_factor_; - return ssb.str(); - } - return ""; - } - - private: - double max_padding_factor_; -}; - } // namespace class RtpSenderTest : public ::testing::Test { @@ -138,8 +119,7 @@ class RtpSenderTest : public ::testing::Test { std::vector(), std::vector(), nullptr, - clock_), - kMarkerBit(true) {} + clock_) {} void SetUp() override { SetUpRtpSender(true, false, nullptr); } @@ -191,8 +171,7 @@ class RtpSenderTest : public ::testing::Test { std::unique_ptr packet_history_; std::unique_ptr rtp_sender_; - const bool kMarkerBit; - FieldTrialConfig field_trials_; + const test::ScopedKeyValueConfig field_trials_; std::unique_ptr BuildRtpPacket(int payload_type, bool marker_bit, @@ -562,7 +541,7 @@ TEST_F(RtpSenderTest, KeepsTimestampsOnPayloadPadding) { // Timestamps as set based on capture time in RtpSenderTest. const int64_t start_time = clock_->TimeInMilliseconds(); const uint32_t start_timestamp = start_time * kTimestampTicksPerMs; - const size_t kPayloadSize = 600; + const size_t kPayloadSize = 200; const size_t kRtxHeaderSize = 2; // Start by sending one media packet and putting in the packet history. @@ -1108,9 +1087,8 @@ TEST_F(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) { } TEST_F(RtpSenderTest, LimitsPayloadPaddingSize) { - // Limit RTX payload padding to 2x target size. - const double kFactor = 2.0; - field_trials_.SetMaxPaddingFactor(kFactor); + // RTX payload padding is limited to 3x target size. + const double kFactor = 3.0; SetUpRtpSender(false, false, nullptr); rtp_sender_->SetRtxPayloadType(kRtxPayload, kPayload); rtp_sender_->SetRtxStatus(kRtxRetransmitted | kRtxRedundantPayloads); @@ -1313,11 +1291,10 @@ TEST_F(RtpSenderTest, DoesntFecProtectRetransmissions) { } TEST_F(RtpSenderTest, MarksPacketsWithKeyframeStatus) { - FieldTrialBasedConfig field_trials; RTPSenderVideo::Config video_config; video_config.clock = clock_; video_config.rtp_sender = rtp_sender_.get(); - video_config.field_trials = &field_trials; + video_config.field_trials = &field_trials_; RTPSenderVideo rtp_sender_video(video_config); const uint8_t kPayloadType = 127;