move relay server priority assignment to port_allocator

which knows more about the internals of ICE.
Remove the relay server config priority field which was used to
specify the relative priority of TURN servers. This is now handled
internally by CreateRelayPortArgs without being exposed.

Also rename BasicPortAllocator::AddTurnServer to
BasicPortAllocator::AddTurnServerForTesting since it is a test-only
method.

BUG=webrtc:13195,webrtc:14539

Change-Id: Id36cbf0187b7a84d1a9b53860f31994f3c7589f0
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/280224
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Jonas Oreland <jonaso@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38520}

p2p/base/p2p_transport_channel_unittest.cc

@@ -2737,8 +2737,8 @@ TEST_P(P2PTransportChannelMultihomedTest, TestFailoverWithManyConnections) {
   RelayServerConfig turn_server;
   turn_server.credentials = kRelayCredentials;
   turn_server.ports.push_back(ProtocolAddress(kTurnTcpIntAddr, PROTO_TCP));
-  GetAllocator(0)->AddTurnServer(turn_server);
+  GetAllocator(0)->AddTurnServerForTesting(turn_server);
-  GetAllocator(1)->AddTurnServer(turn_server);
+  GetAllocator(1)->AddTurnServerForTesting(turn_server);
   // Enable IPv6
   SetAllocatorFlags(
       0, PORTALLOCATOR_ENABLE_IPV6 | PORTALLOCATOR_ENABLE_IPV6_ON_WIFI);
@@ -5238,7 +5238,7 @@ TEST_P(P2PTransportChannelMostLikelyToWorkFirstTest, TestTcpTurn) {
   RelayServerConfig config;
   config.credentials = kRelayCredentials;
   config.ports.push_back(ProtocolAddress(kTurnTcpIntAddr, PROTO_TCP));
-  allocator()->AddTurnServer(config);
+  allocator()->AddTurnServerForTesting(config);
 
   P2PTransportChannel& ch = StartTransportChannel(true, 500, &field_trials_);
   EXPECT_TRUE_WAIT(ch.ports().size() == 3, kDefaultTimeout);

p2p/base/port_allocator.h

@@ -171,14 +171,12 @@ struct RTC_EXPORT RelayServerConfig {
   ~RelayServerConfig();
 
   bool operator==(const RelayServerConfig& o) const {
-    return ports == o.ports && credentials == o.credentials &&
+    return ports == o.ports && credentials == o.credentials;
-           priority == o.priority;
   }
   bool operator!=(const RelayServerConfig& o) const { return !(*this == o); }
 
   PortList ports;
   RelayCredentials credentials;
-  int priority = 0;
   TlsCertPolicy tls_cert_policy = TlsCertPolicy::TLS_CERT_POLICY_SECURE;
   std::vector<std::string> tls_alpn_protocols;
   std::vector<std::string> tls_elliptic_curves;

p2p/base/turn_port.h

@@ -81,7 +81,7 @@ class TurnPort : public Port {
     return absl::WrapUnique(
         new TurnPort(args.network_thread, args.socket_factory, args.network,
                      socket, args.username, args.password, *args.server_address,
-                     args.config->credentials, args.config->priority,
+                     args.config->credentials, args.relative_priority,
                      args.config->tls_alpn_protocols,
                      args.config->tls_elliptic_curves, args.turn_customizer,
                      args.config->tls_cert_verifier, args.field_trials));
@@ -100,7 +100,7 @@ class TurnPort : public Port {
         new TurnPort(args.network_thread, args.socket_factory, args.network,
                      min_port, max_port, args.username, args.password,
                      *args.server_address, args.config->credentials,
-                     args.config->priority, args.config->tls_alpn_protocols,
+                     args.relative_priority, args.config->tls_alpn_protocols,
                      args.config->tls_elliptic_curves, args.turn_customizer,
                      args.config->tls_cert_verifier, args.field_trials));
   }

p2p/client/basic_port_allocator.cc

@@ -298,7 +298,8 @@ PortAllocatorSession* BasicPortAllocator::CreateSessionInternal(
   return session;
 }
 
-void BasicPortAllocator::AddTurnServer(const RelayServerConfig& turn_server) {
+void BasicPortAllocator::AddTurnServerForTesting(
+    const RelayServerConfig& turn_server) {
   CheckRunOnValidThreadAndInitialized();
   std::vector<RelayServerConfig> new_turn_servers = turn_servers();
   new_turn_servers.push_back(turn_server);
@@ -1647,12 +1648,17 @@ void AllocationSequence::CreateRelayPorts() {
     return;
   }
 
+  // Relative priority of candidates from this TURN server in relation
+  // to the candidates from other servers. Required because ICE priorities
+  // need to be unique.
+  int relative_priority = config_->relays.size();
   for (RelayServerConfig& relay : config_->relays) {
-    CreateTurnPort(relay);
+    CreateTurnPort(relay, relative_priority--);
   }
 }
 
-void AllocationSequence::CreateTurnPort(const RelayServerConfig& config) {
+void AllocationSequence::CreateTurnPort(const RelayServerConfig& config,
+                                        int relative_priority) {
   PortList::const_iterator relay_port;
   for (relay_port = config.ports.begin(); relay_port != config.ports.end();
        ++relay_port) {
@@ -1685,6 +1691,7 @@ void AllocationSequence::CreateTurnPort(const RelayServerConfig& config) {
     args.config = &config;
     args.turn_customizer = session_->allocator()->turn_customizer();
     args.field_trials = session_->allocator()->field_trials();
+    args.relative_priority = relative_priority;
 
     std::unique_ptr<cricket::Port> port;
     // Shared socket mode must be enabled only for UDP based ports. Hence

p2p/client/basic_port_allocator.h

@@ -79,7 +79,7 @@ class RTC_EXPORT BasicPortAllocator : public PortAllocator {
       absl::string_view ice_pwd) override;
 
   // Convenience method that adds a TURN server to the configuration.
-  void AddTurnServer(const RelayServerConfig& turn_server);
+  void AddTurnServerForTesting(const RelayServerConfig& turn_server);
 
   RelayPortFactoryInterface* relay_port_factory() {
     CheckRunOnValidThreadIfInitialized();
@@ -386,11 +386,9 @@ class AllocationSequence : public sigslot::has_slots<> {
   void Start();
   void Stop();
 
- protected:
-  // For testing.
-  void CreateTurnPort(const RelayServerConfig& config);
-
  private:
+  void CreateTurnPort(const RelayServerConfig& config, int relative_priority);
+
   typedef std::vector<ProtocolType> ProtocolList;
 
   void Process(int epoch);

p2p/client/basic_port_allocator_unittest.cc

@@ -251,7 +251,7 @@ class BasicPortAllocatorTestBase : public ::testing::Test,
   void AddTurnServers(const rtc::SocketAddress& udp_turn,
                       const rtc::SocketAddress& tcp_turn) {
     RelayServerConfig turn_server = CreateTurnServers(udp_turn, tcp_turn);
-    allocator_->AddTurnServer(turn_server);
+    allocator_->AddTurnServerForTesting(turn_server);
   }
 
   bool CreateSession(int component) {
@@ -1818,7 +1818,7 @@ TEST_F(BasicPortAllocatorTestWithRealClock,
   turn_server.credentials = credentials;
   turn_server.ports.push_back(
       ProtocolAddress(rtc::SocketAddress("localhost", 3478), PROTO_UDP));
-  allocator_->AddTurnServer(turn_server);
+  allocator_->AddTurnServerForTesting(turn_server);
 
   allocator_->set_step_delay(kMinimumStepDelay);
   allocator_->set_flags(allocator().flags() |
@@ -2496,6 +2496,29 @@ TEST_F(BasicPortAllocatorTest, TestDoNotUseTurnServerAsStunSever) {
   EXPECT_EQ(1U, port_config.StunServers().size());
 }
 
+// Test that candidates from different servers get assigned a unique local
+// preference (the middle 16 bits of the priority)
+TEST_F(BasicPortAllocatorTest, AssignsUniqueLocalPreferencetoRelayCandidates) {
+  allocator_->SetCandidateFilter(CF_RELAY);
+  allocator_->AddTurnServerForTesting(
+      CreateTurnServers(kTurnUdpIntAddr, SocketAddress()));
+  allocator_->AddTurnServerForTesting(
+      CreateTurnServers(kTurnUdpIntAddr, SocketAddress()));
+  allocator_->AddTurnServerForTesting(
+      CreateTurnServers(kTurnUdpIntAddr, SocketAddress()));
+
+  AddInterface(kClientAddr);
+  ASSERT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP));
+  session_->StartGettingPorts();
+  ASSERT_TRUE_SIMULATED_WAIT(candidate_allocation_done_,
+                             kDefaultAllocationTimeout, fake_clock);
+  EXPECT_EQ(3u, candidates_.size());
+  EXPECT_GT((candidates_[0].priority() >> 8) & 0xFFFF,
+            (candidates_[1].priority() >> 8) & 0xFFFF);
+  EXPECT_GT((candidates_[1].priority() >> 8) & 0xFFFF,
+            (candidates_[2].priority() >> 8) & 0xFFFF);
+}
+
 // Test that no more than allocator.max_ipv6_networks() IPv6 networks are used
 // to gather candidates.
 TEST_F(BasicPortAllocatorTest, TwoIPv6AreSelectedBecauseOfMaxIpv6Limit) {

p2p/client/relay_port_factory_interface.h

@@ -45,6 +45,10 @@ struct CreateRelayPortArgs {
   std::string password;
   webrtc::TurnCustomizer* turn_customizer = nullptr;
   const webrtc::FieldTrialsView* field_trials = nullptr;
+  // Relative priority of candidates from this TURN server in relation
+  // to the candidates from other servers. Required because ICE priorities
+  // need to be unique.
+  int relative_priority = 0;
 };
 
 // A factory for creating RelayPort's.

pc/ice_server_parsing.cc

@@ -338,13 +338,6 @@ RTCError ParseIceServersOrError(
                            "ICE server parsing failed: Empty uri.");
     }
   }
-  // Candidates must have unique priorities, so that connectivity checks
-  // are performed in a well-defined order.
-  int priority = static_cast<int>(turn_servers->size() - 1);
-  for (cricket::RelayServerConfig& turn_server : *turn_servers) {
-    // First in the list gets highest priority.
-    turn_server.priority = priority--;
-  }
   return RTCError::OK();
 }
 

pc/ice_server_parsing_unittest.cc

@@ -237,22 +237,4 @@ TEST_F(IceServerParsingTest, ParseMultipleUrls) {
   EXPECT_EQ(1U, turn_servers_.size());
 }
 
-// Ensure that TURN servers are given unique priorities,
-// so that their resulting candidates have unique priorities.
-TEST_F(IceServerParsingTest, TurnServerPrioritiesUnique) {
-  PeerConnectionInterface::IceServers servers;
-  PeerConnectionInterface::IceServer server;
-  server.urls.push_back("turn:hostname");
-  server.urls.push_back("turn:hostname2");
-  server.username = "foo";
-  server.password = "bar";
-  servers.push_back(server);
-
-  EXPECT_TRUE(
-      webrtc::ParseIceServersOrError(servers, &stun_servers_, &turn_servers_)
-          .ok());
-  EXPECT_EQ(2U, turn_servers_.size());
-  EXPECT_NE(turn_servers_[0].priority, turn_servers_[1].priority);
-}
-
 }  // namespace webrtc