From b57178b8362e48c70f6e7701217456dec540414d Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Thu, 2 May 2024 16:48:16 +0200 Subject: [PATCH] Query WebRTC-KeyframeInterval through propagated field trials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:42220378 Change-Id: I3556ec6b280bf6c03e6c3a20949a19e182eed2b8 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/349640 Reviewed-by: Erik Språng Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#42349} --- rtc_base/experiments/BUILD.gn | 1 - .../experiments/keyframe_interval_settings.cc | 11 +++------- .../experiments/keyframe_interval_settings.h | 6 ++--- .../keyframe_interval_settings_unittest.cc | 22 ++++++++----------- video/encoder_rtcp_feedback.cc | 10 +++++---- video/encoder_rtcp_feedback.h | 6 ++--- video/encoder_rtcp_feedback_unittest.cc | 3 ++- video/video_send_stream_impl.cc | 2 +- 8 files changed, 26 insertions(+), 35 deletions(-) diff --git a/rtc_base/experiments/BUILD.gn b/rtc_base/experiments/BUILD.gn index dfce9ee2d4..603449d485 100644 --- a/rtc_base/experiments/BUILD.gn +++ b/rtc_base/experiments/BUILD.gn @@ -152,7 +152,6 @@ rtc_library("keyframe_interval_settings_experiment") { deps = [ ":field_trial_parser", "../../api:field_trials_view", - "../../api/transport:field_trial_based_config", ] absl_deps = [ "//third_party/abseil-cpp/absl/types:optional" ] } diff --git a/rtc_base/experiments/keyframe_interval_settings.cc b/rtc_base/experiments/keyframe_interval_settings.cc index 413e2a91d5..df31d29f12 100644 --- a/rtc_base/experiments/keyframe_interval_settings.cc +++ b/rtc_base/experiments/keyframe_interval_settings.cc @@ -10,7 +10,7 @@ #include "rtc_base/experiments/keyframe_interval_settings.h" -#include "api/transport/field_trial_based_config.h" +#include "api/field_trials_view.h" namespace webrtc { @@ -21,15 +21,10 @@ constexpr char kFieldTrialName[] = "WebRTC-KeyframeInterval"; } // namespace KeyframeIntervalSettings::KeyframeIntervalSettings( - const FieldTrialsView* const key_value_config) + const FieldTrialsView& key_value_config) : min_keyframe_send_interval_ms_("min_keyframe_send_interval_ms") { ParseFieldTrial({&min_keyframe_send_interval_ms_}, - key_value_config->Lookup(kFieldTrialName)); -} - -KeyframeIntervalSettings KeyframeIntervalSettings::ParseFromFieldTrials() { - FieldTrialBasedConfig field_trial_config; - return KeyframeIntervalSettings(&field_trial_config); + key_value_config.Lookup(kFieldTrialName)); } absl::optional KeyframeIntervalSettings::MinKeyframeSendIntervalMs() diff --git a/rtc_base/experiments/keyframe_interval_settings.h b/rtc_base/experiments/keyframe_interval_settings.h index aff7854516..df1f679bac 100644 --- a/rtc_base/experiments/keyframe_interval_settings.h +++ b/rtc_base/experiments/keyframe_interval_settings.h @@ -17,20 +17,18 @@ namespace webrtc { -// TODO(bugs.webrtc.org/10427): Remove and replace with proper configuration +// TODO: bugs.webrtc.org/42220470 - Remove and replace with proper configuration // parameter, or move to using FIR if intent is to avoid triggering multiple // times to PLIs corresponding to the same request when RTT is large. class KeyframeIntervalSettings final { public: - static KeyframeIntervalSettings ParseFromFieldTrials(); + explicit KeyframeIntervalSettings(const FieldTrialsView& key_value_config); // Sender side. // The encoded keyframe send rate is <= 1/MinKeyframeSendIntervalMs(). absl::optional MinKeyframeSendIntervalMs() const; private: - explicit KeyframeIntervalSettings(const FieldTrialsView* key_value_config); - FieldTrialOptional min_keyframe_send_interval_ms_; }; diff --git a/rtc_base/experiments/keyframe_interval_settings_unittest.cc b/rtc_base/experiments/keyframe_interval_settings_unittest.cc index 25cebbcd70..6aca483df0 100644 --- a/rtc_base/experiments/keyframe_interval_settings_unittest.cc +++ b/rtc_base/experiments/keyframe_interval_settings_unittest.cc @@ -10,33 +10,29 @@ #include "rtc_base/experiments/keyframe_interval_settings.h" -#include "test/field_trial.h" +#include "test/explicit_key_value_config.h" #include "test/gtest.h" namespace webrtc { namespace { +using test::ExplicitKeyValueConfig; + TEST(KeyframeIntervalSettingsTest, ParsesMinKeyframeSendIntervalMs) { - EXPECT_FALSE(KeyframeIntervalSettings::ParseFromFieldTrials() + EXPECT_FALSE(KeyframeIntervalSettings(ExplicitKeyValueConfig("")) .MinKeyframeSendIntervalMs()); - test::ScopedFieldTrials field_trials( + ExplicitKeyValueConfig field_trials( "WebRTC-KeyframeInterval/min_keyframe_send_interval_ms:100/"); - EXPECT_EQ(KeyframeIntervalSettings::ParseFromFieldTrials() - .MinKeyframeSendIntervalMs(), + EXPECT_EQ(KeyframeIntervalSettings(field_trials).MinKeyframeSendIntervalMs(), 100); } TEST(KeyframeIntervalSettingsTest, DoesNotParseIncorrectValues) { - EXPECT_FALSE(KeyframeIntervalSettings::ParseFromFieldTrials() - .MinKeyframeSendIntervalMs()); - - test::ScopedFieldTrials field_trials( + ExplicitKeyValueConfig field_trials( "WebRTC-KeyframeInterval/min_keyframe_send_interval_ms:a/"); - EXPECT_FALSE(KeyframeIntervalSettings::ParseFromFieldTrials() - .MinKeyframeSendIntervalMs()); - EXPECT_FALSE(KeyframeIntervalSettings::ParseFromFieldTrials() - .MinKeyframeSendIntervalMs()); + EXPECT_FALSE( + KeyframeIntervalSettings(field_trials).MinKeyframeSendIntervalMs()); } } // namespace diff --git a/video/encoder_rtcp_feedback.cc b/video/encoder_rtcp_feedback.cc index c9f3f735e1..d9d9aea2f0 100644 --- a/video/encoder_rtcp_feedback.cc +++ b/video/encoder_rtcp_feedback.cc @@ -14,9 +14,11 @@ #include #include "absl/types/optional.h" +#include "api/environment/environment.h" #include "api/video_codecs/video_encoder.h" #include "rtc_base/checks.h" #include "rtc_base/experiments/keyframe_interval_settings.h" +#include "system_wrappers/include/clock.h" namespace webrtc { @@ -25,14 +27,14 @@ constexpr int kMinKeyframeSendIntervalMs = 300; } // namespace EncoderRtcpFeedback::EncoderRtcpFeedback( - Clock* clock, + const Environment& env, bool per_layer_keyframes, const std::vector& ssrcs, VideoStreamEncoderInterface* encoder, std::function( uint32_t ssrc, const std::vector& seq_nums)> get_packet_infos) - : clock_(clock), + : env_(env), ssrcs_(ssrcs), per_layer_keyframes_(per_layer_keyframes), get_packet_infos_(std::move(get_packet_infos)), @@ -40,7 +42,7 @@ EncoderRtcpFeedback::EncoderRtcpFeedback( time_last_packet_delivery_queue_(per_layer_keyframes ? ssrcs.size() : 1, Timestamp::Zero()), min_keyframe_send_interval_( - TimeDelta::Millis(KeyframeIntervalSettings::ParseFromFieldTrials() + TimeDelta::Millis(KeyframeIntervalSettings(env_.field_trials()) .MinKeyframeSendIntervalMs() .value_or(kMinKeyframeSendIntervalMs))) { RTC_DCHECK(!ssrcs.empty()); @@ -60,7 +62,7 @@ void EncoderRtcpFeedback::OnReceivedIntraFrameRequest(uint32_t ssrc) { size_t ssrc_index = per_layer_keyframes_ ? std::distance(ssrcs_.begin(), it) : 0; RTC_CHECK_LE(ssrc_index, time_last_packet_delivery_queue_.size()); - const Timestamp now = clock_->CurrentTime(); + const Timestamp now = env_.clock().CurrentTime(); if (time_last_packet_delivery_queue_[ssrc_index] + min_keyframe_send_interval_ > now) diff --git a/video/encoder_rtcp_feedback.h b/video/encoder_rtcp_feedback.h index 4b92bd2c74..29832dbd6d 100644 --- a/video/encoder_rtcp_feedback.h +++ b/video/encoder_rtcp_feedback.h @@ -13,13 +13,13 @@ #include #include +#include "api/environment/environment.h" #include "api/sequence_checker.h" #include "api/units/time_delta.h" #include "api/units/timestamp.h" #include "call/rtp_video_sender_interface.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "rtc_base/system/no_unique_address.h" -#include "system_wrappers/include/clock.h" #include "video/video_stream_encoder_interface.h" namespace webrtc { @@ -32,7 +32,7 @@ class EncoderRtcpFeedback : public RtcpIntraFrameObserver, public RtcpLossNotificationObserver { public: EncoderRtcpFeedback( - Clock* clock, + const Environment& env, bool per_layer_keyframes, const std::vector& ssrcs, VideoStreamEncoderInterface* encoder, @@ -50,7 +50,7 @@ class EncoderRtcpFeedback : public RtcpIntraFrameObserver, bool decodability_flag) override; private: - Clock* const clock_; + const Environment env_; const std::vector ssrcs_; const bool per_layer_keyframes_; const std::function( diff --git a/video/encoder_rtcp_feedback_unittest.cc b/video/encoder_rtcp_feedback_unittest.cc index fdece629e6..62140e81a8 100644 --- a/video/encoder_rtcp_feedback_unittest.cc +++ b/video/encoder_rtcp_feedback_unittest.cc @@ -12,6 +12,7 @@ #include +#include "api/environment/environment_factory.h" #include "test/gmock.h" #include "test/gtest.h" #include "video/test/mock_video_stream_encoder.h" @@ -27,7 +28,7 @@ class VideoEncoderFeedbackKeyframeTestBase : public ::testing::Test { std::vector ssrcs) : simulated_clock_(123456789), encoder_(), - encoder_rtcp_feedback_(&simulated_clock_, + encoder_rtcp_feedback_(CreateEnvironment(&simulated_clock_), per_layer_pli_handling, ssrcs, &encoder_, diff --git a/video/video_send_stream_impl.cc b/video/video_send_stream_impl.cc index e4ee2e5342..682183ea09 100644 --- a/video/video_send_stream_impl.cc +++ b/video/video_send_stream_impl.cc @@ -418,7 +418,7 @@ VideoSendStreamImpl::VideoSendStreamImpl( metronome, config_.encoder_selector)), encoder_feedback_( - &env_.clock(), + env_, SupportsPerLayerPictureLossIndication( encoder_config.video_format.parameters), config_.rtp.ssrcs,