WebRTC-DeprecateGlobalFieldTrialString/Enabled/ - part 16/inf
This cl/ adds the feature actually injecting a FieldTrialsView into
PeerConnectionFactory, or into a PeerConnection or both.
The field trials used for a PeerConnection is those specified in
PeerConnectionDependencies. Otherwise will those from
PeerConnectionFactoryDependencies be used (and until we're finished with
this conversion, the global string fallback is used as last resort).
Note that it is currently not possible to create 2 FieldTrials
objects concurrently...due to global string,
so this cl/ is mostly (but entirely) for show, i.e one _can_
realistically inject them into a PeerConnectionFactory.
Bug: webrtc:10335
Change-Id: Id2e60525f48a1f8293c1dd0be771e3ed03790963
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/258134
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Jonas Oreland <jonaso@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36578}
diff --git a/api/create_peerconnection_factory.cc b/api/create_peerconnection_factory.cc
index c41b6d6..4a01b2f 100644
--- a/api/create_peerconnection_factory.cc
+++ b/api/create_peerconnection_factory.cc
@@ -38,7 +38,8 @@
std::unique_ptr<VideoDecoderFactory> video_decoder_factory,
rtc::scoped_refptr<AudioMixer> audio_mixer,
rtc::scoped_refptr<AudioProcessing> audio_processing,
- AudioFrameProcessor* audio_frame_processor) {
+ AudioFrameProcessor* audio_frame_processor,
+ std::unique_ptr<FieldTrialsView> field_trials) {
PeerConnectionFactoryDependencies dependencies;
dependencies.network_thread = network_thread;
dependencies.worker_thread = worker_thread;
@@ -47,7 +48,11 @@
dependencies.call_factory = CreateCallFactory();
dependencies.event_log_factory = std::make_unique<RtcEventLogFactory>(
dependencies.task_queue_factory.get());
- dependencies.trials = std::make_unique<webrtc::FieldTrialBasedConfig>();
+ if (field_trials) {
+ dependencies.trials = std::move(field_trials);
+ } else {
+ dependencies.trials = std::make_unique<webrtc::FieldTrialBasedConfig>();
+ }
if (network_thread) {
// TODO(bugs.webrtc.org/13145): Add an rtc::SocketFactory* argument.
diff --git a/api/create_peerconnection_factory.h b/api/create_peerconnection_factory.h
index 4eb0a00..efebc5f 100644
--- a/api/create_peerconnection_factory.h
+++ b/api/create_peerconnection_factory.h
@@ -49,7 +49,8 @@
std::unique_ptr<VideoDecoderFactory> video_decoder_factory,
rtc::scoped_refptr<AudioMixer> audio_mixer,
rtc::scoped_refptr<AudioProcessing> audio_processing,
- AudioFrameProcessor* audio_frame_processor = nullptr);
+ AudioFrameProcessor* audio_frame_processor = nullptr,
+ std::unique_ptr<FieldTrialsView> field_trials = nullptr);
} // namespace webrtc
diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h
index 5f1f099..c326799 100644
--- a/api/peer_connection_interface.h
+++ b/api/peer_connection_interface.h
@@ -1398,6 +1398,9 @@
std::unique_ptr<rtc::SSLCertificateVerifier> tls_cert_verifier;
std::unique_ptr<webrtc::VideoBitrateAllocatorFactory>
video_bitrate_allocator_factory;
+ // Optional field trials to use.
+ // Overrides those from PeerConnectionFactoryDependencies.
+ std::unique_ptr<FieldTrialsView> trials;
};
// PeerConnectionFactoryDependencies holds all of the PeerConnectionFactory
diff --git a/pc/connection_context.cc b/pc/connection_context.cc
index 736a6d7..d7c80eb 100644
--- a/pc/connection_context.cc
+++ b/pc/connection_context.cc
@@ -145,7 +145,7 @@
// If network_monitor_factory_ is non-null, it will be used to create a
// network monitor while on the network thread.
default_network_manager_ = std::make_unique<rtc::BasicNetworkManager>(
- network_monitor_factory_.get(), socket_factory, &trials());
+ network_monitor_factory_.get(), socket_factory, &field_trials());
default_socket_factory_ =
std::make_unique<rtc::BasicPacketSocketFactory>(socket_factory);
diff --git a/pc/connection_context.h b/pc/connection_context.h
index 3b8ac07..e4aa715 100644
--- a/pc/connection_context.h
+++ b/pc/connection_context.h
@@ -75,7 +75,11 @@
rtc::Thread* network_thread() { return network_thread_; }
const rtc::Thread* network_thread() const { return network_thread_; }
- const FieldTrialsView& trials() const { return *trials_.get(); }
+ // Field trials associated with the PeerConnectionFactory.
+ // Note: that there can be different field trials for different
+ // PeerConnections (but they are not supposed change after creating the
+ // PeerConnection).
+ const FieldTrialsView& field_trials() const { return *trials_.get(); }
// Accessors only used from the PeerConnectionFactory class
rtc::BasicNetworkManager* default_network_manager() {
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 0705c84..e789179 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -509,6 +509,7 @@
PeerConnectionDependencies& dependencies,
bool dtls_enabled)
: context_(context),
+ trials_(std::move(dependencies.trials), &context->field_trials()),
options_(options),
observer_(dependencies.observer),
is_unified_plan_(is_unified_plan),
@@ -719,7 +720,7 @@
}
};
- config.field_trials = &context_->trials();
+ config.field_trials = trials_.get();
transport_controller_.reset(
new JsepTransportController(network_thread(), port_allocator_.get(),
@@ -1692,8 +1693,7 @@
bool PeerConnection::StartRtcEventLog(
std::unique_ptr<RtcEventLogOutput> output) {
int64_t output_period_ms = webrtc::RtcEventLog::kImmediateOutput;
- if (absl::StartsWith(context_->trials().Lookup("WebRTC-RtcEventLogNewFormat"),
- "Enabled")) {
+ if (trials().IsEnabled("WebRTC-RtcEventLogNewFormat")) {
output_period_ms = 5000;
}
return StartRtcEventLog(std::move(output), output_period_ms);
@@ -2045,8 +2045,7 @@
// by experiment.
if (configuration.disable_ipv6) {
port_allocator_flags &= ~(cricket::PORTALLOCATOR_ENABLE_IPV6);
- } else if (absl::StartsWith(context_->trials().Lookup("WebRTC-IPv6Default"),
- "Disabled")) {
+ } else if (trials().IsDisabled("WebRTC-IPv6Default")) {
port_allocator_flags &= ~(cricket::PORTALLOCATOR_ENABLE_IPV6);
}
if (configuration.disable_ipv6_on_wifi) {
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index 919d377..84a8810 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -437,7 +437,7 @@
}
void RequestUsagePatternReportForTesting();
- const FieldTrialsView& trials() override { return context_->trials(); }
+ const FieldTrialsView& trials() const override { return *trials_; }
protected:
// Available for rtc::scoped_refptr creation
@@ -592,6 +592,12 @@
InitializeRtcpCallback();
const rtc::scoped_refptr<ConnectionContext> context_;
+ // Field trials active for this PeerConnection is the first of:
+ // a) Specified in PeerConnectionDependencies (owned).
+ // b) Accessed via ConnectionContext (e.g PeerConnectionFactoryDependencies>
+ // c) Created as Default (FieldTrialBasedConfig).
+ const webrtc::AlwaysValidPointer<const FieldTrialsView, FieldTrialBasedConfig>
+ trials_;
const PeerConnectionFactoryInterface::Options options_;
PeerConnectionObserver* observer_ RTC_GUARDED_BY(signaling_thread()) =
nullptr;
diff --git a/pc/peer_connection_factory.cc b/pc/peer_connection_factory.cc
index c598440..aa9f328 100644
--- a/pc/peer_connection_factory.cc
+++ b/pc/peer_connection_factory.cc
@@ -243,9 +243,12 @@
worker_thread()->Invoke<std::unique_ptr<RtcEventLog>>(
RTC_FROM_HERE, [this] { return CreateRtcEventLog_w(); });
+ const FieldTrialsView* trials =
+ dependencies.trials ? dependencies.trials.get() : &field_trials();
std::unique_ptr<Call> call = worker_thread()->Invoke<std::unique_ptr<Call>>(
- RTC_FROM_HERE,
- [this, &event_log] { return CreateCall_w(event_log.get()); });
+ RTC_FROM_HERE, [this, &event_log, trials] {
+ return CreateCall_w(event_log.get(), *trials);
+ });
auto result = PeerConnection::Create(context_, options_, std::move(event_log),
std::move(call), configuration,
@@ -307,7 +310,8 @@
}
std::unique_ptr<Call> PeerConnectionFactory::CreateCall_w(
- RtcEventLog* event_log) {
+ RtcEventLog* event_log,
+ const FieldTrialsView& field_trials) {
RTC_DCHECK_RUN_ON(worker_thread());
webrtc::Call::Config call_config(event_log, network_thread());
@@ -324,7 +328,7 @@
FieldTrialParameter<DataRate> max_bandwidth("max",
DataRate::KilobitsPerSec(2000));
ParseFieldTrial({&min_bandwidth, &start_bandwidth, &max_bandwidth},
- trials().Lookup("WebRTC-PcFactoryDefaultBitrates"));
+ field_trials.Lookup("WebRTC-PcFactoryDefaultBitrates"));
call_config.bitrate_config.min_bitrate_bps =
rtc::saturated_cast<int>(min_bandwidth->bps());
@@ -347,7 +351,7 @@
RTC_LOG(LS_INFO) << "Using default network controller factory";
}
- call_config.trials = &trials();
+ call_config.trials = &field_trials;
call_config.rtp_transport_controller_send_factory =
transport_controller_send_factory_.get();
call_config.metronome = metronome_.get();
@@ -356,7 +360,7 @@
}
bool PeerConnectionFactory::IsTrialEnabled(absl::string_view key) const {
- return absl::StartsWith(trials().Lookup(key), "Enabled");
+ return absl::StartsWith(field_trials().Lookup(key), "Enabled");
}
} // namespace webrtc
diff --git a/pc/peer_connection_factory.h b/pc/peer_connection_factory.h
index 4ee102a..16695f2 100644
--- a/pc/peer_connection_factory.h
+++ b/pc/peer_connection_factory.h
@@ -115,7 +115,9 @@
return options_;
}
- const FieldTrialsView& trials() const { return context_->trials(); }
+ const FieldTrialsView& field_trials() const {
+ return context_->field_trials();
+ }
protected:
// Constructor used by the static Create() method. Modifies the dependencies.
@@ -138,7 +140,8 @@
}
std::unique_ptr<RtcEventLog> CreateRtcEventLog_w();
- std::unique_ptr<Call> CreateCall_w(RtcEventLog* event_log);
+ std::unique_ptr<Call> CreateCall_w(RtcEventLog* event_log,
+ const FieldTrialsView& field_trials);
rtc::scoped_refptr<ConnectionContext> context_;
PeerConnectionFactoryInterface::Options options_
diff --git a/pc/peer_connection_internal.h b/pc/peer_connection_internal.h
index 66897ee..762f9b1 100644
--- a/pc/peer_connection_internal.h
+++ b/pc/peer_connection_internal.h
@@ -123,6 +123,8 @@
virtual void TeardownDataChannelTransport_n() = 0;
virtual void SetSctpDataMid(const std::string& mid) = 0;
virtual void ResetSctpDataMid() = 0;
+
+ virtual const FieldTrialsView& trials() const = 0;
};
// Functions defined in this class are called by other objects,
@@ -178,8 +180,6 @@
virtual void NoteDataAddedEvent() {}
// Handler for the "channel closed" signal
virtual void OnSctpDataChannelClosed(DataChannelInterface* channel) {}
-
- virtual const FieldTrialsView& trials() = 0;
};
} // namespace webrtc
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc
index d942548..1ad793c 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -1201,7 +1201,7 @@
// Use 100 kbps as the default minimum screencast bitrate unless this path is
// kill-switched.
if (!video_options_.screencast_min_bitrate_kbps.has_value() &&
- !context_->trials().IsEnabled(kDefaultScreencastMinBitrateKillSwitch)) {
+ !pc_->trials().IsEnabled(kDefaultScreencastMinBitrateKillSwitch)) {
video_options_.screencast_min_bitrate_kbps = 100;
}
audio_options_.combined_audio_video_bwe =
@@ -1235,7 +1235,8 @@
[this](const rtc::scoped_refptr<rtc::RTCCertificate>& certificate) {
RTC_DCHECK_RUN_ON(signaling_thread());
transport_controller_s()->SetLocalCertificate(certificate);
- });
+ },
+ pc_->trials());
if (pc_->options()->disable_encryption) {
webrtc_session_desc_factory_->SetSdesPolicy(cricket::SEC_DISABLED);
diff --git a/pc/test/fake_peer_connection_base.h b/pc/test/fake_peer_connection_base.h
index c886305..f629f04 100644
--- a/pc/test/fake_peer_connection_base.h
+++ b/pc/test/fake_peer_connection_base.h
@@ -360,7 +360,7 @@
void SetSctpDataMid(const std::string& mid) override {}
void ResetSctpDataMid() override {}
- const FieldTrialsView& trials() override { return field_trials_; }
+ const FieldTrialsView& trials() const override { return field_trials_; }
protected:
webrtc::test::ScopedKeyValueConfig field_trials_;
diff --git a/pc/webrtc_session_description_factory.cc b/pc/webrtc_session_description_factory.cc
index 09f2ee2..7ccc3bf 100644
--- a/pc/webrtc_session_description_factory.cc
+++ b/pc/webrtc_session_description_factory.cc
@@ -135,9 +135,10 @@
std::unique_ptr<rtc::RTCCertificateGeneratorInterface> cert_generator,
const rtc::scoped_refptr<rtc::RTCCertificate>& certificate,
std::function<void(const rtc::scoped_refptr<rtc::RTCCertificate>&)>
- on_certificate_ready)
+ on_certificate_ready,
+ const FieldTrialsView& field_trials)
: signaling_thread_(context->signaling_thread()),
- transport_desc_factory_(context->trials()),
+ transport_desc_factory_(field_trials),
session_desc_factory_(context->channel_manager(),
&transport_desc_factory_),
// RFC 4566 suggested a Network Time Protocol (NTP) format timestamp
diff --git a/pc/webrtc_session_description_factory.h b/pc/webrtc_session_description_factory.h
index 79171f7..3f15190 100644
--- a/pc/webrtc_session_description_factory.h
+++ b/pc/webrtc_session_description_factory.h
@@ -86,7 +86,8 @@
std::unique_ptr<rtc::RTCCertificateGeneratorInterface> cert_generator,
const rtc::scoped_refptr<rtc::RTCCertificate>& certificate,
std::function<void(const rtc::scoped_refptr<rtc::RTCCertificate>&)>
- on_certificate_ready);
+ on_certificate_ready,
+ const FieldTrialsView& field_trials);
virtual ~WebRtcSessionDescriptionFactory();
WebRtcSessionDescriptionFactory(const WebRtcSessionDescriptionFactory&) =
diff --git a/rtc_base/memory/always_valid_pointer.h b/rtc_base/memory/always_valid_pointer.h
index c6e0a70..a878083 100644
--- a/rtc_base/memory/always_valid_pointer.h
+++ b/rtc_base/memory/always_valid_pointer.h
@@ -37,6 +37,36 @@
RTC_DCHECK(pointer_);
}
+ // Create a pointer by
+ // a) taking over ownership of |instance|
+ // b) or fallback to |pointer|, without taking ownership.
+ // c) or Default.
+ AlwaysValidPointer(std::unique_ptr<Interface>&& instance, Interface* pointer)
+ : owned_instance_(
+ instance
+ ? std::move(instance)
+ : (pointer == nullptr ? std::make_unique<Default>() : nullptr)),
+ pointer_(owned_instance_ ? owned_instance_.get() : pointer) {
+ RTC_DCHECK(pointer_);
+ }
+
+ // Create a pointer by
+ // a) taking over ownership of |instance|
+ // b) or fallback to |pointer|, without taking ownership.
+ // c) or Default (with forwarded args).
+ template <typename... Args>
+ AlwaysValidPointer(std::unique_ptr<Interface>&& instance,
+ Interface* pointer,
+ Args... args)
+ : owned_instance_(
+ instance ? std::move(instance)
+ : (pointer == nullptr
+ ? std::make_unique<Default>(std::move(args...))
+ : nullptr)),
+ pointer_(owned_instance_ ? owned_instance_.get() : pointer) {
+ RTC_DCHECK(pointer_);
+ }
+
Interface* get() { return pointer_; }
Interface* operator->() { return pointer_; }
Interface& operator*() { return *pointer_; }
diff --git a/rtc_base/memory/always_valid_pointer_unittest.cc b/rtc_base/memory/always_valid_pointer_unittest.cc
index dbb0671..92cf56b 100644
--- a/rtc_base/memory/always_valid_pointer_unittest.cc
+++ b/rtc_base/memory/always_valid_pointer_unittest.cc
@@ -46,4 +46,42 @@
EXPECT_EQ(*ptr, "keso");
}
+TEST(AlwaysValidPointerTest, TakeOverOwnershipOfInstance) {
+ std::string str("keso");
+ std::unique_ptr<std::string> str2 = std::make_unique<std::string>("kent");
+ AlwaysValidPointer<std::string> ptr(std::move(str2), &str);
+ EXPECT_EQ(*ptr, "kent");
+ EXPECT_EQ(str2, nullptr);
+}
+
+TEST(AlwaysValidPointerTest, TakeOverOwnershipFallbackOnPointer) {
+ std::string str("keso");
+ std::unique_ptr<std::string> str2;
+ AlwaysValidPointer<std::string> ptr(std::move(str2), &str);
+ EXPECT_EQ(*ptr, "keso");
+}
+
+TEST(AlwaysValidPointerTest, TakeOverOwnershipFallbackOnDefault) {
+ std::unique_ptr<std::string> str;
+ std::string* str_ptr = nullptr;
+ AlwaysValidPointer<std::string> ptr(std::move(str), str_ptr);
+ EXPECT_EQ(*ptr, "");
+}
+
+TEST(AlwaysValidPointerTest,
+ TakeOverOwnershipFallbackOnDefaultWithForwardedArgument) {
+ std::unique_ptr<std::string> str2;
+ AlwaysValidPointer<std::string> ptr(std::move(str2), nullptr, "keso");
+ EXPECT_EQ(*ptr, "keso");
+}
+
+TEST(AlwaysValidPointerTest, TakeOverOwnershipDoesNotForwardDefaultArguments) {
+ std::unique_ptr<std::string> str = std::make_unique<std::string>("kalle");
+ std::unique_ptr<std::string> str2 = std::make_unique<std::string>("anka");
+ AlwaysValidPointer<std::string> ptr(std::move(str), nullptr, *str2);
+ EXPECT_EQ(*ptr, "kalle");
+ EXPECT_TRUE(!str);
+ EXPECT_EQ(*str2, "anka");
+}
+
} // namespace webrtc
