Adding the ability to change ICE servers through SetConfiguration.

talk/app/webrtc/peerconnection.cc

 /*
  * libjingle
  * Copyright 2012 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 284 matching lines @@
       break;
     }
     case INVALID:
     default:
       LOG(WARNING) << "Configuration not supported: " << url;
       return false;
   }
   return true;
 }
 
+void ConvertToCricketIceServers(
+    const std::vector<StunConfiguration>& stuns,
+    const std::vector<TurnConfiguration>& turns,
+    cricket::ServerAddresses* cricket_stuns,
+    std::vector<cricket::RelayServerConfig>* cricket_turns) {

[pthatcher1, 2015/10/15 05:36:58]

How feasible is it to simply put StunConfiguration and TurnConfiguration into
portallocator.h and push those down the whole way?  Or the other way around: use
the cricket stuff all the way up?  It's ugly that we have to convert between the
two?  

If it's not feasible right now, please leave a TODO explaining what would need
to happen to make it feasible.

[Taylor Brandstetter, 2015/10/15 17:13:37]

Right now, the Chromium "PortAllocator factories" use StunConfiguration and
TurnConfiguration, so it would be a little messy to transplant them. I think the
easiest thing to do would be to wait until the factory classes are removed
(assuming you're ok with that). Then ParseIceServers can just convert directly
between URLs and the cricket structures. That would be 2 CLs from now.

+  RTC_DCHECK(cricket_stuns && cricket_turns);
+  for (const StunConfiguration& stun : stuns) {
+    cricket_stuns->insert(stun.server);
+  }
+
+  int priority = static_cast<int>(turns.size() - 1);
+  for (const TurnConfiguration& turn : turns) {
+    cricket::RelayCredentials credentials(turn.username, turn.password);
+    cricket::RelayServerConfig relay_server(cricket::RELAY_TURN);
+    cricket::ProtocolType protocol;
+    // Using VERIFY because ParseIceServers should have already caught an
+    // invalid transport type.
+    if (!VERIFY(
+            cricket::StringToProto(turn.transport_type.c_str(), &protocol))) {
+      LOG(LS_WARNING) << "Ignoring TURN server " << turn.server << ". "
+                      << "Reason= Incorrect " << turn.transport_type
+                      << " transport parameter.";
+    } else {
+      relay_server.ports.push_back(
+          cricket::ProtocolAddress(turn.server, protocol, turn.secure));
+      relay_server.credentials = credentials;
+      relay_server.priority = priority;
+      cricket_turns->push_back(relay_server);
+    }
+    // First in the list gets highest priority.
+    --priority;
+  }
+}
+
 }  // namespace
 
 namespace webrtc {
 
 bool ParseIceServers(const PeerConnectionInterface::IceServers& servers,
                      StunConfigurations* stun_config,
                      TurnConfigurations* turn_config) {
   for (const webrtc::PeerConnectionInterface::IceServer& server : servers) {
     if (!server.urls.empty()) {
       for (const std::string& url : server.urls) {
@@ skipping 58 matching lines @@
     receiver->Stop();
   }
 }
 
 bool PeerConnection::Initialize(
     const PeerConnectionInterface::RTCConfiguration& configuration,
     const MediaConstraintsInterface* constraints,
     PortAllocatorFactoryInterface* allocator_factory,
     rtc::scoped_ptr<DtlsIdentityStoreInterface> dtls_identity_store,
     PeerConnectionObserver* observer) {
-  RTC_DCHECK(observer != NULL);
-  if (!observer)
+  RTC_DCHECK(observer != nullptr);

[pthatcher1, 2015/10/15 05:36:58]

Can it just be RTC_DCHECK(observer)?

[Taylor Brandstetter, 2015/10/15 17:13:37]

"observer != nullptr" is used everywhere else in this file, so I figured I
should be consistent.

+  if (!observer) {
     return false;
-  observer_ = observer;
+  }
 
   std::vector<PortAllocatorFactoryInterface::StunConfiguration> stun_config;
   std::vector<PortAllocatorFactoryInterface::TurnConfiguration> turn_config;
   if (!ParseIceServers(configuration.servers, &stun_config, &turn_config)) {
     return false;
   }
-  port_allocator_.reset(
+  rtc::scoped_ptr<cricket::PortAllocator> allocator(
       allocator_factory->CreatePortAllocator(stun_config, turn_config));
+  return Initialize(configuration, constraints, allocator.Pass(),
+                    dtls_identity_store.Pass(), observer);

[pthatcher1, 2015/10/15 05:36:58]

Does this mean the ice servers are passed twice and pass into the port allocator
twice?  That seems strange.  Can we make it do it just once?

[Taylor Brandstetter, 2015/10/15 17:13:37]

Right now the Chromium P2PPortAllocator doesn't support SetIceServers. So until
it does (which will be the next CL in this series) we need to pass in the
servers through CreatePortAllocator.

[pthatcher1, 2015/10/20 18:31:26]

Can you put a comment, then, that says "this Initialize function parses ICE
servers a second time, but it will be removed once all port allocators support
SetIceServers."?

[Taylor Brandstetter, 2015/10/20 22:01:33]

Done.

+}
+
+bool PeerConnection::Initialize(
+    const PeerConnectionInterface::RTCConfiguration& configuration,
+    const MediaConstraintsInterface* constraints,
+    rtc::scoped_ptr<cricket::PortAllocator> allocator,
+    rtc::scoped_ptr<DtlsIdentityStoreInterface> dtls_identity_store,
+    PeerConnectionObserver* observer) {
+  RTC_DCHECK(observer != nullptr);
+  if (!observer) {
+    return false;
+  }
+  observer_ = observer;
+
+  port_allocator_ = allocator.Pass();
+
+  std::vector<PortAllocatorFactoryInterface::StunConfiguration> stun_config;
+  std::vector<PortAllocatorFactoryInterface::TurnConfiguration> turn_config;
+  if (!ParseIceServers(configuration.servers, &stun_config, &turn_config)) {
+    return false;
+  }
+
+  cricket::ServerAddresses cricket_stuns;
+  std::vector<cricket::RelayServerConfig> cricket_turns;
+  ConvertToCricketIceServers(stun_config, turn_config, &cricket_stuns,
+                             &cricket_turns);
+  port_allocator_->SetIceServers(cricket_stuns, cricket_turns);
 
   // To handle both internal and externally created port allocator, we will
   // enable BUNDLE here.
   int portallocator_flags = port_allocator_->flags();
   portallocator_flags |= cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET |
                          cricket::PORTALLOCATOR_ENABLE_IPV6;
   bool value;
   // If IPv6 flag was specified, we'll not override it by experiment.
   if (FindConstraint(
           constraints, MediaConstraintsInterface::kEnableIPv6, &value, NULL)) {
@@ skipping 297 matching lines @@
 }
 
 bool PeerConnection::SetConfiguration(const RTCConfiguration& config) {
   if (port_allocator_) {
     std::vector<PortAllocatorFactoryInterface::StunConfiguration> stuns;
     std::vector<PortAllocatorFactoryInterface::TurnConfiguration> turns;
     if (!ParseIceServers(config.servers, &stuns, &turns)) {
       return false;
     }
 
-    std::vector<rtc::SocketAddress> stun_hosts;
-    typedef std::vector<StunConfiguration>::const_iterator StunIt;
-    for (StunIt stun_it = stuns.begin(); stun_it != stuns.end(); ++stun_it) {
-      stun_hosts.push_back(stun_it->server);
-    }
-
-    rtc::SocketAddress stun_addr;
-    if (!stun_hosts.empty()) {
-      stun_addr = stun_hosts.front();
-      LOG(LS_INFO) << "SetConfiguration: StunServer Address: "
-                   << stun_addr.ToString();
-    }
-
-    for (size_t i = 0; i < turns.size(); ++i) {
-      cricket::RelayCredentials credentials(turns[i].username,
-                                            turns[i].password);
-      cricket::RelayServerConfig relay_server(cricket::RELAY_TURN);
-      cricket::ProtocolType protocol;
-      if (cricket::StringToProto(turns[i].transport_type.c_str(), &protocol)) {
-        relay_server.ports.push_back(cricket::ProtocolAddress(
-            turns[i].server, protocol, turns[i].secure));
-        relay_server.credentials = credentials;
-        LOG(LS_INFO) << "SetConfiguration: TurnServer Address: "
-                     << turns[i].server.ToString();
-      } else {
-        LOG(LS_WARNING) << "Ignoring TURN server " << turns[i].server << ". "
-                        << "Reason= Incorrect " << turns[i].transport_type
-                        << " transport parameter.";
-      }
-    }
+    cricket::ServerAddresses cricket_stuns;
+    std::vector<cricket::RelayServerConfig> cricket_turns;
+    ConvertToCricketIceServers(stuns, turns, &cricket_stuns, &cricket_turns);
+    port_allocator_->SetIceServers(cricket_stuns, cricket_turns);
   }
   session_->SetIceConfig(session_->ParseIceConfig(config));
   return session_->SetIceTransports(config.type);
 }
 
 bool PeerConnection::AddIceCandidate(
     const IceCandidateInterface* ice_candidate) {
   return session_->ProcessIceMessage(ice_candidate);
 }
 
@@ skipping 258 matching lines @@
 std::vector<rtc::scoped_refptr<RtpReceiverInterface>>::iterator
 PeerConnection::FindReceiverForTrack(MediaStreamTrackInterface* track) {
   return std::find_if(
       receivers_.begin(), receivers_.end(),
       [track](const rtc::scoped_refptr<RtpReceiverInterface>& receiver) {
         return receiver->track() == track;
       });
 }
 
 }  // namespace webrtc