Adding IceConfig option to assume TURN/TURN candidate pairs will work.

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 14 matching lines @@
 #include "webrtc/system_wrappers/include/field_trial.h"
 
 namespace {
 
 // messages for queuing up work for ourselves
 enum { MSG_SORT = 1, MSG_CHECK_AND_PING };
 
 // The minimum improvement in RTT that justifies a switch.
 static const double kMinImprovement = 10;
 
-bool IsRelayRelay(cricket::Connection* conn) {
+bool IsRelayRelay(const cricket::Connection* conn) {
   return conn->local_candidate().type() == cricket::RELAY_PORT_TYPE &&
          conn->remote_candidate().type() == cricket::RELAY_PORT_TYPE;
 }
 
 bool IsUdp(cricket::Connection* conn) {
   return conn->local_candidate().relay_protocol() == cricket::UDP_PROTOCOL_NAME;
 }
 
 cricket::PortInterface::CandidateOrigin GetOrigin(cricket::PortInterface* port,
                                          cricket::PortInterface* origin_port) {
   if (!origin_port)
     return cricket::PortInterface::ORIGIN_MESSAGE;
   else if (port == origin_port)
     return cricket::PortInterface::ORIGIN_THIS_PORT;
   else
     return cricket::PortInterface::ORIGIN_OTHER_PORT;
 }
 
-// Compares two connections based only on the candidate and network information.
-// Returns positive if |a| is better than |b|.
-int CompareConnectionCandidates(cricket::Connection* a,
-                                cricket::Connection* b) {
-  uint32_t a_cost = a->ComputeNetworkCost();
-  uint32_t b_cost = b->ComputeNetworkCost();
-  // Smaller cost is better.
-  if (a_cost < b_cost) {
-    return 1;
-  }
-  if (a_cost > b_cost) {
-    return -1;
-  }
-
-  // Compare connection priority. Lower values get sorted last.
-  if (a->priority() > b->priority())
-    return 1;
-  if (a->priority() < b->priority())
-    return -1;
-
-  // If we're still tied at this point, prefer a younger generation.
-  return (a->remote_candidate().generation() + a->port()->generation()) -
-         (b->remote_candidate().generation() + b->port()->generation());
-}
-
-// Compare two connections based on their writing, receiving, and connected
-// states.
-int CompareConnectionStates(cricket::Connection* a, cricket::Connection* b) {
-  // Sort based on write-state.  Better states have lower values.
-  if (a->write_state() < b->write_state())
-    return 1;
-  if (a->write_state() > b->write_state())
-    return -1;
-
-  // We prefer a receiving connection to a non-receiving, higher-priority
-  // connection when sorting connections and choosing which connection to
-  // switch to.
-  if (a->receiving() && !b->receiving())
-    return 1;
-  if (!a->receiving() && b->receiving())
-    return -1;
-
-  // WARNING: Some complexity here about TCP reconnecting.
-  // When a TCP connection fails because of a TCP socket disconnecting, the
-  // active side of the connection will attempt to reconnect for 5 seconds while
-  // pretending to be writable (the connection is not set to the unwritable
-  // state).  On the passive side, the connection also remains writable even
-  // though it is disconnected, and a new connection is created when the active
-  // side connects.  At that point, there are two TCP connections on the passive
-  // side: 1. the old, disconnected one that is pretending to be writable, and
-  // 2.  the new, connected one that is maybe not yet writable.  For purposes of
-  // pruning, pinging, and selecting the best connection, we want to treat the
-  // new connection as "better" than the old one.  We could add a method called
-  // something like Connection::ImReallyBadEvenThoughImWritable, but that is
-  // equivalent to the existing Connection::connected(), which we already have.
-  // So, in code throughout this file, we'll check whether the connection is
-  // connected() or not, and if it is not, treat it as "worse" than a connected
-  // one, even though it's writable.  In the code below, we're doing so to make
-  // sure we treat a new writable connection as better than an old disconnected
-  // connection.
-
-  // In the case where we reconnect TCP connections, the original best
-  // connection is disconnected without changing to WRITE_TIMEOUT. In this case,
-  // the new connection, when it becomes writable, should have higher priority.
-  if (a->write_state() == cricket::Connection::STATE_WRITABLE &&
-      b->write_state() == cricket::Connection::STATE_WRITABLE) {
-    if (a->connected() && !b->connected()) {
-      return 1;
-    }
-    if (!a->connected() && b->connected()) {
-      return -1;
-    }
-  }
-  return 0;
-}
-
-int CompareConnections(cricket::Connection* a, cricket::Connection* b) {
-  int state_cmp = CompareConnectionStates(a, b);
-  if (state_cmp != 0) {
-    return state_cmp;
-  }
-  // Compare the candidate information.
-  return CompareConnectionCandidates(a, b);
-}
-
-// Wraps the comparison connection into a less than operator that puts higher
-// priority writable connections first.
-class ConnectionCompare {
- public:
-  bool operator()(const cricket::Connection *ca,
-                  const cricket::Connection *cb) {
-    cricket::Connection* a = const_cast<cricket::Connection*>(ca);
-    cricket::Connection* b = const_cast<cricket::Connection*>(cb);
-
-    // Compare first on writability and static preferences.
-    int cmp = CompareConnections(a, b);
-    if (cmp > 0)
-      return true;
-    if (cmp < 0)
-      return false;
-
-    // Otherwise, sort based on latency estimate.
-    return a->rtt() < b->rtt();
-
-    // Should we bother checking for the last connection that last received
-    // data? It would help rendezvous on the connection that is also receiving
-    // packets.
-    //
-    // TODO: Yes we should definitely do this.  The TCP protocol gains
-    // efficiency by being used bidirectionally, as opposed to two separate
-    // unidirectional streams.  This test should probably occur before
-    // comparison of local prefs (assuming combined prefs are the same).  We
-    // need to be careful though, not to bounce back and forth with both sides
-    // trying to rendevous with the other.
-  }
-};

[honghaiz3, 2016/06/17 16:28:16]

I think you don't need to put anything above this point to the class variable. 
You only need to do so if you need to access some class variable like ice_role. 

[Taylor Brandstetter, 2016/06/20 17:15:45]

I no longer need to, but it doesn't hurt, and it may be useful in the future.
Also, didn't Peter mention that one of your CLs would benefit from this?

[honghaiz3, 2016/06/20 22:31:03]

I don't see a reason of needing the transportchannel internal states/variables
when comparing the the CompareConnectionState, or CompareConnectionCandidates
between two connections in the foreseeable future. If it happens in the future,
we can do that later. 
ShouldSwitch and ConnectionCompare may depend on the transportchannel(ice role)
and it is a good idea to move them to the class. 

What I will need in my CL is the latter two but not the former two. 

[pthatcher1, 2016/06/21 06:20:50]

I think we should make these all member methods or none member methods.  It's
weird to have it split.  And since we need some of them to be member methods,
I'd prefer them all to be member methods.

-
-// Determines whether we should switch between two connections, based first on
-// connection states, static preferences, and then (if those are equal) on
-// latency estimates.
-bool ShouldSwitch(cricket::Connection* a_conn,
-                  cricket::Connection* b_conn,
-                  cricket::IceRole ice_role) {
-  if (a_conn == b_conn)
-    return false;
-
-  if (!a_conn || !b_conn)  // don't think the latter should happen
-    return true;
-
-  // We prefer to switch to a writable and receiving connection over a
-  // non-writable or non-receiving connection, even if the latter has
-  // been nominated by the controlling side.
-  int state_cmp = CompareConnectionStates(a_conn, b_conn);
-  if (state_cmp != 0) {
-    return state_cmp < 0;
-  }
-  if (ice_role == cricket::ICEROLE_CONTROLLED && a_conn->nominated()) {
-    LOG(LS_VERBOSE) << "Controlled side did not switch due to nominated status";
-    return false;
-  }
-
-  int prefs_cmp = CompareConnectionCandidates(a_conn, b_conn);
-  if (prefs_cmp != 0) {
-    return prefs_cmp < 0;
-  }
-
-  return b_conn->rtt() <= a_conn->rtt() + kMinImprovement;
-}
-
 }  // unnamed namespace
 
 namespace cricket {
 
 // When the socket is unwritable, we will use 10 Kbps (ignoring IP+UDP headers)
 // for pinging.  When the socket is writable, we will use only 1 Kbps because
 // we don't want to degrade the quality on a modem.  These numbers should work
 // well on a 28.8K modem, which is the slowest connection on which the voice
 // quality is reasonable at all.
 static const int PING_PACKET_SIZE = 60 * 8;
@@ skipping 30 matching lines @@
       sort_dirty_(false),
       remote_ice_mode_(ICEMODE_FULL),
       ice_role_(ICEROLE_UNKNOWN),
       tiebreaker_(0),
       gathering_state_(kIceGatheringNew),
       check_receiving_interval_(MIN_CHECK_RECEIVING_INTERVAL * 5),
       config_(MIN_CHECK_RECEIVING_INTERVAL * 50 /* receiving_timeout */,
               0 /* backup_connection_ping_interval */,
               false /* gather_continually */,
               false /* prioritize_most_likely_candidate_pairs */,
-              MAX_CURRENT_STRONG_INTERVAL /* max_strong_interval */) {
+              MAX_CURRENT_STRONG_INTERVAL /* max_strong_interval */,
+              true /* fully_relayed_createpermission_needed */) {
   uint32_t weak_ping_interval = ::strtoul(
       webrtc::field_trial::FindFullName("WebRTC-StunInterPacketDelay").c_str(),
       nullptr, 10);
   if (weak_ping_interval) {
     weak_ping_interval_ = static_cast<int>(weak_ping_interval);
   }
 }
 
 P2PTransportChannel::~P2PTransportChannel() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
@@ skipping 167 matching lines @@
       config.prioritize_most_likely_candidate_pairs;
   LOG(LS_INFO) << "Set ping most likely connection to "
                << config_.prioritize_most_likely_candidate_pairs;
 
   if (config.max_strong_interval >= 0 &&
       config_.max_strong_interval != config.max_strong_interval) {
     config_.max_strong_interval = config.max_strong_interval;
     LOG(LS_INFO) << "Set max strong interval to "
                  << config_.max_strong_interval;
   }
+
+  if (config.fully_relayed_createpermission_needed !=
+      config_.fully_relayed_createpermission_needed) {
+    if (!connections_.empty()) {
+      LOG(LS_ERROR) << "Trying to change TURN-TURN CreatePermission needed "
+                    << "while connections already exist!";
+    } else {
+      config_.fully_relayed_createpermission_needed =
+          config.fully_relayed_createpermission_needed;
+      LOG(LS_INFO) << "Set fully-relayed CreatePermission needed to "
+                   << config_.fully_relayed_createpermission_needed;
+    }
+  }
 }
 
 const IceConfig& P2PTransportChannel::config() const {
   return config_;
 }
 
 // Go into the state of processing candidates, and running in general
 void P2PTransportChannel::Connect() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   if (ice_ufrag_.empty() || ice_pwd_.empty()) {
@@ skipping 226 matching lines @@
       return;
     } else {
       ASSERT(false);
       port->SendBindingErrorResponse(stun_msg, address,
                                      STUN_ERROR_SERVER_ERROR,
                                      STUN_ERROR_REASON_SERVER_ERROR);
       return;
     }
   }
 
