Skip to content

Commit

Permalink
Query WebRTC-KeyframeInterval through propagated field trials
Browse files Browse the repository at this point in the history
Bug: webrtc:42220378
Change-Id: I3556ec6b280bf6c03e6c3a20949a19e182eed2b8
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/349640
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42349}
  • Loading branch information
DanilChapovalov authored and WebRTC LUCI CQ committed May 20, 2024
1 parent f317f71 commit b57178b
Show file tree
Hide file tree
Showing 8 changed files with 26 additions and 35 deletions.
1 change: 0 additions & 1 deletion rtc_base/experiments/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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" ]
}
Expand Down
11 changes: 3 additions & 8 deletions rtc_base/experiments/keyframe_interval_settings.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand All @@ -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<int> KeyframeIntervalSettings::MinKeyframeSendIntervalMs()
Expand Down
6 changes: 2 additions & 4 deletions rtc_base/experiments/keyframe_interval_settings.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<int> MinKeyframeSendIntervalMs() const;

private:
explicit KeyframeIntervalSettings(const FieldTrialsView* key_value_config);

FieldTrialOptional<int> min_keyframe_send_interval_ms_;
};

Expand Down
22 changes: 9 additions & 13 deletions rtc_base/experiments/keyframe_interval_settings_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 6 additions & 4 deletions video/encoder_rtcp_feedback.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@
#include <utility>

#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 {

Expand All @@ -25,22 +27,22 @@ constexpr int kMinKeyframeSendIntervalMs = 300;
} // namespace

EncoderRtcpFeedback::EncoderRtcpFeedback(
Clock* clock,
const Environment& env,
bool per_layer_keyframes,
const std::vector<uint32_t>& ssrcs,
VideoStreamEncoderInterface* encoder,
std::function<std::vector<RtpSequenceNumberMap::Info>(
uint32_t ssrc,
const std::vector<uint16_t>& 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)),
video_stream_encoder_(encoder),
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());
Expand All @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions video/encoder_rtcp_feedback.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@
#include <functional>
#include <vector>

#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 {
Expand All @@ -32,7 +32,7 @@ class EncoderRtcpFeedback : public RtcpIntraFrameObserver,
public RtcpLossNotificationObserver {
public:
EncoderRtcpFeedback(
Clock* clock,
const Environment& env,
bool per_layer_keyframes,
const std::vector<uint32_t>& ssrcs,
VideoStreamEncoderInterface* encoder,
Expand All @@ -50,7 +50,7 @@ class EncoderRtcpFeedback : public RtcpIntraFrameObserver,
bool decodability_flag) override;

private:
Clock* const clock_;
const Environment env_;
const std::vector<uint32_t> ssrcs_;
const bool per_layer_keyframes_;
const std::function<std::vector<RtpSequenceNumberMap::Info>(
Expand Down
3 changes: 2 additions & 1 deletion video/encoder_rtcp_feedback_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

#include <memory>

#include "api/environment/environment_factory.h"
#include "test/gmock.h"
#include "test/gtest.h"
#include "video/test/mock_video_stream_encoder.h"
Expand All @@ -27,7 +28,7 @@ class VideoEncoderFeedbackKeyframeTestBase : public ::testing::Test {
std::vector<uint32_t> ssrcs)
: simulated_clock_(123456789),
encoder_(),
encoder_rtcp_feedback_(&simulated_clock_,
encoder_rtcp_feedback_(CreateEnvironment(&simulated_clock_),
per_layer_pli_handling,
ssrcs,
&encoder_,
Expand Down
2 changes: 1 addition & 1 deletion video/video_send_stream_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ VideoSendStreamImpl::VideoSendStreamImpl(
metronome,
config_.encoder_selector)),
encoder_feedback_(
&env_.clock(),
env_,
SupportsPerLayerPictureLossIndication(
encoder_config.video_format.parameters),
config_.rtp.ssrcs,
Expand Down

0 comments on commit b57178b

Please sign in to comment.