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

Taylor Brandstetter

deadbeef@webrtc.org changed reviewers: + honghaiz@webrtc.org, pthatcher@webrtc.org

4 years, 6 months ago (2016-06-14 22:25:26 UTC) #1

Taylor Brandstetter

https://codereview.webrtc.org/2063823008/diff/1/webrtc/p2p/base/p2ptransportchannel.cc File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2063823008/diff/1/webrtc/p2p/base/p2ptransportchannel.cc#newcode970 webrtc/p2p/base/p2ptransportchannel.cc:970: return -1; This is the only difference from the ...

4 years, 6 months ago (2016-06-14 22:25:27 UTC) #2

honghaiz3

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransportchannel.cc File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransportchannel.cc#newcode958 webrtc/p2p/base/p2ptransportchannel.cc:958: return MAPPED_WRITE_STATE_TIMEOUT; Should we consider putting the write state ...

4 years, 6 months ago (2016-06-15 16:37:18 UTC) #3

Taylor Brandstetter

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransportchannel.cc File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransportchannel.cc#newcode958 webrtc/p2p/base/p2ptransportchannel.cc:958: return MAPPED_WRITE_STATE_TIMEOUT; On 2016/06/15 16:37:18, honghaiz3 wrote: > Should ...

4 years, 6 months ago (2016-06-15 17:08:56 UTC) #4

pthatcher1

Mostly just style stuff. The only thing I feel strongly about is avoiding an extra ...

4 years, 6 months ago (2016-06-15 19:12:47 UTC) #5

Mostly just style stuff.

The only thing I feel strongly about is avoiding an extra enum.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:39: };
Can we just add Connection::STATE_WRITE_PRESUMED to the Connection state enum?

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:314: }
Why not just allow it and document that it doesn't affect existing candidate
pairs?

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:958: return MAPPED_WRITE_STATE_TIMEOUT;
On 2016/06/15 17:08:56, Taylor Brandstetter wrote:
> On 2016/06/15 16:37:18, honghaiz3 wrote:
> > 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. 
> 
> 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?

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.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:972: }
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)

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1031: }
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;
}

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1039: }
Similarly here:

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

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1041: // If we're still tied at this
point, prefer a younger generation.
Could you add "(larger generation number)" to make it clear younger == bigger?

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1070: // trying to rendevous with the
other.
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.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1077: const Connection* b) const {
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.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1084: }
This might be more readable as:

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

if (!selected) {
  return true;
}

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

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1093: if (ice_role_ == ICEROLE_CONTROLLED
&& a->nominated()) {
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;
}

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1103: return b->rtt() <= a->rtt() +
kMinImprovement;
This might be more readable as:

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

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1283: // CreatePermission required), act
like we're already writable to the upper
I think the phrased "presumed writable" might be slightly better than "probably
writable.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1285: bool writable =
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_);

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:116: cricket::Candidate
CreateHostCandidate(const std::string& ip,
CreateUdpHostCandidate?

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:130: cricket::Candidate
CreateRelayCandidate(const std::string& ip,
CreateUdpRelayCandidate?

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:141: return c;
It seems like this could use a common CreateUdpCandidate(type, ip, port,
priority, ufrag);

Then you'd have:

return CreateUdpCandidate(cricket::RELAY_PORT_TYPE, ip, port, priority, ufrag);

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1362:
EXPECT_EQ(ICEROLE_CONTROLLED, ports_before[i]->GetIceRole());
Could we split this CL in 2, one for doing this cleanup and refactoring, and one
for adding the new control knob?  It would make the diffs more clear.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1580: // false and there's a
TURN-TURN candidate pair, it's assumed to be writable
I think it's more "presumed" than "assumed":

http://grammarist.com/usage/assume-presume/

But I'm not sure, nor am I sure that it matters.  What do you think?

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transport.h:179: bool turn_to_turn_createpermission_needed =
true;
I was thinking of a name like presume_connected_fully_relayed_candidate_pairs =
false;  

Later we'll want something in RtcConfig that's similar, perhaps
presume_connected_fully_relayed_ice.

"Fully relayed" == "TURN-TURN", except also covers future ways of doing
fully-relayed which may not even involve TURN.

honghaiz3

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransportchannel.cc File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransportchannel.cc#newcode958 webrtc/p2p/base/p2ptransportchannel.cc:958: return MAPPED_WRITE_STATE_TIMEOUT; On 2016/06/15 19:12:46, pthatcher1 wrote: > On ...

4 years, 6 months ago (2016-06-15 19:26:36 UTC) #6

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:958: return MAPPED_WRITE_STATE_TIMEOUT;
On 2016/06/15 19:12:46, pthatcher1 wrote:
> On 2016/06/15 17:08:56, Taylor Brandstetter wrote:
> > On 2016/06/15 16:37:18, honghaiz3 wrote:
> > > 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. 
> > 
> > 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?
> 
> 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.

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

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransportchannel.cc File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransportchannel.cc#newcode314 webrtc/p2p/base/p2ptransportchannel.cc:314: } On 2016/06/15 19:12:46, pthatcher1 wrote: > Why not ...

4 years, 6 months ago (2016-06-16 00:13:41 UTC) #7

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:314: }
On 2016/06/15 19:12:46, pthatcher1 wrote:
> Why not just allow it and document that it doesn't affect existing candidate
> pairs?

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.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:958: return MAPPED_WRITE_STATE_TIMEOUT;
On 2016/06/15 19:12:46, pthatcher1 wrote:
> On 2016/06/15 17:08:56, Taylor Brandstetter wrote:
> > On 2016/06/15 16:37:18, honghaiz3 wrote:
> > > 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. 
> > 
> > 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?
> 
> 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.

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

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:972: }
On 2016/06/15 19:12:46, pthatcher1 wrote:
> 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)

