PeerConnection construction now happens in one step
Update member variable order to more accurately reflect the actual
construction steps.
Remove the Initalize() method but InitializeNetworkThread() remains
and is now called from the constructor (as opposed to being called
from inside Create() post construction).
After this and the previous CL, the PeerConnection::Create method
requires parsed server information and validated parameters. The
prototype has also changed which means that anyone that might have
depended on the previous contract, will get a compilation error,
prompting them to update their code (i.e. not a silent change).
Bug: webrtc:398857495
Change-Id: Ic40a7b9a3bdca1161511be8675b664d38be381cc
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/379500
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#44026}
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 9446f2b..2d80f27 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -479,46 +479,28 @@
return !(*this == o);
}
-// TODO(bugs.webrtc.org/398857495): Change the `Create()` and `Initialize()`
-// methods together with `PeerConnectionFactory::CreatePeerConnectionOrError()`
-// so that:
-// * All configuration checking and setup of dependencies is done by the
-// factory. This includes the `ParseAndValidateIceServersFromConfiguration()`
-// step that's currently done in `Initialize()`.
-// * Either completely remove the `Create()` and `Initialize()` methods or
-// change them so that they return void (can't fail).
-// This means that a `PeerConnection` instance can be constructed simply by
-// using the constructor and the config error checking is moved out of the
-// class.
rtc::scoped_refptr<PeerConnection> PeerConnection::Create(
const Environment& env,
rtc::scoped_refptr<ConnectionContext> context,
const PeerConnectionFactoryInterface::Options& options,
std::unique_ptr<Call> call,
const PeerConnectionInterface::RTCConfiguration& configuration,
- PeerConnectionDependencies dependencies,
+ PeerConnectionDependencies& dependencies,
const cricket::ServerAddresses& stun_servers,
const std::vector<cricket::RelayServerConfig>& turn_servers) {
RTC_DCHECK(cricket::IceConfig(configuration).IsValid().ok());
RTC_DCHECK(dependencies.observer);
RTC_DCHECK(dependencies.async_dns_resolver_factory);
- RTC_CHECK(dependencies.allocator);
+ RTC_DCHECK(dependencies.allocator);
bool is_unified_plan =
configuration.sdp_semantics == SdpSemantics::kUnifiedPlan;
bool dtls_enabled = DtlsEnabled(configuration, options, dependencies);
- // Perf trace scope for the full initialization of a PeerConnection instance.
TRACE_EVENT0("webrtc", "PeerConnection::Create");
-
- // The PeerConnection constructor consumes some, but not all, dependencies.
- auto pc = rtc::make_ref_counted<PeerConnection>(
+ return rtc::make_ref_counted<PeerConnection>(
configuration, env, context, options, is_unified_plan, std::move(call),
- dependencies, dtls_enabled);
- pc->Initialize(std::move(dependencies.cert_generator),
- std::move(dependencies.video_bitrate_allocator_factory),
- stun_servers, turn_servers);
- return pc;
+ dependencies, stun_servers, turn_servers, dtls_enabled);
}
PeerConnection::PeerConnection(
@@ -529,12 +511,15 @@
bool is_unified_plan,
std::unique_ptr<Call> call,
PeerConnectionDependencies& dependencies,
+ const cricket::ServerAddresses& stun_servers,
+ const std::vector<cricket::RelayServerConfig>& turn_servers,
bool dtls_enabled)
: env_(env),
context_(context),
options_(options),
observer_(dependencies.observer),
is_unified_plan_(is_unified_plan),
+ dtls_enabled_(dtls_enabled),
configuration_(configuration),
async_dns_resolver_factory_(
std::move(dependencies.async_dns_resolver_factory)),
@@ -556,13 +541,46 @@
// Due to this constraint session id `session_id_` is max limited to
// LLONG_MAX.
session_id_(rtc::ToString(rtc::CreateRandomId64() & LLONG_MAX)),
- dtls_enabled_(dtls_enabled),
data_channel_controller_(this),
message_handler_(signaling_thread()),
weak_factory_(this) {
// Field trials specific to the peerconnection should be owned by the `env`,
RTC_DCHECK(dependencies.trials == nullptr);
+ transport_controller_copy_ =
+ InitializeNetworkThread(stun_servers, turn_servers);
+
+ if (call_ptr_) {
+ worker_thread()->BlockingCall([this, tc = transport_controller_copy_] {
+ RTC_DCHECK_RUN_ON(worker_thread());
+ call_->SetPayloadTypeSuggester(tc);
+ });
+ }
+
+ sdp_handler_ = SdpOfferAnswerHandler::Create(
+ this, configuration_, std::move(dependencies.cert_generator),
+ std::move(dependencies.video_bitrate_allocator_factory), context_.get(),
+ transport_controller_copy_);
+ rtp_manager_ = std::make_unique<RtpTransmissionManager>(
+ env_, IsUnifiedPlan(), context_.get(), &usage_pattern_, observer_,
+ legacy_stats_.get(), [this]() {
+ RTC_DCHECK_RUN_ON(signaling_thread());
+ sdp_handler_->UpdateNegotiationNeeded();
+ });
+ // Add default audio/video transceivers for Plan B SDP.
+ if (!IsUnifiedPlan()) {
+ rtp_manager_->transceivers()->Add(
+ RtpTransceiverProxyWithInternal<RtpTransceiver>::Create(
+ signaling_thread(),
+ rtc::make_ref_counted<RtpTransceiver>(cricket::MEDIA_TYPE_AUDIO,
+ context_.get())));
+ rtp_manager_->transceivers()->Add(
+ RtpTransceiverProxyWithInternal<RtpTransceiver>::Create(
+ signaling_thread(),
+ rtc::make_ref_counted<RtpTransceiver>(cricket::MEDIA_TYPE_VIDEO,
+ context_.get())));
+ }
+
const int delay_ms = configuration_.report_usage_pattern_delay_ms
? *configuration_.report_usage_pattern_delay_ms
: REPORT_USAGE_PATTERN_DELAY_MS;
@@ -633,61 +651,24 @@
data_channel_controller_.PrepareForShutdown();
}
-void PeerConnection::Initialize(
- std::unique_ptr<rtc::RTCCertificateGeneratorInterface> cert_generator,
- std::unique_ptr<webrtc::VideoBitrateAllocatorFactory>
- video_bitrate_allocator_factory,
+JsepTransportController* PeerConnection::InitializeNetworkThread(
const cricket::ServerAddresses& stun_servers,
const std::vector<cricket::RelayServerConfig>& turn_servers) {
RTC_DCHECK_RUN_ON(signaling_thread());
NoteServerUsage(usage_pattern_, stun_servers, turn_servers);
-
- // Network thread initialization.
- transport_controller_copy_ =
- network_thread()->BlockingCall([&, config = &configuration_] {
- RTC_DCHECK_RUN_ON(network_thread());
- RTC_DCHECK(network_thread_safety_->alive());
- InitializePortAllocatorResult pa_result =
- InitializePortAllocator_n(stun_servers, turn_servers, *config);
- // Send information about IPv4/IPv6 status.
- PeerConnectionAddressFamilyCounter address_family =
- pa_result.enable_ipv6 ? kPeerConnection_IPv6 : kPeerConnection_IPv4;
- RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.IPMetrics",
- address_family,
- kPeerConnectionAddressFamilyCounter_Max);
- return InitializeTransportController_n(*config);
- });
-
- if (call_ptr_) {
- worker_thread()->BlockingCall([this, tc = transport_controller_copy_] {
- RTC_DCHECK_RUN_ON(worker_thread());
- call_->SetPayloadTypeSuggester(tc);
- });
- }
-
- sdp_handler_ = SdpOfferAnswerHandler::Create(
- this, configuration_, std::move(cert_generator),
- std::move(video_bitrate_allocator_factory), context_.get(),
- transport_controller_copy_);
-
- rtp_manager_ = std::make_unique<RtpTransmissionManager>(
- env_, IsUnifiedPlan(), context_.get(), &usage_pattern_, observer_,
- legacy_stats_.get(), [this]() {
- RTC_DCHECK_RUN_ON(signaling_thread());
- sdp_handler_->UpdateNegotiationNeeded();
- });
- // Add default audio/video transceivers for Plan B SDP.
- if (!IsUnifiedPlan()) {
- rtp_manager_->transceivers()->Add(
- RtpTransceiverProxyWithInternal<RtpTransceiver>::Create(
- signaling_thread(), rtc::make_ref_counted<RtpTransceiver>(
- cricket::MEDIA_TYPE_AUDIO, context())));
- rtp_manager_->transceivers()->Add(
- RtpTransceiverProxyWithInternal<RtpTransceiver>::Create(
- signaling_thread(), rtc::make_ref_counted<RtpTransceiver>(
- cricket::MEDIA_TYPE_VIDEO, context())));
- }
+ return network_thread()->BlockingCall([&, config = &configuration_] {
+ RTC_DCHECK_RUN_ON(network_thread());
+ RTC_DCHECK(network_thread_safety_->alive());
+ InitializePortAllocatorResult pa_result =
+ InitializePortAllocator_n(stun_servers, turn_servers, *config);
+ // Send information about IPv4/IPv6 status.
+ PeerConnectionAddressFamilyCounter address_family =
+ pa_result.enable_ipv6 ? kPeerConnection_IPv6 : kPeerConnection_IPv4;
+ RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.IPMetrics", address_family,
+ kPeerConnectionAddressFamilyCounter_Max);
+ return InitializeTransportController_n(*config);
+ });
}
JsepTransportController* PeerConnection::InitializeTransportController_n(
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index 9049d87..fb0cb3d 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -120,7 +120,7 @@
const PeerConnectionFactoryInterface::Options& options,
std::unique_ptr<Call> call,
const PeerConnectionInterface::RTCConfiguration& configuration,
- PeerConnectionDependencies dependencies,
+ PeerConnectionDependencies& dependencies,
const cricket::ServerAddresses& stun_servers,
const std::vector<cricket::RelayServerConfig>& turn_servers);
@@ -472,15 +472,18 @@
bool is_unified_plan,
std::unique_ptr<Call> call,
PeerConnectionDependencies& dependencies,
+ const cricket::ServerAddresses& stun_servers,
+ const std::vector<cricket::RelayServerConfig>& turn_servers,
bool dtls_enabled);
~PeerConnection() override;
private:
- void Initialize(
- std::unique_ptr<rtc::RTCCertificateGeneratorInterface> cert_generator,
- std::unique_ptr<webrtc::VideoBitrateAllocatorFactory>
- video_bitrate_allocator_factory,
+ // Called from the constructor to apply the server configuration on the
+ // network thread and initialize network thread related state (see
+ // InitializeTransportController_n). The return value of this function is used
+ // to set the initial value of `transport_controller_copy_`.
+ JsepTransportController* InitializeNetworkThread(
const cricket::ServerAddresses& stun_servers,
const std::vector<cricket::RelayServerConfig>& turn_servers);
JsepTransportController* InitializeTransportController_n(
@@ -623,6 +626,8 @@
std::function<void(const RtpPacketReceived& parsed_packet)>
InitializeUnDemuxablePacketHandler();
+ bool CanAttemptDtlsStunPiggybacking(const RTCConfiguration& configuration);
+
const Environment env_;
const rtc::scoped_refptr<ConnectionContext> context_;
const PeerConnectionFactoryInterface::Options options_;
@@ -630,6 +635,12 @@
nullptr;
const bool is_unified_plan_;
+ const bool dtls_enabled_;
+ bool return_histogram_very_quickly_ RTC_GUARDED_BY(signaling_thread()) =
+ false;
+ // Did the connectionState ever change to `connected`?
+ // Used to gather metrics only the first such state change.
+ bool was_ever_connected_ RTC_GUARDED_BY(signaling_thread()) = false;
IceConnectionState ice_connection_state_ RTC_GUARDED_BY(signaling_thread()) =
kIceConnectionNew;
@@ -677,15 +688,6 @@
const std::string session_id_;
- // The transport controller is set and used on the network thread.
- // Some functions pass the value of the transport_controller_ pointer
- // around as arguments while running on the signaling thread; these
- // use the transport_controller_copy.
- std::unique_ptr<JsepTransportController> transport_controller_
- RTC_GUARDED_BY(network_thread());
- JsepTransportController* transport_controller_copy_
- RTC_GUARDED_BY(signaling_thread()) = nullptr;
-
// `sctp_mid_` is the content name (MID) in SDP.
// Note: this is used as the data channel MID by both SCTP and data channel
// transports. It is set when either transport is initialized and unset when
@@ -697,15 +699,7 @@
std::optional<std::string> sctp_mid_n_ RTC_GUARDED_BY(network_thread());
std::string sctp_transport_name_s_ RTC_GUARDED_BY(signaling_thread());
- // The machinery for handling offers and answers. Const after initialization.
- std::unique_ptr<SdpOfferAnswerHandler> sdp_handler_
- RTC_GUARDED_BY(signaling_thread()) RTC_PT_GUARDED_BY(signaling_thread());
-
- const bool dtls_enabled_;
-
UsagePattern usage_pattern_ RTC_GUARDED_BY(signaling_thread());
- bool return_histogram_very_quickly_ RTC_GUARDED_BY(signaling_thread()) =
- false;
// The DataChannelController is accessed from both the signaling thread
// and networking thread. It is a thread-aware object.
@@ -715,19 +709,27 @@
PeerConnectionMessageHandler message_handler_
RTC_GUARDED_BY(signaling_thread());
+ PayloadTypePicker payload_type_picker_;
+
+ // The transport controller is set and used on the network thread.
+ // Some functions pass the value of the transport_controller_ pointer
+ // around as arguments while running on the signaling thread; these
+ // use the transport_controller_copy.
+ std::unique_ptr<JsepTransportController> transport_controller_
+ RTC_GUARDED_BY(network_thread());
+ JsepTransportController* transport_controller_copy_
+ RTC_GUARDED_BY(signaling_thread()) = nullptr;
+
+ // The machinery for handling offers and answers. Const after initialization.
+ std::unique_ptr<SdpOfferAnswerHandler> sdp_handler_
+ RTC_GUARDED_BY(signaling_thread()) RTC_PT_GUARDED_BY(signaling_thread());
+
// Administration of senders, receivers and transceivers
// Accessed on both signaling and network thread. Const after Initialize().
std::unique_ptr<RtpTransmissionManager> rtp_manager_;
- // Did the connectionState ever change to `connected`?
- // Used to gather metrics only the first such state change.
- bool was_ever_connected_ RTC_GUARDED_BY(signaling_thread()) = false;
-
- PayloadTypePicker payload_type_picker_;
// This variable needs to be the last one in the class.
WeakPtrFactory<PeerConnection> weak_factory_;
-
- bool CanAttemptDtlsStunPiggybacking(const RTCConfiguration& configuration);
};
} // namespace webrtc
diff --git a/pc/peer_connection_factory.cc b/pc/peer_connection_factory.cc
index ed0671a..cda5824 100644
--- a/pc/peer_connection_factory.cc
+++ b/pc/peer_connection_factory.cc
@@ -296,18 +296,17 @@
});
auto pc = PeerConnection::Create(env, context_, options_, std::move(call),
- configuration, std::move(dependencies),
- stun_servers, turn_servers);
+ configuration, dependencies, stun_servers,
+ turn_servers);
// We configure the proxy with a pointer to the network thread for methods
// that need to be invoked there rather than on the signaling thread.
// Internally, the proxy object has a member variable named `worker_thread_`
// which will point to the network thread (and not the factory's
// worker_thread()). All such methods have thread checks though, so the code
// should still be clear (outside of macro expansion).
- rtc::scoped_refptr<PeerConnectionInterface> result_proxy =
+ return rtc::scoped_refptr<PeerConnectionInterface>(
PeerConnectionProxy::Create(signaling_thread(), network_thread(),
- std::move(pc));
- return result_proxy;
+ std::move(pc)));
}
rtc::scoped_refptr<MediaStreamInterface>
