From 573b145ab51ea59394b4763dece7b54751eea424 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Tue, 21 Jun 2022 11:37:29 +0200 Subject: [PATCH] Reland "Move injection of PacketSocketFactory from PC to PCF" This is a reland of commit 905c3a6c73d293882ef11942066ccda52a9e14d1 Change from previous attempt is between ps#1 and ps#2: Use PeerConnectionFactoryInterface::Options to clear the `network_ignore_mask`. Original change's description: > Move injection of PacketSocketFactory from PC to PCF > > Injection via PeerConnectionDependecies was broken, in not accepting > ownership of the injected object. > > Bug: webrtc:7447, webrtc:14204 > Change-Id: Ic53f05d51928b006fc1e46d502633d88471eb518 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/266140 > Reviewed-by: Harald Alvestrand > Commit-Queue: Niels Moller > Cr-Commit-Position: refs/heads/main@{#37270} Bug: webrtc:7447, webrtc:14204 Change-Id: Ic78ebec2e88a8c44699015c8c7a44e137f44253a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/265982 Reviewed-by: Harald Alvestrand Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/main@{#37290} --- api/BUILD.gn | 12 +++++++ api/peer_connection_interface.h | 6 ++-- api/test/mock_packet_socket_factory.h | 49 ++++++++++++++++++++++++++ pc/BUILD.gn | 1 + pc/connection_context.cc | 8 +++-- pc/connection_context.h | 4 +-- pc/peer_connection_factory.cc | 11 +----- pc/peer_connection_factory_unittest.cc | 40 +++++++++++++++++++++ 8 files changed, 113 insertions(+), 18 deletions(-) create mode 100644 api/test/mock_packet_socket_factory.h diff --git a/api/BUILD.gn b/api/BUILD.gn index 5f6bc499f8..cd1c104a76 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -1100,6 +1100,17 @@ if (rtc_include_tests) { ] } + rtc_source_set("mock_packet_socket_factory") { + visibility = [ "*" ] + testonly = true + sources = [ "test/mock_packet_socket_factory.h" ] + + deps = [ + ":packet_socket_factory", + "../test:test_support", + ] + } + rtc_source_set("mock_peerconnectioninterface") { visibility = [ "*" ] testonly = true @@ -1322,6 +1333,7 @@ if (rtc_include_tests) { ":mock_frame_decryptor", ":mock_frame_encryptor", ":mock_media_stream_interface", + ":mock_packet_socket_factory", ":mock_peer_connection_factory_interface", ":mock_peerconnectioninterface", ":mock_rtp", diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h index 3e70855d0c..8a6ae960bd 100644 --- a/api/peer_connection_interface.h +++ b/api/peer_connection_interface.h @@ -1379,10 +1379,9 @@ struct RTC_EXPORT PeerConnectionDependencies final { PeerConnectionObserver* observer = nullptr; // Optional dependencies // TODO(bugs.webrtc.org/7447): remove port allocator once downstream is - // updated. For now, you can only set one of allocator and - // packet_socket_factory, not both. + // updated. The recommended way to inject networking components is to pass a + // PacketSocketFactory when creating the PeerConnectionFactory. std::unique_ptr allocator; - std::unique_ptr packet_socket_factory; // Factory for creating resolvers that look up hostnames in DNS std::unique_ptr async_dns_resolver_factory; @@ -1422,6 +1421,7 @@ struct RTC_EXPORT PeerConnectionFactoryDependencies final { rtc::Thread* worker_thread = nullptr; rtc::Thread* signaling_thread = nullptr; rtc::SocketFactory* socket_factory = nullptr; + std::unique_ptr packet_socket_factory; std::unique_ptr task_queue_factory; std::unique_ptr media_engine; std::unique_ptr call_factory; diff --git a/api/test/mock_packet_socket_factory.h b/api/test/mock_packet_socket_factory.h new file mode 100644 index 0000000000..7e59556385 --- /dev/null +++ b/api/test/mock_packet_socket_factory.h @@ -0,0 +1,49 @@ +/* + * Copyright (c) 2022 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#ifndef API_TEST_MOCK_PACKET_SOCKET_FACTORY_H_ +#define API_TEST_MOCK_PACKET_SOCKET_FACTORY_H_ + +#include +#include + +#include "api/packet_socket_factory.h" +#include "test/gmock.h" + +namespace rtc { +class MockPacketSocketFactory : public PacketSocketFactory { + public: + MOCK_METHOD(AsyncPacketSocket*, + CreateUdpSocket, + (const SocketAddress&, uint16_t, uint16_t), + (override)); + MOCK_METHOD(AsyncListenSocket*, + CreateServerTcpSocket, + (const SocketAddress&, uint16_t, uint16_t, int opts), + (override)); + MOCK_METHOD(AsyncPacketSocket*, + CreateClientTcpSocket, + (const SocketAddress& local_address, + const SocketAddress&, + const ProxyInfo&, + const std::string&, + const PacketSocketTcpOptions&), + (override)); + MOCK_METHOD(std::unique_ptr, + CreateAsyncDnsResolver, + (), + (override)); +}; + +static_assert(!std::is_abstract_v, ""); + +} // namespace rtc + +#endif // API_TEST_MOCK_PACKET_SOCKET_FACTORY_H_ diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 8e6523aeeb..b5f22ac65a 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -2327,6 +2327,7 @@ if (rtc_include_tests && !build_with_chromium) { "../api:make_ref_counted", "../api:media_stream_interface", "../api:mock_encoder_selector", + "../api:mock_packet_socket_factory", "../api:mock_video_track", "../api:packet_socket_factory", "../api:priority", diff --git a/pc/connection_context.cc b/pc/connection_context.cc index f89ca5fde5..de0597b0b8 100644 --- a/pc/connection_context.cc +++ b/pc/connection_context.cc @@ -104,6 +104,7 @@ ConnectionContext::ConnectionContext( std::move(dependencies->network_monitor_factory)), default_network_manager_(std::move(dependencies->network_manager)), call_factory_(std::move(dependencies->call_factory)), + default_socket_factory_(std::move(dependencies->packet_socket_factory)), sctp_factory_( MaybeCreateSctpFactory(std::move(dependencies->sctp_factory), network_thread(), @@ -151,9 +152,10 @@ ConnectionContext::ConnectionContext( default_network_manager_ = std::make_unique( network_monitor_factory_.get(), socket_factory, &field_trials()); } - default_socket_factory_ = - std::make_unique(socket_factory); - + if (!default_socket_factory_) { + default_socket_factory_ = + std::make_unique(socket_factory); + } // Set warning levels on the threads, to give warnings when response // may be slower than is expected of the thread. // Since some of the threads may be the same, start with the least diff --git a/pc/connection_context.h b/pc/connection_context.h index e8a109abff..415ae121b5 100644 --- a/pc/connection_context.h +++ b/pc/connection_context.h @@ -91,7 +91,7 @@ class ConnectionContext final RTC_DCHECK_RUN_ON(signaling_thread_); return default_network_manager_.get(); } - rtc::BasicPacketSocketFactory* default_socket_factory() { + rtc::PacketSocketFactory* default_socket_factory() { RTC_DCHECK_RUN_ON(signaling_thread_); return default_socket_factory_.get(); } @@ -140,7 +140,7 @@ class ConnectionContext final std::unique_ptr const call_factory_ RTC_GUARDED_BY(worker_thread()); - std::unique_ptr default_socket_factory_ + std::unique_ptr default_socket_factory_ RTC_GUARDED_BY(signaling_thread_); std::unique_ptr const sctp_factory_; }; diff --git a/pc/peer_connection_factory.cc b/pc/peer_connection_factory.cc index 276d7ef2cd..e99ddce08c 100644 --- a/pc/peer_connection_factory.cc +++ b/pc/peer_connection_factory.cc @@ -204,9 +204,6 @@ PeerConnectionFactory::CreatePeerConnectionOrError( const PeerConnectionInterface::RTCConfiguration& configuration, PeerConnectionDependencies dependencies) { RTC_DCHECK_RUN_ON(signaling_thread()); - RTC_DCHECK(!(dependencies.allocator && dependencies.packet_socket_factory)) - << "You can't set both allocator and packet_socket_factory; " - "the former is going away (see bugs.webrtc.org/7447"; // Set internal defaults if optional dependencies are not set. if (!dependencies.cert_generator) { @@ -215,14 +212,8 @@ PeerConnectionFactory::CreatePeerConnectionOrError( network_thread()); } if (!dependencies.allocator) { - rtc::PacketSocketFactory* packet_socket_factory; - if (dependencies.packet_socket_factory) - packet_socket_factory = dependencies.packet_socket_factory.get(); - else - packet_socket_factory = context_->default_socket_factory(); - dependencies.allocator = std::make_unique( - context_->default_network_manager(), packet_socket_factory, + context_->default_network_manager(), context_->default_socket_factory(), configuration.turn_customizer); dependencies.allocator->SetPortRange( configuration.port_allocator_config.min_port, diff --git a/pc/peer_connection_factory_unittest.cc b/pc/peer_connection_factory_unittest.cc index a2b7daa27e..01785ef186 100644 --- a/pc/peer_connection_factory_unittest.cc +++ b/pc/peer_connection_factory_unittest.cc @@ -20,6 +20,7 @@ #include "api/data_channel_interface.h" #include "api/jsep.h" #include "api/media_stream_interface.h" +#include "api/test/mock_packet_socket_factory.h" #include "api/video_codecs/builtin_video_decoder_factory.h" #include "api/video_codecs/builtin_video_encoder_factory.h" #include "media/base/fake_frame_source.h" @@ -31,6 +32,8 @@ #include "p2p/base/port_interface.h" #include "pc/test/fake_audio_capture_module.h" #include "pc/test/fake_video_track_source.h" +#include "pc/test/mock_peer_connection_observers.h" +#include "rtc_base/gunit.h" #include "rtc_base/rtc_certificate_generator.h" #include "rtc_base/socket_address.h" #include "rtc_base/time_utils.h" @@ -52,9 +55,11 @@ using webrtc::PeerConnectionObserver; using webrtc::VideoTrackInterface; using webrtc::VideoTrackSourceInterface; +using ::testing::_; using ::testing::AtLeast; using ::testing::InvokeWithoutArgs; using ::testing::NiceMock; +using ::testing::Return; namespace { @@ -536,3 +541,38 @@ TEST(PeerConnectionFactoryDependenciesTest, UsesNetworkManager) { called.Wait(kWaitTimeoutMs); } + +TEST(PeerConnectionFactoryDependenciesTest, UsesPacketSocketFactory) { + constexpr int64_t kWaitTimeoutMs = 10000; + auto mock_socket_factory = + std::make_unique>(); + + rtc::Event called; + EXPECT_CALL(*mock_socket_factory, CreateUdpSocket(_, _, _)) + .WillOnce(InvokeWithoutArgs([&] { + called.Set(); + return nullptr; + })) + .WillRepeatedly(Return(nullptr)); + + webrtc::PeerConnectionFactoryDependencies pcf_dependencies; + pcf_dependencies.packet_socket_factory = std::move(mock_socket_factory); + + rtc::scoped_refptr pcf = + CreateModularPeerConnectionFactory(std::move(pcf_dependencies)); + + // By default, localhost addresses are ignored, which makes tests fail if test + // machine is offline. + PeerConnectionFactoryInterface::Options options; + options.network_ignore_mask = 0; + pcf->SetOptions(options); + + PeerConnectionInterface::RTCConfiguration config; + config.ice_candidate_pool_size = 2; + NullPeerConnectionObserver observer; + auto pc = pcf->CreatePeerConnectionOrError( + config, webrtc::PeerConnectionDependencies(&observer)); + ASSERT_TRUE(pc.ok()); + + called.Wait(kWaitTimeoutMs); +}