Done.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1031: }
On 2016/06/15 19:12:46, pthatcher1 wrote:
> 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;
> }

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.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1039: }
On 2016/06/15 19:12:46, pthatcher1 wrote:
> Similarly here:
> 
> // Prefer higher priority
> int priority_comp = (a->priority() - b->priority());
> if (priority_comp != 0) {
>   return priority_comp;
> }

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

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1041: // If we're still tied at this
point, prefer a younger generation.
On 2016/06/15 19:12:46, pthatcher1 wrote:
> Could you add "(larger generation number)" to make it clear younger == bigger?

Done.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1070: // trying to rendevous with the
other.
On 2016/06/15 19:12:46, pthatcher1 wrote:
> 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.

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

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1077: const Connection* b) const {
On 2016/06/15 19:12:46, pthatcher1 wrote:
> 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.

Done.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1084: }
On 2016/06/15 19:12:46, pthatcher1 wrote:
> This might be more readable as:
> 
> if (!conn) {
>   return false;  // This shouldn't happen.
> }
> 
> if (!selected) {
>   return true;
> }
> 
> if (conn == selected) {
>   return false;
> }
> 

Done.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1093: if (ice_role_ == ICEROLE_CONTROLLED
&& a->nominated()) {
On 2016/06/15 19:12:46, pthatcher1 wrote:
> 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;
> }
> 

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.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1103: return b->rtt() <= a->rtt() +
kMinImprovement;
On 2016/06/15 19:12:46, pthatcher1 wrote:
> This might be more readable as:
> 
> return (selected->rtt() - conn->rtt()) > kMinImprovement;

Done.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1283: // CreatePermission required), act
like we're already writable to the upper
On 2016/06/15 19:12:46, pthatcher1 wrote:
> I think the phrased "presumed writable" might be slightly better than
"probably
> writable.

Done.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1285: bool writable =
On 2016/06/15 19:12:46, pthatcher1 wrote:
> 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_);

Done.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:116: cricket::Candidate
CreateHostCandidate(const std::string& ip,
On 2016/06/15 19:12:47, pthatcher1 wrote:
> CreateUdpHostCandidate?

Done.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:130: cricket::Candidate
CreateRelayCandidate(const std::string& ip,
On 2016/06/15 19:12:47, pthatcher1 wrote:
> CreateUdpRelayCandidate?

Done.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:141: return c;
On 2016/06/15 19:12:47, pthatcher1 wrote:
> It seems like this could use a common CreateUdpCandidate(type, ip, port,
> priority, ufrag);
> 
> Then you'd have:
> 
> return CreateUdpCandidate(cricket::RELAY_PORT_TYPE, ip, port, priority,
ufrag);

Done.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1580: // false and there's a
TURN-TURN candidate pair, it's assumed to be writable
On 2016/06/15 19:12:47, pthatcher1 wrote:
> I think it's more "presumed" than "assumed":
> 
> http://grammarist.com/usage/assume-presume/
> 
> But I'm not sure, nor am I sure that it matters.  What do you think?

Done. I think you take more care to make sure your code is grammatically correct
than anyone I know. :)

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transport.h:179: bool turn_to_turn_createpermission_needed =
true;
On 2016/06/15 19:12:47, pthatcher1 wrote:
> I was thinking of a name like presume_connected_fully_relayed_candidate_pairs
=
> false;  
> 
> Later we'll want something in RtcConfig that's similar, perhaps
> presume_connected_fully_relayed_ice.
> 
> "Fully relayed" == "TURN-TURN", except also covers future ways of doing
> fully-relayed which may not even involve TURN.

I don't like just "presume connected" because it doesn't tell us *why* they
should be presumed connected. The actual piece of information being provided is
that CreatePermission is not needed for fully relayed connections. So I think
that's what the variable should be called; nothing more, nothing less. Unless
there are other reasons why a fully-relayed connection may be presumed
connected.

pthatcher1

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransportchannel.cc File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransportchannel.cc#newcode1031 webrtc/p2p/base/p2ptransportchannel.cc:1031: } On 2016/06/16 00:13:40, Taylor Brandstetter wrote: > On ...

4 years, 6 months ago (2016-06-16 23:31:36 UTC) #8

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1031: }
On 2016/06/16 00:13:40, Taylor Brandstetter wrote:
> On 2016/06/15 19:12:46, pthatcher1 wrote:
> > 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;
> > }
> 
> 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.

True.

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/2063823008/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transport.h:179: bool turn_to_turn_createpermission_needed =
true;
On 2016/06/16 00:13:41, Taylor Brandstetter wrote:
> On 2016/06/15 19:12:47, pthatcher1 wrote:
> > I was thinking of a name like
presume_connected_fully_relayed_candidate_pairs
> =
> > false;  
> > 
> > Later we'll want something in RtcConfig that's similar, perhaps
> > presume_connected_fully_relayed_ice.
> > 
> > "Fully relayed" == "TURN-TURN", except also covers future ways of doing
> > fully-relayed which may not even involve TURN.
> 
> I don't like just "presume connected" because it doesn't tell us *why* they
> should be presumed connected. The actual piece of information being provided
is
> that CreatePermission is not needed for fully relayed connections. So I think
> that's what the variable should be called; nothing more, nothing less. Unless
> there are other reasons why a fully-relayed connection may be presumed
> connected.

But this flag doesn't just mean CreatePermission isn't needed.  It's also saying
the application is opting into sending media before an ICE check response is
received, which I think is a bigger deal.  So something about that needs to be
in the name.

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:1419: }
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)"

honghaiz3

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.h#newcode459 webrtc/p2p/base/port.h:459: bool presumed_writable() const { When do we need to ...

4 years, 6 months ago (2016-06-17 01:55:03 UTC) #9

honghaiz3

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#newcode855 webrtc/p2p/base/port.cc:855: write_state_ = InitialWriteState(); Instead of setting the write_state_ every ...

4 years, 6 months ago (2016-06-17 16:22:07 UTC) #10

honghaiz3

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/p2ptransportchannel.cc File webrtc/p2p/base/p2ptransportchannel.cc (left): https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/p2ptransportchannel.cc#oldcode169 webrtc/p2p/base/p2ptransportchannel.cc:169: }; I think you don't need to put anything ...