-  Connection* connection =
-      port->CreateConnection(remote_candidate, PortInterface::ORIGIN_THIS_PORT);
+  Connection* connection = port->CreateConnection(
+      remote_candidate, PortInterface::ORIGIN_THIS_PORT, config_);
   if (!connection) {
     ASSERT(false);
     port->SendBindingErrorResponse(stun_msg, address, STUN_ERROR_SERVER_ERROR,
                                    STUN_ERROR_REASON_SERVER_ERROR);
     return;
   }
 
   LOG(LS_INFO) << "Adding connection from "
                << (remote_candidate_is_new ? "peer reflexive" : "resurrected")
                << " candidate: " << remote_candidate.ToString();
@@ skipping 161 matching lines @@
   Connection* connection = port->GetConnection(remote_candidate.address());
   if (connection == nullptr ||
       connection->remote_candidate().generation() <
           remote_candidate.generation()) {
     // Don't create a connection if this is a candidate we received in a
     // message and we are not allowed to make outgoing connections.
     PortInterface::CandidateOrigin origin = GetOrigin(port, origin_port);
     if (origin == PortInterface::ORIGIN_MESSAGE && incoming_only_) {
       return false;
     }
-    Connection* connection = port->CreateConnection(remote_candidate, origin);
+    Connection* connection =
+        port->CreateConnection(remote_candidate, origin, config_);
     if (!connection) {
       return false;
     }
     AddConnection(connection);
     LOG_J(LS_INFO, this) << "Created connection with origin=" << origin << ", ("
                          << connections_.size() << " total)";
     return true;
   }
 
   // No new connection was created.
