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

webrtc/p2p/base/port.cc

Index: webrtc/p2p/base/port.cc
diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc
index 0add358ec91392917091e957dfc39d28224be5af..411777e05c60e9010a06ef9926aad80a321246dd 100644
--- a/webrtc/p2p/base/port.cc
+++ b/webrtc/p2p/base/port.cc
@@ -825,11 +825,11 @@ class ConnectionRequest : public StunRequest {
 
 Connection::Connection(Port* port,
                        size_t index,
-                       const Candidate& remote_candidate)
+                       const Candidate& remote_candidate,
+                       const IceConfig& config)
     : port_(port),
       local_candidate_index_(index),
       remote_candidate_(remote_candidate),
-      write_state_(STATE_WRITE_INIT),
       receiving_(false),
       connected_(true),
       pruned_(false),
@@ -847,9 +847,12 @@ Connection::Connection(Port* port,
       reported_(false),
       state_(STATE_WAITING),
       receiving_timeout_(WEAK_CONNECTION_RECEIVE_TIMEOUT),
-      time_created_ms_(rtc::TimeMillis()) {
+      time_created_ms_(rtc::TimeMillis()),
+      fully_relayed_createpermission_needed_(
+          config.fully_relayed_createpermission_needed) {
   // All of our connections start in WAITING state.
   // TODO(mallinath) - Start connections from STATE_FROZEN.
+  write_state_ = InitialWriteState();

[honghaiz3, 2016/06/17 16:22:07]

Instead of setting the write_state_ every time it was set to WRITE_INIT, 
can we keep this as before, but only change the return value of write_state() to
be PRESUMED_WRITABLE (and change the writable()) if the write state is
WRITE_INIT and it is a turn-turn connection without needing permission request?
It will be more like mapping the write_state a little bit. Perhaps we can call
it mapped_writable_state() and only expose that method but keep the actual
write_state private. 
The benefit of this approach is that it won't mess up the current state machine
of write_state, and you don't need to worry about that someone else may set the
state to WRITE_INIT accidentally. 

For example, you haven't considered the state transition from PRESUMED_WRITABLE
to WRITE_UNRELIABLE. 

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

Isn't that effectively the same as having two enums? One method returns four
possible values, the other returns five.

I think if we're adding PRESUMED_WRITABLE as a WriteState, we should go all the
way, and make sure the state machine handles it in every circumstance.

What still needs to be done for PRESUMED_WRITABLE->WRITE_UNRELIABLE? Also, how
is this transition possible?

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

Which method will return four possible values?
My suggestion was that internally Connection only has 4 states for the
state-transition as before. But when exposed to outside, it has the extra state
presumed_writable. Conceptually, PRESUMED_WRITABLE is not a physically state of
a connection, it is a mapped state or an INIT state just pretending to be
writable. It only adds an enum state instead of creating an extra enum classes. 


If a turn-turn connection does not get stun ping response for 5 seconds, it will
become WRITE_UNRELIABLE, right? That's the part I am worried. If you think
PRESUMED_WRITABLE is a physical state, you need to handle the state transition
to and from it every time. If you think it is a mapped state, you don't need to
worry about the state transition to and from that state, just return a different
value when write_state() is called. 

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

I's like having one internal enum with 4 values and one external enum with 5
values.
Are you arguing that because the external enum has 5 states, that the internal
enum must also have 5 states?
PRESUME_WRITABLE is just INIT with an extra piece of information.  And
INIT->WRITE_UNRELIABLE is impossible, I believe.  So you're right that
transition shouldn't happen.

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

No, I think it would go to INIT (it would eventually stop presuming and just go
back).
I agree with Honghai that not storing that state internally seems better.  That
would make it easier to calculate the "presumability" on demand rather than
updating it after anything changes (like 5 failed pings).  

[honghaiz3, 2016/06/21 19:37:57]

Ah. You are right. I thought of presumed_writable as "writable". 
But still if we consider presumed_writable as write_init, we have missed the
transition from presumed_writable to write_timeout. 

On the other hand, there might be risk that we don't transit from
presumed_writable to write_unreliable because we may miss a state-change event
from writable to unwritable on the transport channel. 

[Taylor Brandstetter, 2016/06/22 00:37:07]

I'm completely confused. Peter, the reason I did this is because you said you
felt strongly about not having an extra enum. So, did you actually like the
extra enum, but you wanted it moved to Connection? If so, why? Is calling
MappedWriteState(conn) less convenient than calling conn->MappedWriteState()?
The latter requires passing the IceConfig down to the connection so it's
actually more complex.

[pthatcher1, 2016/06/22 05:46:13]

Sorry for the confusion.I like having one enum.

What I meant is we have two options:

A.  Do what you're doing and store write_state_ = PRESUMED_WRITABLE whenever it
would normally go to WRITE_INIT.
 
B.  Store WRITE_INIT and then make write_state() do this:

if (write_state_ == WRITE_INIT && presumed_writable()) {
  return PRESUMED_WRITABLE;
}
return write_state_;


Then you wouldn't need to worry about all the places where you change the
write_state_ to PRESUMED_WRITABLE.


I think I'd prefer B.  

[Taylor Brandstetter, 2016/06/22 15:34:43]

That's effectively the same as having two enums, as I said before. But it's
worse, because now someone reading the code has to know that write_state_ can
only take four possible values, but write_state() can take five values. It's not
implicit in the type any more. So that's something I'm very opposed to. If we
went this route, I would implement it by having Connection::InternalWriteState
which is private and Connection::WriteState which is public.

But then, that's basically the same as my original CL, but more complex. So
let's go a little deeper: what did you not like about the initial CL with
MappedWriteState? Was it the amount of extra code required? Worry that someone
may use Connection::WriteState when they meant to use MappedWriteState?
Confusion about having two enums with such similar meanings? Any of these
problems can be solved with minor changes to the original CL, but not like
this...

   // Wire up to send stun packets
   requests_.SignalSendPacket.connect(this, &Connection::OnSendStunPacket);
   LOG_J(LS_INFO, this) << "Connection created";
@@ -958,9 +961,9 @@ void Connection::OnReadPacket(
 
     // If timed out sending writability checks, start up again
     if (!pruned_ && (write_state_ == STATE_WRITE_TIMEOUT)) {
+      set_write_state(InitialWriteState());
       LOG(LS_WARNING) << "Received a data packet on a timed-out Connection. "
-                      << "Resetting state to STATE_WRITE_INIT.";
-      set_write_state(STATE_WRITE_INIT);
+                      << "Reset state to " << write_state_ << ".";
     }
   } else if (!msg) {
     // The packet was STUN, but failed a check and was handled internally.
@@ -1037,7 +1040,7 @@ void Connection::HandleBindingRequest(IceMessage* msg) {
 
   // If it timed out on writing check, start up again
   if (!pruned_ && write_state_ == STATE_WRITE_TIMEOUT) {
-    set_write_state(STATE_WRITE_INIT);
+    set_write_state(InitialWriteState());
   }
 
   if (port_->GetIceRole() == ICEROLE_CONTROLLED) {
@@ -1066,7 +1069,7 @@ void Connection::HandleBindingRequest(IceMessage* msg) {
 }
 
 void Connection::OnReadyToSend() {
-  if (write_state_ == STATE_WRITABLE) {
+  if (presumed_writable()) {
     SignalReadyToSend(this);
   }
 }
@@ -1246,11 +1249,12 @@ std::string Connection::ToString() const {
     '-',  // not receiving (false)
     'R',  // receiving (true)
   };
-  const char WRITE_STATE_ABBREV[4] = {
-    'W',  // STATE_WRITABLE
-    'w',  // STATE_WRITE_UNRELIABLE
-    '-',  // STATE_WRITE_INIT
-    'x',  // STATE_WRITE_TIMEOUT
+  const char WRITE_STATE_ABBREV[5] = {
+      'W',  // STATE_WRITABLE
+      'P',  // STATE_PRESUMED_WRITABLE
+      'w',  // STATE_WRITE_UNRELIABLE
+      '-',  // STATE_WRITE_INIT
+      'x',  // STATE_WRITE_TIMEOUT
   };
   const std::string ICESTATE[4] = {
     "W",  // STATE_WAITING
@@ -1367,6 +1371,15 @@ void Connection::OnConnectionRequestSent(ConnectionRequest* request) {
   }
 }
 
+Connection::WriteState Connection::InitialWriteState() const {
+  if (!fully_relayed_createpermission_needed_ &&
+      local_candidate().type() == RELAY_PORT_TYPE &&
+      remote_candidate().type() == RELAY_PORT_TYPE) {
+    return STATE_PRESUMED_WRITABLE;
+  }
+  return STATE_WRITE_INIT;
+}
+
 void Connection::HandleRoleConflictFromPeer() {
   port_->SignalRoleConflict(port_);
 }
@@ -1399,6 +1412,11 @@ void Connection::MaybeUpdatePeerReflexiveCandidate(
       remote_candidate_.password() == new_candidate.password() &&
       remote_candidate_.generation() == new_candidate.generation()) {
     remote_candidate_ = new_candidate;
+    // If it turns out that the peer reflexive candidate was a TURN candidate,
+    // we may need to change the state to STATE_PRESUMED_WRITABLE.
+    if (write_state() == STATE_WRITE_INIT) {
+      set_write_state(InitialWriteState());
+    }

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

Oh, this is a good point.  It would be faster if we presumed writable if the
local type is relay and remote type is peer reflexive.  If it's peer reflexive,
that means we got a ping from the other side and can start sending media/DTLS
even before the remote candidate is signalled to us.

In other words, up in the InitialWriteState(), change "remote_candidate().type()
== RELAY_PORT_TYPE" to "(remote_candidate().type() == RELAY_PORT_TYPE ||
remote_candidate().type() == PRFLX_PORT_TYPE)"

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

But we don't know that the prflx candidate is a TURN candidate. In the long term
this can be resolved with a new STUN attribute, as we talked about, but for now
it seems risky to assume TURN<->PRFLX will always work.

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

I don't think it's risky to presume a TURN<->PRFLX candidate pair is writable. 
It's very likely to be writable.   I  might even argue that we could presume
writability for all candidate pairs with a remote peer reflexive candidate,
because it's so likely we'll be able to ping back.  That's why we have triggered
checks: because it's so likely.  

If we don't presume it's writable, then our outgoing pings will be delayed until
we receive the signaled candidates, which may be a fair bit later.   So I think
we should fix this.

[Taylor Brandstetter, 2016/06/22 00:37:07]

The outgoing pings won't be delayed. Just the outgoing DTLS server hello. But
it's already delayed until a remote description is received, so it doesn't make
much difference.

But, I guess it's worth a try. I just strongly want to avoid the scenario where
we ramp up our DTLS retransmission timer on a connection that won't succeed.

[pthatcher1, 2016/06/22 05:46:13]

Sorry, you're right.  I meant the outgoing dtls/media.  DTLS will be delayed as
long as the time between when we set the remote fingerprint and when we add a
remote ICE candidate is greater than 0, which it always is (for lots of
reasons).
If you're right about the time between staring DTLS and getting the remote
candidate being 0 (no delay), and all of the candidate pairs are TURN-TURN, then
the behavior here is identical, is it not?

In any case, I think it's very, very likely to succeed, so we're not risking
much.

   }
 }
 
@@ -1492,8 +1510,9 @@ void Connection::MaybeAddPrflxCandidate(ConnectionRequest* request,
 
 ProxyConnection::ProxyConnection(Port* port,
                                  size_t index,
-                                 const Candidate& remote_candidate)
-    : Connection(port, index, remote_candidate) {}
+                                 const Candidate& remote_candidate,
+                                 const IceConfig& config)
+    : Connection(port, index, remote_candidate, config) {}
 
 int ProxyConnection::Send(const void* data, size_t size,
                           const rtc::PacketOptions& options) {