4 years, 6 months ago (2016-06-17 16:28:17 UTC) #11

Taylor Brandstetter

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/p2ptransportchannel.cc File webrtc/p2p/base/p2ptransportchannel.cc (left): https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/p2ptransportchannel.cc#oldcode169 webrtc/p2p/base/p2ptransportchannel.cc:169: }; On 2016/06/17 16:28:16, honghaiz3 wrote: > I think ...

4 years, 6 months ago (2016-06-20 17:15:45 UTC) #12

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (left):

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:169: };
On 2016/06/17 16:28:16, honghaiz3 wrote:
> 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. 

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?

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:855: write_state_ = InitialWriteState();
On 2016/06/17 16:22:07, honghaiz3 wrote:
> 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. 

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?

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:1419: }
On 2016/06/16 23:31:36, pthatcher1 wrote:
> 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)"

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.

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:459: bool presumed_writable() const {
On 2016/06/17 01:55:03, honghaiz3 wrote:
> When do we need to differentiate presumed_writable vs writable ?
> Does it make sense just to let writable() return 
>    write_state_ == STATE_WRITABLE ||
>            write_state_ == STATE_PRESUMED_WRITABLE;

There are a few notable places. For example, the logic for selecting a
connection to send a ping on. I assume if a connection is "presumed writable",
we should treat it as unwritable for the purpose of sending pings. Because if it
turns out that the "presumed writable" connection *isn't* writable, we want to
find out as soon as possible.

honghaiz3

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/p2ptransportchannel.cc File webrtc/p2p/base/p2ptransportchannel.cc (left): https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/p2ptransportchannel.cc#oldcode169 webrtc/p2p/base/p2ptransportchannel.cc:169: }; On 2016/06/20 17:15:45, Taylor Brandstetter wrote: > On ...

4 years, 6 months ago (2016-06-20 22:31:03 UTC) #13

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (left):

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:169: };
On 2016/06/20 17:15:45, Taylor Brandstetter wrote:
> On 2016/06/17 16:28:16, honghaiz3 wrote:
> > 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. 
> 
> 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?

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.

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:855: write_state_ = InitialWriteState();
On 2016/06/20 17:15:45, Taylor Brandstetter wrote:
> On 2016/06/17 16:22:07, honghaiz3 wrote:
> > 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. 
> 
> 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?
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.

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:459: bool presumed_writable() const {
On 2016/06/20 17:15:45, Taylor Brandstetter wrote:
> On 2016/06/17 01:55:03, honghaiz3 wrote:
> > When do we need to differentiate presumed_writable vs writable ?
> > Does it make sense just to let writable() return 
> >    write_state_ == STATE_WRITABLE ||
> >            write_state_ == STATE_PRESUMED_WRITABLE;
> 
> There are a few notable places. For example, the logic for selecting a
> connection to send a ping on. I assume if a connection is "presumed writable",
> we should treat it as unwritable for the purpose of sending pings. Because if
it
> turns out that the "presumed writable" connection *isn't* writable, we want to
> find out as soon as possible.

If I understand correctly, that is the only place we may want a different
writable() method. I hope we could avoid two writable(writable and
presumed_writable) methods. From the p2ptransportchannel's perspective, when it
asks a connection whether it is writable, it should always return a single
result (basically PRESUMED_WRITABLE or WRITABLE). In the rare case (for stun
ping), can we just directly check the write_state() instead of having another
writable method if it is only called once or twice?

pthatcher1

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/p2ptransportchannel.cc File webrtc/p2p/base/p2ptransportchannel.cc (left): https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/p2ptransportchannel.cc#oldcode169 webrtc/p2p/base/p2ptransportchannel.cc:169: }; On 2016/06/20 22:31:03, honghaiz3 wrote: > On 2016/06/20 ...

4 years, 6 months ago (2016-06-21 06:20:50 UTC) #14

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (left):

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:169: };
On 2016/06/20 22:31:03, honghaiz3 wrote:
> On 2016/06/20 17:15:45, Taylor Brandstetter wrote:
> > On 2016/06/17 16:28:16, honghaiz3 wrote:
> > > 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. 
> > 
> > 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?
> 
> 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. 

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.

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:855: write_state_ = InitialWriteState();
On 2016/06/20 17:15:45, Taylor Brandstetter wrote:
> On 2016/06/17 16:22:07, honghaiz3 wrote:
> > 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. 
> 
> Isn't that effectively the same as having two enums? One method returns four
> possible values, the other returns five.

I's like having one internal enum with 4 values and one external enum with 5
values.

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

Are you arguing that because the external enum has 5 states, that the internal
enum must also have 5 states?

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

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.

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:855: write_state_ = InitialWriteState();
On 2016/06/20 22:31:03, honghaiz3 wrote:
> On 2016/06/20 17:15:45, Taylor Brandstetter wrote:
> > On 2016/06/17 16:22:07, honghaiz3 wrote:
> > > 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. 
> > 
> > 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?
> 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?