@@ skipping 175 matching lines @@
 }
 
 // Prepare for best candidate sorting.
 void P2PTransportChannel::RequestSort() {
   if (!sort_dirty_) {
     worker_thread_->Post(RTC_FROM_HERE, this, MSG_SORT);
     sort_dirty_ = true;
   }
 }
 
+// Compare two connections based on their writing, receiving, and connected
+// states.
+int P2PTransportChannel::CompareConnectionStates(const Connection* a,
+                                                 const Connection* b) const {
+  // Sort based on write-state. Better states have lower values.
+  int write_state_cmp =
+      static_cast<int>(b->write_state()) - static_cast<int>(a->write_state());
+  if (write_state_cmp != 0) {
+    return write_state_cmp;
+  }
+
+  // We prefer a receiving connection to a non-receiving, higher-priority
+  // connection when sorting connections and choosing which connection to
+  // switch to.
+  if (a->receiving() && !b->receiving()) {
+    return 1;
+  }
+  if (!a->receiving() && b->receiving()) {
+    return -1;
+  }
+
+  // WARNING: Some complexity here about TCP reconnecting.
+  // When a TCP connection fails because of a TCP socket disconnecting, the
+  // active side of the connection will attempt to reconnect for 5 seconds while
+  // pretending to be writable (the connection is not set to the unwritable
+  // state).  On the passive side, the connection also remains writable even
+  // though it is disconnected, and a new connection is created when the active
+  // side connects.  At that point, there are two TCP connections on the passive
+  // side: 1. the old, disconnected one that is pretending to be writable, and
+  // 2.  the new, connected one that is maybe not yet writable.  For purposes of
+  // pruning, pinging, and selecting the best connection, we want to treat the
+  // new connection as "better" than the old one.  We could add a method called
+  // something like Connection::ImReallyBadEvenThoughImWritable, but that is
+  // equivalent to the existing Connection::connected(), which we already have.
+  // So, in code throughout this file, we'll check whether the connection is
+  // connected() or not, and if it is not, treat it as "worse" than a connected
+  // one, even though it's writable.  In the code below, we're doing so to make
+  // sure we treat a new writable connection as better than an old disconnected
+  // connection.
+
+  // In the case where we reconnect TCP connections, the original best
+  // connection is disconnected without changing to WRITE_TIMEOUT. In this case,
+  // the new connection, when it becomes writable, should have higher priority.
+  if (a->write_state() == Connection::STATE_WRITABLE &&
+      b->write_state() == Connection::STATE_WRITABLE) {
+    if (a->connected() && !b->connected()) {
+      return 1;
+    }
+    if (!a->connected() && b->connected()) {
+      return -1;
+    }
+  }
+  return 0;
+}
+
+// Compares two connections based only on the candidate and network information.
+// Returns positive if |a| is better than |b|.
+int P2PTransportChannel::CompareConnectionCandidates(
+    const Connection* a,
+    const Connection* b) const {
+  // Prefer lower network cost.
+  uint32_t a_cost = a->ComputeNetworkCost();
+  uint32_t b_cost = b->ComputeNetworkCost();
+  // Smaller cost is better.
+  if (a_cost < b_cost) {
+    return 1;
+  }
+  if (a_cost > b_cost) {
+    return -1;
+  }
+
+  // Compare connection priority. Lower values get sorted last.
+  if (a->priority() > b->priority()) {
+    return 1;
+  }
+  if (a->priority() < b->priority()) {
+    return -1;
+  }
+
+  // If we're still tied at this point, prefer a younger generation.
+  // (Younger generation means a larger generation number).
+  return (a->remote_candidate().generation() + a->port()->generation()) -
+         (b->remote_candidate().generation() + b->port()->generation());
+}
+
+int P2PTransportChannel::CompareConnections(const Connection* a,
+                                            const Connection* b) const {
+  // Compare first on writability and static preferences.
+  int state_cmp = CompareConnectionStates(a, b);
+  if (state_cmp != 0) {
+    return state_cmp;
+  }
+  // Then compare the candidate information.
+  int candidates_cmp = CompareConnectionCandidates(a, b);
+  if (candidates_cmp != 0) {
+    return candidates_cmp;
+  }
+  // Otherwise, compare based on latency estimate.
+  return b->rtt() - a->rtt();
+
+  // Should we bother checking for the last connection that last received
+  // data? It would help rendezvous on the connection that is also receiving
+  // packets.
+  //
+  // TODO(deadbeef): Yes we should definitely do this.  The TCP protocol gains
+  // efficiency by being used bidirectionally, as opposed to two separate
+  // unidirectional streams.  This test should probably occur before
+  // comparison of local prefs (assuming combined prefs are the same).  We
+  // need to be careful though, not to bounce back and forth with both sides
+  // trying to rendevous with the other.
+}
+
+// Determines whether we should switch between two connections, based first on
+// connection states, static preferences, and then (if those are equal) on
+// latency estimates.
+bool P2PTransportChannel::ShouldSwitchSelectedConnection(
+    const Connection* selected,
+    const Connection* conn) const {
+  if (selected == conn) {
+    return false;
+  }
+
+  if (!selected || !conn) {  // don't think the latter should happen
+    return true;
+  }
+
+  // We prefer to switch to a writable and receiving connection over a
+  // non-writable or non-receiving connection, even if the latter has
+  // been nominated by the controlling side.
+  int state_cmp = CompareConnectionStates(selected, conn);
+  if (state_cmp != 0) {
+    return state_cmp < 0;
+  }
+  if (ice_role_ == ICEROLE_CONTROLLED && selected->nominated()) {
+    LOG(LS_VERBOSE) << "Controlled side did not switch due to nominated status";
+    return false;
+  }
+
+  int prefs_cmp = CompareConnectionCandidates(selected, conn);
+  if (prefs_cmp != 0) {
+    return prefs_cmp < 0;
+  }
+
+  return selected->rtt() - conn->rtt() >= kMinImprovement;
+}
+
 // Sort the available connections to find the best one.  We also monitor
 // the number of available connections and the current state.
 void P2PTransportChannel::SortConnections() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   // Make sure the connection states are up-to-date since this affects how they
   // will be sorted.
   UpdateConnectionStates();
 
   // Any changes after this point will require a re-sort.
   sort_dirty_ = false;
 
   // Find the best alternative connection by sorting.  It is important to note
   // that amongst equal preference, writable connections, this will choose the
   // one whose estimated latency is lowest.  So it is the only one that we
   // need to consider switching to.
