Relanding: Implement RTCConfiguration.iceCandidatePoolSize.

webrtc/p2p/base/p2ptransportchannel.cc

 /*
  *  Copyright 2004 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.
  */
 
@@ skipping 250 matching lines @@
 
 P2PTransportChannel::~P2PTransportChannel() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   for (size_t i = 0; i < allocator_sessions_.size(); ++i)
     delete allocator_sessions_[i];
 }
 
 // Add the allocator session to our list so that we know which sessions
 // are still active.
-void P2PTransportChannel::AddAllocatorSession(PortAllocatorSession* session) {
+void P2PTransportChannel::AddAllocatorSession(PortAllocatorSession* session,
+                                              bool pooled) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   session->set_generation(static_cast<uint32_t>(allocator_sessions_.size()));
   allocator_sessions_.push_back(session);
 
   // We now only want to apply new candidates that we receive to the ports
   // created by this new session because these are replacing those of the
   // previous sessions.
   ports_.clear();
 
   session->SignalPortReady.connect(this, &P2PTransportChannel::OnPortReady);
   session->SignalCandidatesReady.connect(
       this, &P2PTransportChannel::OnCandidatesReady);
   session->SignalCandidatesAllocationDone.connect(
       this, &P2PTransportChannel::OnCandidatesAllocationDone);
-  session->StartGettingPorts();
+  if (!pooled) {
+    session->StartGettingPorts();
+  }

[pthatcher1, 2016/05/05 21:51:24]

Instead of having a bool passed in, I think it might be cleaner to have two
methods:

AddAllocatorSession(session);   // Adds, but doesn't start
AddAndStartAllocatorSession(session);   // Adds and starts

Or just have AddAllocatorSession(session) and StartAllocatorSession(session) and
have the caller call two methods instead of AddAndStartAllocatorSession.

[Taylor Brandstetter, 2016/05/06 03:53:34]

Not much point in making a method (StartAllocatorSession) that just calls one
other method. But moving StartGettingPorts out of here makes sense.

[pthatcher1, 2016/05/07 00:21:44]

That was actually the first idea I had.  Now seeing it, I agree it's better.

 }

[pthatcher1, 2016/05/05 21:51:24]

Actually, is StartGettingPorts a no-op if it's already started?  If so, couldn't
we just always call StartGettingPorts and avoid this bool?

[Taylor Brandstetter, 2016/05/06 03:53:34]

No, it isn't a no-op.

 
 void P2PTransportChannel::AddConnection(Connection* connection) {
   connections_.push_back(connection);
   unpinged_connections_.insert(connection);
   connection->set_remote_ice_mode(remote_ice_mode_);
   connection->set_receiving_timeout(config_.receiving_timeout);
   connection->SignalReadPacket.connect(
       this, &P2PTransportChannel::OnReadPacket);
   connection->SignalReadyToSend.connect(
       this, &P2PTransportChannel::OnReadyToSend);
@@ skipping 165 matching lines @@
 
 void P2PTransportChannel::MaybeStartGathering() {
   // Start gathering if we never started before, or if an ICE restart occurred.
   if (allocator_sessions_.empty() ||
       IceCredentialsChanged(allocator_sessions_.back()->ice_ufrag(),
                             allocator_sessions_.back()->ice_pwd(), ice_ufrag_,
                             ice_pwd_)) {
     if (gathering_state_ != kIceGatheringGathering) {
       gathering_state_ = kIceGatheringGathering;
       SignalGatheringState(this);
     }

[Taylor Brandstetter, 2016/05/05 19:56:22]

You might think "we shouldn't use pooled sessions when doing an ICE restart".
But one use case provided by JSEP is calling SetConfiguration to change the set
of ICE servers (which refreshes the pool), then calling setLocalDescription
later to do an ICE restart. In this case the (new) pooled session *should* be
used.

So, if the application *doesn't* want pooled sessions to be used, it should just
call SetConfiguration with a pool size of 0.

-    // Time for a new allocator
-    AddAllocatorSession(allocator_->CreateSession(
-        SessionId(), transport_name(), component(), ice_ufrag_, ice_pwd_));
+    // Time for a new allocator.
+    PortAllocatorSession* pooled_session = allocator_->GetPooledSession(
+        transport_name(), component(), ice_ufrag_, ice_pwd_);
+    if (pooled_session) {
+      AddAllocatorSession(pooled_session, true);
+      // Process the pooled session's existing candidates/ports, if they exist.
+      OnCandidatesReady(pooled_session, pooled_session->ReadyCandidates());
+      for (PortInterface* port : pooled_session->ReadyPorts()) {
+        OnPortReady(pooled_session, port);
+      }
+      if (pooled_session->CandidatesAllocationDone()) {
+        OnCandidatesAllocationDone(pooled_session);
+      }
+    } else {
+      AddAllocatorSession(
+          allocator_->CreateSession(SessionId(), transport_name(), component(),
+                                    ice_ufrag_, ice_pwd_),
+          false);
+    }
   }
 }
 
 // A new port is available, attempt to make connections for it
 void P2PTransportChannel::OnPortReady(PortAllocatorSession *session,
                                       PortInterface* port) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   // Set in-effect options on the new port
   for (OptionMap::const_iterator it = options_.begin();
@@ skipping 1150 matching lines @@
 
   // During the initial state when nothing has been pinged yet, return the first
   // one in the ordered |connections_|.
   return *(std::find_if(connections_.begin(), connections_.end(),
                         [conn1, conn2](Connection* conn) {
                           return conn == conn1 || conn == conn2;
                         }));
 }
 
 }  // namespace cricket