No, I think it would go to INIT (it would eventually stop presuming and just go
back).

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

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

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:1419: }
On 2016/06/20 17:15:45, Taylor Brandstetter wrote:
> On 2016/06/16 23:31:36, pthatcher1 wrote:
> > 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)"
> 
> 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.

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.

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:459: bool presumed_writable() const {
On 2016/06/20 22:31:03, honghaiz3 wrote:
> On 2016/06/20 17:15:45, Taylor Brandstetter wrote:
> > On 2016/06/17 01:55:03, honghaiz3 wrote:
> > > When do we need to differentiate presumed_writable vs writable ?
> > > Does it make sense just to let writable() return 
> > >    write_state_ == STATE_WRITABLE ||
> > >            write_state_ == STATE_PRESUMED_WRITABLE;
> > 
> > There are a few notable places. For example, the logic for selecting a
> > connection to send a ping on. I assume if a connection is "presumed
writable",
> > we should treat it as unwritable for the purpose of sending pings. Because
if
> it
> > turns out that the "presumed writable" connection *isn't* writable, we want
to
> > find out as soon as possible.
> 
> If I understand correctly, that is the only place we may want a different
> writable() method. I hope we could avoid two writable(writable and
> presumed_writable) methods. From the p2ptransportchannel's perspective, when
it
> asks a connection whether it is writable, it should always return a single
> result (basically PRESUMED_WRITABLE or WRITABLE). In the rare case (for stun
> ping), can we just directly check the write_state() instead of having another
> writable method if it is only called once or twice? 

The problem is that if someone calls writable() and doesn't know the difference,
they might be surprised.  If we choose to make writable() == WRITABLE ||
PRESUMED_WRITABLE, then they might be surprised that it's writable when it's
only presumably writable.  If we choose to make writable() == WRITABLE, they
might be surprised that it's not writable when it's presumably writable.

There are 15 callers.  Perhaps we should remove writable() and make each one
check write_state() so that there is never ambiguity.

It would make it easier if we reversed the order of the enum so that a caller
could do this:

"if write_state() >= STATE_PRESUMED_WRITABLE" instead of "if write_state() ==
STATE_WRITABLE || write_state() == STATE_PRESUMED_WRITABLE"

Either that or we could remove writable() in favor of presumed_writable() and
certainly_writable().  But that seems a bit too much.

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/transport...
webrtc/p2p/base/transport.h:179: bool fully_relayed_createpermission_needed =
true;
I still think this needs to reflect that we are opting into "presume
writability" territory.  

"presume_writability_when_fully_relayed" or something like that.

honghaiz3

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#newcode855 webrtc/p2p/base/port.cc:855: write_state_ = InitialWriteState(); On 2016/06/21 06:20:50, pthatcher1 wrote: > ...

4 years, 6 months ago (2016-06-21 19:37:57 UTC) #15

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:855: write_state_ = InitialWriteState();
On 2016/06/21 06:20:50, pthatcher1 wrote:
> On 2016/06/20 22:31:03, honghaiz3 wrote:
> > On 2016/06/20 17:15:45, Taylor Brandstetter wrote:
> > > On 2016/06/17 16:22:07, honghaiz3 wrote:
> > > > 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. 
> > > 
> > > 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?
> > 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?
> 
> No, I think it would go to INIT (it would eventually stop presuming and just
go
> back).
> 
> > 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. 
> 
> 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).  
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

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#newcode855 webrtc/p2p/base/port.cc:855: write_state_ = InitialWriteState(); On 2016/06/21 06:20:50, pthatcher1 wrote: > ...

4 years, 6 months ago (2016-06-22 00:37:07 UTC) #16

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:855: write_state_ = InitialWriteState();
On 2016/06/21 06:20:50, pthatcher1 wrote:
> On 2016/06/20 22:31:03, honghaiz3 wrote:
> > On 2016/06/20 17:15:45, Taylor Brandstetter wrote:
> > > On 2016/06/17 16:22:07, honghaiz3 wrote:
> > > > 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. 
> > > 
> > > 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?
> > 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?
> 
> No, I think it would go to INIT (it would eventually stop presuming and just
go
> back).
> 
> > 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. 
> 
> 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).  

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.

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:1419: }
On 2016/06/21 06:20:50, pthatcher1 wrote:
> On 2016/06/20 17:15:45, Taylor Brandstetter wrote:
> > On 2016/06/16 23:31:36, pthatcher1 wrote:
> > > 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)"
> > 
> > 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.
> 
> 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.

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.

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:459: bool presumed_writable() const {
On 2016/06/21 06:20:50, pthatcher1 wrote:
> On 2016/06/20 22:31:03, honghaiz3 wrote:
> > On 2016/06/20 17:15:45, Taylor Brandstetter wrote:
> > > On 2016/06/17 01:55:03, honghaiz3 wrote:
> > > > When do we need to differentiate presumed_writable vs writable ?
> > > > Does it make sense just to let writable() return 
> > > >    write_state_ == STATE_WRITABLE ||
> > > >            write_state_ == STATE_PRESUMED_WRITABLE;
> > > 
> > > There are a few notable places. For example, the logic for selecting a
> > > connection to send a ping on. I assume if a connection is "presumed
> writable",
> > > we should treat it as unwritable for the purpose of sending pings. Because
> if
> > it
> > > turns out that the "presumed writable" connection *isn't* writable, we
want
> to
> > > find out as soon as possible.
> > 
> > If I understand correctly, that is the only place we may want a different
> > writable() method. I hope we could avoid two writable(writable and
> > presumed_writable) methods. From the p2ptransportchannel's perspective, when
> it
> > asks a connection whether it is writable, it should always return a single
> > result (basically PRESUMED_WRITABLE or WRITABLE). In the rare case (for stun
> > ping), can we just directly check the write_state() instead of having
another
> > writable method if it is only called once or twice? 
> 
> The problem is that if someone calls writable() and doesn't know the
difference,
> they might be surprised.  If we choose to make writable() == WRITABLE ||
> PRESUMED_WRITABLE, then they might be surprised that it's writable when it's
> only presumably writable.  If we choose to make writable() == WRITABLE, they
> might be surprised that it's not writable when it's presumably writable.
> 
> There are 15 callers.  Perhaps we should remove writable() and make each one
> check write_state() so that there is never ambiguity.
> 
> It would make it easier if we reversed the order of the enum so that a caller
> could do this:
> 
> "if write_state() >= STATE_PRESUMED_WRITABLE" instead of "if write_state() ==
> STATE_WRITABLE || write_state() == STATE_PRESUMED_WRITABLE"
> 
> Either that or we could remove writable() in favor of presumed_writable() and
> certainly_writable().  But that seems a bit too much.
> 
> 
> 

My reasoning:

1. I don't want to change the meaning of writable(). I don't think anyone should
be surprised if a connection is not writable() before it's received a ping
response, because that's the way it's always worked.
2. I want to avoid checking write_state() directly from P2PTransportChannel
because that creates more fragile code (if we ever were to add a new state).

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/transport...
webrtc/p2p/base/transport.h:179: bool fully_relayed_createpermission_needed =
true;
On 2016/06/21 06:20:50, pthatcher1 wrote:
> I still think this needs to reflect that we are opting into "presume
> writability" territory.  
> 
> "presume_writability_when_fully_relayed" or something like that.

Done.

pthatcher1

I think it's mostly just a style/readability decision at this point. The only thing really ...

4 years, 6 months ago (2016-06-22 05:46:14 UTC) #17

I think it's mostly just a style/readability decision at this point.

The only thing really blocking this CL in my view is that  I would like to see
the logic for presumed_writable include the case where TURN<->PRFLX happens.

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:855: write_state_ = InitialWriteState();
On 2016/06/22 00:37:07, Taylor Brandstetter wrote:
> On 2016/06/21 06:20:50, pthatcher1 wrote:
> > On 2016/06/20 22:31:03, honghaiz3 wrote:
> > > On 2016/06/20 17:15:45, Taylor Brandstetter wrote:
> > > > On 2016/06/17 16:22:07, honghaiz3 wrote:
> > > > > 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. 
> > > > 
> > > > 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?
> > > 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?
> > 
> > No, I think it would go to INIT (it would eventually stop presuming and just
> go
> > back).
> > 
> > > 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. 
> > 
> > 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).  
> 
> 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.

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.

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:1419: }
On 2016/06/22 00:37:07, Taylor Brandstetter wrote:
> On 2016/06/21 06:20:50, pthatcher1 wrote:
> > On 2016/06/20 17:15:45, Taylor Brandstetter wrote:
> > > On 2016/06/16 23:31:36, pthatcher1 wrote:
> > > > 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)"
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.
> 

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

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

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.

