Relanding: Implement RTCConfiguration.iceCandidatePoolSize.

webrtc/p2p/base/portallocator.h

Index: webrtc/p2p/base/portallocator.h
diff --git a/webrtc/p2p/base/portallocator.h b/webrtc/p2p/base/portallocator.h
index 6fb79b065e62f1ee1f8f33d5023569bca5e93108..9e255b9d0427ec1c42954966baeec1391423374c 100644
--- a/webrtc/p2p/base/portallocator.h
+++ b/webrtc/p2p/base/portallocator.h
@@ -11,6 +11,8 @@
 #ifndef WEBRTC_P2P_BASE_PORTALLOCATOR_H_
 #define WEBRTC_P2P_BASE_PORTALLOCATOR_H_
 
+#include <deque>
+#include <memory>
 #include <string>
 #include <vector>
 
@@ -19,6 +21,7 @@
 #include "webrtc/base/helpers.h"
 #include "webrtc/base/proxyinfo.h"
 #include "webrtc/base/sigslot.h"
+#include "webrtc/base/thread.h"
 
 namespace cricket {
 
@@ -82,6 +85,11 @@ struct RelayCredentials {
   RelayCredentials(const std::string& username, const std::string& password)
       : username(username), password(password) {}
 
+  bool operator==(const RelayCredentials& o) const {
+    return username == o.username && password == o.password;
+  }
+  bool operator!=(const RelayCredentials& o) const { return !(*this == o); }
+
   std::string username;
   std::string password;
 };
@@ -102,6 +110,12 @@ struct RelayServerConfig {
         ProtocolAddress(rtc::SocketAddress(address, port), proto, secure));
   }
 
+  bool operator==(const RelayServerConfig& o) const {
+    return type == o.type && ports == o.ports && credentials == o.credentials &&
+           priority == o.priority;
+  }
+  bool operator!=(const RelayServerConfig& o) const { return !(*this == o); }
+
   RelayType type;
   PortList ports;
   RelayCredentials credentials;
