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

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 d83726eab5a5b53c1af9edbda62f93907ac5e22c..88506a41aad7d84617ebcbafaf150fc9cc827438 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -29,10 +29,19 @@ namespace {
 // messages for queuing up work for ourselves
 enum { MSG_SORT = 1, MSG_CHECK_AND_PING };
 
+// Mapped write states used by P2PTransportChannel::MappedWriteState.
+enum {
+  MAPPED_WRITE_STATE_WRITABLE = 1,
+  MAPPED_WRITE_STATE_PROBABLY_WRITABLE,
+  MAPPED_WRITE_STATE_UNRELIABLE,
+  MAPPED_WRITE_STATE_INIT,
+  MAPPED_WRITE_STATE_TIMEOUT
+};

[pthatcher1, 2016/06/15 19:12:46]

Can we just add Connection::STATE_WRITE_PRESUMED to the Connection state enum?

+
 // 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;
 }
@@ -51,155 +60,6 @@ cricket::PortInterface::CandidateOrigin GetOrigin(cricket::PortInterface* port,
     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.
-  }
-};
-
-// 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 {
@@ -250,7 +110,8 @@ P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
               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 /* turn_to_turn_createpermission_needed */) {
   uint32_t weak_ping_interval = ::strtoul(
       webrtc::field_trial::FindFullName("WebRTC-StunInterPacketDelay").c_str(),
       nullptr, 10);
@@ -438,6 +299,19 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) {
     LOG(LS_INFO) << "Set max strong interval to "
                  << config_.max_strong_interval;
   }
+
+  if (config.turn_to_turn_createpermission_needed !=
+      config_.turn_to_turn_createpermission_needed) {
+    if (!connections_.empty()) {
+      LOG(LS_ERROR) << "Trying to change TURN-TURN CreatePermission needed "
+                    << "while connections already exist!";
+    } else {
+      config_.turn_to_turn_createpermission_needed =
+          config.turn_to_turn_createpermission_needed;
+      LOG(LS_INFO) << "Set TURN-TURN CreatePermission needed to "
+                   << config_.turn_to_turn_createpermission_needed;
+    }
+  }

[pthatcher1, 2016/06/15 19:12:46]

Why not just allow it and document that it doesn't affect existing candidate
pairs?

[Taylor Brandstetter, 2016/06/16 00:13:40]

It's easier to just disallow it then to allow it in a half-working state
(probably without good unit tests), allowing hard-to-discover bugs to creep in.
We do this same thing in other places. For example, SetIceTiebreaker.

 }
 
 const IceConfig& P2PTransportChannel::config() const {
@@ -1063,6 +937,172 @@ void P2PTransportChannel::RequestSort() {
   }
 }
 
+int P2PTransportChannel::MappedWriteState(const cricket::Connection* a) const {
+  switch (a->write_state()) {
+    case Connection::STATE_WRITABLE:
+      return MAPPED_WRITE_STATE_WRITABLE;
+    case Connection::STATE_WRITE_UNRELIABLE:
+      return MAPPED_WRITE_STATE_UNRELIABLE;
+    case Connection::STATE_WRITE_INIT:
+      // If the connection hasn't received a binding response, but it's a TURN-
+      // TURN connection and CreatePermission isn't needed, the connection is
+      // "probably writable" which is just a step below writable.
+      if (!config_.turn_to_turn_createpermission_needed && IsRelayRelay(a)) {
+        return MAPPED_WRITE_STATE_PROBABLY_WRITABLE;
+      }
+      return MAPPED_WRITE_STATE_INIT;
+    case Connection::STATE_WRITE_TIMEOUT:
+      return MAPPED_WRITE_STATE_TIMEOUT;
+    default:
+      RTC_DCHECK(false);
+      return MAPPED_WRITE_STATE_TIMEOUT;

[honghaiz3, 2016/06/15 16:37:18]

Should we consider putting the write state mapping to the connection class
instead of this class? 
All it needs is to pass the config_.turn_to_turn_createpermission_needed to the
Connection class. If we do that, transportchannel need not be aware of the write
state mapping depending on whether it is turn-turn connection. 

[Taylor Brandstetter, 2016/06/15 17:08:56]

I considered that. But that would mean we'd be letting the connection sorting
logic leak from P2PTransportChannel to Connection, just to save some lines of
code. I guess that's ok if we consider that Connection is just an implementation
detail of P2PTransportChannel and will always be tightly coupled. But if that's
the decision, we should at least document it in a comment.

Peter, do you have a preference?

[pthatcher1, 2016/06/15 19:12:46]

What if we passed the initial Connection state into the Connection when we
create it?  If it's TURN-TURN and the flag is set, we pass in
STATE_WRITE_PRESUMED, and when it's not, we pass in STATE_WRITE_INIT.

Either that or just call set_write_state(Connection::STATE_WRITE_PRESUMED)
immediately after creating it in CreateConnection.  That would probably be
easier because we wouldn't have to add anything to the constructor or
Port::CreateConnection.

[honghaiz3, 2016/06/15 19:26:36]

There is one case in which the write_state reset to INIT (when the connection
had write_timedout but getting an stun ping I think). Should its state be
PROBABLY_WRITABLE in that case (at least the current CL assumed so)?
If the answer is yes, it would be easier to change the get_write_state() and
writable() method to do the mapping. 

[Taylor Brandstetter, 2016/06/16 00:13:40]

I *really* don't want to pass a state in, or make set_write_state public. That
would mean we treat Connection as both a POD struct and a class. There's also
the issue that a connection may initially be relay/prflx and change to
relay/relay.

I'll just pass the flag in as Honghai suggested. Actually, I'm passing the whole
IceConfig so that it will be easier if we want to add something later. Take a
look. Though it does make this CL significantly larger...

+  }
+}
+
+// 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.
+  if (MappedWriteState(a) < MappedWriteState(b)) {
+    return 1;
+  }
+  if (MappedWriteState(a) > MappedWriteState(b)) {
+    return -1;
+  }