https://codereview.webrtc.org/2063823008/diff/60001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2063823008/diff/60001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:941: }
I apologize about this, but now that I see this code and the code below, I
realize it is more consistent and readable to have it the way it was before:

if (a->write_state() < b->write_state()) {
  return 1;
}
if (a->write_state() > b->write_state()) {
  return -1;
}


Don't block this CL on it, though.

Taylor Brandstetter

https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#newcode855 webrtc/p2p/base/port.cc:855: write_state_ = InitialWriteState(); On 2016/06/22 05:46:13, pthatcher1 wrote: > ...

4 years, 6 months ago (2016-06-22 15:34:44 UTC) #18

Taylor Brandstetter

On 2016/06/22 15:34:44, Taylor Brandstetter wrote: > https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc > File webrtc/p2p/base/port.cc (right): > > https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#newcode855 ...

4 years, 6 months ago (2016-06-22 15:52:37 UTC) #19

On 2016/06/22 15:34:44, Taylor Brandstetter wrote:
> https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc
> File webrtc/p2p/base/port.cc (right):
> 
>
https://codereview.webrtc.org/2063823008/diff/40001/webrtc/p2p/base/port.cc#n...
> webrtc/p2p/base/port.cc:855: write_state_ = InitialWriteState();
> On 2016/06/22 05:46:13, pthatcher1 wrote:
> > 
> > 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.  
> 
> 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...

See patch set 6 for an example.

pthatcher1

lgtm https://codereview.webrtc.org/2063823008/diff/100001/webrtc/p2p/base/p2ptransportchannel.cc File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2063823008/diff/100001/webrtc/p2p/base/p2ptransportchannel.cc#newcode947 webrtc/p2p/base/p2ptransportchannel.cc:947: } To go along with the write state ...

4 years, 6 months ago (2016-06-22 16:08:25 UTC) #20

Taylor Brandstetter

https://codereview.webrtc.org/2063823008/diff/100001/webrtc/p2p/base/p2ptransportchannel.cc File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2063823008/diff/100001/webrtc/p2p/base/p2ptransportchannel.cc#newcode947 webrtc/p2p/base/p2ptransportchannel.cc:947: } On 2016/06/22 16:08:24, pthatcher1 wrote: > To go ...

4 years, 6 months ago (2016-06-22 16:18:28 UTC) #21

honghaiz3

https://codereview.webrtc.org/2063823008/diff/120001/webrtc/p2p/base/p2ptransportchannel.cc File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2063823008/diff/120001/webrtc/p2p/base/p2ptransportchannel.cc#newcode941 webrtc/p2p/base/p2ptransportchannel.cc:941: if (b->write_state() < a->write_state()) { I am not sure ...

4 years, 6 months ago (2016-06-22 16:48:01 UTC) #22

Taylor Brandstetter

https://codereview.webrtc.org/2063823008/diff/120001/webrtc/p2p/base/p2ptransportchannel.cc File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2063823008/diff/120001/webrtc/p2p/base/p2ptransportchannel.cc#newcode941 webrtc/p2p/base/p2ptransportchannel.cc:941: if (b->write_state() < a->write_state()) { On 2016/06/22 16:48:01, honghaiz3 ...

4 years, 6 months ago (2016-06-22 17:15:47 UTC) #23

pthatcher1

https://codereview.webrtc.org/2063823008/diff/120001/webrtc/p2p/base/p2ptransportchannel.cc File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2063823008/diff/120001/webrtc/p2p/base/p2ptransportchannel.cc#newcode941 webrtc/p2p/base/p2ptransportchannel.cc:941: if (b->write_state() < a->write_state()) { On 2016/06/22 17:15:46, Taylor ...

4 years, 6 months ago (2016-06-22 17:20:19 UTC) #24

Taylor Brandstetter

Description was changed from ========== Adding IceConfig option to assume TURN/TURN candidate pairs will work. ...

4 years, 6 months ago (2016-06-22 22:42:29 UTC) #26

Taylor Brandstetter

Only adding "NOTRY" because Android bots are currently failing a "get version" step.

4 years, 6 months ago (2016-06-22 22:43:04 UTC) #27

Taylor Brandstetter

