Move dependency setup and config check code over to factory.
This more consistently keeps the dependency setup code in the
PeerConnectionFactory class (previously was split) and also
places the responsibilities of doing error handling for config
checks on the factory class whereas the PC class has DCHECKs for
parameters being valid.
The effect of this is that the config is checked for correctness
before constructing the Call object, which requires more complex
teardown than necessary if the parameters are simply bad.
Fixes: webrtc:398857495
Bug: webrtc:398857495
Change-Id: I9874bb1ebee337b13e31eac35e17788d338739a6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/379380
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#44016}
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index 710f215..816d41f 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -1212,7 +1212,6 @@
"../media:rtc_media_config",
"../media:stream_params",
"../modules/rtp_rtcp:rtp_rtcp_format",
- "../p2p:basic_async_resolver_factory",
"../p2p:connection",
"../p2p:connection_info",
"../p2p:dtls_transport_internal",
@@ -1516,6 +1515,7 @@
"../call:rtp_interfaces",
"../call:rtp_sender",
"../media:media_engine",
+ "../p2p:basic_async_resolver_factory",
"../p2p:basic_packet_socket_factory",
"../p2p:basic_port_allocator",
"../p2p:connection",
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index cfc8575..7666787 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -70,7 +70,6 @@
#include "media/base/stream_params.h"
#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
#include "modules/rtp_rtcp/source/rtp_packet_received.h"
-#include "p2p/base/basic_async_resolver_factory.h"
#include "p2p/base/connection_info.h"
#include "p2p/base/ice_transport_internal.h"
#include "p2p/base/p2p_constants.h"
@@ -509,6 +508,17 @@
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.
RTCErrorOr<rtc::scoped_refptr<PeerConnection>> PeerConnection::Create(
const Environment& env,
rtc::scoped_refptr<ConnectionContext> context,
@@ -516,45 +526,15 @@
std::unique_ptr<Call> call,
const PeerConnectionInterface::RTCConfiguration& configuration,
PeerConnectionDependencies dependencies) {
- // TODO(https://crbug.com/webrtc/13528): Remove support for kPlanB.
- if (configuration.sdp_semantics == SdpSemantics::kPlanB_DEPRECATED) {
- RTC_LOG(LS_WARNING)
- << "PeerConnection constructed with legacy SDP semantics!";
- }
-
- RTCError config_error = cricket::IceConfig(configuration).IsValid();
- if (!config_error.ok()) {
- RTC_LOG(LS_ERROR) << "Invalid ICE configuration: "
- << config_error.message();
- return config_error;
- }
-
- if (!dependencies.allocator) {
- RTC_LOG(LS_ERROR)
- << "PeerConnection initialized without a PortAllocator? "
- "This shouldn't happen if using PeerConnectionFactory.";
- return RTCError(
- RTCErrorType::INVALID_PARAMETER,
- "Attempt to create a PeerConnection without a PortAllocatorFactory");
- }
-
- if (!dependencies.observer) {
- // TODO(deadbeef): Why do we do this?
- RTC_LOG(LS_ERROR) << "PeerConnection initialized without a "
- "PeerConnectionObserver";
- return RTCError(RTCErrorType::INVALID_PARAMETER,
- "Attempt to create a PeerConnection without an observer");
- }
+ RTC_DCHECK(cricket::IceConfig(configuration).IsValid().ok());
+ RTC_DCHECK(dependencies.observer);
+ RTC_DCHECK(dependencies.async_dns_resolver_factory);
+ RTC_CHECK(dependencies.allocator);
bool is_unified_plan =
configuration.sdp_semantics == SdpSemantics::kUnifiedPlan;
bool dtls_enabled = DtlsEnabled(configuration, options, dependencies);
- if (!dependencies.async_dns_resolver_factory) {
- dependencies.async_dns_resolver_factory =
- std::make_unique<BasicAsyncDnsResolverFactory>();
- }
-
// The PeerConnection constructor consumes some, but not all, dependencies.
auto pc = rtc::make_ref_counted<PeerConnection>(
env, context, options, is_unified_plan, std::move(call), dependencies,
diff --git a/pc/peer_connection_factory.cc b/pc/peer_connection_factory.cc
index bb01488..b17338a 100644
--- a/pc/peer_connection_factory.cc
+++ b/pc/peer_connection_factory.cc
@@ -23,6 +23,7 @@
#include "api/units/data_rate.h"
#include "call/rtp_transport_controller_send_factory.h"
#include "media/base/media_engine.h"
+#include "p2p/base/basic_async_resolver_factory.h"
#include "p2p/base/default_ice_transport_factory.h"
#include "p2p/base/port_allocator.h"
#include "p2p/client/basic_port_allocator.h"
@@ -207,6 +208,25 @@
PeerConnectionDependencies dependencies) {
RTC_DCHECK_RUN_ON(signaling_thread());
+ // TODO(https://crbug.com/webrtc/13528): Remove support for kPlanB.
+ if (configuration.sdp_semantics == SdpSemantics::kPlanB_DEPRECATED) {
+ RTC_LOG(LS_WARNING)
+ << "PeerConnection constructed with legacy SDP semantics!";
+ }
+
+ RTCError err = cricket::IceConfig(configuration).IsValid();
+ if (!err.ok()) {
+ RTC_LOG(LS_ERROR) << "Invalid ICE configuration: " << err.message();
+ return err;
+ }
+
+ if (!dependencies.observer) {
+ RTC_LOG(LS_ERROR) << "PeerConnection initialized without a "
+ "PeerConnectionObserver";
+ return RTCError(RTCErrorType::INVALID_PARAMETER,
+ "Attempt to create a PeerConnection without an observer");
+ }
+
EnvironmentFactory env_factory(context_->env());
// Field trials active for this PeerConnection is the first of:
@@ -230,6 +250,12 @@
std::make_unique<rtc::RTCCertificateGenerator>(signaling_thread(),
network_thread());
}
+
+ if (!dependencies.async_dns_resolver_factory) {
+ dependencies.async_dns_resolver_factory =
+ std::make_unique<BasicAsyncDnsResolverFactory>();
+ }
+
if (!dependencies.allocator) {
dependencies.allocator = std::make_unique<cricket::BasicPortAllocator>(
context_->default_network_manager(), context_->default_socket_factory(),