[pthatcher1, 2016/06/15 19:12:46]

This could be like the other comps:
// Prefer better (lower) values
int write_state_comp = b->write_state() - a->write_state();
if (write_state_comp != 0) {
  return write_state_comp;
}

(Assuming the compiler doesn't get mad about substracting enums)

[Taylor Brandstetter, 2016/06/16 00:13:41]

Done.

+
+  // 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 {
+  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;
+  }

[pthatcher1, 2016/06/15 19:12:46]

To make it a lot like the other comp code, should we make it like so (which is
also shorter)?

// Prefer lower network cost
int cost_comp = (b->ComputeNetworkCost() - a->ComputeNetworkCost());
if (cost_comp != 0) 
  return cost_comp;
}

[Taylor Brandstetter, 2016/06/16 00:13:40]

Hmm. Now we're talking about subtracting unsigned ints that may possibly use all
their bits, which we can't even store the result of unless we change the return
type to int64_t. And even then it's fragile. So I'd prefer to leave this.

[pthatcher1, 2016/06/16 23:31:36]

True.

+
+  // Compare connection priority. Lower values get sorted last.
+  if (a->priority() > b->priority()) {
+    return 1;
+  }
+  if (a->priority() < b->priority()) {
+    return -1;
+  }

[pthatcher1, 2016/06/15 19:12:46]

Similarly here:

// Prefer higher priority
int priority_comp = (a->priority() - b->priority());
if (priority_comp != 0) {
  return priority_comp;
}

[Taylor Brandstetter, 2016/06/16 00:13:40]

See above. Plus, these are uint64_t's. Even if we returned an int64_t we
wouldn't have enough bits.

+
+  // If we're still tied at this point, prefer a younger generation.

[pthatcher1, 2016/06/15 19:12:46]

Could you add "(larger generation number)" to make it clear younger == bigger?

[Taylor Brandstetter, 2016/06/16 00:13:40]

Done.

+  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.

[pthatcher1, 2016/06/15 19:12:46]