@@ -124,6 +138,8 @@ class PortAllocatorSession : public sigslot::has_slots<> {
   void set_flags(uint32_t flags) { flags_ = flags; }
   std::string content_name() const { return content_name_; }
   int component() const { return component_; }
+  const std::string& ice_ufrag() const { return ice_ufrag_; }
+  const std::string& ice_pwd() const { return ice_pwd_; }
 
   // Starts gathering STUN and Relay configurations.
   virtual void StartGettingPorts() = 0;
@@ -133,6 +149,14 @@ class PortAllocatorSession : public sigslot::has_slots<> {
   // Whether the process of getting ports has been stopped.
   virtual bool IsGettingPorts() = 0;
 
+  // Another way of getting the information provided by the signals below.
+  //
+  // Ports and candidates are not guaranteed to be in the same order as the
+  // signals were emitted in.
+  virtual std::vector<PortInterface*> ReadyPorts() const = 0;
+  virtual std::vector<Candidate> ReadyCandidates() const = 0;
+  virtual bool CandidatesAllocationDone() const = 0;
+
   sigslot::signal2<PortAllocatorSession*, PortInterface*> SignalPortReady;
   sigslot::signal2<PortAllocatorSession*,
                    const std::vector<Candidate>&> SignalCandidatesReady;
@@ -142,25 +166,46 @@ class PortAllocatorSession : public sigslot::has_slots<> {
   virtual void set_generation(uint32_t generation) { generation_ = generation; }
   sigslot::signal1<PortAllocatorSession*> SignalDestroyed;
 
-  const std::string& ice_ufrag() const { return ice_ufrag_; }
-  const std::string& ice_pwd() const { return ice_pwd_; }
-
  protected:
+  // This method is called when a pooled session (which doesn't have these
+  // properties initially) is returned by PortAllocator::GetPooledSession,
+  // and the content name, component, and ICE ufrag/pwd are updated.
+  //
+  // A subclass may need to override this method to perform additional actions,
+  // such as applying the updated information to ports and candidates.
+  virtual void UpdateTransportInformationInternal() {}

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

And here, SetIceParametersInternal might be a better name.

Actually, why not just make SetIceParameters protected and let the unit test
override the whole things and call the base SetIceParameters first instead of
having SetIceParameters call this thing last?

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

I don't like saying "if a derived class overrides this, it must call the base
class implementation". It's bad design; it creates unnecessary dependencies
between base and derived classes.

I did rename it though.

+
   // TODO(deadbeef): Get rid of these when everyone switches to ice_ufrag and
   // ice_pwd.
   const std::string& username() const { return ice_ufrag_; }
   const std::string& password() const { return ice_pwd_; }
 
-  std::string content_name_;
-  int component_;
-
  private:
+  void SetTransportInformation(const std::string& content_name,
+                               int component,
+                               const std::string& ice_ufrag,
+                               const std::string& ice_pwd) {
+    content_name_ = content_name;
+    component_ = component;
+    ice_ufrag_ = ice_ufrag;
+    ice_pwd_ = ice_pwd;
+    UpdateTransportInformationInternal();
+  }
+
   uint32_t flags_;
   uint32_t generation_;
+  std::string content_name_;
+  int component_;
   std::string ice_ufrag_;
   std::string ice_pwd_;
+
+  friend class PortAllocator;

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

Why do we need this?

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

SetTransportInformation is private. Only PortAllocator should call it.

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

Can you put a comment explaining this?

And... is there a good way to avoid this?

[Taylor Brandstetter, 2016/05/09 22:07:10]

This method can be either an implementation detail (private, as is) or part of
the interface (and called by P2PTransportChannel). I prefer the former since it
makes the interface simpler, and I can't think of why P2PTransportChannel would
get a pooled session and *not* want this method called.

 };
 
+// Note that all methods of this class that deal with sessions (such as
+// CreateSession, SetConfiguration, GetPooledSession, and the destructor) should
+// be called on the same thread. Other methods don't have this restriction,
+// but they aren't guaranteed to be synchronized.

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

As mentioned in another place, I think we should either say "all methods must be
called on the same thread" or "all methods can be called on any thread".  Having
it mixed is too easy to break later.  

So, we either do a thread hop here, or a thread hope for all calls, or use a
lock on the parts that need to be synchronized.  

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

I was just documenting the behavior we previously had, which was undocumented.
But I agree it should be fixed.

 class PortAllocator : public sigslot::has_slots<> {
  public:
   PortAllocator() :
@@ -174,10 +219,25 @@ class PortAllocator : public sigslot::has_slots<> {
   }
   virtual ~PortAllocator() {}
 
-  // Set STUN and TURN servers to be used in future sessions.
-  virtual void SetIceServers(
-      const ServerAddresses& stun_servers,
-      const std::vector<RelayServerConfig>& turn_servers) = 0;
+  // Set STUN and TURN servers to be used in future sessions, and set
+  // candidate pool size, as described in JSEP.
+  //
+  // If the servers are changing and the candidate pool size is nonzero,
+  // existing pooled sessions will be destroyed and new ones created.
+  //
+  // If the servers are not changing but the candidate pool size is,
+  // pooled sessions will be either created or destroyed as necessary.
+  void SetConfiguration(const ServerAddresses& stun_servers,
+                        const std::vector<RelayServerConfig>& turn_servers,
+                        int candidate_pool_size);
+
+  const ServerAddresses& stun_servers() const { return stun_servers_; }
+
+  const std::vector<RelayServerConfig>& turn_servers() const {
+    return turn_servers_;
+  }
+
+  int candidate_pool_size() const { return candidate_pool_size_; }
 
   // Sets the network types to ignore.
   // Values are defined by the AdapterType enum.
@@ -193,6 +253,19 @@ class PortAllocator : public sigslot::has_slots<> {
       const std::string& ice_ufrag,
       const std::string& ice_pwd);
 
+  // Get an available pooled session and set the transport information on it.
+  //
+  // Caller takes ownership of the returned session.
+  //
+  // If no pooled sessions are available, returns null.
+  PortAllocatorSession* GetPooledSession(const std::string& content_name,
+                                         int component,
+                                         const std::string& ice_ufrag,
+                                         const std::string& ice_pwd);

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

If it takes ownership, should it return a unique_ptr<PortAllocatorSession> to
make that clear?  And shouldn't it also be called something like
TakePooledSession?  

Finally, it seems weird that it both takes the session and alters it.  Can't the
caller alter it after taking it, if that's what it wants?

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

I was just matching CreateSession (which also returns a raw pointer even though
the caller also takes ownership). I can change that as well though.

I disagree with forcing the caller to alter it. That would impose an additional
requirement on the caller and also force SetTransportInformation to be public.

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

I can see the merit in "PopPooledSession + SetIceParameters"  in one method. 
But we do need a good name for it.  What's the opposite of "put in a generic
pool"?  Withdraw?   Actually, "Take" might be good for that.  
 

+
+  // Returns the next session that would be returned by GetPooledSession.
+  const PortAllocatorSession* PeekPooledSession() const;

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

It seems like this one should be called GetPooledSession(), since all it does is
get it.

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

"Get/Peek", "Pop/Peek", "Take/Get", I don't care much. "Get/Peek" matches our
current scheme though (for example, messagequeue.h).

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

I think I'd prefer Take/Get

[Taylor Brandstetter, 2016/05/09 22:07:10]

I don't like it much but I'll compromise... Done.

+
   uint32_t flags() const { return flags_; }
   void set_flags(uint32_t flags) { flags_ = flags; }
 
@@ -251,6 +324,16 @@ class PortAllocator : public sigslot::has_slots<> {
   bool allow_tcp_listen_;
   uint32_t candidate_filter_;
   std::string origin_;
+
+ private:
+  ServerAddresses stun_servers_;
+  std::vector<RelayServerConfig> turn_servers_;
+  // The last size passed into SetConfiguration.
+  int candidate_pool_size_ = 0;
+  // This variable represents the total number of pooled sessions
+  // both owned by this class and taken by GetPooledSession.
+  int allocated_pooled_sessions_ = 0;

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

Can we call these two numbers something similar, like
target_pooled_session_count_ and allocated_pooled_session_count_?  

And how is allocated_pooled_session_count_ different than
pooled_sessions_.size()?  Can you make that more clear in the comment?

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

It represents the number of currently pooled sessions + the ones that were given
away. So if GetPooledSession is called, pooled_sessions_.size() goes down but
allocated_pooled_sessions_ doesn't.

This is so that, for example, if you pool 2 sessions, then they both get taken,
then you change the pool size to 3, only 1 additional session gets pooled,
rather than 3.

I don't know how to make the comment any clearer though without making it too
long. What would you suggest?

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

OK, I get it now.  Yes, your comment is good already. 

+  std::deque<std::unique_ptr<PortAllocatorSession>> pooled_sessions_;
 };
 
 }  // namespace cricket