From f972489f323fac1688cd13da7528eeddb9486929 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Wed, 12 Feb 2025 14:04:12 +0100 Subject: [PATCH] Migrate PCLF not to create BasicPortAllocator itself Instead rely on PeerConnectionFactory to create it Bug: webrtc:42232556 Change-Id: I5710f979e0a030057b16c20fbf088ea2303be760 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/376882 Commit-Queue: Danil Chapovalov Reviewed-by: Artem Titov Cr-Commit-Position: refs/heads/main@{#43881} --- api/test/network_emulation_manager.h | 18 ++++-------- api/test/pclf/BUILD.gn | 3 +- api/test/pclf/media_quality_test_params.h | 34 ++++++++--------------- api/test/pclf/peer_configurer.cc | 15 ++-------- api/test/pclf/peer_configurer.h | 3 -- test/pc/e2e/BUILD.gn | 2 -- test/pc/e2e/test_peer_factory.cc | 24 +++++----------- 7 files changed, 29 insertions(+), 70 deletions(-) diff --git a/api/test/network_emulation_manager.h b/api/test/network_emulation_manager.h index 03a9d2d600..e2154e285e 100644 --- a/api/test/network_emulation_manager.h +++ b/api/test/network_emulation_manager.h @@ -26,7 +26,6 @@ #include "api/packet_socket_factory.h" #include "api/test/network_emulation/cross_traffic.h" #include "api/test/network_emulation/network_emulation_interfaces.h" -#include "api/test/peer_network_dependencies.h" #include "api/test/simulated_network.h" #include "api/test/time_controller.h" #include "api/units/data_rate.h" @@ -141,29 +140,24 @@ class EmulatedNetworkManagerInterface { // EmulatedNetworkManagerInterface implementation. // Deprecated in favor of injecting NetworkManager into PeerConnectionFactory // instead of creating and injecting BasicPortAllocator into PeerConnection. - // TODO: bugs.webrtc.org/42232556 - Cleanup usages of this accessor in WebRTC, - // and then deprecate or remove it. - virtual absl::Nonnull network_manager() = 0; + [[deprecated("bugs.webrtc.org/42232556")]] // + virtual absl::Nonnull + network_manager() = 0; // Returns packet socket factory that have to be injected // into WebRTC to properly setup network emulation. Returned factory is owned // by EmulatedNetworkManagerInterface implementation. // Deprecated in favor of injecting SocketFactory into PeerConnectionFactory // instead of creating and injecting BasicPortAllocator into PeerConnection. - // TODO: bugs.webrtc.org/42232556 - Cleanup usages of this accessor in WebRTC, - // and then deprecate or remove it. - virtual absl::Nonnull packet_socket_factory() = 0; + [[deprecated("bugs.webrtc.org/42232556")]] // + virtual absl::Nonnull + packet_socket_factory() = 0; // Returns objects to pass to PeerConnectionFactoryDependencies. virtual absl::Nonnull socket_factory() = 0; virtual absl::Nonnull> ReleaseNetworkManager() = 0; - [[deprecated("bugs.webrtc.org/42232556")]] // - webrtc::webrtc_pc_e2e::PeerNetworkDependencies - network_dependencies() { - return {network_thread(), network_manager(), packet_socket_factory()}; - } // Returns list of endpoints that are associated with this instance. Pointers // are guaranteed to be non-null and are owned by NetworkEmulationManager. virtual std::vector endpoints() const = 0; diff --git a/api/test/pclf/BUILD.gn b/api/test/pclf/BUILD.gn index bd31964bb1..bd5295c4b8 100644 --- a/api/test/pclf/BUILD.gn +++ b/api/test/pclf/BUILD.gn @@ -44,7 +44,6 @@ rtc_library("media_quality_test_params") { "../..:field_trials_view", "../..:ice_transport_interface", "../..:libjingle_peerconnection_api", - "../..:packet_socket_factory", "../..:scoped_refptr", "../../../p2p:port_allocator", "../../../rtc_base:checks", @@ -82,8 +81,8 @@ rtc_library("peer_configurer") { "../..:ice_transport_interface", "../..:libjingle_peerconnection_api", "../..:network_emulation_manager_api", - "../..:peer_network_dependencies", "../..:scoped_refptr", + "../../../p2p:port_allocator", "../../../rtc_base:checks", "../../../rtc_base:rtc_certificate_generator", "../../../rtc_base:ssl", diff --git a/api/test/pclf/media_quality_test_params.h b/api/test/pclf/media_quality_test_params.h index aae4d22a07..429b33fdb2 100644 --- a/api/test/pclf/media_quality_test_params.h +++ b/api/test/pclf/media_quality_test_params.h @@ -15,6 +15,7 @@ #include #include #include +#include #include #include "api/async_dns_resolver.h" @@ -26,7 +27,6 @@ #include "api/field_trials_view.h" #include "api/ice_transport_interface.h" #include "api/neteq/neteq_factory.h" -#include "api/packet_socket_factory.h" #include "api/peer_connection_interface.h" #include "api/rtc_event_log/rtc_event_log_factory_interface.h" #include "api/scoped_refptr.h" @@ -56,6 +56,8 @@ namespace webrtc_pc_e2e { // can override only some parts of media engine like video encoder/decoder // factories. struct PeerConnectionFactoryComponents { + std::unique_ptr network_manager; + rtc::SocketFactory* socket_factory = nullptr; std::unique_ptr event_log_factory; std::unique_ptr fec_controller_factory; std::unique_ptr network_controller_factory; @@ -79,18 +81,8 @@ struct PeerConnectionFactoryComponents { // Separate class was introduced to clarify which components can be // overridden. For example observer, which is required to // PeerConnectionDependencies, will be provided by fixture implementation, -// so client can't inject its own. Also only network manager can be overridden -// inside port allocator. +// so client can't inject its own. struct PeerConnectionComponents { - PeerConnectionComponents(rtc::NetworkManager* network_manager, - rtc::PacketSocketFactory* packet_socket_factory) - : network_manager(network_manager), - packet_socket_factory(packet_socket_factory) { - RTC_CHECK(network_manager); - } - - rtc::NetworkManager* const network_manager; - rtc::PacketSocketFactory* const packet_socket_factory; std::unique_ptr async_dns_resolver_factory; std::unique_ptr cert_generator; @@ -102,15 +94,15 @@ struct PeerConnectionComponents { // has a network thread, that will be used to communicate with another peers. struct InjectableComponents { InjectableComponents(rtc::Thread* network_thread, - rtc::NetworkManager* network_manager, - rtc::PacketSocketFactory* packet_socket_factory) + std::unique_ptr network_manager, + rtc::SocketFactory* socket_factory) : network_thread(network_thread), worker_thread(nullptr), pcf_dependencies(std::make_unique()), - pc_dependencies( - std::make_unique(network_manager, - packet_socket_factory)) { + pc_dependencies(std::make_unique()) { RTC_CHECK(network_thread); + pcf_dependencies->network_manager = std::move(network_manager); + pcf_dependencies->socket_factory = socket_factory; } rtc::Thread* const network_thread; @@ -128,14 +120,12 @@ struct Params { std::optional name; // If `audio_config` is set audio stream will be configured std::optional audio_config; - // Flags to set on `cricket::PortAllocator`. If not set, - // cricket::kDefaultPortAllocatorFlags will be used and - // cricket::PORTALLOCATOR_DISABLE_TCP will be disabled. + // Flags to override `rtc_configuration.port_allocator_config.flags` // // IMPORTANT: if you use WebRTC Network Emulation // (api/test/network_emulation_manager.h) and set this field, remember to set - // cricket::PORTALLOCATOR_DISABLE_TCP to 0. - std::optional port_allocator_flags = std::nullopt; + // cricket::PORTALLOCATOR_DISABLE_TCP. + uint32_t port_allocator_flags = cricket::PORTALLOCATOR_DISABLE_TCP; // If `rtc_event_log_path` is set, an RTCEventLog will be saved in that // location and it will be available for further analysis. std::optional rtc_event_log_path; diff --git a/api/test/pclf/peer_configurer.cc b/api/test/pclf/peer_configurer.cc index 1002b62aa4..38b4994c2c 100644 --- a/api/test/pclf/peer_configurer.cc +++ b/api/test/pclf/peer_configurer.cc @@ -35,11 +35,11 @@ #include "api/test/network_emulation_manager.h" #include "api/test/pclf/media_configuration.h" #include "api/test/pclf/media_quality_test_params.h" -#include "api/test/peer_network_dependencies.h" #include "api/transport/bitrate_settings.h" #include "api/transport/network_control.h" #include "api/video_codecs/video_decoder_factory.h" #include "api/video_codecs/video_encoder_factory.h" +#include "p2p/base/port_allocator.h" #include "rtc_base/checks.h" #include "rtc_base/rtc_certificate_generator.h" #include "rtc_base/ssl_certificate.h" @@ -47,20 +47,11 @@ namespace webrtc { namespace webrtc_pc_e2e { -PeerConfigurer::PeerConfigurer( - const PeerNetworkDependencies& network_dependencies) - : components_(std::make_unique( - network_dependencies.network_thread, - network_dependencies.network_manager, - network_dependencies.packet_socket_factory)), - params_(std::make_unique()), - configurable_params_(std::make_unique()) {} - PeerConfigurer::PeerConfigurer(EmulatedNetworkManagerInterface& network) : components_(std::make_unique( network.network_thread(), - network.network_manager(), - network.packet_socket_factory())), + network.ReleaseNetworkManager(), + network.socket_factory())), params_(std::make_unique()), configurable_params_(std::make_unique()) {} diff --git a/api/test/pclf/peer_configurer.h b/api/test/pclf/peer_configurer.h index 2c835619fb..38ce8c3ae1 100644 --- a/api/test/pclf/peer_configurer.h +++ b/api/test/pclf/peer_configurer.h @@ -33,7 +33,6 @@ #include "api/test/network_emulation_manager.h" #include "api/test/pclf/media_configuration.h" #include "api/test/pclf/media_quality_test_params.h" -#include "api/test/peer_network_dependencies.h" #include "api/transport/bitrate_settings.h" #include "api/transport/network_control.h" #include "api/video_codecs/video_decoder_factory.h" @@ -51,8 +50,6 @@ class PeerConfigurer { absl::variant, CapturingDeviceIndex>; - [[deprecated("bugs.webrtc.org/42232556")]] explicit PeerConfigurer( - const PeerNetworkDependencies& network_dependencies); explicit PeerConfigurer(EmulatedNetworkManagerInterface& network); // Sets peer name that will be used to report metrics related to this peer. diff --git a/test/pc/e2e/BUILD.gn b/test/pc/e2e/BUILD.gn index 94e6c62a1f..92f6b41a4b 100644 --- a/test/pc/e2e/BUILD.gn +++ b/test/pc/e2e/BUILD.gn @@ -121,8 +121,6 @@ if (!build_with_chromium) { "../../../api/video_codecs:video_codecs_api", "../../../modules/audio_device:test_audio_device_module", "../../../modules/audio_processing/aec_dump", - "../../../p2p:basic_port_allocator", - "../../../p2p:port_allocator", "../../../pc:pc_test_utils", "../../../rtc_base:checks", "../../../rtc_base:threading", diff --git a/test/pc/e2e/test_peer_factory.cc b/test/pc/e2e/test_peer_factory.cc index 4c359a4d60..927dabaf92 100644 --- a/test/pc/e2e/test_peer_factory.cc +++ b/test/pc/e2e/test_peer_factory.cc @@ -34,8 +34,6 @@ #include "api/video_codecs/video_decoder_factory.h" #include "api/video_codecs/video_encoder_factory.h" #include "modules/audio_device/include/test_audio_device.h" -#include "p2p/base/port_allocator.h" -#include "p2p/client/basic_port_allocator.h" #include "pc/test/mock_peer_connection_observers.h" #include "rtc_base/checks.h" #include "rtc_base/system/file_wrapper.h" @@ -206,6 +204,8 @@ PeerConnectionFactoryDependencies CreatePCFDependencies( pcf_deps.signaling_thread = signaling_thread; pcf_deps.worker_thread = worker_thread; pcf_deps.network_thread = network_thread; + pcf_deps.socket_factory = pcf_dependencies->socket_factory; + pcf_deps.network_manager = std::move(pcf_dependencies->network_manager); pcf_deps.event_log_factory = std::move(pcf_dependencies->event_log_factory); pcf_deps.task_queue_factory = time_controller.CreateTaskQueueFactory(); @@ -247,22 +247,10 @@ PeerConnectionFactoryDependencies CreatePCFDependencies( // from InjectableComponents::PeerConnectionComponents. PeerConnectionDependencies CreatePCDependencies( MockPeerConnectionObserver* observer, - std::optional port_allocator_flags, std::unique_ptr pc_dependencies) { PeerConnectionDependencies pc_deps(observer); - auto port_allocator = std::make_unique( - pc_dependencies->network_manager, pc_dependencies->packet_socket_factory); - // This test does not support TCP by default. - int flags = - cricket::kDefaultPortAllocatorFlags | cricket::PORTALLOCATOR_DISABLE_TCP; - if (port_allocator_flags.has_value()) { - flags = *port_allocator_flags; - } - port_allocator->set_flags(port_allocator->flags() | flags); - - pc_deps.allocator = std::move(port_allocator); if (pc_dependencies->async_dns_resolver_factory != nullptr) { pc_deps.async_dns_resolver_factory = @@ -346,9 +334,11 @@ std::unique_ptr TestPeerFactory::CreateTestPeer( } // Create peer connection. - PeerConnectionDependencies pc_deps = - CreatePCDependencies(observer.get(), params->port_allocator_flags, - std::move(components->pc_dependencies)); + PeerConnectionDependencies pc_deps = CreatePCDependencies( + observer.get(), std::move(components->pc_dependencies)); + + params->rtc_configuration.port_allocator_config.flags = + params->port_allocator_flags; rtc::scoped_refptr peer_connection = peer_connection_factory ->CreatePeerConnectionOrError(params->rtc_configuration,