Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1668073002:
Add network cost as part of the connection comparison. (Closed)
DescriptionAdd network cost as part of the connection ranking.
For now, the network cost is purely based on the network type (cellular has cost 0xFFFF and everything else has cost 0).
Add cost to the candidate signaling and the stun request signaling (which is needed for peer reflexive candidates).
BUG=webrtc:4325
Committed: https://crrev.com/e1a0c942d670c5eeecb367a8d644c7ae9b999890
Cr-Commit-Position: refs/heads/master@{#11642}
Patch Set 1 : #
Total comments: 29
Patch Set 2 : #Patch Set 3 : #
Total comments: 7
Patch Set 4 : Add more tests and address comments #Patch Set 5 : Merge with head #Patch Set 6 : Fix sdp test error (do not signal network cost if it is 0) #
Messages
Total messages: 39 (15 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Add network metric as part of the connection comparison. BUG= ========== to ========== Add network cost as part of the connection ranking. For now, the network cost is purely based on the network type (cellular has cost 99 and everything else has cost 0). Add cost to the candidate signaling and the stun request signaling (which is needed for peer reflexive candidates). BUG= ==========
honghaiz@webrtc.org changed reviewers: + juberti@webrtc.org, pthatcher@webrtc.org
This is the draft CL for prioritizing Wifi over cellualr. Have not included rtt in the connection rank yet. Tested with AppRTCDemo. Will add tests once we generally agree with the design, naming, etc. By the way, I think the name of "cost" or "network_cost" perhaps better represents the restriction requested by each side, so I chose this name in the code. But, let me know if you feel the other way.
"Cost" is a good name, but let's pick good names for the values. This looks about right. Do you want to start adding unit tests? https://codereview.webrtc.org/1668073002/diff/40001/talk/app/webrtc/webrtcsdp.cc File talk/app/webrtc/webrtcsdp.cc (right): https://codereview.webrtc.org/1668073002/diff/40001/talk/app/webrtc/webrtcsdp... talk/app/webrtc/webrtcsdp.cc:1110: candidate->set_network_cost(std::min(cost, cricket::kMaxNetworkCost)); I think we should pick "cost" or "network cost" but not both. I'd say just go with "cost". A client might choose to use this for TURN vs. not as well. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/candidate.h File webrtc/p2p/base/candidate.h (right): https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/candidate... webrtc/p2p/base/candidate.h:32: const uint32_t kMaxNetworkCost = 99; I'd prefer to make this an enum. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:582: uint32_t network_cost = (cost_attr) ? cost_attr->value() : 0; Hmm... I didn't think about peer-reflexive candidates. That's a good point. We do need an attribute for that case. It's kind of sad that it will make the packets larger. I guess at least we can avoid sending it if it's zero/default. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:199: network_cost_ = (network_->type() == rtc::ADAPTER_TYPE_CELLULAR) ? 99 : 0; Yeah, I definitely would prefer an enum, something like: enum CandidateCost { CANDIDATE_COST_NONE = 0, // use "none" in SDP CANDIDATE_COST_HIGH = 1, // use "high" in SDP CANDIDATE_COST_LAST_RESTORT = 2 // use "last-restort" in SDP } https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:1163: int Connection::ComputeNetworkCost() const { Everwhere you have "networkcost", please just make it "cost". https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:399: // A virtual cost perceived by the user based on the network type. It is You should say, usually according to network type (WiFi vs. Cellular). I could be other things. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:400: // used to determine the rank of connections. Instead of talking about rank, just say that cost takes precedence over priority. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h File webrtc/p2p/base/stun.h (right): https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h#ne... webrtc/p2p/base/stun.h:607: STUN_ATTR_NETWORK_COST = 0x0026, // UInt32 According to RFC 5389, section 18.2, we have to choose a value in the range 0xC000 - 0xFFFF and make sure no one else is using it. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h#ne... webrtc/p2p/base/stun.h:624: case STUN_ATTR_NETWORK_COST: 32-bits is overkill. Let's make it uint8. We'll have to add a little code to parse a uint8, but that should be trivial. Actually, I'm tempted to save a byte by making us have no-content and instead have two attributes: one for HIGH and one for LAST_RESORT, just like CONTROLLED/CONTROLLING. Let's ask Justin what he thinks about that. It might be an over-optimization.
juberti@chromium.org changed reviewers: + juberti@chromium.org
I think there are a few open design questions, so we should try to nail down the design in the design doc first. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:49: if (a_cost < b_cost) { Can simply do return (b_cost - a_cost); https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:582: uint32_t network_cost = (cost_attr) ? cost_attr->value() : 0; On 2016/02/04 23:16:20, pthatcher1 wrote: > Hmm... I didn't think about peer-reflexive candidates. That's a good point. We > do need an attribute for that case. It's kind of sad that it will make the > packets larger. I guess at least we can avoid sending it if it's zero/default. It could also be inferred from the base, although that breaks down when we don't signal any candidates at all. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:199: network_cost_ = (network_->type() == rtc::ADAPTER_TYPE_CELLULAR) ? 99 : 0; On 2016/02/04 23:16:20, pthatcher1 wrote: > Yeah, I definitely would prefer an enum, something like: > > enum CandidateCost { > CANDIDATE_COST_NONE = 0, // use "none" in SDP > CANDIDATE_COST_HIGH = 1, // use "high" in SDP > CANDIDATE_COST_LAST_RESTORT = 2 // use "last-restort" in SDP > } I don't really like the enum since it's not easily forward-extensible. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:1163: int Connection::ComputeNetworkCost() const { On 2016/02/04 23:16:20, pthatcher1 wrote: > Everwhere you have "networkcost", please just make it "cost". Not sure about this. Network cost seems clearer than just 'cost', which seems like an antonym for 'priority'. Maybe this should be 'network_priority'. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h File webrtc/p2p/base/stun.h (right): https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h#ne... webrtc/p2p/base/stun.h:624: case STUN_ATTR_NETWORK_COST: On 2016/02/04 23:16:21, pthatcher1 wrote: > 32-bits is overkill. Let's make it uint8. We'll have to add a little code to > parse a uint8, but that should be trivial. > > Actually, I'm tempted to save a byte by making us have no-content and instead > have two attributes: one for HIGH and one for LAST_RESORT, just like > CONTROLLED/CONTROLLING. Let's ask Justin what he thinks about that. It might be > an over-optimization. attribs are padded to 32 bits, so no point in going smaller than 32 bits.
https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:199: network_cost_ = (network_->type() == rtc::ADAPTER_TYPE_CELLULAR) ? 99 : 0; On 2016/02/04 23:31:10, juberti2 wrote: > On 2016/02/04 23:16:20, pthatcher1 wrote: > > Yeah, I definitely would prefer an enum, something like: > > > > enum CandidateCost { > > CANDIDATE_COST_NONE = 0, // use "none" in SDP > > CANDIDATE_COST_HIGH = 1, // use "high" in SDP > > CANDIDATE_COST_LAST_RESTORT = 2 // use "last-restort" in SDP > > } > > I don't really like the enum since it's not easily forward-extensible. The algorithm we have designed so far only works for certain know values: A/B/C, or whatever we call them. It's not designed for arbitrary cost values. If we choose A = 0 and C = 99 right now, we effectively have an enum, whether we call it that or not. If our algorithm works with specific values, I think it should be an enum. If it works with arbitrary values in a range, then we should make it an integer. But the later is not what we've designed so far. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:1163: int Connection::ComputeNetworkCost() const { On 2016/02/04 23:31:10, juberti2 wrote: > On 2016/02/04 23:16:20, pthatcher1 wrote: > > Everwhere you have "networkcost", please just make it "cost". > > Not sure about this. Network cost seems clearer than just 'cost', which seems > like an antonym for 'priority'. Maybe this should be 'network_priority'. I don't feel strongly one way or the other, I just want it consistent. If it's "network cost" then the SDP should have "network-cost", not just "cost". https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h File webrtc/p2p/base/stun.h (right): https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h#ne... webrtc/p2p/base/stun.h:624: case STUN_ATTR_NETWORK_COST: On 2016/02/04 23:31:10, juberti2 wrote: > On 2016/02/04 23:16:21, pthatcher1 wrote: > > 32-bits is overkill. Let's make it uint8. We'll have to add a little code to > > parse a uint8, but that should be trivial. > > > > Actually, I'm tempted to save a byte by making us have no-content and instead > > have two attributes: one for HIGH and one for LAST_RESORT, just like > > CONTROLLED/CONTROLLING. Let's ask Justin what he thinks about that. It might > be > > an over-optimization. > > attribs are padded to 32 bits, so no point in going smaller than 32 bits. Yes, I forgot about that. It's pretty lame when you're trying to save bytes. 4 bytes seems like a lot to waste. Should we make the default non-zero so that cellular networks are the one that can avoid sending it and WiFi networks are the one that needs to send it?
On 2016/02/04 23:51:18, pthatcher1 wrote: > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc > File webrtc/p2p/base/port.cc (right): > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... > webrtc/p2p/base/port.cc:199: network_cost_ = (network_->type() == > rtc::ADAPTER_TYPE_CELLULAR) ? 99 : 0; > On 2016/02/04 23:31:10, juberti2 wrote: > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > Yeah, I definitely would prefer an enum, something like: > > > > > > enum CandidateCost { > > > CANDIDATE_COST_NONE = 0, // use "none" in SDP > > > CANDIDATE_COST_HIGH = 1, // use "high" in SDP > > > CANDIDATE_COST_LAST_RESTORT = 2 // use "last-restort" in SDP > > > } > > > > I don't really like the enum since it's not easily forward-extensible. > > The algorithm we have designed so far only works for certain know values: A/B/C, > or whatever we call them. It's not designed for arbitrary cost values. If we > choose A = 0 and C = 99 right now, we effectively have an enum, whether we call > it that or not. If our algorithm works with specific values, I think it should > be an enum. If it works with arbitrary values in a range, then we should make > it an integer. But the later is not what we've designed so far. Honghai showed me a metric function that would work with arbitrary values, IIUC. I found that attractive. > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... > webrtc/p2p/base/port.cc:1163: int Connection::ComputeNetworkCost() const { > On 2016/02/04 23:31:10, juberti2 wrote: > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > Everwhere you have "networkcost", please just make it "cost". > > > > Not sure about this. Network cost seems clearer than just 'cost', which seems > > like an antonym for 'priority'. Maybe this should be 'network_priority'. > > I don't feel strongly one way or the other, I just want it consistent. If it's > "network cost" then the SDP should have "network-cost", not just "cost". > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h > File webrtc/p2p/base/stun.h (right): > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h#ne... > webrtc/p2p/base/stun.h:624: case STUN_ATTR_NETWORK_COST: > On 2016/02/04 23:31:10, juberti2 wrote: > > On 2016/02/04 23:16:21, pthatcher1 wrote: > > > 32-bits is overkill. Let's make it uint8. We'll have to add a little code > to > > > parse a uint8, but that should be trivial. > > > > > > Actually, I'm tempted to save a byte by making us have no-content and > instead > > > have two attributes: one for HIGH and one for LAST_RESORT, just like > > > CONTROLLED/CONTROLLING. Let's ask Justin what he thinks about that. It > might > > be > > > an over-optimization. > > > > attribs are padded to 32 bits, so no point in going smaller than 32 bits. > > Yes, I forgot about that. It's pretty lame when you're trying to save bytes. > 4 bytes seems like a lot to waste. Should we make the default non-zero so that > cellular networks are the one that can avoid sending it and WiFi networks are > the one that needs to send it? interesting idea, but that would mean we assume all old clients are on cellular networks. Perhaps that's what we want (i.e. network_priority of zero is the default).
On 2016/02/05 00:39:29, juberti2 wrote: > On 2016/02/04 23:51:18, pthatcher1 wrote: > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc > > File webrtc/p2p/base/port.cc (right): > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... > > webrtc/p2p/base/port.cc:199: network_cost_ = (network_->type() == > > rtc::ADAPTER_TYPE_CELLULAR) ? 99 : 0; > > On 2016/02/04 23:31:10, juberti2 wrote: > > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > > Yeah, I definitely would prefer an enum, something like: > > > > > > > > enum CandidateCost { > > > > CANDIDATE_COST_NONE = 0, // use "none" in SDP > > > > CANDIDATE_COST_HIGH = 1, // use "high" in SDP > > > > CANDIDATE_COST_LAST_RESTORT = 2 // use "last-restort" in SDP > > > > } > > > > > > I don't really like the enum since it's not easily forward-extensible. > > > > The algorithm we have designed so far only works for certain know values: > A/B/C, > > or whatever we call them. It's not designed for arbitrary cost values. If we > > choose A = 0 and C = 99 right now, we effectively have an enum, whether we > call > > it that or not. If our algorithm works with specific values, I think it > should > > be an enum. If it works with arbitrary values in a range, then we should make > > it an integer. But the later is not what we've designed so far. > > Honghai showed me a metric function that would work with arbitrary values, IIUC. > I found that attractive. To account for the fact that if both sides use cellular, its rtt needs to 2 * DELTA smaller than a connection that uses Wifi on both sides, I designed the cost metric like this: cost of a connection = local candidate network cost + remote candidate_network cost + round(rtt/100ms) (although in this CL I have not included the rtt yet). A user can pick a very large value (like 99) as the cost for cellular if the user intends to use cellular as the last resort. And a user can choose a smaller cost value (e.g., 2-5) for case B if he intends to use cellular if cellular improved the quality (rtt) significantly. and choose cost 0 for case A. We can discuss more on this tomorrow. > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... > > webrtc/p2p/base/port.cc:1163: int Connection::ComputeNetworkCost() const { > > On 2016/02/04 23:31:10, juberti2 wrote: > > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > > Everwhere you have "networkcost", please just make it "cost". > > > > > > Not sure about this. Network cost seems clearer than just 'cost', which > seems > > > like an antonym for 'priority'. Maybe this should be 'network_priority'. > > > > I don't feel strongly one way or the other, I just want it consistent. If > it's > > "network cost" then the SDP should have "network-cost", not just "cost". > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h > > File webrtc/p2p/base/stun.h (right): > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h#ne... > > webrtc/p2p/base/stun.h:624: case STUN_ATTR_NETWORK_COST: > > On 2016/02/04 23:31:10, juberti2 wrote: > > > On 2016/02/04 23:16:21, pthatcher1 wrote: > > > > 32-bits is overkill. Let's make it uint8. We'll have to add a little > code > > to > > > > parse a uint8, but that should be trivial. > > > > > > > > Actually, I'm tempted to save a byte by making us have no-content and > > instead > > > > have two attributes: one for HIGH and one for LAST_RESORT, just like > > > > CONTROLLED/CONTROLLING. Let's ask Justin what he thinks about that. It > > might > > > be > > > > an over-optimization. > > > > > > attribs are padded to 32 bits, so no point in going smaller than 32 bits. > > > > Yes, I forgot about that. It's pretty lame when you're trying to save bytes. > > > 4 bytes seems like a lot to waste. Should we make the default non-zero so > that > > cellular networks are the one that can avoid sending it and WiFi networks are > > the one that needs to send it? > > interesting idea, but that would mean we assume all old clients are on cellular > networks. Perhaps that's what we want (i.e. network_priority of zero is the > default).
Made a few changes based on the comments and will leave the rest until further discussion. https://codereview.webrtc.org/1668073002/diff/40001/talk/app/webrtc/webrtcsdp.cc File talk/app/webrtc/webrtcsdp.cc (right): https://codereview.webrtc.org/1668073002/diff/40001/talk/app/webrtc/webrtcsdp... talk/app/webrtc/webrtcsdp.cc:1110: candidate->set_network_cost(std::min(cost, cricket::kMaxNetworkCost)); On 2016/02/04 23:16:20, pthatcher1 wrote: > I think we should pick "cost" or "network cost" but not both. I'd say just go > with "cost". A client might choose to use this for TURN vs. not as well. Discussed in other comments. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/candidate.h File webrtc/p2p/base/candidate.h (right): https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/candidate... webrtc/p2p/base/candidate.h:32: const uint32_t kMaxNetworkCost = 99; On 2016/02/04 23:16:20, pthatcher1 wrote: > I'd prefer to make this an enum. Acknowledged. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:49: if (a_cost < b_cost) { On 2016/02/04 23:31:10, juberti2 wrote: > Can simply do > > return (b_cost - a_cost); Not really because in the case that they are equal, it should not return but continue to the next step. I thought about doing something like cost_diff = a->ComputeNetworkCost() - b->ComputeNetworkCost() if (cost_diff != 0) return cost_diff. This is slightly simpler but easier to make mistake about the sign. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:582: uint32_t network_cost = (cost_attr) ? cost_attr->value() : 0; On 2016/02/04 23:31:10, juberti2 wrote: > On 2016/02/04 23:16:20, pthatcher1 wrote: > > Hmm... I didn't think about peer-reflexive candidates. That's a good point. > We > > do need an attribute for that case. It's kind of sad that it will make the > > packets larger. I guess at least we can avoid sending it if it's > zero/default. > > It could also be inferred from the base, although that breaks down when we don't > signal any candidates at all. Guess we'd better have a reliable way to get it in any circumstance. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:199: network_cost_ = (network_->type() == rtc::ADAPTER_TYPE_CELLULAR) ? 99 : 0; On 2016/02/04 23:51:18, pthatcher1 wrote: > On 2016/02/04 23:31:10, juberti2 wrote: > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > Yeah, I definitely would prefer an enum, something like: > > > > > > enum CandidateCost { > > > CANDIDATE_COST_NONE = 0, // use "none" in SDP > > > CANDIDATE_COST_HIGH = 1, // use "high" in SDP > > > CANDIDATE_COST_LAST_RESTORT = 2 // use "last-restort" in SDP > > > } > > > > I don't really like the enum since it's not easily forward-extensible. > > The algorithm we have designed so far only works for certain know values: A/B/C, > or whatever we call them. It's not designed for arbitrary cost values. If we > choose A = 0 and C = 99 right now, we effectively have an enum, whether we call > it that or not. If our algorithm works with specific values, I think it should > be an enum. If it works with arbitrary values in a range, then we should make > it an integer. But the later is not what we've designed so far. Perhaps, we can use enum to define a few integer values which can be used. like enum CandidateCost { CANDIDATE_COST_NONE = 0, CANDIDATE_COST_LOW = 2, CANDIDATE_COST_MEDIUM = 4, CANDIDATE_COST_HIGH = 10, CANDIDATE_COST_MAX = 99 } Will leave this open depending on further discussion. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:1163: int Connection::ComputeNetworkCost() const { On 2016/02/04 23:51:18, pthatcher1 wrote: > On 2016/02/04 23:31:10, juberti2 wrote: > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > Everwhere you have "networkcost", please just make it "cost". > > > > Not sure about this. Network cost seems clearer than just 'cost', which seems > > like an antonym for 'priority'. Maybe this should be 'network_priority'. > > I don't feel strongly one way or the other, I just want it consistent. If it's > "network cost" then the SDP should have "network-cost", not just "cost". When I put "cost" in the sdp, I intended to save a few bytes. Not sure if that is super-critical though. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:399: // A virtual cost perceived by the user based on the network type. It is On 2016/02/04 23:16:21, pthatcher1 wrote: > You should say, usually according to network type (WiFi vs. Cellular). I could > be other things. Done. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:400: // used to determine the rank of connections. On 2016/02/04 23:16:21, pthatcher1 wrote: > Instead of talking about rank, just say that cost takes precedence over > priority. Done.
On 2016/02/05 00:39:29, juberti2 wrote: > On 2016/02/04 23:51:18, pthatcher1 wrote: > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc > > File webrtc/p2p/base/port.cc (right): > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... > > webrtc/p2p/base/port.cc:199: network_cost_ = (network_->type() == > > rtc::ADAPTER_TYPE_CELLULAR) ? 99 : 0; > > On 2016/02/04 23:31:10, juberti2 wrote: > > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > > Yeah, I definitely would prefer an enum, something like: > > > > > > > > enum CandidateCost { > > > > CANDIDATE_COST_NONE = 0, // use "none" in SDP > > > > CANDIDATE_COST_HIGH = 1, // use "high" in SDP > > > > CANDIDATE_COST_LAST_RESTORT = 2 // use "last-restort" in SDP > > > > } > > > > > > I don't really like the enum since it's not easily forward-extensible. > > > > The algorithm we have designed so far only works for certain know values: > A/B/C, > > or whatever we call them. It's not designed for arbitrary cost values. If we > > choose A = 0 and C = 99 right now, we effectively have an enum, whether we > call > > it that or not. If our algorithm works with specific values, I think it > should > > be an enum. If it works with arbitrary values in a range, then we should make > > it an integer. But the later is not what we've designed so far. > > Honghai showed me a metric function that would work with arbitrary values, IIUC. > I found that attractive. > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... > > webrtc/p2p/base/port.cc:1163: int Connection::ComputeNetworkCost() const { > > On 2016/02/04 23:31:10, juberti2 wrote: > > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > > Everwhere you have "networkcost", please just make it "cost". > > > > > > Not sure about this. Network cost seems clearer than just 'cost', which > seems > > > like an antonym for 'priority'. Maybe this should be 'network_priority'. > > > > I don't feel strongly one way or the other, I just want it consistent. If > it's > > "network cost" then the SDP should have "network-cost", not just "cost". > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h > > File webrtc/p2p/base/stun.h (right): > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h#ne... > > webrtc/p2p/base/stun.h:624: case STUN_ATTR_NETWORK_COST: > > On 2016/02/04 23:31:10, juberti2 wrote: > > > On 2016/02/04 23:16:21, pthatcher1 wrote: > > > > 32-bits is overkill. Let's make it uint8. We'll have to add a little > code > > to > > > > parse a uint8, but that should be trivial. > > > > > > > > Actually, I'm tempted to save a byte by making us have no-content and > > instead > > > > have two attributes: one for HIGH and one for LAST_RESORT, just like > > > > CONTROLLED/CONTROLLING. Let's ask Justin what he thinks about that. It > > might > > > be > > > > an over-optimization. > > > > > > attribs are padded to 32 bits, so no point in going smaller than 32 bits. > > > > Yes, I forgot about that. It's pretty lame when you're trying to save bytes. > > > 4 bytes seems like a lot to waste. Should we make the default non-zero so > that > > cellular networks are the one that can avoid sending it and WiFi networks are > > the one that needs to send it? > > interesting idea, but that would mean we assume all old clients are on cellular > networks. Perhaps that's what we want (i.e. network_priority of zero is the > default). For old clients, either we wrongly think WiFi is expensive or we wrongly think cellular is cheap. Which is worse? If we're going to be a "gracious" controller, I'd saying thinking WiFi is expensive is better, which would lend toward a default of a non-zero default.
https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:199: network_cost_ = (network_->type() == rtc::ADAPTER_TYPE_CELLULAR) ? 99 : 0; On 2016/02/04 23:51:18, pthatcher1 wrote: > On 2016/02/04 23:31:10, juberti2 wrote: > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > Yeah, I definitely would prefer an enum, something like: > > > > > > enum CandidateCost { > > > CANDIDATE_COST_NONE = 0, // use "none" in SDP > > > CANDIDATE_COST_HIGH = 1, // use "high" in SDP > > > CANDIDATE_COST_LAST_RESTORT = 2 // use "last-restort" in SDP > > > } > > > > I don't really like the enum since it's not easily forward-extensible. > > The algorithm we have designed so far only works for certain know values: A/B/C, > or whatever we call them. It's not designed for arbitrary cost values. If we > choose A = 0 and C = 99 right now, we effectively have an enum, whether we call > it that or not. If our algorithm works with specific values, I think it should > be an enum. If it works with arbitrary values in a range, then we should make > it an integer. But the later is not what we've designed so far. Thinking about this some more: I do now see merit in using the same thing in the SDP and the STUN attr (an integer or a string in both), since the mapping is much easier. And an integer is clearly better for the STUN packets, so you're right about that. Let's make it an int.
On 2016/02/05 01:42:35, pthatcher1 wrote: > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc > File webrtc/p2p/base/port.cc (right): > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... > webrtc/p2p/base/port.cc:199: network_cost_ = (network_->type() == > rtc::ADAPTER_TYPE_CELLULAR) ? 99 : 0; > On 2016/02/04 23:51:18, pthatcher1 wrote: > > On 2016/02/04 23:31:10, juberti2 wrote: > > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > > Yeah, I definitely would prefer an enum, something like: > > > > > > > > enum CandidateCost { > > > > CANDIDATE_COST_NONE = 0, // use "none" in SDP > > > > CANDIDATE_COST_HIGH = 1, // use "high" in SDP > > > > CANDIDATE_COST_LAST_RESTORT = 2 // use "last-restort" in SDP > > > > } > > > > > > I don't really like the enum since it's not easily forward-extensible. > > > > The algorithm we have designed so far only works for certain know values: > A/B/C, > > or whatever we call them. It's not designed for arbitrary cost values. If we > > choose A = 0 and C = 99 right now, we effectively have an enum, whether we > call > > it that or not. If our algorithm works with specific values, I think it > should > > be an enum. If it works with arbitrary values in a range, then we should make > > it an integer. But the later is not what we've designed so far. > > Thinking about this some more: I do now see merit in using the same thing in the > SDP and the STUN attr (an integer or a string in both), since the mapping is > much easier. And an integer is clearly better for the STUN packets, so you're > right about that. Let's make it an int. Persuasive argument.
On 2016/02/05 01:05:17, honghaiz3 wrote: > On 2016/02/05 00:39:29, juberti2 wrote: > > On 2016/02/04 23:51:18, pthatcher1 wrote: > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc > > > File webrtc/p2p/base/port.cc (right): > > > > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... > > > webrtc/p2p/base/port.cc:199: network_cost_ = (network_->type() == > > > rtc::ADAPTER_TYPE_CELLULAR) ? 99 : 0; > > > On 2016/02/04 23:31:10, juberti2 wrote: > > > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > > > Yeah, I definitely would prefer an enum, something like: > > > > > > > > > > enum CandidateCost { > > > > > CANDIDATE_COST_NONE = 0, // use "none" in SDP > > > > > CANDIDATE_COST_HIGH = 1, // use "high" in SDP > > > > > CANDIDATE_COST_LAST_RESTORT = 2 // use "last-restort" in SDP > > > > > } > > > > > > > > I don't really like the enum since it's not easily forward-extensible. > > > > > > The algorithm we have designed so far only works for certain know values: > > A/B/C, > > > or whatever we call them. It's not designed for arbitrary cost values. If > we > > > choose A = 0 and C = 99 right now, we effectively have an enum, whether we > > call > > > it that or not. If our algorithm works with specific values, I think it > > should > > > be an enum. If it works with arbitrary values in a range, then we should > make > > > it an integer. But the later is not what we've designed so far. > > > > Honghai showed me a metric function that would work with arbitrary values, > IIUC. > > I found that attractive. > To account for the fact that if both sides use cellular, its rtt needs to 2 * > DELTA smaller than a connection that uses Wifi on both sides, > I designed the cost metric like this: > cost of a connection = local candidate network cost + remote candidate_network > cost + round(rtt/100ms) > (although in this CL I have not included the rtt yet). > A user can pick a very large value (like 99) as the cost for cellular if the > user intends to use cellular as the last resort. > And a user can choose a smaller cost value (e.g., 2-5) for case B if he intends > to use cellular if cellular improved the quality (rtt) significantly. > and choose cost 0 for case A. > We can discuss more on this tomorrow. I'll have to think through this some more, because it's quite a bit different than what we came up with before. My first thought is that it seems a little too tied to RTT. What if we decide to use other things in the future, like bandwidth, jitter, packet loss, whatever? Will it still work? > > > > > > > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... > > > webrtc/p2p/base/port.cc:1163: int Connection::ComputeNetworkCost() const { > > > On 2016/02/04 23:31:10, juberti2 wrote: > > > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > > > Everwhere you have "networkcost", please just make it "cost". > > > > > > > > Not sure about this. Network cost seems clearer than just 'cost', which > > seems > > > > like an antonym for 'priority'. Maybe this should be 'network_priority'. > > > > > > I don't feel strongly one way or the other, I just want it consistent. If > > it's > > > "network cost" then the SDP should have "network-cost", not just "cost". > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h > > > File webrtc/p2p/base/stun.h (right): > > > > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h#ne... > > > webrtc/p2p/base/stun.h:624: case STUN_ATTR_NETWORK_COST: > > > On 2016/02/04 23:31:10, juberti2 wrote: > > > > On 2016/02/04 23:16:21, pthatcher1 wrote: > > > > > 32-bits is overkill. Let's make it uint8. We'll have to add a little > > code > > > to > > > > > parse a uint8, but that should be trivial. > > > > > > > > > > Actually, I'm tempted to save a byte by making us have no-content and > > > instead > > > > > have two attributes: one for HIGH and one for LAST_RESORT, just like > > > > > CONTROLLED/CONTROLLING. Let's ask Justin what he thinks about that. It > > > might > > > > be > > > > > an over-optimization. > > > > > > > > attribs are padded to 32 bits, so no point in going smaller than 32 bits. > > > > > > Yes, I forgot about that. It's pretty lame when you're trying to save > bytes. > > > > > 4 bytes seems like a lot to waste. Should we make the default non-zero so > > that > > > cellular networks are the one that can avoid sending it and WiFi networks > are > > > the one that needs to send it? > > > > interesting idea, but that would mean we assume all old clients are on > cellular > > networks. Perhaps that's what we want (i.e. network_priority of zero is the > > default).
On 2016/02/05 01:40:22, pthatcher1 wrote: > On 2016/02/05 00:39:29, juberti2 wrote: > > On 2016/02/04 23:51:18, pthatcher1 wrote: > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc > > > File webrtc/p2p/base/port.cc (right): > > > > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... > > > webrtc/p2p/base/port.cc:199: network_cost_ = (network_->type() == > > > rtc::ADAPTER_TYPE_CELLULAR) ? 99 : 0; > > > On 2016/02/04 23:31:10, juberti2 wrote: > > > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > > > Yeah, I definitely would prefer an enum, something like: > > > > > > > > > > enum CandidateCost { > > > > > CANDIDATE_COST_NONE = 0, // use "none" in SDP > > > > > CANDIDATE_COST_HIGH = 1, // use "high" in SDP > > > > > CANDIDATE_COST_LAST_RESTORT = 2 // use "last-restort" in SDP > > > > > } > > > > > > > > I don't really like the enum since it's not easily forward-extensible. > > > > > > The algorithm we have designed so far only works for certain know values: > > A/B/C, > > > or whatever we call them. It's not designed for arbitrary cost values. If > we > > > choose A = 0 and C = 99 right now, we effectively have an enum, whether we > > call > > > it that or not. If our algorithm works with specific values, I think it > > should > > > be an enum. If it works with arbitrary values in a range, then we should > make > > > it an integer. But the later is not what we've designed so far. > > > > Honghai showed me a metric function that would work with arbitrary values, > IIUC. > > I found that attractive. > > > > > > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... > > > webrtc/p2p/base/port.cc:1163: int Connection::ComputeNetworkCost() const { > > > On 2016/02/04 23:31:10, juberti2 wrote: > > > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > > > Everwhere you have "networkcost", please just make it "cost". > > > > > > > > Not sure about this. Network cost seems clearer than just 'cost', which > > seems > > > > like an antonym for 'priority'. Maybe this should be 'network_priority'. > > > > > > I don't feel strongly one way or the other, I just want it consistent. If > > it's > > > "network cost" then the SDP should have "network-cost", not just "cost". > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h > > > File webrtc/p2p/base/stun.h (right): > > > > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h#ne... > > > webrtc/p2p/base/stun.h:624: case STUN_ATTR_NETWORK_COST: > > > On 2016/02/04 23:31:10, juberti2 wrote: > > > > On 2016/02/04 23:16:21, pthatcher1 wrote: > > > > > 32-bits is overkill. Let's make it uint8. We'll have to add a little > > code > > > to > > > > > parse a uint8, but that should be trivial. > > > > > > > > > > Actually, I'm tempted to save a byte by making us have no-content and > > > instead > > > > > have two attributes: one for HIGH and one for LAST_RESORT, just like > > > > > CONTROLLED/CONTROLLING. Let's ask Justin what he thinks about that. It > > > might > > > > be > > > > > an over-optimization. > > > > > > > > attribs are padded to 32 bits, so no point in going smaller than 32 bits. > > > > > > Yes, I forgot about that. It's pretty lame when you're trying to save > bytes. > > > > > 4 bytes seems like a lot to waste. Should we make the default non-zero so > > that > > > cellular networks are the one that can avoid sending it and WiFi networks > are > > > the one that needs to send it? > > > > interesting idea, but that would mean we assume all old clients are on > cellular > > networks. Perhaps that's what we want (i.e. network_priority of zero is the > > default). > > For old clients, either we wrongly think WiFi is expensive or we wrongly think > cellular is cheap. Which is worse? If we're going to be a "gracious" > controller, I'd saying thinking WiFi is expensive is better, which would lend > toward a default of a non-zero default. I think we should invert it and make it network_priority, where 0 is least preferred and MAX_UINT is most preferred. If omitted, we could pick some 'balanced' value, e.g. MAX_UINT / 2. However, I tend to think that this is all pretty crufty, and what we should really do is just define a more compact ICE attribute where we can send everything we care about - controlling, net_priority, username - all in a single chunk.
On 2016/02/05 01:45:52, pthatcher1 wrote: > On 2016/02/05 01:05:17, honghaiz3 wrote: > > On 2016/02/05 00:39:29, juberti2 wrote: > > > On 2016/02/04 23:51:18, pthatcher1 wrote: > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc > > > > File webrtc/p2p/base/port.cc (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... > > > > webrtc/p2p/base/port.cc:199: network_cost_ = (network_->type() == > > > > rtc::ADAPTER_TYPE_CELLULAR) ? 99 : 0; > > > > On 2016/02/04 23:31:10, juberti2 wrote: > > > > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > > > > Yeah, I definitely would prefer an enum, something like: > > > > > > > > > > > > enum CandidateCost { > > > > > > CANDIDATE_COST_NONE = 0, // use "none" in SDP > > > > > > CANDIDATE_COST_HIGH = 1, // use "high" in SDP > > > > > > CANDIDATE_COST_LAST_RESTORT = 2 // use "last-restort" in SDP > > > > > > } > > > > > > > > > > I don't really like the enum since it's not easily forward-extensible. > > > > > > > > The algorithm we have designed so far only works for certain know values: > > > A/B/C, > > > > or whatever we call them. It's not designed for arbitrary cost values. > If > > we > > > > choose A = 0 and C = 99 right now, we effectively have an enum, whether we > > > call > > > > it that or not. If our algorithm works with specific values, I think it > > > should > > > > be an enum. If it works with arbitrary values in a range, then we should > > make > > > > it an integer. But the later is not what we've designed so far. > > > > > > Honghai showed me a metric function that would work with arbitrary values, > > IIUC. > > > I found that attractive. > > To account for the fact that if both sides use cellular, its rtt needs to 2 * > > DELTA smaller than a connection that uses Wifi on both sides, > > I designed the cost metric like this: > > cost of a connection = local candidate network cost + remote candidate_network > > cost + round(rtt/100ms) > > (although in this CL I have not included the rtt yet). > > A user can pick a very large value (like 99) as the cost for cellular if the > > user intends to use cellular as the last resort. > > And a user can choose a smaller cost value (e.g., 2-5) for case B if he > intends > > to use cellular if cellular improved the quality (rtt) significantly. > > and choose cost 0 for case A. > > We can discuss more on this tomorrow. > > I'll have to think through this some more, because it's quite a bit different > than what we came up with before. My first thought is that it seems a little > too tied to RTT. What if we decide to use other things in the future, like > bandwidth, jitter, packet loss, whatever? Will it still work? Basically I think we will need to combine everything into a single metric. If the metric is the larger, the better, it will be something like bandwidth. If it is the smaller, the better, it will be something like rtt or network cost (rtt can also be viewed as a network cost). Bandwidth metric can be converted into a metric similar to the cost metric by doing 1 / bandwidth. If one metric (like bandwidth) is more important than rtt, just put it on the higher bits or scale it with a larger factor. > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... > > > > webrtc/p2p/base/port.cc:1163: int Connection::ComputeNetworkCost() const { > > > > On 2016/02/04 23:31:10, juberti2 wrote: > > > > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > > > > Everwhere you have "networkcost", please just make it "cost". > > > > > > > > > > Not sure about this. Network cost seems clearer than just 'cost', which > > > seems > > > > > like an antonym for 'priority'. Maybe this should be 'network_priority'. > > > > > > > > I don't feel strongly one way or the other, I just want it consistent. If > > > it's > > > > "network cost" then the SDP should have "network-cost", not just "cost". > > > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h > > > > File webrtc/p2p/base/stun.h (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h#ne... > > > > webrtc/p2p/base/stun.h:624: case STUN_ATTR_NETWORK_COST: > > > > On 2016/02/04 23:31:10, juberti2 wrote: > > > > > On 2016/02/04 23:16:21, pthatcher1 wrote: > > > > > > 32-bits is overkill. Let's make it uint8. We'll have to add a little > > > code > > > > to > > > > > > parse a uint8, but that should be trivial. > > > > > > > > > > > > Actually, I'm tempted to save a byte by making us have no-content and > > > > instead > > > > > > have two attributes: one for HIGH and one for LAST_RESORT, just like > > > > > > CONTROLLED/CONTROLLING. Let's ask Justin what he thinks about that. > It > > > > might > > > > > be > > > > > > an over-optimization. > > > > > > > > > > attribs are padded to 32 bits, so no point in going smaller than 32 > bits. > > > > > > > > Yes, I forgot about that. It's pretty lame when you're trying to save > > bytes. > > > > > > > 4 bytes seems like a lot to waste. Should we make the default non-zero so > > > that > > > > cellular networks are the one that can avoid sending it and WiFi networks > > are > > > > the one that needs to send it? > > > > > > interesting idea, but that would mean we assume all old clients are on > > cellular > > > networks. Perhaps that's what we want (i.e. network_priority of zero is the > > > default).
On 2016/02/05 01:50:20, juberti2 wrote: > On 2016/02/05 01:40:22, pthatcher1 wrote: > > On 2016/02/05 00:39:29, juberti2 wrote: > > > On 2016/02/04 23:51:18, pthatcher1 wrote: > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc > > > > File webrtc/p2p/base/port.cc (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... > > > > webrtc/p2p/base/port.cc:199: network_cost_ = (network_->type() == > > > > rtc::ADAPTER_TYPE_CELLULAR) ? 99 : 0; > > > > On 2016/02/04 23:31:10, juberti2 wrote: > > > > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > > > > Yeah, I definitely would prefer an enum, something like: > > > > > > > > > > > > enum CandidateCost { > > > > > > CANDIDATE_COST_NONE = 0, // use "none" in SDP > > > > > > CANDIDATE_COST_HIGH = 1, // use "high" in SDP > > > > > > CANDIDATE_COST_LAST_RESTORT = 2 // use "last-restort" in SDP > > > > > > } > > > > > > > > > > I don't really like the enum since it's not easily forward-extensible. > > > > > > > > The algorithm we have designed so far only works for certain know values: > > > A/B/C, > > > > or whatever we call them. It's not designed for arbitrary cost values. > If > > we > > > > choose A = 0 and C = 99 right now, we effectively have an enum, whether we > > > call > > > > it that or not. If our algorithm works with specific values, I think it > > > should > > > > be an enum. If it works with arbitrary values in a range, then we should > > make > > > > it an integer. But the later is not what we've designed so far. > > > > > > Honghai showed me a metric function that would work with arbitrary values, > > IIUC. > > > I found that attractive. > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... > > > > webrtc/p2p/base/port.cc:1163: int Connection::ComputeNetworkCost() const { > > > > On 2016/02/04 23:31:10, juberti2 wrote: > > > > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > > > > Everwhere you have "networkcost", please just make it "cost". > > > > > > > > > > Not sure about this. Network cost seems clearer than just 'cost', which > > > seems > > > > > like an antonym for 'priority'. Maybe this should be 'network_priority'. > > > > > > > > I don't feel strongly one way or the other, I just want it consistent. If > > > it's > > > > "network cost" then the SDP should have "network-cost", not just "cost". > > > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h > > > > File webrtc/p2p/base/stun.h (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/stun.h#ne... > > > > webrtc/p2p/base/stun.h:624: case STUN_ATTR_NETWORK_COST: > > > > On 2016/02/04 23:31:10, juberti2 wrote: > > > > > On 2016/02/04 23:16:21, pthatcher1 wrote: > > > > > > 32-bits is overkill. Let's make it uint8. We'll have to add a little > > > code > > > > to > > > > > > parse a uint8, but that should be trivial. > > > > > > > > > > > > Actually, I'm tempted to save a byte by making us have no-content and > > > > instead > > > > > > have two attributes: one for HIGH and one for LAST_RESORT, just like > > > > > > CONTROLLED/CONTROLLING. Let's ask Justin what he thinks about that. > It > > > > might > > > > > be > > > > > > an over-optimization. > > > > > > > > > > attribs are padded to 32 bits, so no point in going smaller than 32 > bits. > > > > > > > > Yes, I forgot about that. It's pretty lame when you're trying to save > > bytes. > > > > > > > 4 bytes seems like a lot to waste. Should we make the default non-zero so > > > that > > > > cellular networks are the one that can avoid sending it and WiFi networks > > are > > > > the one that needs to send it? > > > > > > interesting idea, but that would mean we assume all old clients are on > > cellular > > > networks. Perhaps that's what we want (i.e. network_priority of zero is the > > > default). > > > > For old clients, either we wrongly think WiFi is expensive or we wrongly think > > cellular is cheap. Which is worse? If we're going to be a "gracious" > > controller, I'd saying thinking WiFi is expensive is better, which would lend > > toward a default of a non-zero default. > > I think we should invert it and make it network_priority, where 0 is least > preferred and MAX_UINT is most preferred. > If omitted, we could pick some 'balanced' value, e.g. MAX_UINT / 2. > Having omission mean "balanced" seems like a good idea. At first, I didn't like the idea of reversing the direction, but the more I think about it, the more I like the idea of 0 meaning "last resort". And I like the idea of having net_prioirty be just like priority, except that it takes precedence. It's like we have an inter-network-interface priority (the new one) and an intra-network-interface priority (the old one). > However, I tend to think that this is all pretty crufty, and what we should > really do is just define a more compact ICE attribute where we can send > everything we care about - controlling, net_priority, username - all in a single > chunk. I like that idea, assuming we can make it take less space. If we do, we should probably make the range of network priority smaller than 32 bits.
Description was changed from ========== Add network cost as part of the connection ranking. For now, the network cost is purely based on the network type (cellular has cost 99 and everything else has cost 0). Add cost to the candidate signaling and the stun request signaling (which is needed for peer reflexive candidates). BUG= ========== to ========== Add network cost as part of the connection ranking. For now, the network cost is purely based on the network type (cellular has cost 0xFFFF and everything else has cost 0). Add cost to the candidate signaling and the stun request signaling (which is needed for peer reflexive candidates). BUG= ==========
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Revised the CL based on offline discussion. PTAL. https://codereview.webrtc.org/1668073002/diff/40001/talk/app/webrtc/webrtcsdp.cc File talk/app/webrtc/webrtcsdp.cc (right): https://codereview.webrtc.org/1668073002/diff/40001/talk/app/webrtc/webrtcsdp... talk/app/webrtc/webrtcsdp.cc:1110: candidate->set_network_cost(std::min(cost, cricket::kMaxNetworkCost)); On 2016/02/05 01:36:46, honghaiz3 wrote: > On 2016/02/04 23:16:20, pthatcher1 wrote: > > I think we should pick "cost" or "network cost" but not both. I'd say just go > > with "cost". A client might choose to use this for TURN vs. not as well. > Discussed in other comments. I think it might be good to put "cost" on the signaling to save a few bytes. For the rest, I am using NetworkCost.
Very close. Just need to beef up the unit tests a bit and a few minor tweaks. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:1163: int Connection::ComputeNetworkCost() const { On 2016/02/05 01:36:46, honghaiz3 wrote: > On 2016/02/04 23:51:18, pthatcher1 wrote: > > On 2016/02/04 23:31:10, juberti2 wrote: > > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > > Everwhere you have "networkcost", please just make it "cost". > > > > > > Not sure about this. Network cost seems clearer than just 'cost', which > seems > > > like an antonym for 'priority'. Maybe this should be 'network_priority'. > > > > I don't feel strongly one way or the other, I just want it consistent. If > it's > > "network cost" then the SDP should have "network-cost", not just "cost". > When I put "cost" in the sdp, I intended to save a few bytes. Not sure if that > is super-critical though. Saving bytes in SDP is of minor importance. If someone wants to save bytes, they shouldn't use SDP to start with. Can you please just make it network-cost in the SDP to be consistent? https://codereview.webrtc.org/1668073002/diff/120001/webrtc/p2p/base/candidate.h File webrtc/p2p/base/candidate.h (right): https://codereview.webrtc.org/1668073002/diff/120001/webrtc/p2p/base/candidat... webrtc/p2p/base/candidate.h:30: const uint32_t kMaxNetworkCost = 0xFFFF; Can we make it something that is going to look reasonable in the SDP/signalling? LIke, 1,000? That would be equivalent to a 10 second RTT with 10ms resolution, which sounds like a reasonable max, and would look good in the signalling. Plus, it would leave us plenty of head room for things in the future. https://codereview.webrtc.org/1668073002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1668073002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1688: 1000); This would be more readable if you did something like: auto wifi0 = kAlternateAddrs[0]; auto cell0 = kPublicAddrs[0]; auto wifi1 = kAlternateAddrs[1]; auto cell1 = kPublicAddrs[1]; LocalCandidate(ep1_ch1())->address().EqualIPs(wifi0) And similar https://codereview.webrtc.org/1668073002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1689: } Should we have a test case that WiFi -> Cell is preferred over Cell -> Cell and WiFi -> WiFi is preferred over WiFi -> Cell?
Thanks! PTAL. https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1668073002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:1163: int Connection::ComputeNetworkCost() const { On 2016/02/12 00:07:12, pthatcher1 wrote: > On 2016/02/05 01:36:46, honghaiz3 wrote: > > On 2016/02/04 23:51:18, pthatcher1 wrote: > > > On 2016/02/04 23:31:10, juberti2 wrote: > > > > On 2016/02/04 23:16:20, pthatcher1 wrote: > > > > > Everwhere you have "networkcost", please just make it "cost". > > > > > > > > Not sure about this. Network cost seems clearer than just 'cost', which > > seems > > > > like an antonym for 'priority'. Maybe this should be 'network_priority'. > > > > > > I don't feel strongly one way or the other, I just want it consistent. If > > it's > > > "network cost" then the SDP should have "network-cost", not just "cost". > > When I put "cost" in the sdp, I intended to save a few bytes. Not sure if that > > is super-critical though. > > Saving bytes in SDP is of minor importance. If someone wants to save bytes, > they shouldn't use SDP to start with. > > Can you please just make it network-cost in the SDP to be consistent? Done. https://codereview.webrtc.org/1668073002/diff/120001/webrtc/p2p/base/candidate.h File webrtc/p2p/base/candidate.h (right): https://codereview.webrtc.org/1668073002/diff/120001/webrtc/p2p/base/candidat... webrtc/p2p/base/candidate.h:30: const uint32_t kMaxNetworkCost = 0xFFFF; On 2016/02/12 00:07:12, pthatcher1 wrote: > Can we make it something that is going to look reasonable in the SDP/signalling? > LIke, 1,000? That would be equivalent to a 10 second RTT with 10ms resolution, > which sounds like a reasonable max, and would look good in the signalling. > Plus, it would leave us plenty of head room for things in the future. Changed to 999. (digit 9 is a better convention to be the maximum number for human being (or maybe just me)). https://codereview.webrtc.org/1668073002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1668073002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1688: 1000); On 2016/02/12 00:07:12, pthatcher1 wrote: > This would be more readable if you did something like: > auto wifi0 = kAlternateAddrs[0]; > auto cell0 = kPublicAddrs[0]; > auto wifi1 = kAlternateAddrs[1]; > auto cell1 = kPublicAddrs[1]; > > LocalCandidate(ep1_ch1())->address().EqualIPs(wifi0) > > And similar Done. We really just need wifi and cellular. https://codereview.webrtc.org/1668073002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1689: } On 2016/02/12 00:07:12, pthatcher1 wrote: > Should we have a test case that WiFi -> Cell is preferred over Cell -> Cell and > WiFi -> WiFi is preferred over WiFi -> Cell? I think the above test has covered Wifi-Wifi is preferred over Wifi-Cell and Cell-Cell Added one for Wifi-Celluar over Cellular-Cellular.
lgtm https://codereview.webrtc.org/1668073002/diff/120001/webrtc/p2p/base/candidate.h File webrtc/p2p/base/candidate.h (right): https://codereview.webrtc.org/1668073002/diff/120001/webrtc/p2p/base/candidat... webrtc/p2p/base/candidate.h:30: const uint32_t kMaxNetworkCost = 0xFFFF; On 2016/02/12 19:28:43, honghaiz3 wrote: > On 2016/02/12 00:07:12, pthatcher1 wrote: > > Can we make it something that is going to look reasonable in the > SDP/signalling? > > LIke, 1,000? That would be equivalent to a 10 second RTT with 10ms > resolution, > > which sounds like a reasonable max, and would look good in the signalling. > > Plus, it would leave us plenty of head room for things in the future. > Changed to 999. (digit 9 is a better convention to be the maximum number for > human being (or maybe just me)). Sounds good.
Justin, Do you want to take another look at the CL? If I do not hear from you, will assume you are busy and land it on Monday afternoon.
Description was changed from ========== Add network cost as part of the connection ranking. For now, the network cost is purely based on the network type (cellular has cost 0xFFFF and everything else has cost 0). Add cost to the candidate signaling and the stun request signaling (which is needed for peer reflexive candidates). BUG= ========== to ========== Add network cost as part of the connection ranking. For now, the network cost is purely based on the network type (cellular has cost 0xFFFF and everything else has cost 0). Add cost to the candidate signaling and the stun request signaling (which is needed for peer reflexive candidates). BUG=webrtc:4325 ==========
The CQ bit was checked by honghaiz@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1668073002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1668073002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1668073002/#ps180001 (title: "Fix sdp test error (do not signal network cost if it is 0)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1668073002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1668073002/180001
Message was sent while issue was closed.
Description was changed from ========== Add network cost as part of the connection ranking. For now, the network cost is purely based on the network type (cellular has cost 0xFFFF and everything else has cost 0). Add cost to the candidate signaling and the stun request signaling (which is needed for peer reflexive candidates). BUG=webrtc:4325 ========== to ========== Add network cost as part of the connection ranking. For now, the network cost is purely based on the network type (cellular has cost 0xFFFF and everything else has cost 0). Add cost to the candidate signaling and the stun request signaling (which is needed for peer reflexive candidates). BUG=webrtc:4325 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add network cost as part of the connection ranking. For now, the network cost is purely based on the network type (cellular has cost 0xFFFF and everything else has cost 0). Add cost to the candidate signaling and the stun request signaling (which is needed for peer reflexive candidates). BUG=webrtc:4325 ========== to ========== Add network cost as part of the connection ranking. For now, the network cost is purely based on the network type (cellular has cost 0xFFFF and everything else has cost 0). Add cost to the candidate signaling and the stun request signaling (which is needed for peer reflexive candidates). BUG=webrtc:4325 Committed: https://crrev.com/e1a0c942d670c5eeecb367a8d644c7ae9b999890 Cr-Commit-Position: refs/heads/master@{#11642} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e1a0c942d670c5eeecb367a8d644c7ae9b999890 Cr-Commit-Position: refs/heads/master@{#11642} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