The CQ bit was checked by deadbeef@webrtc.org

4 years, 6 months ago (2016-06-22 22:43:39 UTC) #28

Taylor Brandstetter

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org, honghaiz@webrtc.org Link to ...

4 years, 6 months ago (2016-06-22 22:43:39 UTC) #29

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2063823008/160001

4 years, 6 months ago (2016-06-22 22:43:51 UTC) #30

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 6 months ago (2016-06-22 22:45:28 UTC) #31

commit-bot: I haz the power

Failed to apply patch for webrtc/p2p/base/p2ptransportchannel_unittest.cc: While running git apply --index -3 -p1; error: patch ...

4 years, 6 months ago (2016-06-22 22:45:30 UTC) #32

Failed to apply patch for webrtc/p2p/base/p2ptransportchannel_unittest.cc:
While running git apply --index -3 -p1;
  error: patch failed: webrtc/p2p/base/p2ptransportchannel_unittest.cc:33
  Falling back to three-way merge...
  Applied patch to 'webrtc/p2p/base/p2ptransportchannel_unittest.cc' with
conflicts.
  U webrtc/p2p/base/p2ptransportchannel_unittest.cc

Patch:       webrtc/p2p/base/p2ptransportchannel_unittest.cc
Index: webrtc/p2p/base/p2ptransportchannel_unittest.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc
b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
index
e5e63a3fa0c23d212a17eccb3c8dd7915c4fa49f..ab064f1b86ff84d28069f2d3b9b6bbd94d92d329
100644
--- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc
+++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -33,15 +33,14 @@
 #include "webrtc/base/thread.h"
 #include "webrtc/base/virtualsocketserver.h"
 