-  ConnectionCompare cmp;
-  std::stable_sort(connections_.begin(), connections_.end(), cmp);
+  std::stable_sort(connections_.begin(), connections_.end(),
+                   [this](const Connection* a, const Connection* b) {
+                     return CompareConnections(a, b) > 0;
+                   });
   LOG(LS_VERBOSE) << "Sorting " << connections_.size()
                   << " available connections:";
   for (size_t i = 0; i < connections_.size(); ++i) {
     LOG(LS_VERBOSE) << connections_[i]->ToString();
   }
 
   Connection* top_connection =
       (connections_.size() > 0) ? connections_[0] : nullptr;
 
   // If necessary, switch to the new choice.
   // Note that |top_connection| doesn't have to be writable to become the best
   // connection although it will have higher priority if it is writable.
-  if (ShouldSwitch(best_connection_, top_connection, ice_role_)) {
+  if (ShouldSwitchSelectedConnection(best_connection_, top_connection)) {
     LOG(LS_INFO) << "Switching best connection: " << top_connection->ToString();
     SwitchBestConnectionTo(top_connection);
   }
 
   // Controlled side can prune only if the best connection has been nominated.
   // because otherwise it may delete the connection that will be selected by
   // the controlling side.
   if (ice_role_ == ICEROLE_CONTROLLING || best_nominated_connection()) {
     PruneConnections();
   }
@@ skipping 68 matching lines @@
                            << old_best_connection->ToString();
     }
     LOG_J(LS_INFO, this) << "New best connection: "
                          << best_connection_->ToString();
     SignalRouteChange(this, best_connection_->remote_candidate());
     // This is a temporary, but safe fix to webrtc issue 5705.
     // TODO(honghaiz): Make all EWOULDBLOCK error routed through the transport
     // channel so that it knows whether the media channel is allowed to
     // send; then it will only signal ready-to-send if the media channel
     // has been disallowed to send.
-    if (best_connection_->writable()) {
+    if (best_connection_->presumed_writable()) {
       SignalReadyToSend(this);
     }
   } else {
     LOG_J(LS_INFO, this) << "No best connection";
   }
   // TODO(honghaiz): rename best_connection_ with selected_connection_ or
   // selected_candidate pair_.
   SignalSelectedCandidatePairChanged(this, best_connection_,
                                      last_sent_packet_id_);
 }
@@ skipping 34 matching lines @@
         RTC_DCHECK(state == STATE_CONNECTING || state == STATE_COMPLETED);
         break;
       default:
         RTC_DCHECK(false);
         break;
     }
     state_ = state;
     SignalStateChanged(this);
   }
 
-  bool writable = best_connection_ && best_connection_->writable();
-  set_writable(writable);
+  // If our best connection is "presumed writable" (TURN-TURN with no
+  // CreatePermission required), act like we're already writable to the upper
+  // layers, so they can start media quicker.
+  set_writable(best_connection_ && best_connection_->presumed_writable());
 
   bool receiving = false;
   for (const Connection* connection : connections_) {
     if (connection->receiving()) {
       receiving = true;
       break;
     }
   }
   set_receiving(receiving);
 }
@@ skipping 436 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