Add 16-bit network id to the candidate signaling.

Add 16-bit network id to the candidate signaling.
Also include that in the stun-ping request as part of the
network-info attribute.
Change the network cost to be 16 bits.

BUG=

Committed: https://crrev.com/a0c44eaa82de0c33a94b2eb06fa1dd89fd27aa60
Cr-Commit-Position: refs/heads/master@{#12110}

[honghaiz3, 2016-03-18 01:01:28]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2016-03-18 01:01:37]

Patchset #1 (id:20001) has been deleted

[honghaiz3, 2016-03-18 01:01:45]

Patchset #1 (id:40001) has been deleted

[honghaiz3, 2016-03-18 01:01:56]

Patchset #1 (id:60001) has been deleted

[honghaiz3, 2016-03-18 01:03:06]

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

[honghaiz3, 2016-03-18 01:03:07]

PTAL.

[pthatcher1, 2016-03-21 17:49:24]

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/base/network.cc
File webrtc/base/network.cc (right):

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/base/network.cc#ne...
webrtc/base/network.cc:289: net->set_id(next_available_network_id_++);
Should we skip the 0-value if it rolls over?

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/base/network.h#new...
webrtc/base/network.h:176: uint16_t next_available_network_id_ = 1;
We should probably make a comment that we're OK with this rolling over, and why.

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

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:630: (cost_attr) ?
static_cast<uint16_t>(cost_attr->value()) : 0;
Why can't we just have it be the same attribute, but fill in more bytes with the
network ID?

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

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:701: network_info = (network_info << 16) |
connection_->port()->network_cost();
Shouldn't we still have this behind an "if":

if (network_info > 0) {
  request->AddAttribute(...);
}

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/p2p/base/stun.h
File webrtc/p2p/base/stun.h (right):

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/p2p/base/stun.h#ne...
webrtc/p2p/base/stun.h:612: STUN_ATTR_NETWORK_INFO = 0xC058
I think we could just rename 0xC057 to STUN_ATTR_NETWORK_INFO and not have to
change the attribute ID.

[honghaiz3, 2016-03-22 16:30:03]

PTAL. Thanks!

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/base/network.cc
File webrtc/base/network.cc (right):

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/base/network.cc#ne...
webrtc/base/network.cc:289: net->set_id(next_available_network_id_++);
On 2016/03/21 17:49:24, pthatcher1 wrote:
> Should we skip the 0-value if it rolls over?

It is highly unlikely to roll over to 0. 
If a new network is generated once every 5 seconds, it would take 64000 * 5  =
320000 seconds to roll over. That's about 80 hours.  
Even if it does, having a network id 0 is still fine.

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/base/network.h#new...
webrtc/base/network.h:176: uint16_t next_available_network_id_ = 1;
On 2016/03/21 17:49:24, pthatcher1 wrote:
> We should probably make a comment that we're OK with this rolling over, and
why.

Done.

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

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:630: (cost_attr) ?
static_cast<uint16_t>(cost_attr->value()) : 0;
On 2016/03/21 17:49:24, pthatcher1 wrote:
> Why can't we just have it be the same attribute, but fill in more bytes with
the
> network ID?  
If a new client talks to an old client, the old  client will treat the new
client' ID+cost as cost. 
Perhaps it may not be a big issue as not so many clients have implemented the
network cost yet.

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

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:701: network_info = (network_info << 16) |
connection_->port()->network_cost();
On 2016/03/21 17:49:24, pthatcher1 wrote:
> Shouldn't we still have this behind an "if":
> 
> if (network_info > 0) {
>   request->AddAttribute(...);
> }

With this implementation, network id will always be > 0 (unless 2^16 network ids
have been generated, which is unlikely happen and happens only once).

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/p2p/base/stun.h
File webrtc/p2p/base/stun.h (right):

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/p2p/base/stun.h#ne...
webrtc/p2p/base/stun.h:612: STUN_ATTR_NETWORK_INFO = 0xC058
On 2016/03/21 17:49:24, pthatcher1 wrote:
> I think we could just rename 0xC057 to STUN_ATTR_NETWORK_INFO and not have to
> change the attribute ID.  
I am afraid of potential confusions between an old and a new client as I
explained in another response.

[pthatcher1, 2016-03-22 21:12:10]

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

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:630: (cost_attr) ?
static_cast<uint16_t>(cost_attr->value()) : 0;
On 2016/03/22 16:30:03, honghaiz3 wrote:
> On 2016/03/21 17:49:24, pthatcher1 wrote:
> > Why can't we just have it be the same attribute, but fill in more bytes with
> the
> > network ID?  
> If a new client talks to an old client, the old  client will treat the new
> client' ID+cost as cost. 
> Perhaps it may not be a big issue as not so many clients have implemented the
> network cost yet.

I think since the network cost is signalled, and this is really just for
peer-reflexive candidates, and since hardly any clients using this have been
deployed, we can safely switch over.

[honghaiz3, 2016-03-23 18:59:33]

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

https://codereview.webrtc.org/1815473002/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:630: (cost_attr) ?
static_cast<uint16_t>(cost_attr->value()) : 0;
On 2016/03/22 21:12:09, pthatcher1 wrote:
> On 2016/03/22 16:30:03, honghaiz3 wrote:
> > On 2016/03/21 17:49:24, pthatcher1 wrote:
> > > Why can't we just have it be the same attribute, but fill in more bytes
with
> > the
> > > network ID?  
> > If a new client talks to an old client, the old  client will treat the new
> > client' ID+cost as cost. 
> > Perhaps it may not be a big issue as not so many clients have implemented
the
> > network cost yet.
> 
> I think since the network cost is signalled, and this is really just for
> peer-reflexive candidates, and since hardly any clients using this have been
> deployed, we can safely switch over.

Done.

[pthatcher1, 2016-03-23 22:35:48]

lgtm

[honghaiz3, 2016-03-23 23:02:39]

The CQ bit was checked by honghaiz@webrtc.org

[commit-bot: I haz the power, 2016-03-23 23:02:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1815473002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1815473002/120001

[commit-bot: I haz the power, 2016-03-23 23:07:53]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2016-03-23 23:07:57]

Description was changed from

==========
Add 16-bit network id to the candidate signaling.
Also include that in the stun-ping request as part of the
network-info attribute.
Change the network cost to be 16 bits.

BUG=
==========

to

==========
Add 16-bit network id to the candidate signaling.
Also include that in the stun-ping request as part of the
network-info attribute.
Change the network cost to be 16 bits.

BUG=

Committed: https://crrev.com/a0c44eaa82de0c33a94b2eb06fa1dd89fd27aa60
Cr-Commit-Position: refs/heads/master@{#12110}
==========

[commit-bot: I haz the power, 2016-03-23 23:07:58]

Patchset 3 (id:??) landed as
https://crrev.com/a0c44eaa82de0c33a94b2eb06fa1dd89fd27aa60
Cr-Commit-Position: refs/heads/master@{#12110}