Relanding: Implement RTCConfiguration.iceCandidatePoolSize.

webrtc/p2p/base/portallocator.h

 /*
  *  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.
  */
 
 #ifndef WEBRTC_P2P_BASE_PORTALLOCATOR_H_
 #define WEBRTC_P2P_BASE_PORTALLOCATOR_H_
 
+#include <deque>
+#include <memory>
 #include <string>
 #include <vector>
 
 #include "webrtc/p2p/base/port.h"
 #include "webrtc/p2p/base/portinterface.h"
 #include "webrtc/base/helpers.h"
 #include "webrtc/base/proxyinfo.h"
 #include "webrtc/base/sigslot.h"
+#include "webrtc/base/thread.h"
 
 namespace cricket {
 
 // PortAllocator is responsible for allocating Port types for a given
 // P2PSocket. It also handles port freeing.
 //
 // Clients can override this class to control port allocation, including
 // what kinds of ports are allocated.
 
 enum {
@@ skipping 43 matching lines @@
   CF_RELAY = 0x4,
   CF_ALL = 0x7,
 };
 
 // TODO(deadbeef): Rename to TurnCredentials (and username to ufrag).
 struct RelayCredentials {
   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;
 };
 
 typedef std::vector<ProtocolAddress> PortList;
 // TODO(deadbeef): Rename to TurnServerConfig.
 struct RelayServerConfig {
   RelayServerConfig(RelayType type) : type(type), priority(0) {}
 
   RelayServerConfig(const std::string& address,
                     int port,
                     const std::string& username,
                     const std::string& password,
                     ProtocolType proto,
                     bool secure)
       : type(RELAY_TURN), credentials(username, password) {
     ports.push_back(
         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;
   int priority;
 };
 
 class PortAllocatorSession : public sigslot::has_slots<> {
  public:
   // Content name passed in mostly for logging and debugging.
   PortAllocatorSession(const std::string& content_name,
                        int component,
                        const std::string& ice_ufrag,
                        const std::string& ice_pwd,
                        uint32_t flags);
 
   // Subclasses should clean up any ports created.
   virtual ~PortAllocatorSession() {}
 
   uint32_t flags() const { return flags_; }
   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;
   virtual void StopGettingPorts() = 0;
   // Only stop the existing gathering process but may start new ones if needed.
   virtual void ClearGettingPorts() = 0;
   // 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;
   sigslot::signal1<PortAllocatorSession*> SignalCandidatesAllocationDone;
 
   virtual uint32_t generation() { return generation_; }
   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.

 
- protected:
   // 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_; }
 
+ 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_;
-
- private:
-  uint32_t flags_;
-  uint32_t generation_;
   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() :
       flags_(kDefaultPortAllocatorFlags),
       min_port_(0),
       max_port_(0),
       step_delay_(kDefaultStepDelay),
       allow_tcp_listen_(true),
       candidate_filter_(CF_ALL) {
     // This will allow us to have old behavior on non webrtc clients.
   }
   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.
   // For instance, calling this with
   // ADAPTER_TYPE_ETHERNET | ADAPTER_TYPE_LOOPBACK will ignore Ethernet and
   // loopback interfaces.
   virtual void SetNetworkIgnoreMask(int network_ignore_mask) = 0;
 
   PortAllocatorSession* CreateSession(
       const std::string& sid,
       const std::string& content_name,
       int component,
       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; }
 
   const std::string& user_agent() const { return agent_; }
   const rtc::ProxyInfo& proxy() const { return proxy_; }
   void set_proxy(const std::string& agent, const rtc::ProxyInfo& proxy) {
     agent_ = agent;
     proxy_ = proxy;
   }
 
@@ skipping 38 matching lines @@
 
   uint32_t flags_;
   std::string agent_;
   rtc::ProxyInfo proxy_;
   int min_port_;
   int max_port_;
   uint32_t step_delay_;
   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
 
 #endif  // WEBRTC_P2P_BASE_PORTALLOCATOR_H_