Relanding: Implement RTCConfiguration.iceCandidatePoolSize.

webrtc/p2p/base/p2ptransportchannel.cc

Index: webrtc/p2p/base/p2ptransportchannel.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
index acbcfd77e7165281e43588b4f7ce21dc2bf1f025..6bf54d83c0e57552dec8468e6919da87d95bdb98 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -268,7 +268,8 @@ P2PTransportChannel::~P2PTransportChannel() {
 
 // 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()));
@@ -284,7 +285,9 @@ void P2PTransportChannel::AddAllocatorSession(PortAllocatorSession* session) {
       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) {
@@ -472,9 +475,25 @@ void P2PTransportChannel::MaybeStartGathering() {
       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);
+    }
   }
 }