That's a good point.  It would make sense for the controlled side to prioritize
the connections on which the controlled side is sending media, treating that
like a nomination (for regular nomination) or a "real nomination" (for
aggressive nomination).  Once we have "renomination", we should do something
similar where the last nominated connection is prioritized higher for pinging.  
In fact, that might lead to more simple code for doing selection (so we wouldn't
need so many methods here.

I think we should not have the controlling side also doing this.  

Honghai is going to be changing the controlled side soon anyway to make it
switch less often when the controlling side is aggressive.  I think that would
fit well with this thinking.

[Taylor Brandstetter, 2016/06/16 00:13:40]

FYI: I didn't write this comment, I just moved it and added myself as owner
since it didn't have an owner.

+}
+
+// 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::ShouldSwitch(const Connection* a,
+                                       const Connection* b) const {

[pthatcher1, 2016/06/15 19:12:46]

If you're going to move this, we might as well name it something more clear,
like ShouldReselectConnection or ShouldSwitchSelectedConnection. 

And changing from "a" and "b" to "selected" and "conn" might also make it more
readable.

[Taylor Brandstetter, 2016/06/16 00:13:41]

Done.

+  if (a == b) {
+    return false;
+  }
+
+  if (!a || !b) {  // don't think the latter should happen
+    return true;
+  }

[pthatcher1, 2016/06/15 19:12:46]

This might be more readable as:

if (!conn) {
  return false;  // This shouldn't happen.
}

if (!selected) {
  return true;
}

if (conn == selected) {
  return false;
}

[Taylor Brandstetter, 2016/06/16 00:13:40]

Done.

+
+  // 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, b);
+  if (state_cmp != 0) {
+    return state_cmp < 0;
+  }
+  if (ice_role_ == ICEROLE_CONTROLLED && a->nominated()) {

[pthatcher1, 2016/06/15 19:12:46]

Would it make sense to have a few helper methods?

private:

bool controlling() { return ice_role_ == ICEROLE_CONTROLLING || remote_ice_mode_
== ICEMODE_LITE ; }
bool controlled() { return !controlling(); }

Then this would be:

if (controlled() && selected->nominated()) {
  LOG(LS_VERBOSE) << "Controlled side did not switch due to nominated status";
  return false;
}

[Taylor Brandstetter, 2016/06/16 00:13:40]

We don't need the former function. If the other side is ICE_LITE, the transport
controller should have set our role to "controlling".

And I don't see that "controlled() && a->nominated()" is that much better than
"ice_role_ == ICEROLE_CONTROLLED && a->nominated()". I slightly prefer not
having an extra layer of indirection, and just checking the member variable
directly.

+    LOG(LS_VERBOSE) << "Controlled side did not switch due to nominated status";
+    return false;
+  }
+
+  int prefs_cmp = CompareConnectionCandidates(a, b);
+  if (prefs_cmp != 0) {
+    return prefs_cmp < 0;
+  }
+
+  return b->rtt() <= a->rtt() + kMinImprovement;

[pthatcher1, 2016/06/15 19:12:46]

This might be more readable as:

return (selected->rtt() - conn->rtt()) > kMinImprovement;

[Taylor Brandstetter, 2016/06/16 00:13:40]

Done.

+}
+
 // Sort the available connections to find the best one.  We also monitor
 // the number of available connections and the current state.
 void P2PTransportChannel::SortConnections() {
@@ -1079,8 +1119,10 @@ void P2PTransportChannel::SortConnections() {
   // 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) {
@@ -1093,7 +1135,7 @@ void P2PTransportChannel::SortConnections() {
   // 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 (ShouldSwitch(best_connection_, top_connection)) {
     LOG(LS_INFO) << "Switching best connection: " << top_connection->ToString();
     SwitchBestConnectionTo(top_connection);
   }
@@ -1237,7 +1279,14 @@ void P2PTransportChannel::UpdateState() {
     SignalStateChanged(this);
   }
 
-  bool writable = best_connection_ && best_connection_->writable();
+  // If our best connection is "probably writable" (TURN-TURN with no
+  // CreatePermission required), act like we're already writable to the upper

[pthatcher1, 2016/06/15 19:12:46]

I think the phrased "presumed writable" might be slightly better than "probably
writable.

[Taylor Brandstetter, 2016/06/16 00:13:41]

Done.

+  // layers, so they can start media quicker.
+  bool writable =

[pthatcher1, 2016/06/15 19:12:46]

Can we make a method called PresumedWritable()?  Then this would be
best_connection_ && PresumedWritable(best_connection_).  In fact, if
ProbablyWritable returned false when passed nullptr, it could just be "bool
writable = PresumedWritable(best_connection_);

[Taylor Brandstetter, 2016/06/16 00:13:41]

Done.

+      best_connection_ &&
+      (MappedWriteState(best_connection_) == MAPPED_WRITE_STATE_WRITABLE ||
+       MappedWriteState(best_connection_) ==
+           MAPPED_WRITE_STATE_PROBABLY_WRITABLE);
   set_writable(writable);
 
   bool receiving = false;