-using cricket::kDefaultPortAllocatorFlags;
-using cricket::kMinimumStepDelay;
-using cricket::kDefaultStepDelay;
-using cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET;
-using cricket::ServerAddresses;
-using cricket::MIN_PINGS_AT_WEAK_PING_INTERVAL;
+namespace {
+
 using rtc::SocketAddress;
 
-static const int kDefaultTimeout = 1000;
+// Default timeout for tests in this file.
+// Should be large enough for slow buildbots to run the tests reliably.
+static const int kDefaultTimeout = 10000;
+
 static const int kOnlyLocalPorts = cricket::PORTALLOCATOR_DISABLE_STUN |
                                    cricket::PORTALLOCATOR_DISABLE_RELAY |
                                    cricket::PORTALLOCATOR_DISABLE_TCP;
@@ -106,9 +105,9 @@ static const uint64_t kTiebreaker2 = 22222;
 
 enum { MSG_ADD_CANDIDATES, MSG_REMOVE_CANDIDATES };
 
-static cricket::IceConfig CreateIceConfig(int receiving_timeout,
-                                          bool gather_continually,
-                                          int backup_ping_interval = -1) {
+cricket::IceConfig CreateIceConfig(int receiving_timeout,
+                                   bool gather_continually,
+                                   int backup_ping_interval = -1) {
   cricket::IceConfig config;
   config.receiving_timeout = receiving_timeout;
   config.gather_continually = gather_continually;
@@ -116,6 +115,25 @@ static cricket::IceConfig CreateIceConfig(int
receiving_timeout,
   return config;
 }
 
+cricket::Candidate CreateUdpCandidate(const std::string& type,
+                                      const std::string& ip,
+                                      int port,
+                                      int priority,
+                                      const std::string& ufrag = "") {
+  cricket::Candidate c;
+  c.set_address(rtc::SocketAddress(ip, port));
+  c.set_component(cricket::ICE_CANDIDATE_COMPONENT_DEFAULT);
+  c.set_protocol(cricket::UDP_PROTOCOL_NAME);
+  c.set_priority(priority);
+  c.set_username(ufrag);
+  c.set_type(type);
+  return c;
+}
+
+}  // namespace {
+
+namespace cricket {
+
 // This test simulates 2 P2P endpoints that want to establish connectivity
 // with each other over various network topologies and conditions, which can be
 // specified in each individial test.
@@ -142,27 +160,35 @@ class P2PTransportChannelTestBase : public testing::Test,
         nss_(new rtc::NATSocketServer(vss_.get())),
         ss_(new rtc::FirewallSocketServer(nss_.get())),
         ss_scope_(ss_.get()),
-        stun_server_(cricket::TestStunServer::Create(main_, kStunAddr)),
+        stun_server_(TestStunServer::Create(main_, kStunAddr)),
         turn_server_(main_, kTurnUdpIntAddr, kTurnUdpExtAddr),
-        relay_server_(main_, kRelayUdpIntAddr, kRelayUdpExtAddr,
-                      kRelayTcpIntAddr, kRelayTcpExtAddr,
-                      kRelaySslTcpIntAddr, kRelaySslTcpExtAddr),
-        socks_server1_(ss_.get(), kSocksProxyAddrs[0],
-                       ss_.get(), kSocksProxyAddrs[0]),
-        socks_server2_(ss_.get(), kSocksProxyAddrs[1],
-                       ss_.get(), kSocksProxyAddrs[1]),
+        relay_server_(main_,
+                      kRelayUdpIntAddr,
+                      kRelayUdpExtAddr,
+                      kRelayTcpIntAddr,
+                      kRelayTcpExtAddr,
+                      kRelaySslTcpIntAddr,
+                      kRelaySslTcpExtAddr),
+        socks_server1_(ss_.get(),
+                       kSocksProxyAddrs[0],
+                       ss_.get(),
+                       kSocksProxyAddrs[0]),
+        socks_server2_(ss_.get(),
+                       kSocksProxyAddrs[1],
+                       ss_.get(),
+                       kSocksProxyAddrs[1]),
         force_relay_(false) {
-    ep1_.role_ = cricket::ICEROLE_CONTROLLING;
-    ep2_.role_ = cricket::ICEROLE_CONTROLLED;
+    ep1_.role_ = ICEROLE_CONTROLLING;
+    ep2_.role_ = ICEROLE_CONTROLLED;
 
     ServerAddresses stun_servers;
     stun_servers.insert(kStunAddr);
-    ep1_.allocator_.reset(new cricket::BasicPortAllocator(
-        &ep1_.network_manager_,
-        stun_servers, kRelayUdpIntAddr, kRelayTcpIntAddr,
kRelaySslTcpIntAddr));
-    ep2_.allocator_.reset(new cricket::BasicPortAllocator(
-        &ep2_.network_manager_,
-        stun_servers, kRelayUdpIntAddr, kRelayTcpIntAddr,
kRelaySslTcpIntAddr));
+    ep1_.allocator_.reset(new BasicPortAllocator(
+        &ep1_.network_manager_, stun_servers, kRelayUdpIntAddr,
+        kRelayTcpIntAddr, kRelaySslTcpIntAddr));
+    ep2_.allocator_.reset(new BasicPortAllocator(
+        &ep2_.network_manager_, stun_servers, kRelayUdpIntAddr,
+        kRelayTcpIntAddr, kRelaySslTcpIntAddr));
   }
 
  protected:
@@ -216,29 +242,28 @@ class P2PTransportChannelTestBase : public testing::Test,
 
     std::string name_;  // TODO - Currently not used.
     std::list<std::string> ch_packets_;
-    std::unique_ptr<cricket::P2PTransportChannel> ch_;
+    std::unique_ptr<P2PTransportChannel> ch_;
   };
 
   struct CandidatesData : public rtc::MessageData {
-    CandidatesData(cricket::TransportChannel* ch, const cricket::Candidate& c)
+    CandidatesData(TransportChannel* ch, const Candidate& c)
         : channel(ch), candidates(1, c) {}
-    CandidatesData(cricket::TransportChannel* ch,
-                   const std::vector<cricket::Candidate>& cc)
+    CandidatesData(TransportChannel* ch, const std::vector<Candidate>& cc)
         : channel(ch), candidates(cc) {}
-    cricket::TransportChannel* channel;
-    cricket::Candidates candidates;
+    TransportChannel* channel;
+    Candidates candidates;
   };
 
   struct Endpoint {
     Endpoint()
-        : role_(cricket::ICEROLE_UNKNOWN),
+        : role_(ICEROLE_UNKNOWN),
           tiebreaker_(0),
           role_conflict_(false),
           save_candidates_(false) {}
-    bool HasChannel(cricket::TransportChannel* ch) {
+    bool HasChannel(TransportChannel* ch) {
       return (ch == cd1_.ch_.get() || ch == cd2_.ch_.get());
     }
-    ChannelData* GetChannelData(cricket::TransportChannel* ch) {
+    ChannelData* GetChannelData(TransportChannel* ch) {
       if (!HasChannel(ch)) return NULL;
       if (cd1_.ch_.get() == ch)
         return &cd1_;
@@ -246,8 +271,8 @@ class P2PTransportChannelTestBase : public testing::Test,
         return &cd2_;
     }
 
-    void SetIceRole(cricket::IceRole role) { role_ = role; }
-    cricket::IceRole ice_role() { return role_; }
+    void SetIceRole(IceRole role) { role_ = role; }
+    IceRole ice_role() { return role_; }
     void SetIceTiebreaker(uint64_t tiebreaker) { tiebreaker_ = tiebreaker; }
     uint64_t GetIceTiebreaker() { return tiebreaker_; }
     void OnRoleConflict(bool role_conflict) { role_conflict_ = role_conflict; }
@@ -260,17 +285,18 @@ class P2PTransportChannelTestBase : public testing::Test,
     }
 
     rtc::FakeNetworkManager network_manager_;
-    std::unique_ptr<cricket::BasicPortAllocator> allocator_;
+    std::unique_ptr<BasicPortAllocator> allocator_;
     ChannelData cd1_;
     ChannelData cd2_;
-    cricket::IceRole role_;
+    IceRole role_;
     uint64_t tiebreaker_;
     bool role_conflict_;
     bool save_candidates_;
     std::vector<CandidatesData*> saved_candidates_;
+    bool ready_to_send_ = false;
   };
 
-  ChannelData* GetChannelData(cricket::TransportChannel* channel) {
+  ChannelData* GetChannelData(TransportChannel* channel) {
     if (ep1_.HasChannel(channel))
       return ep1_.GetChannelData(channel);
     else
@@ -283,13 +309,11 @@ class P2PTransportChannelTestBase : public testing::Test,
     std::string ice_ufrag_ep2_cd1_ch = kIceUfrag[1];
     std::string ice_pwd_ep2_cd1_ch = kIcePwd[1];
     ep1_.cd1_.ch_.reset(CreateChannel(
-        0, cricket::ICE_CANDIDATE_COMPONENT_DEFAULT,
-        ice_ufrag_ep1_cd1_ch, ice_pwd_ep1_cd1_ch,
-        ice_ufrag_ep2_cd1_ch, ice_pwd_ep2_cd1_ch));
+        0, ICE_CANDIDATE_COMPONENT_DEFAULT, ice_ufrag_ep1_cd1_ch,
+        ice_pwd_ep1_cd1_ch, ice_ufrag_ep2_cd1_ch, ice_pwd_ep2_cd1_ch));
     ep2_.cd1_.ch_.reset(CreateChannel(
-        1, cricket::ICE_CANDIDATE_COMPONENT_DEFAULT,
-        ice_ufrag_ep2_cd1_ch, ice_pwd_ep2_cd1_ch,
-        ice_ufrag_ep1_cd1_ch, ice_pwd_ep1_cd1_ch));
+        1, ICE_CANDIDATE_COMPONENT_DEFAULT, ice_ufrag_ep2_cd1_ch,
+        ice_pwd_ep2_cd1_ch, ice_ufrag_ep1_cd1_ch, ice_pwd_ep1_cd1_ch));
     ep1_.cd1_.ch_->MaybeStartGathering();
     ep2_.cd1_.ch_->MaybeStartGathering();
     if (num == 2) {
@@ -298,26 +322,25 @@ class P2PTransportChannelTestBase : public testing::Test,
       std::string ice_ufrag_ep2_cd2_ch = kIceUfrag[3];
       std::string ice_pwd_ep2_cd2_ch = kIcePwd[3];
       ep1_.cd2_.ch_.reset(CreateChannel(
-          0, cricket::ICE_CANDIDATE_COMPONENT_DEFAULT,
-          ice_ufrag_ep1_cd2_ch, ice_pwd_ep1_cd2_ch,
-          ice_ufrag_ep2_cd2_ch, ice_pwd_ep2_cd2_ch));
+          0, ICE_CANDIDATE_COMPONENT_DEFAULT, ice_ufrag_ep1_cd2_ch,
+          ice_pwd_ep1_cd2_ch, ice_ufrag_ep2_cd2_ch, ice_pwd_ep2_cd2_ch));
       ep2_.cd2_.ch_.reset(CreateChannel(
-          1, cricket::ICE_CANDIDATE…
(message too large)

commit-bot: I haz the power

Description was changed from ========== Adding IceConfig option to assume TURN/TURN candidate pairs will work. ...

4 years, 6 months ago (2016-06-22 23:02:02 UTC) #33

commit-bot: I haz the power

Patchset 9 (id:??) landed as https://crrev.com/8e6134eae4117a239de67c9a9dae8f5e3235d803 Cr-Commit-Position: refs/heads/master@{#13263}

4 years, 6 months ago (2016-06-22 23:02:04 UTC) #34

honghaiz3

A revert of this CL (patchset #9 id:160001) has been created in https://codereview.webrtc.org/2090823002/ by honghaiz@webrtc.org. ...

4 years, 6 months ago (2016-06-22 23:08:28 UTC) #35

Taylor Brandstetter

Description was changed from ========== Adding IceConfig option to assume TURN/TURN candidate pairs will work. ...

4 years, 6 months ago (2016-06-22 23:33:25 UTC) #36

Taylor Brandstetter

Description was changed from ========== Adding IceConfig option to assume TURN/TURN candidate pairs will work. ...

4 years, 6 months ago (2016-06-22 23:34:45 UTC) #37

Taylor Brandstetter

Honghai and I landed CLs at the same time and they slightly conflicted. I merged ...

4 years, 6 months ago (2016-06-22 23:35:33 UTC) #38

Taylor Brandstetter

The CQ bit was checked by deadbeef@webrtc.org

4 years, 6 months ago (2016-06-23 00:12:33 UTC) #39

Taylor Brandstetter

The patchset sent to the CQ was uploaded after l-g-t-m from honghaiz@webrtc.org, pthatcher@webrtc.org Link to ...

4 years, 6 months ago (2016-06-23 00:12:33 UTC) #40

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2063823008/180001

4 years, 6 months ago (2016-06-23 00:12:42 UTC) #41

commit-bot: I haz the power

Description was changed from ========== Adding IceConfig option to assume TURN/TURN candidate pairs will work. ...

4 years, 6 months ago (2016-06-23 00:14:18 UTC) #42

commit-bot: I haz the power

Description was changed from ========== Adding IceConfig option to assume TURN/TURN candidate pairs will work. ...

4 years, 6 months ago (2016-06-23 00:14:25 UTC) #44

commit-bot: I haz the power

Patchset 10 (id:??) landed as https://crrev.com/14f97f5bc65f2319642719d5a061a3747f586bb7 Cr-Commit-Position: refs/heads/master@{#13268}

4 years, 6 months ago (2016-06-23 00:14:27 UTC) #45

honghaiz3

On 2016/06/23 00:14:27, commit-bot: I haz the power wrote: > Patchset 10 (id:??) landed as ...

4 years, 6 months ago (2016-06-24 17:26:38 UTC) #46

pthatcher1

Good catch. Having that check inside of ProxyConnection::Send seems kind of strange to me anyway, ...

4 years, 6 months ago (2016-06-24 17:42:04 UTC) #47

Taylor Brandstetter

4 years, 6 months ago (2016-06-24 18:18:26 UTC) #48

Message was sent while issue was closed.

On 2016/06/24 17:42:04, pthatcher1 wrote:
> Good catch.
> 
> Having that check inside of ProxyConnection::Send seems kind of strange to me
> anyway, especially since the only thing that calls it is
> P2PTransportChannel::SendPacket, and that thing already has a very similar
> check:
> 
>   if (best_connection_ == NULL) {
>     error_ = EWOULDBLOCK;
>     return -1;
>   }
> 
> So why not combine the two "EWOULDBLOCK" checks into one place in
> P2PTransportChannel::SendPacket and have it be like this:
> 
> if (!SelectedConnectionWritableOrPresumedWritable()) {
>   error_ = EWOULDBLOCK;
>   return -1;
> }
> 
> bool P2PTransportChannel::SelectedConnectionWritableOrPresumedWritable() {
>   return selected_connection_ && (selected_connection_->writable() || 
>                                   PresumedWritable(selected_connection_)
> }
> 
> That last method could also replace line 1266, which has the same logic.

I agree with Peter's suggestion. Like I said earlier, I'd like to have
Connection be as simple a class as possible and let P2PTransportChannel be the
place where the ICE state machine logic is defined.

Issue 2063823008: Adding IceConfig option to assume TURN/TURN candidate pairs will work. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Formatting. #

Patch Set 3 : Responding to comments. Doing "presumed writable" determination in Connection. #

Patch Set 4 : Rename IceConfig option and fix transition to STATE_WRITE_UNRELIABLE. #

Patch Set 5 : Changing write state comparison back to how it was originally. #

Patch Set 6 : Demonstrating another solution that doesn't require 2 enums. #

Patch Set 7 : Improving code readability. #

Patch Set 8 : Fixing "presumed writable" vs "unreliable" comparison. #

Patch Set 9 : Adding unit test and merging with master. #

Patch Set 10 : Merge with master again.. #

Messages