Remove Environment from ConnectionContext
and make call sites that used it be explicit about their environments.
The reason is that ConnectionContext can be shared between PeerConnetions,
while Environment can be different between PeerConnections.
Bug: None
Change-Id: I91d70f2045548a972964d3eea391107c05d674b0
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/410140
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Jonas Oreland <jonaso@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#45679}
diff --git a/pc/connection_context.cc b/pc/connection_context.cc
index f47b8d8..486f054 100644
--- a/pc/connection_context.cc
+++ b/pc/connection_context.cc
@@ -101,10 +101,9 @@
}),
signaling_thread_(MaybeWrapThread(dependencies->signaling_thread,
wraps_current_thread_)),
- env_(env),
media_engine_(
dependencies->media_factory != nullptr
- ? dependencies->media_factory->CreateMediaEngine(env_,
+ ? dependencies->media_factory->CreateMediaEngine(env,
*dependencies)
: nullptr),
network_monitor_factory_(
diff --git a/pc/connection_context.h b/pc/connection_context.h
index d54b419..b8f223b 100644
--- a/pc/connection_context.h
+++ b/pc/connection_context.h
@@ -64,17 +64,12 @@
Thread* network_thread() { return network_thread_; }
const Thread* network_thread() const { return network_thread_; }
- // Environment associated with the PeerConnectionFactory.
- // Note: environments are different for different PeerConnections,
- // but they are not supposed to change after creating the PeerConnection.
- const Environment& env() const { return env_; }
-
// Accessors only used from the PeerConnectionFactory class
- NetworkManager* default_network_manager() {
+ NetworkManager* default_network_manager() const {
RTC_DCHECK_RUN_ON(signaling_thread_);
return default_network_manager_.get();
}
- PacketSocketFactory* default_socket_factory() {
+ PacketSocketFactory* default_socket_factory() const {
RTC_DCHECK_RUN_ON(signaling_thread_);
return default_socket_factory_.get();
}
@@ -87,7 +82,7 @@
// use RTX, but so far, no code has been found that sets it to false.
// Kept in the API in order to ease introduction if we want to resurrect
// the functionality.
- bool use_rtx() { return use_rtx_; }
+ bool use_rtx() const { return use_rtx_; }
// For use by tests.
void set_use_rtx(bool use_rtx) { use_rtx_ = use_rtx; }
@@ -111,8 +106,6 @@
AlwaysValidPointer<Thread> const worker_thread_;
Thread* const signaling_thread_;
- const Environment env_;
-
// This object is const over the lifetime of the ConnectionContext, and is
// only altered in the destructor.
std::unique_ptr<MediaEngineInterface> media_engine_;
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 2836661..58884a2 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -131,7 +131,7 @@
: self_(self),
codec_vendor_(self_->context()->media_engine(),
self_->context()->use_rtx(),
- self_->context()->env().field_trials()) {}
+ self_->trials()) {}
webrtc::PayloadTypeSuggester* PayloadTypeSuggester() override {
return self_->transport_controller_s();
@@ -746,9 +746,8 @@
config.rtcp_mux_policy = configuration.rtcp_mux_policy;
config.crypto_options = configuration.crypto_options;
- // Maybe enable PQC from FieldTrials
- config.crypto_options.ephemeral_key_exchange_cipher_groups.Update(
- &context_->env().field_trials());
+ // Maybe enable or disable PQC from FieldTrials
+ config.crypto_options.ephemeral_key_exchange_cipher_groups.Update(&trials());
config.transport_observer = this;
config.rtcp_handler = InitializeRtcpCallback();
config.un_demuxable_packet_handler = InitializeUnDemuxablePacketHandler();
@@ -1143,8 +1142,7 @@
std::vector<Codec> codecs;
// Gather the current codec capabilities to allow checking scalabilityMode and
// codec selection against supported values.
- CodecVendor codec_vendor(context_->media_engine(), false,
- context_->env().field_trials());
+ CodecVendor codec_vendor(context_->media_engine(), false, trials());
if (media_type == webrtc::MediaType::VIDEO) {
codecs = codec_vendor.video_send_codecs().codecs();
} else {
diff --git a/pc/peer_connection_factory.cc b/pc/peer_connection_factory.cc
index a0b8848..bcd71be 100644
--- a/pc/peer_connection_factory.cc
+++ b/pc/peer_connection_factory.cc
@@ -87,21 +87,24 @@
// Static
scoped_refptr<PeerConnectionFactory> PeerConnectionFactory::Create(
PeerConnectionFactoryDependencies dependencies) {
- auto context = ConnectionContext::Create(AssembleEnvironment(dependencies),
- &dependencies);
+ Environment env = AssembleEnvironment(dependencies);
+ auto context = ConnectionContext::Create(env, &dependencies);
if (!context) {
return nullptr;
}
- return make_ref_counted<PeerConnectionFactory>(context, &dependencies);
+ return make_ref_counted<PeerConnectionFactory>(env, context, &dependencies);
}
PeerConnectionFactory::PeerConnectionFactory(
+ Environment env,
scoped_refptr<ConnectionContext> context,
PeerConnectionFactoryDependencies* dependencies)
- : context_(context),
+ : env_(env),
+ context_(context ? context
+ : ConnectionContext::Create(env_, dependencies)),
codec_vendor_(context_->media_engine(),
context_->use_rtx(),
- context_->env().field_trials()),
+ env_.field_trials()),
event_log_factory_(std::move(dependencies->event_log_factory)),
fec_controller_factory_(std::move(dependencies->fec_controller_factory)),
@@ -115,10 +118,9 @@
PeerConnectionFactory::PeerConnectionFactory(
PeerConnectionFactoryDependencies dependencies)
- : PeerConnectionFactory(
- ConnectionContext::Create(AssembleEnvironment(dependencies),
- &dependencies),
- &dependencies) {}
+ : PeerConnectionFactory(AssembleEnvironment(dependencies),
+ /* context= */ nullptr,
+ &dependencies) {}
PeerConnectionFactory::~PeerConnectionFactory() {
RTC_DCHECK_RUN_ON(signaling_thread());
@@ -239,7 +241,7 @@
"Attempt to create a PeerConnection without an observer");
}
- EnvironmentFactory env_factory(context_->env());
+ EnvironmentFactory env_factory(env_);
// Field trials active for this PeerConnection is the first of:
// a) Specified in the PeerConnectionDependencies
diff --git a/pc/peer_connection_factory.h b/pc/peer_connection_factory.h
index a368aee..00adac2 100644
--- a/pc/peer_connection_factory.h
+++ b/pc/peer_connection_factory.h
@@ -102,26 +102,26 @@
return options_;
}
- const FieldTrialsView& field_trials() const {
- return context_->env().field_trials();
- }
+ const FieldTrialsView& field_trials() const { return env_.field_trials(); }
MediaEngineInterface* media_engine() const;
CodecVendor& CodecVendorForTesting() { return codec_vendor_; }
protected:
// Constructor used by the static Create() method. Modifies the dependencies.
- PeerConnectionFactory(scoped_refptr<ConnectionContext> context,
+ PeerConnectionFactory(Environment env,
+ scoped_refptr<ConnectionContext> context,
PeerConnectionFactoryDependencies* dependencies);
- // Constructor for use in testing. Ignores the possibility of initialization
- // failure. The dependencies are passed in by std::move().
+ // Constructor for use in testing. The dependencies are passed in by
+ // std::move().
explicit PeerConnectionFactory(
PeerConnectionFactoryDependencies dependencies);
virtual ~PeerConnectionFactory();
private:
+ Environment env_;
Thread* network_thread() const { return context_->network_thread(); }
std::unique_ptr<Call> CreateCall_w(
diff --git a/pc/peer_connection_factory_unittest.cc b/pc/peer_connection_factory_unittest.cc
index 5630611..519ab3e 100644
--- a/pc/peer_connection_factory_unittest.cc
+++ b/pc/peer_connection_factory_unittest.cc
@@ -292,10 +292,12 @@
OpenH264DecoderTemplateAdapter, Dav1dDecoderTemplateAdapter>>(),
EnableMedia(pcf_dependencies);
+ Environment env = CreateEnvironment();
scoped_refptr<ConnectionContext> context =
- ConnectionContext::Create(CreateEnvironment(), &pcf_dependencies);
+ ConnectionContext::Create(env, &pcf_dependencies);
context->set_use_rtx(false);
- return make_ref_counted<PeerConnectionFactory>(context, &pcf_dependencies);
+ return make_ref_counted<PeerConnectionFactory>(env, context,
+ &pcf_dependencies);
}
// Verify creation of PeerConnection using internal ADM, video factory and
diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc
index 8f7cadd..c723327 100644
--- a/pc/rtp_transceiver.cc
+++ b/pc/rtp_transceiver.cc
@@ -839,7 +839,7 @@
RTC_DCHECK(content);
if (sdp_type == SdpType::kAnswer) {
negotiated_header_extensions_ = content->rtp_header_extensions();
- if (context_->env().field_trials().IsEnabled(
+ if (env_.field_trials().IsEnabled(
"WebRTC-HeaderExtensionNegotiateMemory")) {
header_extensions_to_negotiate_ = GetNegotiatedHeaderExtensions();
}
diff --git a/pc/rtp_transceiver_unittest.cc b/pc/rtp_transceiver_unittest.cc
index ef209d3..ad018c5 100644
--- a/pc/rtp_transceiver_unittest.cc
+++ b/pc/rtp_transceiver_unittest.cc
@@ -69,16 +69,13 @@
class RtpTransceiverTest : public testing::Test {
public:
RtpTransceiverTest()
- : dependencies_(MakeDependencies()),
- context_(
- ConnectionContext::Create(CreateTestEnvironment(), &dependencies_)),
- codec_lookup_helper_(context_.get()) {}
+ : env_(CreateTestEnvironment()),
+ dependencies_(MakeDependencies()),
+ context_(ConnectionContext::Create(env_, &dependencies_)),
+ codec_lookup_helper_(context_.get(), env_.field_trials()) {}
protected:
- // For the test purpose reuse environment from the Connection Context.
- // Note that in prod code such environment is per PeerConnectionFactory,
- // while RtpTransceiver expects PeerConnection specific environment.
- const Environment& env() const { return context_->env(); }
+ const Environment& env() const { return env_; }
FakeMediaEngine* media_engine() {
// We know this cast is safe because we supplied the fake implementation
// in MakeDependencies().
@@ -102,6 +99,7 @@
return d;
}
+ Environment env_;
PeerConnectionFactoryDependencies dependencies_;
scoped_refptr<ConnectionContext> context_;
FakeCodecLookupHelper codec_lookup_helper_;
@@ -195,8 +193,7 @@
RtpReceiverProxyWithInternal<RtpReceiverInternal>::Create(
Thread::Current(), Thread::Current(), std::move(receiver)),
context(), codec_lookup_helper(),
- media_engine()->voice().GetRtpHeaderExtensions(
- &context()->env().field_trials()),
+ media_engine()->voice().GetRtpHeaderExtensions(&env().field_trials()),
/* on_negotiation_needed= */ [] {});
}
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc
index 9bf6557..3ac6bdb 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -4393,7 +4393,7 @@
MediaType::AUDIO, GetDefaultMidForPlanB(MediaType::AUDIO),
RtpTransceiverDirectionFromSendRecv(send_audio, recv_audio), false);
options.header_extensions =
- media_engine()->voice().GetRtpHeaderExtensions(&env_.field_trials());
+ media_engine()->voice().GetRtpHeaderExtensions(&pc_->trials());
session_options->media_description_options.push_back(options);
audio_index = session_options->media_description_options.size() - 1;
}
@@ -4402,7 +4402,7 @@
MediaType::VIDEO, GetDefaultMidForPlanB(MediaType::VIDEO),
RtpTransceiverDirectionFromSendRecv(send_video, recv_video), false);
options.header_extensions =
- media_engine()->video().GetRtpHeaderExtensions(&env_.field_trials());
+ media_engine()->video().GetRtpHeaderExtensions(&pc_->trials());
session_options->media_description_options.push_back(options);
video_index = session_options->media_description_options.size() - 1;
}
@@ -5536,7 +5536,7 @@
*audio_index = session_options->media_description_options.size() - 1;
}
session_options->media_description_options.back().header_extensions =
- media_engine()->voice().GetRtpHeaderExtensions(&env_.field_trials());
+ media_engine()->voice().GetRtpHeaderExtensions(&pc_->trials());
} else if (IsVideoContent(&content)) {
// If we already have an video m= section, reject this extra one.
if (*video_index) {
@@ -5552,7 +5552,7 @@
*video_index = session_options->media_description_options.size() - 1;
}
session_options->media_description_options.back().header_extensions =
- media_engine()->video().GetRtpHeaderExtensions(&env_.field_trials());
+ media_engine()->video().GetRtpHeaderExtensions(&pc_->trials());
} else if (IsUnsupportedContent(&content)) {
session_options->media_description_options.push_back(
MediaDescriptionOptions(MediaType::UNSUPPORTED, content.mid(),
diff --git a/pc/test/fake_codec_lookup_helper.h b/pc/test/fake_codec_lookup_helper.h
index 1a793e6..bbaaaef 100644
--- a/pc/test/fake_codec_lookup_helper.h
+++ b/pc/test/fake_codec_lookup_helper.h
@@ -22,12 +22,14 @@
class FakeCodecLookupHelper : public CodecLookupHelper {
public:
- explicit FakeCodecLookupHelper(ConnectionContext* context)
+ explicit FakeCodecLookupHelper(ConnectionContext* context,
+ const FieldTrialsView& field_trials)
: context_(context),
- codec_vendor_(std::make_unique<::webrtc::CodecVendor>(
- context->media_engine(),
- context->use_rtx(),
- context->env().field_trials())) {}
+ field_trials_(&field_trials),
+ codec_vendor_(
+ std::make_unique<::webrtc::CodecVendor>(context->media_engine(),
+ context->use_rtx(),
+ *field_trials_)) {}
webrtc::PayloadTypeSuggester* PayloadTypeSuggester() override {
// Not used in this test.
RTC_CHECK_NOTREACHED();
@@ -40,12 +42,12 @@
// result to show up in the codec vendor's output.
void Reset() {
codec_vendor_ = std::make_unique<::webrtc::CodecVendor>(
- context_->media_engine(), context_->use_rtx(),
- context_->env().field_trials());
+ context_->media_engine(), context_->use_rtx(), *field_trials_);
}
private:
- ConnectionContext* context_;
+ ConnectionContext* const context_;
+ const FieldTrialsView* absl_nonnull field_trials_;
std::unique_ptr<::webrtc::CodecVendor> codec_vendor_;
};
diff --git a/pc/test/fake_peer_connection_for_stats.h b/pc/test/fake_peer_connection_for_stats.h
index 888c63d..87ab786 100644
--- a/pc/test/fake_peer_connection_for_stats.h
+++ b/pc/test/fake_peer_connection_for_stats.h
@@ -244,13 +244,13 @@
worker_thread_(Thread::Current()),
signaling_thread_(Thread::Current()),
// TODO(hta): remove separate thread variables and use context.
+ env_(CreateEnvironment()),
dependencies_(MakeDependencies()),
- context_(
- ConnectionContext::Create(CreateEnvironment(), &dependencies_)),
+ context_(ConnectionContext::Create(env_, &dependencies_)),
local_streams_(StreamCollection::Create()),
remote_streams_(StreamCollection::Create()),
data_channel_controller_(network_thread_),
- codec_lookup_helper_(context_.get()) {}
+ codec_lookup_helper_(context_.get(), env_.field_trials()) {}
~FakePeerConnectionForStats() {
for (auto transceiver : transceivers_) {
@@ -545,9 +545,9 @@
scoped_refptr<RtpTransceiverProxyWithInternal<RtpTransceiver>>
CreateTransceiverOfType(MediaType media_type) {
auto transceiver = RtpTransceiverProxyWithInternal<RtpTransceiver>::Create(
- signaling_thread_, make_ref_counted<RtpTransceiver>(
- context_->env(), media_type, context_.get(),
- &codec_lookup_helper_));
+ signaling_thread_,
+ make_ref_counted<RtpTransceiver>(env_, media_type, context_.get(),
+ &codec_lookup_helper_));
transceiver->internal()->set_current_direction(
RtpTransceiverDirection::kSendRecv);
transceivers_.push_back(transceiver);
@@ -558,6 +558,7 @@
Thread* const worker_thread_;
Thread* const signaling_thread_;
+ Environment env_;
PeerConnectionFactoryDependencies dependencies_;
scoped_refptr<ConnectionContext> context_;
