Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2163403002:
Prepare for ICE-renomination (Closed)
DescriptionPrepare for ICE renomination.
Add an ICE nomination attribute. If a connection switched on the controlling side, increase the nomination value set in the attribute.
The controlled side will also be ready for re-nomination option; it will switch if a nomination comes with a higher nomination value even though it may be at a lower priority.
Plus, don't nominate or re-nominate if the nomination value at the current connection has been acknowledged.
BUG=
R=deadbeef@webrtc.org, pthatcher@webrtc.org
Committed: https://crrev.com/8cd8f817487d3cd7e973ab822505bc8a5f3ced6f
Cr-Commit-Position: refs/heads/master@{#13631}
Patch Set 1 : . #
Total comments: 25
Patch Set 2 : . #
Total comments: 3
Patch Set 3 : Add tests and keep aggressive nomination #Patch Set 4 : . #Patch Set 5 : Minor fix in the test #
Total comments: 5
Patch Set 6 : . #
Total comments: 44
Patch Set 7 : . #
Total comments: 8
Patch Set 8 : . #Patch Set 9 : Updated comments #
Messages
Total messages: 44 (21 generated)
Description was changed from ========== Stop aggressive nomination and prepare for renomination. It will behave like passive-aggressive nomination. The controlling side will nominate only the selected connection and will only do so when it is writable. The controlled side starts to send as soon as a check succeeds, and it may switch connection based on the connection state and received media. The controlled side will also be ready for re-nomination option; it will switch if a nomination comes with a higher nomination value even though it may be at a lower priority. Plus, don't nominate or re-nominate if the current connection and the nomination value has been acklowledged. BUG= ========== to ========== Prepare for renomination and move away from aggressive nomination. It will behave like passive-aggressive nomination. The controlling side will nominate only the selected connection and will only do so when it is writable. The controlled side starts to send as soon as a check succeeds, and it may switch connection based on the connection state and the received media. The controlled side will also be ready for re-nomination option; it will switch if a nomination comes with a higher nomination value even though it may be at a lower priority. Plus, don't nominate or re-nominate if the current connection and the nomination value has been acknowledged. BUG= ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
honghaiz@webrtc.org changed reviewers: + deadbeef@webrtc.org, pthatcher@webrtc.org
This has pretty significant change on the ICE behavior. On the receiving-side, a client is able to do ICE renomination. On the sending-side, it moves away from aggressive nomination to passive-aggressive nomination. It will be great if you can take a look at the behavior change I made before I proceed to add tests. All p2ptransportchannel tests have passed though.
https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:1605: use_candidate_attr = true; If I understand this right: When a peer doesn't support renomination, we fall back to aggressive nomination. Except it's half-aggressive nomination, since we only add "USE_CANDIDATE" if the pair is selected and writable. This means an additional round-trip will be required before nominating the best pair. This seems like an unnecessary performance regression for existing applications. Can't we just fall back to normal aggressive nomination if the peer doesn't support renomination? Also, even with renomination, this assumes that if the peer supports renomination, it also supports "passive aggressive nomination" (allowing it to send media before a pair is nominated). I don't think we can make that assumption; the renomination draft doesn't mention passive aggressive nomination. I would propose nominating a pair before it's writable, using the "nomination value" to ensure the best connection is selected in the end. https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:406: bool peer_supports_renomination_ = false; Where is this variable set? https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:407: int nomination_value_ = 0; Could add a comment explaining that this is the value put in the "nomination" attribute for the next nominated pair when using renomination. Also, based on the terminology you're using (where "nominating" = something we nominate, "nominated" = something the peer nominates), should this be called "nominating_value_"? https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (left): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc#o... webrtc/p2p/base/port.cc:1323: << ", use_candidate=" << use_candidate Can we log both use_candidate and nominating_value here? Or are you removing it because you can just look for the log for "Sent STUN ping"? That seems fine. https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:1313: << ICESTATE[state()] << "|" << nominated_value() << "|" << priority() It seems like nominating_value would also be useful to know here. https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:515: } RTC_DCHECK if value < nominating_value_? Or have this function return a bool for success, and RTC_DCHECK in p2ptransportchannel? https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:520: void set_nominated_value(int nominated_value) { Why is this public? Isn't it only set internally upon receiving a binding request?
Addressed comments. Still to add tests yet. https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:1605: use_candidate_attr = true; On 2016/07/22 17:36:00, Taylor Brandstetter wrote: > If I understand this right: When a peer doesn't support renomination, we fall > back to aggressive nomination. Except it's half-aggressive nomination, since we > only add "USE_CANDIDATE" if the pair is selected and writable. This means an > additional round-trip will be required before nominating the best pair. This > seems like an unnecessary performance regression for existing applications. > Can't we just fall back to normal aggressive nomination if the peer doesn't > support renomination? > > Also, even with renomination, this assumes that if the peer supports > renomination, it also supports "passive aggressive nomination" (allowing it to > send media before a pair is nominated). I don't think we can make that > assumption; the renomination draft doesn't mention passive aggressive > nomination. I would propose nominating a pair before it's writable, using the > "nomination value" to ensure the best connection is selected in the end. Based on our last discussion with Justin and Peter, we are going to move away from aggressive nomination. I think the goal is to make the code simpler, which will make more maintainable. If people care about performance, they should implement renomination or passive-aggressive nomination. Plus, the difference is one RTT, and only for a third-party client not implementing passive-aggressive nomination (actually only the part that it can send before being nominated). https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:406: bool peer_supports_renomination_ = false; On 2016/07/22 17:36:00, Taylor Brandstetter wrote: > Where is this variable set? That will be in a subsequent CL and from signaling negotiation. https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:407: int nomination_value_ = 0; On 2016/07/22 17:36:00, Taylor Brandstetter wrote: > Could add a comment explaining that this is the value put in the "nomination" > attribute for the next nominated pair when using renomination. Also, based on > the terminology you're using (where "nominating" = something we nominate, > "nominated" = something the peer nominates), should this be called > "nominating_value_"? Done. https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (left): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc#o... webrtc/p2p/base/port.cc:1323: << ", use_candidate=" << use_candidate On 2016/07/22 17:36:01, Taylor Brandstetter wrote: > Can we log both use_candidate and nominating_value here? Or are you removing it > because you can just look for the log for "Sent STUN ping"? That seems fine. This is in the PingResponse. We don't send use_candidate back in the PingResponse. So it will always be false. https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:1313: << ICESTATE[state()] << "|" << nominated_value() << "|" << priority() On 2016/07/22 17:36:01, Taylor Brandstetter wrote: > It seems like nominating_value would also be useful to know here. nominating_value_ could change dynamically. Basically it is value put in the Ping message. If the controlling side nominates a connection and it is acknowledged, it doesn't need to nominate that connection any more. In that case, the nominating value will become 0. https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:515: } On 2016/07/22 17:36:01, Taylor Brandstetter wrote: > RTC_DCHECK if value < nominating_value_? Or have this function return a bool for > success, and RTC_DCHECK in p2ptransportchannel? It may set a zero-value. If we are not using the return value, it is not necessary to return a value. https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:520: void set_nominated_value(int nominated_value) { On 2016/07/22 17:36:01, Taylor Brandstetter wrote: > Why is this public? Isn't it only set internally upon receiving a binding > request? This is made public for testing. I am really replacing the nominated bool variable with a nominated_value here.
Maybe we should have a design review with Peter. I still don't understand why what I'll call "half-aggressive nomination" (nominate the first connection that's writable, then any better connection that becomes writable later) is preferable to normal aggressive nomination (nominate every connection, then when one becomes writable, nominate any better connection that comes along later). https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:1605: use_candidate_attr = true; On 2016/07/22 22:50:11, honghaiz3 wrote: > On 2016/07/22 17:36:00, Taylor Brandstetter wrote: > > If I understand this right: When a peer doesn't support renomination, we fall > > back to aggressive nomination. Except it's half-aggressive nomination, since > we > > only add "USE_CANDIDATE" if the pair is selected and writable. This means an > > additional round-trip will be required before nominating the best pair. This > > seems like an unnecessary performance regression for existing applications. > > Can't we just fall back to normal aggressive nomination if the peer doesn't > > support renomination? > > > > Also, even with renomination, this assumes that if the peer supports > > renomination, it also supports "passive aggressive nomination" (allowing it to > > send media before a pair is nominated). I don't think we can make that > > assumption; the renomination draft doesn't mention passive aggressive > > nomination. I would propose nominating a pair before it's writable, using the > > "nomination value" to ensure the best connection is selected in the end. > > Based on our last discussion with Justin and Peter, we are going to move away > from aggressive nomination. I think the goal is to make the code simpler, which > will make more maintainable. If people care about performance, they should > implement renomination or passive-aggressive nomination. Plus, the difference is > one RTT, and only for a third-party client not implementing passive-aggressive > nomination (actually only the part that it can send before being nominated). > > Maybe I don't have a good understanding of the end goal. But as it is: 1. The logic on the left doesn't seem that different in complexity/maintainability to the logic on the right. 2. This still uses aggressive nomination, it just requires an extra round trip. Which - as I understand it - is what aggressive nomination was designed to avoid. One RTT may not seem like much, but I don't understand what it buys us. 3. We should expect that "third party clients not implementing passive-aggressive nomination" are most third-party clients. It doesn't seem fair to expect them to implement something before it's standardized. We don't even have a way to signal support of it (do we?). https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (left): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc#o... webrtc/p2p/base/port.cc:1323: << ", use_candidate=" << use_candidate On 2016/07/22 22:50:11, honghaiz3 wrote: > On 2016/07/22 17:36:01, Taylor Brandstetter wrote: > > Can we log both use_candidate and nominating_value here? Or are you removing > it > > because you can just look for the log for "Sent STUN ping"? That seems fine. > > This is in the PingResponse. > We don't send use_candidate back in the PingResponse. > So it will always be false. Acknowledged. https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:1313: << ICESTATE[state()] << "|" << nominated_value() << "|" << priority() On 2016/07/22 22:50:11, honghaiz3 wrote: > On 2016/07/22 17:36:01, Taylor Brandstetter wrote: > > It seems like nominating_value would also be useful to know here. > > nominating_value_ could change dynamically. > Basically it is value put in the Ping message. > If the controlling side nominates a connection and it is acknowledged, it > doesn't need to nominate that connection any more. > In that case, the nominating value will become 0. I don't see where nominating_value_ is set to 0. And it seems equally dynamic as nominated_value_. https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:515: } On 2016/07/22 22:50:11, honghaiz3 wrote: > On 2016/07/22 17:36:01, Taylor Brandstetter wrote: > > RTC_DCHECK if value < nominating_value_? Or have this function return a bool > for > > success, and RTC_DCHECK in p2ptransportchannel? > > It may set a zero-value. > If we are not using the return value, it is not necessary to return a value. I'm suggesting to use the return value in a DCHECK (unless you do the DCHECK in this method; then nothing would need the return value). Also, if setting a zero value is always a no-op, there's no point in doing it. https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:520: void set_nominated_value(int nominated_value) { On 2016/07/22 22:50:11, honghaiz3 wrote: > On 2016/07/22 17:36:01, Taylor Brandstetter wrote: > > Why is this public? Isn't it only set internally upon receiving a binding > > request? > > This is made public for testing. > I am really replacing the nominated bool variable with a nominated_value here. If this is public only for unit tests, can you make that clear in a comment? However, it would be much better if the unit tests worked by sending a packet with the nomination attribute. Is it possible to do that? https://codereview.webrtc.org/2163403002/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2163403002/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:1603: nominating_value = nominating_value_; Instead of using a temporary variable "nominating_value" and setting it outside the "if" block, you can just set it here.
Addressed the comments except for the part on whether we should do aggressive nomination if the remote peer does not support ICE renomination. https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (left): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc#o... webrtc/p2p/base/port.cc:1323: << ", use_candidate=" << use_candidate On 2016/07/22 23:48:28, Taylor Brandstetter wrote: > On 2016/07/22 22:50:11, honghaiz3 wrote: > > On 2016/07/22 17:36:01, Taylor Brandstetter wrote: > > > Can we log both use_candidate and nominating_value here? Or are you removing > > it > > > because you can just look for the log for "Sent STUN ping"? That seems fine. > > > > This is in the PingResponse. > > We don't send use_candidate back in the PingResponse. > > So it will always be false. > > Acknowledged. Acknowledged. https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:1313: << ICESTATE[state()] << "|" << nominated_value() << "|" << priority() On 2016/07/22 23:48:28, Taylor Brandstetter wrote: > On 2016/07/22 22:50:11, honghaiz3 wrote: > > On 2016/07/22 17:36:01, Taylor Brandstetter wrote: > > > It seems like nominating_value would also be useful to know here. > > > > nominating_value_ could change dynamically. > > Basically it is value put in the Ping message. > > If the controlling side nominates a connection and it is acknowledged, it > > doesn't need to nominate that connection any more. > > In that case, the nominating value will become 0. > > I don't see where nominating_value_ is set to 0. And it seems equally dynamic as > nominated_value_. Hope this is clearer now with the updated CL. https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:515: } On 2016/07/22 23:48:29, Taylor Brandstetter wrote: > On 2016/07/22 22:50:11, honghaiz3 wrote: > > On 2016/07/22 17:36:01, Taylor Brandstetter wrote: > > > RTC_DCHECK if value < nominating_value_? Or have this function return a bool > > for > > > success, and RTC_DCHECK in p2ptransportchannel? > > > > It may set a zero-value. > > If we are not using the return value, it is not necessary to return a value. > > I'm suggesting to use the return value in a DCHECK (unless you do the DCHECK in > this method; then nothing would need the return value). Also, if setting a zero > value is always a no-op, there's no point in doing it. I changed the implementation a little. Basically we should always set the value. So no need to return or DCHECK. https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:520: void set_nominated_value(int nominated_value) { On 2016/07/22 23:48:28, Taylor Brandstetter wrote: > On 2016/07/22 22:50:11, honghaiz3 wrote: > > On 2016/07/22 17:36:01, Taylor Brandstetter wrote: > > > Why is this public? Isn't it only set internally upon receiving a binding > > > request? > > > > This is made public for testing. > > I am really replacing the nominated bool variable with a nominated_value here. > > > If this is public only for unit tests, can you make that clear in a comment? > However, it would be much better if the unit tests worked by sending a packet > with the nomination attribute. Is it possible to do that? Done. https://codereview.webrtc.org/2163403002/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2163403002/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:1603: nominating_value = nominating_value_; On 2016/07/22 23:48:29, Taylor Brandstetter wrote: > Instead of using a temporary variable "nominating_value" and setting it outside > the "if" block, you can just set it here. That's not the intended behavior. A connection may be the selected one first and become not later. In that case, we want to set nominating_value to 0 here. The same applies to the set_use_candidate_attr.
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:1605: use_candidate_attr = true; On 2016/07/22 23:48:28, Taylor Brandstetter wrote: > On 2016/07/22 22:50:11, honghaiz3 wrote: > > On 2016/07/22 17:36:00, Taylor Brandstetter wrote: > > > If I understand this right: When a peer doesn't support renomination, we > fall > > > back to aggressive nomination. Except it's half-aggressive nomination, since > > we > > > only add "USE_CANDIDATE" if the pair is selected and writable. This means an > > > additional round-trip will be required before nominating the best pair. This > > > seems like an unnecessary performance regression for existing applications. > > > Can't we just fall back to normal aggressive nomination if the peer doesn't > > > support renomination? > > > > > > Also, even with renomination, this assumes that if the peer supports > > > renomination, it also supports "passive aggressive nomination" (allowing it > to > > > send media before a pair is nominated). I don't think we can make that > > > assumption; the renomination draft doesn't mention passive aggressive > > > nomination. I would propose nominating a pair before it's writable, using > the > > > "nomination value" to ensure the best connection is selected in the end. > > > > Based on our last discussion with Justin and Peter, we are going to move away > > from aggressive nomination. I think the goal is to make the code simpler, > which > > will make more maintainable. If people care about performance, they should > > implement renomination or passive-aggressive nomination. Plus, the difference > is > > one RTT, and only for a third-party client not implementing passive-aggressive > > nomination (actually only the part that it can send before being nominated). > > > > > > Maybe I don't have a good understanding of the end goal. But as it is: > > 1. The logic on the left doesn't seem that different in > complexity/maintainability to the logic on the right. > 2. This still uses aggressive nomination, it just requires an extra round trip. > Which - as I understand it - is what aggressive nomination was designed to > avoid. One RTT may not seem like much, but I don't understand what it buys us. > 3. We should expect that "third party clients not implementing > passive-aggressive nomination" are most third-party clients. It doesn't seem > fair to expect them to implement something before it's standardized. We don't > even have a way to signal support of it (do we?). Kept the aggressive nomination behavior.
Description was changed from ========== Prepare for renomination and move away from aggressive nomination. It will behave like passive-aggressive nomination. The controlling side will nominate only the selected connection and will only do so when it is writable. The controlled side starts to send as soon as a check succeeds, and it may switch connection based on the connection state and the received media. The controlled side will also be ready for re-nomination option; it will switch if a nomination comes with a higher nomination value even though it may be at a lower priority. Plus, don't nominate or re-nominate if the current connection and the nomination value has been acknowledged. BUG= ========== to ========== Prepare for ICE renomination. Add an ICE nomination attribute. If a connection switched on the controlling side, increase the nomination value set in the attribute. The controlled side will also be ready for re-nomination option; it will switch if a nomination comes with a higher nomination value even though it may be at a lower priority. Plus, don't nominate or re-nominate if the nomination value at the current connection has been acknowledged. BUG= ==========
Added tests now. PTAL.
https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2163403002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:1313: << ICESTATE[state()] << "|" << nominated_value() << "|" << priority() On 2016/07/25 22:24:40, honghaiz3 wrote: > On 2016/07/22 23:48:28, Taylor Brandstetter wrote: > > On 2016/07/22 22:50:11, honghaiz3 wrote: > > > On 2016/07/22 17:36:01, Taylor Brandstetter wrote: > > > > It seems like nominating_value would also be useful to know here. > > > > > > nominating_value_ could change dynamically. > > > Basically it is value put in the Ping message. > > > If the controlling side nominates a connection and it is acknowledged, it > > > doesn't need to nominate that connection any more. > > > In that case, the nominating value will become 0. > > > > I don't see where nominating_value_ is set to 0. And it seems equally dynamic > as > > nominated_value_. > > Hope this is clearer now with the updated CL. I see that in the updated CL, the nominating value can go to 0. But I still think it would still be valuable to log it in some form. If we log "highest received nomination value" I'd also like to see "highest sent nomination value" when debugging an ICE issue. https://codereview.webrtc.org/2163403002/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2163403002/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:1603: nominating_value = nominating_value_; On 2016/07/25 22:24:41, honghaiz3 wrote: > On 2016/07/22 23:48:29, Taylor Brandstetter wrote: > > Instead of using a temporary variable "nominating_value" and setting it > outside > > the "if" block, you can just set it here. > That's not the intended behavior. > A connection may be the selected one first and become not later. > In that case, we want to set nominating_value to 0 here. > The same applies to the set_use_candidate_attr. I understand. Previously, "set_nominating_value(0)" would have done nothing, but now it has meaning. https://codereview.webrtc.org/2163403002/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2163403002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1617: nominating_value = nominating_value_; If using renomination, shouldn't we only nominate the selected connection? If the selected connection becomes unwritable, then this code would nominate everything with the same value, basically falling back to aggressive nomination. If the selected connection then becomes writable again, but others do too, how would the controlled side know which to use? They all have the same nomination value. https://codereview.webrtc.org/2163403002/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2163403002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2034: 3000, clock); This test relies on both channels becoming "not receiving" at the same time. But what if in the future, the failover algorithm is changed such that there's never a loss of "receiving"? That would break the test. I think the test could be made more change-resilient if you blackholed *both* addresses at the same time, waited for "!receiving", then un-blackholed kAlternateAddrs[0].
I agree that with ICE-renomination, we should disable aggressive nomination when using ICE renomination. But in the same draft, it also says, If one side signals "renomination" and the other does not understand it, then according to the rules of ICE, aggressive nomination is disabled and passive nomination is used, and the controlling side MUST NOT send more than one nomination. I re-read the ICE rfc. It said similar thing: If its peer is using ICE options (present in an ice-options attribute from the peer) that the agent does not understand, the agent MUST use a regular nomination algorithm. So it looks like we should go back to my original implementation except for only nominating once, although I don't see a lot of negative side on the requirement of "nominating once". For the ICE-LITE, the rfc also says we should do regular nomination, but we are nominating multiple times on the selected connection. I understand we don't add the ice-option yet, but we probably don't want to have different behavior on our implementation depending on whether the upper-layer signaled renomination option.
Changed to 1. If peer supports renomination, do renomination but not aggressive nomination (even though aggressive nomination should also work) . 2. If peer does not support renomination and remote ICE is full, do aggressive nomination. https://codereview.webrtc.org/2163403002/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2163403002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1617: nominating_value = nominating_value_; On 2016/07/27 22:20:55, Taylor Brandstetter wrote: > If using renomination, shouldn't we only nominate the selected connection? > > If the selected connection becomes unwritable, then this code would nominate > everything with the same value, basically falling back to aggressive nomination. > > If the selected connection then becomes writable again, but others do too, how > would the controlled side know which to use? They all have the same nomination > value. Done. Although the previous implementation should also work, the renomination draft says to disable aggressive nomination when using renomination. https://codereview.webrtc.org/2163403002/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2163403002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2034: 3000, clock); On 2016/07/27 22:20:55, Taylor Brandstetter wrote: > This test relies on both channels becoming "not receiving" at the same time. But > what if in the future, the failover algorithm is changed such that there's never > a loss of "receiving"? That would break the test. > > I think the test could be made more change-resilient if you blackholed *both* > addresses at the same time, waited for "!receiving", then un-blackholed > kAlternateAddrs[0]. Revised. I think we need to make sure that the controlling side switched the connection and the controlled side increased the nominated value.
lgtm. Even if the draft says we should fall back to regular nomination, I think we should phase aggressive nomination out gradually for the reasons discussed earlier. Also, I still would really like to have logging for "highest nominating value" per connection. I think it would help me make sense of logs much easier. https://codereview.webrtc.org/2163403002/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2163403002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2034: 3000, clock); On 2016/07/28 04:19:14, honghaiz3 wrote: > On 2016/07/27 22:20:55, Taylor Brandstetter wrote: > > This test relies on both channels becoming "not receiving" at the same time. > But > > what if in the future, the failover algorithm is changed such that there's > never > > a loss of "receiving"? That would break the test. > > > > I think the test could be made more change-resilient if you blackholed *both* > > addresses at the same time, waited for "!receiving", then un-blackholed > > kAlternateAddrs[0]. > > Revised. I think we need to make sure that the controlling side switched the > connection and the controlled side increased the nominated value. This looks good. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1289: ++nominating_value_; Just an observation: if the selected pair changes multiple times while still not writable, the remote endpoint will see a jump in the nomination value. This probably is fine though.
https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1144: int cmp = a->nominated_value() - b->nominated_value(); I would call this attribute "nomination()". https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1148: } It would be more consistent and readable to be like last_data_received: if (a->nomination() > b->nomination()) { return a_is_better; } if (a->nomination() < b->nomination()) { return b_is_better; } https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1218: (selected_connection_ && selected_connection_->nominated_value() > 0)) { It would be more readable to define: bool nominated() { return nomination() > 0; } That would make this more readable. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1288: if (peer_supports_renomination_) { remote_parameters().renomination would be just as clear. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1289: ++nominating_value_; On 2016/07/28 16:30:45, Taylor Brandstetter wrote: > Just an observation: if the selected pair changes multiple times while still not > writable, the remote endpoint will see a jump in the nomination value. This > probably is fine though. Yes, that's fine. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1290: } Why not just "++nomination_" every time, but then only send it if the remote peer supports renomination? https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1625: } I think this would be more clear as: bool nominating = (ice_role_ == ICEROLE_CONTROLLING); bool renominating = nominating && remote_parameters_.renomination; bool aggressively_nominating = nominating && !renominating; bool selected == (conn == selected_connection_); if (renominating && selected) { conn->set_nomination_attr(nomination_); } else { conn->set_nomination_attr(0); } if (aggressively_nominating) { conn->set_use_candidate_attr(true); } else { conn->set_use_candidate_attr(false); } last_ping_sent_ms_ = rtc::TimeMillis(); conn->Ping(last_ping_sent_ms_); Notice that this changes the behavior in two ways: 1. We renominate even if it's not writable. I think that's fine. We should always nominate the connection that is selected. 2. When we're aggressively nominating, we nominate *every* ping. That's what the standard says to do, so that's what we should do, at least until we remove aggressive nomination. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:213: } I think we should make a new method, SetRemoteIceParameters that eventually will also contain the ufrag and password (replace SetRemoteIceCredentials and SetRemoteIceMode). It would take a struct like this: struct IceParameters { std::string ufrag; std::string pwd; IceMode mode; bool renomination; } Then we just have remote_parameters_ as the member variable. For now, in this CL, I think we should add remote_parameters_ and SetRemoteIceParameters, and have the struct be this: struct IceParameters { // TODO: Add the following and replace SetRemoteIceCredentials and SetRemoteIceMode. // std::string ufrag; // std::string pwd; // IceMode mode; bool renomination; } https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:415: int nominating_value_ = 0; I'd prefer "nomination" over "nominating_value". And next_nomination_ would perhaps be a more clear name. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2038: kDefaultTimeout, clock); Don't we need to also test that when it's acked, the nomination is set to 0, until the value increases again? https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:764: connection_->acknowledged_nominating_value()) { I think it would be more robust if it were: connection_->nomination() && connection_->nomination() != connection_->acked_nomination() https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:766: STUN_ATTR_NOMINATION_VALUE, connection_->nominating_value())); I'd prefer STUN_ATTR_NOMINATION. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1051: LOG(LS_ERROR) << "Negative nomination value: " << nomination_value; Should we use a uint32_t instead of an int? It can't be negative. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:420: int nomination_value; I'd call this "nomination". https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:520: } I think we should go from including the attribute to not including the attribute if it's no longer the selected candidate pair. In other words, just "nomination_ = nomination" here. Even if we wanted fancier logic, it should be in p2ptransportchannel.cc, not here. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:641: return acknowledged_nominating_value_; acked_nomination would be a better name. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:675: int nominated_value_ = 0; nominating_value/nominated_value is confusing. Perhaps local_nomination/remote_nomination or just nomination/remote_nomination would make more sense. Usually only one will be set, but I guess if there's a controlling conflict, you'd need both. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/stun.h File webrtc/p2p/base/stun.h (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/stun.h#n... webrtc/p2p/base/stun.h:612: STUN_ATTR_NOMINATION_VALUE = 0xC058 How about 0xC001? You nominated it because it's cool :).
https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1625: } On 2016/07/28 22:57:25, pthatcher1 wrote: > I think this would be more clear as: > > bool nominating = (ice_role_ == ICEROLE_CONTROLLING); > bool renominating = nominating && remote_parameters_.renomination; > bool aggressively_nominating = nominating && !renominating; > bool selected == (conn == selected_connection_); > if (renominating && selected) { > conn->set_nomination_attr(nomination_); > } else { > conn->set_nomination_attr(0); > } > if (aggressively_nominating) { > conn->set_use_candidate_attr(true); > } else { > conn->set_use_candidate_attr(false); > } > last_ping_sent_ms_ = rtc::TimeMillis(); > conn->Ping(last_ping_sent_ms_); > > > Notice that this changes the behavior in two ways: > 1. We renominate even if it's not writable. I think that's fine. We should > always nominate the connection that is selected. > 2. When we're aggressively nominating, we nominate *every* ping. That's what > the standard says to do, so that's what we should do, at least until we remove > aggressive nomination. OK, if we also keep bizarro, which I'll call "semi-aggressive": int GetNominationAttr(Connection* conn) { bool selected = (conn == selected_connection_); if (selected) { return nomination_; } return 0; } bool GetUseCandidateAttr(Connection* conn, NominationMode mode) { switch (mode) { case REGULAR: // TODO(honghaiz): Implement regular nomination. return false; case AGGRESSIVE: if (remote_ice_mode_ == ICEMODE_LITE) { return GetUseCandidateAttr(conn, REGULAR); } return true; case SEMI_AGGRESSIVE: bool selected = (conn == selected_connection_); if (remote_ice_mode_ == ICEMODE_LITE) { return selected && conn->writable(); } bool better_than_selected = !selected_connection_ || !selected_connection_->writable (CompareConnectionCandidates(selected_connection_, conn) < 0); return selected || better_than_selected; } } if (ice_role_ == ICEROLE_CONTROLLING) { if (remote_parameters_.renomination) { conn->set_nomination_attr(GetNominationAttr(conn)); conn->set_use_candidate_attr(false); } else { conn->set_nomination_attr(0); conn->set_use_candidate_attr(GetUseCandidateAttr(conn, config_.default_nomination_mode)); } } last_ping_sent_ms_ = rtc::TimeMillis(); conn->Ping(last_ping_sent_ms_);
Patchset #7 (id:200001) has been deleted
Patchset #8 (id:240001) has been deleted
Patchset #7 (id:220001) has been deleted
On 2016/07/28 16:30:45, Taylor Brandstetter wrote: > lgtm. Even if the draft says we should fall back to regular nomination, I think > we should phase aggressive nomination out gradually for the reasons discussed > earlier. > > Also, I still would really like to have logging for "highest nominating value" > per connection. I think it would help me make sense of logs much easier. > > https://codereview.webrtc.org/2163403002/diff/160001/webrtc/p2p/base/p2ptrans... > File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): > > https://codereview.webrtc.org/2163403002/diff/160001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel_unittest.cc:2034: 3000, clock); > On 2016/07/28 04:19:14, honghaiz3 wrote: > > On 2016/07/27 22:20:55, Taylor Brandstetter wrote: > > > This test relies on both channels becoming "not receiving" at the same time. > > But > > > what if in the future, the failover algorithm is changed such that there's > > never > > > a loss of "receiving"? That would break the test. > > > > > > I think the test could be made more change-resilient if you blackholed > *both* > > > addresses at the same time, waited for "!receiving", then un-blackholed > > > kAlternateAddrs[0]. > > > > Revised. I think we need to make sure that the controlling side switched the > > connection and the controlled side increased the nominated value. > > This looks good. > > https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... > File webrtc/p2p/base/p2ptransportchannel.cc (right): > > https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:1289: ++nominating_value_; > Just an observation: if the selected pair changes multiple times while still not > writable, the remote endpoint will see a jump in the nomination value. This > probably is fine though. I added the (local) nomination value of a connection to the log. Keep in mind that the value will become zero once it is not the selected connection any more.
Patchset #7 (id:260001) has been deleted
PTAL. Thanks! https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1144: int cmp = a->nominated_value() - b->nominated_value(); On 2016/07/28 22:57:25, pthatcher1 wrote: > I would call this attribute "nomination()". We can't use nomination for both the local and remote side values. Changed to remote_nomination. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1148: } On 2016/07/28 22:57:25, pthatcher1 wrote: > It would be more consistent and readable to be like last_data_received: > > if (a->nomination() > b->nomination()) { > return a_is_better; > } > > if (a->nomination() < b->nomination()) { > return b_is_better; > } Done. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1218: (selected_connection_ && selected_connection_->nominated_value() > 0)) { On 2016/07/28 22:57:25, pthatcher1 wrote: > It would be more readable to define: > > bool nominated() { > return nomination() > 0; > } > > That would make this more readable. Done. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1288: if (peer_supports_renomination_) { On 2016/07/28 22:57:25, pthatcher1 wrote: > remote_parameters().renomination would be just as clear. Will do this in the next CL. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1289: ++nominating_value_; On 2016/07/28 22:57:25, pthatcher1 wrote: > On 2016/07/28 16:30:45, Taylor Brandstetter wrote: > > Just an observation: if the selected pair changes multiple times while still > not > > writable, the remote endpoint will see a jump in the nomination value. This > > probably is fine though. > > Yes, that's fine. Acknowledged. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1290: } On 2016/07/28 22:57:25, pthatcher1 wrote: > Why not just "++nomination_" every time, but then only send it if the remote > peer supports renomination? Done. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1625: } On 2016/07/29 00:24:41, pthatcher1 wrote: > On 2016/07/28 22:57:25, pthatcher1 wrote: > > I think this would be more clear as: > > > > bool nominating = (ice_role_ == ICEROLE_CONTROLLING); > > bool renominating = nominating && remote_parameters_.renomination; > > bool aggressively_nominating = nominating && !renominating; > > bool selected == (conn == selected_connection_); > > if (renominating && selected) { > > conn->set_nomination_attr(nomination_); > > } else { > > conn->set_nomination_attr(0); > > } > > if (aggressively_nominating) { > > conn->set_use_candidate_attr(true); > > } else { > > conn->set_use_candidate_attr(false); > > } > > last_ping_sent_ms_ = rtc::TimeMillis(); > > conn->Ping(last_ping_sent_ms_); > > > > > > Notice that this changes the behavior in two ways: > > 1. We renominate even if it's not writable. I think that's fine. We should > > always nominate the connection that is selected. > > 2. When we're aggressively nominating, we nominate *every* ping. That's what > > the standard says to do, so that's what we should do, at least until we remove > > aggressive nomination. > > OK, if we also keep bizarro, which I'll call "semi-aggressive": > > int GetNominationAttr(Connection* conn) { > bool selected = (conn == selected_connection_); > if (selected) { > return nomination_; > } > return 0; > } > > bool GetUseCandidateAttr(Connection* conn, NominationMode mode) { > switch (mode) { > case REGULAR: > // TODO(honghaiz): Implement regular nomination. > return false; > case AGGRESSIVE: > if (remote_ice_mode_ == ICEMODE_LITE) { > return GetUseCandidateAttr(conn, REGULAR); > } > return true; > case SEMI_AGGRESSIVE: > bool selected = (conn == selected_connection_); > if (remote_ice_mode_ == ICEMODE_LITE) { > return selected && conn->writable(); > } > bool better_than_selected = !selected_connection_ || > !selected_connection_->writable > (CompareConnectionCandidates(selected_connection_, conn) < 0); > return selected || better_than_selected; > } > } > > if (ice_role_ == ICEROLE_CONTROLLING) { > if (remote_parameters_.renomination) { > conn->set_nomination_attr(GetNominationAttr(conn)); > conn->set_use_candidate_attr(false); > } else { > conn->set_nomination_attr(0); > conn->set_use_candidate_attr(GetUseCandidateAttr(conn, > config_.default_nomination_mode)); > } > } > last_ping_sent_ms_ = rtc::TimeMillis(); > conn->Ping(last_ping_sent_ms_); Done. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:213: } On 2016/07/28 22:57:25, pthatcher1 wrote: > I think we should make a new method, SetRemoteIceParameters that eventually will > also contain the ufrag and password (replace SetRemoteIceCredentials and > SetRemoteIceMode). It would take a struct like this: > > struct IceParameters { > std::string ufrag; > std::string pwd; > IceMode mode; > bool renomination; > } > > Then we just have remote_parameters_ as the member variable. > > > For now, in this CL, I think we should add remote_parameters_ and > SetRemoteIceParameters, and have the struct be this: > > > struct IceParameters { > // TODO: Add the following and replace SetRemoteIceCredentials and > SetRemoteIceMode. > // std::string ufrag; > // std::string pwd; > // IceMode mode; > bool renomination; > } > The struct IceParameters is already there. In fact we have a list of remote parameters and initially it may be empty. I think it would be simpler to make this change in a subsequent CL when I add the signaling changes. Added TODO for this. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:415: int nominating_value_ = 0; On 2016/07/28 22:57:25, pthatcher1 wrote: > I'd prefer "nomination" over "nominating_value". > > And next_nomination_ would perhaps be a more clear name. I choose nomination_ because it is more consistent with use_candidate_attr. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2038: kDefaultTimeout, clock); On 2016/07/28 22:57:25, pthatcher1 wrote: > Don't we need to also test that when it's acked, the nomination is set to 0, > until the value increases again? Done. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:764: connection_->acknowledged_nominating_value()) { On 2016/07/28 22:57:26, pthatcher1 wrote: > I think it would be more robust if it were: > > connection_->nomination() && connection_->nomination() != > connection_->acked_nomination() Done. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:766: STUN_ATTR_NOMINATION_VALUE, connection_->nominating_value())); On 2016/07/28 22:57:26, pthatcher1 wrote: > I'd prefer STUN_ATTR_NOMINATION. Done. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1051: LOG(LS_ERROR) << "Negative nomination value: " << nomination_value; On 2016/07/28 22:57:26, pthatcher1 wrote: > Should we use a uint32_t instead of an int? It can't be negative. I think the code style says we should not use uint32 because a value does not take negative values. It recommends using ASSERT but here we don't want to assert error simply because remote sides sends an invalid attribute. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:420: int nomination_value; On 2016/07/28 22:57:26, pthatcher1 wrote: > I'd call this "nomination". Done. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:520: } On 2016/07/28 22:57:26, pthatcher1 wrote: > I think we should go from including the attribute to not including the attribute > if it's no longer the selected candidate pair. In other words, just > "nomination_ = nomination" here. > > Even if we wanted fancier logic, it should be in p2ptransportchannel.cc, not > here. The check protects packet re-ordering. If for some reason, a ping with lower-nominated value arrived later, we should not set the nominated value to a smaller one. nominated is set in Port class. Not sure why/how to do it in p2ptransportchannel. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:641: return acknowledged_nominating_value_; On 2016/07/28 22:57:26, pthatcher1 wrote: > acked_nomination would be a better name. Done. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:675: int nominated_value_ = 0; On 2016/07/28 22:57:26, pthatcher1 wrote: > nominating_value/nominated_value is confusing. Perhaps > local_nomination/remote_nomination or just nomination/remote_nomination would > make more sense. > > Usually only one will be set, but I guess if there's a controlling conflict, > you'd need both. Done. There might be role switch, so better to have both. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/stun.h File webrtc/p2p/base/stun.h (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/stun.h#n... webrtc/p2p/base/stun.h:612: STUN_ATTR_NOMINATION_VALUE = 0xC058 On 2016/07/28 22:57:26, pthatcher1 wrote: > How about 0xC001? You nominated it because it's cool :). Done.
https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:213: } On 2016/08/03 04:46:56, honghaiz3 wrote: > On 2016/07/28 22:57:25, pthatcher1 wrote: > > I think we should make a new method, SetRemoteIceParameters that eventually > will > > also contain the ufrag and password (replace SetRemoteIceCredentials and > > SetRemoteIceMode). It would take a struct like this: > > > > struct IceParameters { > > std::string ufrag; > > std::string pwd; > > IceMode mode; > > bool renomination; > > } > > > > Then we just have remote_parameters_ as the member variable. > > > > > > For now, in this CL, I think we should add remote_parameters_ and > > SetRemoteIceParameters, and have the struct be this: > > > > > > struct IceParameters { > > // TODO: Add the following and replace SetRemoteIceCredentials and > > SetRemoteIceMode. > > // std::string ufrag; > > // std::string pwd; > > // IceMode mode; > > bool renomination; > > } > > > The struct IceParameters is already there. Oh good, let's add "bool renomination" to it and use it, then. > In fact we have a list of remote > parameters and initially it may be empty. If it's empty, then clearly we don't know the remote endpoint supports renomination, so we shouldn't use it until we do. > I think it would be simpler to make this change in a subsequent CL when I add > the signaling changes. Added TODO for this. I'm OK with you do that in a followup CL. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1051: LOG(LS_ERROR) << "Negative nomination value: " << nomination_value; On 2016/08/03 04:46:56, honghaiz3 wrote: > On 2016/07/28 22:57:26, pthatcher1 wrote: > > Should we use a uint32_t instead of an int? It can't be negative. > I think the code style says we should not use uint32 because a value does not > take negative values. It recommends using ASSERT but here we don't want to > assert error simply because remote sides sends an invalid attribute. It's not just because it can't be negative. What's the range of possible values for the STUN attribute that we receive from the controlling side? Does it use the entire 32-bit space (as the call to GetUint32 implies)? If so, on a 32-bit machine, if an int is 32 bits and the remote side sends us a nomination value of 2**31+1, we'll think it's negative, which would be bad. Am I correct? https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:520: } On 2016/08/03 04:46:56, honghaiz3 wrote: > On 2016/07/28 22:57:26, pthatcher1 wrote: > > I think we should go from including the attribute to not including the > attribute > > if it's no longer the selected candidate pair. In other words, just > > "nomination_ = nomination" here. > > > > Even if we wanted fancier logic, it should be in p2ptransportchannel.cc, not > > here. > > The check protects packet re-ordering. > If for some reason, a ping with lower-nominated value arrived later, we should > not set the nominated value to a smaller one. nominated is set in Port class. > Not sure why/how to do it in p2ptransportchannel. Ah, good point about reordering. Do we have a unit test that covers that? Also, I still think it shouldn't be in set_remote_nomination. Putting it in port.cc would make sense as: if (remote_nomination > 0 && remote_nomination > remote_nomination()) { set_remote_nomination(remote_nomination); ... } Actually, it's that guaranteed to be the same as: if (remote_nomination > remote_nomination()) { set_remote_nomination(remote_nomination); ... } ? By the way, we should make sure that in our standardization that we make it clear that the 0 value must not be used. Call it reserved or something. https://codereview.webrtc.org/2163403002/diff/280001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2163403002/diff/280001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1619: // REGULAR: To be implemented yet. To be implemented yet => Nominate once per ICE restart (Not implemented yet) https://codereview.webrtc.org/2163403002/diff/280001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1621: // ICE_LITE mode, it will behave as if REGULAR. except that when => except when is at ICE_LITE mode => is an ice-lite endpoint https://codereview.webrtc.org/2163403002/diff/280001/webrtc/p2p/base/transport.h File webrtc/p2p/base/transport.h (right): https://codereview.webrtc.org/2163403002/diff/280001/webrtc/p2p/base/transpor... webrtc/p2p/base/transport.h:165: // ICE Nomination mode. Can you move the definitions of these from the other comment to this comment? https://codereview.webrtc.org/2163403002/diff/280001/webrtc/p2p/base/transpor... webrtc/p2p/base/transport.h:206: NominationMode default_nomination_mode = NominationMode::SEMI_AGGRESSIVE; Can you put a TODO to change it to REGULAR?
Thanks! PTAL. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:213: } On 2016/08/03 22:13:25, pthatcher1 wrote: > On 2016/08/03 04:46:56, honghaiz3 wrote: > > On 2016/07/28 22:57:25, pthatcher1 wrote: > > > I think we should make a new method, SetRemoteIceParameters that eventually > > will > > > also contain the ufrag and password (replace SetRemoteIceCredentials and > > > SetRemoteIceMode). It would take a struct like this: > > > > > > struct IceParameters { > > > std::string ufrag; > > > std::string pwd; > > > IceMode mode; > > > bool renomination; > > > } > > > > > > Then we just have remote_parameters_ as the member variable. > > > > > > > > > For now, in this CL, I think we should add remote_parameters_ and > > > SetRemoteIceParameters, and have the struct be this: > > > > > > > > > struct IceParameters { > > > // TODO: Add the following and replace SetRemoteIceCredentials and > > > SetRemoteIceMode. > > > // std::string ufrag; > > > // std::string pwd; > > > // IceMode mode; > > > bool renomination; > > > } > > > > > The struct IceParameters is already there. > > > Oh good, let's add "bool renomination" to it and use it, then. > > > In fact we have a list of remote > > parameters and initially it may be empty. > > If it's empty, then clearly we don't know the remote endpoint supports > renomination, so we shouldn't use it until we do. > > > I think it would be simpler to make this change in a subsequent CL when I add > > the signaling changes. Added TODO for this. > > I'm OK with you do that in a followup CL. > Thanks. Will do it in the next CL. https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1051: LOG(LS_ERROR) << "Negative nomination value: " << nomination_value; On 2016/08/03 22:13:25, pthatcher1 wrote: > On 2016/08/03 04:46:56, honghaiz3 wrote: > > On 2016/07/28 22:57:26, pthatcher1 wrote: > > > Should we use a uint32_t instead of an int? It can't be negative. > > I think the code style says we should not use uint32 because a value does not > > take negative values. It recommends using ASSERT but here we don't want to > > assert error simply because remote sides sends an invalid attribute. > > It's not just because it can't be negative. What's the range of possible values > for the STUN attribute that we receive from the controlling side? Does it use > the entire 32-bit space (as the call to GetUint32 implies)? If so, on a 32-bit > machine, if an int is 32 bits and the remote side sends us a nomination value of > 2**31+1, we'll think it's negative, which would be bad. Am I correct? Done, although I don't think it should go to the negative range (>2billion connection switches?). https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2163403002/diff/180001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:520: } On 2016/08/03 22:13:25, pthatcher1 wrote: > On 2016/08/03 04:46:56, honghaiz3 wrote: > > On 2016/07/28 22:57:26, pthatcher1 wrote: > > > I think we should go from including the attribute to not including the > > attribute > > > if it's no longer the selected candidate pair. In other words, just > > > "nomination_ = nomination" here. > > > > > > Even if we wanted fancier logic, it should be in p2ptransportchannel.cc, not > > > here. > > > > The check protects packet re-ordering. > > If for some reason, a ping with lower-nominated value arrived later, we should > > not set the nominated value to a smaller one. nominated is set in Port class. > > Not sure why/how to do it in p2ptransportchannel. > > Ah, good point about reordering. Do we have a unit test that covers that? > > Also, I still think it shouldn't be in set_remote_nomination. Putting it in > port.cc would make sense as: > > if (remote_nomination > 0 && remote_nomination > remote_nomination()) { > set_remote_nomination(remote_nomination); > ... > } > > Actually, it's that guaranteed to be the same as: > > if (remote_nomination > remote_nomination()) { > set_remote_nomination(remote_nomination); > ... > } > > ? > > > By the way, we should make sure that in our standardization that we make it > clear that the 0 value must not be used. Call it reserved or something. Done. Add a test for ignoring smaller nomination. Will remember this about the standardization. https://codereview.webrtc.org/2163403002/diff/280001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2163403002/diff/280001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1619: // REGULAR: To be implemented yet. On 2016/08/03 22:13:25, pthatcher1 wrote: > To be implemented yet => Nominate once per ICE restart (Not implemented yet) Done. https://codereview.webrtc.org/2163403002/diff/280001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1621: // ICE_LITE mode, it will behave as if REGULAR. On 2016/08/03 22:13:25, pthatcher1 wrote: > except that when => except when > is at ICE_LITE mode => is an ice-lite endpoint Done. https://codereview.webrtc.org/2163403002/diff/280001/webrtc/p2p/base/transport.h File webrtc/p2p/base/transport.h (right): https://codereview.webrtc.org/2163403002/diff/280001/webrtc/p2p/base/transpor... webrtc/p2p/base/transport.h:165: // ICE Nomination mode. On 2016/08/03 22:13:25, pthatcher1 wrote: > Can you move the definitions of these from the other comment to this comment? Done. I kept the details of SEMI_AGGRESSIVE in p2ptransportchannel. It is probably more readable. https://codereview.webrtc.org/2163403002/diff/280001/webrtc/p2p/base/transpor... webrtc/p2p/base/transport.h:206: NominationMode default_nomination_mode = NominationMode::SEMI_AGGRESSIVE; On 2016/08/03 22:13:25, pthatcher1 wrote: > Can you put a TODO to change it to REGULAR? Done.
lgtm At 10 switches per seconds, that's only 6.3 years of switches :). It's not so much the number of switches I'm worried about, as remote endpoints not starting with 1, or some other strange behavior that causes them to use the full allowed range.
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/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2163403002/#ps320001 (title: "Updated comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
The CQ bit was checked by honghaiz@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Prepare for ICE renomination. Add an ICE nomination attribute. If a connection switched on the controlling side, increase the nomination value set in the attribute. The controlled side will also be ready for re-nomination option; it will switch if a nomination comes with a higher nomination value even though it may be at a lower priority. Plus, don't nominate or re-nominate if the nomination value at the current connection has been acknowledged. BUG= ========== to ========== Prepare for ICE renomination. Add an ICE nomination attribute. If a connection switched on the controlling side, increase the nomination value set in the attribute. The controlled side will also be ready for re-nomination option; it will switch if a nomination comes with a higher nomination value even though it may be at a lower priority. Plus, don't nominate or re-nominate if the nomination value at the current connection has been acknowledged. BUG= R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/8cd8f817487d3cd7e973ab822... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:320001) manually as 8cd8f817487d3cd7e973ab822505bc8a5f3ced6f (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Prepare for ICE renomination. Add an ICE nomination attribute. If a connection switched on the controlling side, increase the nomination value set in the attribute. The controlled side will also be ready for re-nomination option; it will switch if a nomination comes with a higher nomination value even though it may be at a lower priority. Plus, don't nominate or re-nominate if the nomination value at the current connection has been acknowledged. BUG= R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/8cd8f817487d3cd7e973ab822... ========== to ========== Prepare for ICE renomination. Add an ICE nomination attribute. If a connection switched on the controlling side, increase the nomination value set in the attribute. The controlled side will also be ready for re-nomination option; it will switch if a nomination comes with a higher nomination value even though it may be at a lower priority. Plus, don't nominate or re-nominate if the nomination value at the current connection has been acknowledged. BUG= R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/8cd8f817487d3cd7e973ab822505bc8a5f3ced6f Cr-Commit-Position: refs/heads/master@{#13631} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
