Adding the ability to change ICE servers through SetConfiguration.

talk/app/webrtc/peerconnection.cc

Index: talk/app/webrtc/peerconnection.cc
diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc
index 86902b03591277b91d4a6106b38e7caf8e04d495..800d50a0b0b8f49826d6f6ae2f5264d53a5446f6 100644
--- a/talk/app/webrtc/peerconnection.cc
+++ b/talk/app/webrtc/peerconnection.cc
@@ -302,6 +302,40 @@ bool ParseIceServerUrl(const PeerConnectionInterface::IceServer& server,
   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 {
@@ -380,18 +414,47 @@ bool PeerConnection::Initialize(
     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.
@@ -709,36 +772,10 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& config) {
       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);