Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2069493002:
Do not switch best connection on the controlled side too frequently (Closed)
DescriptionDo not switch best connection on the controlled side too frequently due to the nomination from the controlling side.
We now use a single rule to determine connection switch on the controlled side. The rule is to select the new best connection based on the following order:
1. writable/receiving/connected state.
2. nominated
3. last time receiving data packet.
4. priority.
5. latency (rtt)
BUG=
R=deadbeef@webrtc.org, pthatcher@webrtc.org
Committed: https://crrev.com/572b094128c4def38dd9f21a4df9e1dd72787fe9
Cr-Commit-Position: refs/heads/master@{#13274}
Patch Set 1 : #
Total comments: 12
Patch Set 2 : Add tests #
Total comments: 45
Patch Set 3 : Address comments #
Total comments: 10
Patch Set 4 : . #Patch Set 5 : Updates a comment #
Total comments: 55
Patch Set 6 : Address comments #Patch Set 7 : Merge with head #
Total comments: 2
Patch Set 8 : address Taylor's comments #
Total comments: 10
Patch Set 9 : Removed the change in port.cc #Patch Set 10 : Changed a comment about pruning. #Patch Set 11 : Using a_is_better and b_is_better to replace 1 and -1 for connection comparision #
Total comments: 2
Patch Set 12 : Merge again #Patch Set 13 : . #Patch Set 14 : Small fix after merge #Patch Set 15 : Merge branch 'master' into nominate_on_controlled_side #
Messages
Total messages: 50 (21 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
honghaiz@webrtc.org changed reviewers: + pthatcher@webrtc.org
I still need to add the tests yet. It will great if you can take a look at the CL.
Description was changed from ========== Switch best connection on the controlled side. On the controlled side, Switch the best connection if 1) a new connection is writable, 2) it was nominated consecutively, or it was nominated and the media is received on it. BUG= ========== to ========== Do not switch best connection on the controlled side too frequently due to the nomination from the controlling side. On the controlled side, Switch the best connection if 1) a new connection is writable, 2) it was nominated consecutively, or it was nominated and the media is received on it. BUG= ==========
Added tests. PTAL.
https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:211: // iii) the new connection has been nominated twice consecutively, or This would be useful for connections not receiving data.
https://codereview.webrtc.org/2069493002/diff/80001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2069493002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:311: Connection* best_connection_ = nullptr; Can we just start calling the "selected_connection_"? The more we have nominated vs. selected being different, the less "best" makes sense as a word. https://codereview.webrtc.org/2069493002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:313: // Connection selected by the controlling agent. This should be used only It should be "connection nominated by the controlling agent". https://codereview.webrtc.org/2069493002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:314: // at controlled side when protocol type is RFC5245. The stuff about protocol type is no longer needed. GICE is gone. https://codereview.webrtc.org/2069493002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:315: Connection* last_nominated_conn_ = nullptr; We could have it be "conn_" everwhere (including selected_connection_) or "connection_" everywhere. I'm fine either way. https://codereview.webrtc.org/2069493002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:316: // The connection on which the data was received most recently. was most recently received. https://codereview.webrtc.org/2069493002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:317: Connection* last_data_received_conn_ = nullptr; I think this should coincide with our "receiving" naming, so "most_recently_receiving_conn_" https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:211: // iii) the new connection has been nominated twice consecutively, or On 2016/06/14 23:18:27, honghaiz3 wrote: > This would be useful for connections not receiving data. As I mentioned in the another comment, I think that would make the purpose of this CL be lost, because an aggressively nominating peer will nominate every candidate pair multiple times. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:216: bool ShouldSwitchOnNominatedOrDataReceived( I think this should be called ShouldSwitchSelectedConnection and should be like it is in Taylor's CL (a member method), but with modified logic. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:224: } Shouldn't this use the "comparison of writable state"? For example, what if both are not writable, or the new connection is only presumed writable? https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:227: } But the new_connection might not be nominated either, so we should probably compare nominated flags like we compare writable states. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:231: } Perhaps instead of recording the last conn that received data, we should record the timestamp of the last time we received data in each connection (in fact, don't we already do that?) and compare those. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:233: return CompareConnectionCandidates(best_connection, new_connection) < 0; I think we should make all the logic very similar where it's comparison based and the comparison have clear precedence: writable_state > receiving_timestamp > nomination_flag > network cost > priority https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:782: } Can this just be this? if (!ShouldSwitchSelectedConnection(best_connection, conn) { LOG(LS_INFO) << "Not switching the best connection on controlled side yet:"; return; } SwitchBestConnectionTo(conn); By the way, I think SwitchBestConnectionTo should be renamed "SwitchSelectedConnection" and should log that the switch happened, and should do RequestSort() rather than having it logged here and RequestSort here. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1499: } Can't this just be if (connection == last_nominated_conn_ && ShouldSwitchSelectedConnection(best_connection_, connection)) { SwitchBestConnectionTo(connection); } ? https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1539: } I think we should just remove these lines and have it be OK if last_nominated_conn_ == best_connection_ instead of nulling-out last_nominated_conn_. Nulling it out no longer matches the name. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1614: last_data_received_conn_ = nullptr; Why would we set this to nullptr? https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1618: } Wouldn't it make more sense to do this? last_data_received_conn_ = connection; if (ShouldSwitchSelectedConnection(best_connection_, connection)) { SwitchBestConnectionTo(connection); } You can take ShouldSwitchSelectedConnection from Taylor's recent CL where he makes it a member method. Or you can wait until his CL lands. It should contain all of the logic for ice_role_, whether best_connection_ == connection, etc. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2005: ++num_selected_candidate_pair_switch_; switch_ => switches_ And you can probably drop "num_" from the front. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2525: // Simulate the aggressive nomination on the controlling side. the aggressive nomination => aggressive nomination https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2530: reset_num_selected_candidate_pair_switch(); Why not have reset return the last value. Then you can just do this: EXPECT_EQ(1, reset_selected_candidate_pair_switches()); https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2543: // Selected candidate pair will switch if it is nominated again. I don't think we should do this one, because an aggressively nominating controlling side will nominate a given candidate pair multiple times, and will still goes frequent switches on the controlled side. Maybe not as frequent, but still frequent. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2549: // Selected candidate pair won't switch because conn3 has lower priority Can you add "until conn3 receives data"?
Only Discuss one comment. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:224: } On 2016/06/15 21:59:53, pthatcher1 wrote: > Shouldn't this use the "comparison of writable state"? For example, what if > both are not writable, or the new connection is only presumed writable? Right. The new connection is already writable. Plus, In the ShouldSwitch method, if the current connection is writable and is nominated, we will not switch. ShouldSwitch is called when sorting connections. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:233: return CompareConnectionCandidates(best_connection, new_connection) < 0; On 2016/06/15 21:59:53, pthatcher1 wrote: > I think we should make all the logic very similar where it's comparison based > and the comparison have clear precedence: > > writable_state > receiving_timestamp > nomination_flag > network cost > priority As I mentioned in another comment, if the current connection is nominated, we don't want to switch simply based on connection sorting. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2543: // Selected candidate pair will switch if it is nominated again. On 2016/06/15 21:59:53, pthatcher1 wrote: > I don't think we should do this one, because an aggressively nominating > controlling side will nominate a given candidate pair multiple times, and will > still goes frequent switches on the controlled side. Maybe not as frequent, but > still frequent. If we do not do this, how do we handle the case that some channel may not receive data? When there are multiple candidate pairs, the chance that a connection is nominated twice consecutively should be pretty small. And when it converge, it should converge to the one selected on the controlling side.
On 2016/06/16 01:13:09, honghaiz3 wrote: > Only Discuss one comment. > > https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... > File webrtc/p2p/base/p2ptransportchannel.cc (right): > > https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:224: } > On 2016/06/15 21:59:53, pthatcher1 wrote: > > Shouldn't this use the "comparison of writable state"? For example, what if > > both are not writable, or the new connection is only presumed writable? > Right. The new connection is already writable. > Plus, In the ShouldSwitch method, if the current connection is writable and is > nominated, we will not switch. ShouldSwitch is called when sorting connections. > > https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:233: return > CompareConnectionCandidates(best_connection, new_connection) < 0; > On 2016/06/15 21:59:53, pthatcher1 wrote: > > I think we should make all the logic very similar where it's comparison based > > and the comparison have clear precedence: > > > > writable_state > receiving_timestamp > nomination_flag > network cost > > priority > > As I mentioned in another comment, if the current connection is nominated, we > don't want to switch simply based on connection sorting. > > https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... > File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): > > https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel_unittest.cc:2543: // Selected candidate pair > will switch if it is nominated again. > On 2016/06/15 21:59:53, pthatcher1 wrote: > > I don't think we should do this one, because an aggressively nominating > > controlling side will nominate a given candidate pair multiple times, and will > > still goes frequent switches on the controlled side. Maybe not as frequent, > but > > still frequent. > > If we do not do this, how do we handle the case that some channel may not > receive data? When there are multiple candidate pairs, the chance that a > connection is nominated twice consecutively should be pretty small. And when it > converge, it should converge to the one selected on the controlling side. Oh. I did not realize I responded a few. Will address other comments later.
Description was changed from ========== Do not switch best connection on the controlled side too frequently due to the nomination from the controlling side. On the controlled side, Switch the best connection if 1) a new connection is writable, 2) it was nominated consecutively, or it was nominated and the media is received on it. BUG= ========== to ========== Do not switch best connection on the controlled side too frequently due to the nomination from the controlling side. On the controlled side, Switch the best connection if 1) a new connection is writable,and 2) it was nominated and the media is received on it. BUG= ==========
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
PTAL. https://codereview.webrtc.org/2069493002/diff/80001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2069493002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:311: Connection* best_connection_ = nullptr; On 2016/06/15 21:59:52, pthatcher1 wrote: > Can we just start calling the "selected_connection_"? The more we have > nominated vs. selected being different, the less "best" makes sense as a word. Done. https://codereview.webrtc.org/2069493002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:313: // Connection selected by the controlling agent. This should be used only On 2016/06/15 21:59:52, pthatcher1 wrote: > It should be "connection nominated by the controlling agent". Done. https://codereview.webrtc.org/2069493002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:314: // at controlled side when protocol type is RFC5245. On 2016/06/15 21:59:52, pthatcher1 wrote: > The stuff about protocol type is no longer needed. GICE is gone. Done. https://codereview.webrtc.org/2069493002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:315: Connection* last_nominated_conn_ = nullptr; On 2016/06/15 21:59:52, pthatcher1 wrote: > We could have it be "conn_" everwhere (including selected_connection_) or > "connection_" everywhere. I'm fine either way. Done. https://codereview.webrtc.org/2069493002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:316: // The connection on which the data was received most recently. On 2016/06/15 21:59:52, pthatcher1 wrote: > was most recently received. Done. https://codereview.webrtc.org/2069493002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:317: Connection* last_data_received_conn_ = nullptr; On 2016/06/15 21:59:52, pthatcher1 wrote: > I think this should coincide with our "receiving" naming, so > "most_recently_receiving_conn_" Using last_receiving_connection_ as it is shorter and should have no confusion with "most_recently..." https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:211: // iii) the new connection has been nominated twice consecutively, or On 2016/06/15 21:59:53, pthatcher1 wrote: > On 2016/06/14 23:18:27, honghaiz3 wrote: > > This would be useful for connections not receiving data. > > As I mentioned in the another comment, I think that would make the purpose of > this CL be lost, because an aggressively nominating peer will nominate every > candidate pair multiple times. Done. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:216: bool ShouldSwitchOnNominatedOrDataReceived( On 2016/06/15 21:59:53, pthatcher1 wrote: > I think this should be called ShouldSwitchSelectedConnection and should be like > it is in Taylor's CL (a member method), but with modified logic. I don't think it should use the current ShouldSwitch because this is based on nomination. If we do that, it may caused Connection keeping on switching if the priority computation of two sides are ever different. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:224: } On 2016/06/15 21:59:53, pthatcher1 wrote: > Shouldn't this use the "comparison of writable state"? For example, what if > both are not writable, or the new connection is only presumed writable? If a nominated connection is not writable, it won't switch. Don't need to compare the writable/receiving state. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:227: } On 2016/06/15 21:59:53, pthatcher1 wrote: > But the new_connection might not be nominated either, so we should probably > compare nominated flags like we compare writable states. This method is primarily called when it is nominated. its state must be nominated except when it is called with data receiving. In the latter case, we shall switch anyway. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:231: } On 2016/06/15 21:59:53, pthatcher1 wrote: > Perhaps instead of recording the last conn that received data, we should record > the timestamp of the last time we received data in each connection (in fact, > don't we already do that?) and compare those. using the timestamp is less effective as that only compared the receiving time of two connections, not equivalent to the connection receiving data most recently. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:782: } On 2016/06/15 21:59:53, pthatcher1 wrote: > Can this just be this? > > if (!ShouldSwitchSelectedConnection(best_connection, conn) { > LOG(LS_INFO) << "Not switching the best connection on controlled side yet:"; > return; > } > > SwitchBestConnectionTo(conn); > > By the way, I think SwitchBestConnectionTo should be renamed > "SwitchSelectedConnection" and should log that the switch happened, and should > do RequestSort() rather than having it logged here and RequestSort here. Done. Regarding the logging, I think the benefit of logging here is that when you debug, you know where/why the connection is switched. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1499: } On 2016/06/15 21:59:53, pthatcher1 wrote: > Can't this just be > > if (connection == last_nominated_conn_ && > ShouldSwitchSelectedConnection(best_connection_, connection)) { > SwitchBestConnectionTo(connection); > } > > ? Ah Yes. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1539: } On 2016/06/15 21:59:53, pthatcher1 wrote: > I think we should just remove these lines and have it be OK if > last_nominated_conn_ == best_connection_ instead of nulling-out > last_nominated_conn_. Nulling it out no longer matches the name. Done. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1614: last_data_received_conn_ = nullptr; On 2016/06/15 21:59:53, pthatcher1 wrote: > Why would we set this to nullptr? Perhaps not necessary. Removed. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1618: } On 2016/06/15 21:59:53, pthatcher1 wrote: > Wouldn't it make more sense to do this? > > last_data_received_conn_ = connection; > if (ShouldSwitchSelectedConnection(best_connection_, connection)) { > SwitchBestConnectionTo(connection); > } > > You can take ShouldSwitchSelectedConnection from Taylor's recent CL where he > makes it a member method. Or you can wait until his CL lands. It should > contain all of the logic for ice_role_, whether best_connection_ == connection, > etc. Done. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2005: ++num_selected_candidate_pair_switch_; On 2016/06/15 21:59:53, pthatcher1 wrote: > switch_ => switches_ > > And you can probably drop "num_" from the front. Done. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2525: // Simulate the aggressive nomination on the controlling side. On 2016/06/15 21:59:53, pthatcher1 wrote: > the aggressive nomination => aggressive nomination Done. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2549: // Selected candidate pair won't switch because conn3 has lower priority On 2016/06/15 21:59:53, pthatcher1 wrote: > Can you add "until conn3 receives data"? Done.
pthatcher@webrtc.org changed reviewers: + deadbeef@webrtc.org
The biggest change I'd like to see is to have one ShouldSwitchSelectedConnection that is all the places. I think if we do so, the logic which be much more clear. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:216: bool ShouldSwitchOnNominatedOrDataReceived( On 2016/06/16 23:03:48, honghaiz3 wrote: > On 2016/06/15 21:59:53, pthatcher1 wrote: > > I think this should be called ShouldSwitchSelectedConnection and should be > like > > it is in Taylor's CL (a member method), but with modified logic. > > I don't think it should use the current ShouldSwitch because this is based on > nomination. If we do that, it may caused Connection keeping on switching if the > priority computation of two sides are ever different. That will only happen if the controlling side isn't sending media, in which case there's only one candidate pair being used anyway. And I think this would simplify our code quite a bit: the logic for switching is always the same regardless of when or why we decide to check if we should switch. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:224: } True. "if (!wrtible()) { return false; }" is more simple. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:227: } On 2016/06/16 23:03:49, honghaiz3 wrote: > On 2016/06/15 21:59:53, pthatcher1 wrote: > > But the new_connection might not be nominated either, so we should probably > > compare nominated flags like we compare writable states. > > This method is primarily called when it is nominated. its state must be > nominated except when it is called with data receiving. In the latter case, we > shall switch anyway. If the controlling side isn't aggressive and none of the connections are nominated, this logic becomes "select the last writable connection that calls this method". And we assume this will only be called when those things make sense. That's pretty subtle. It would prefer it it were more explicit. Something like this: if (best_connection == nullptr) { return true; } if (connection->nominated() != best_connection->nominated()) { return connection->nominated(); } https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:231: } On 2016/06/16 23:03:48, honghaiz3 wrote: > On 2016/06/15 21:59:53, pthatcher1 wrote: > > Perhaps instead of recording the last conn that received data, we should > record > > the timestamp of the last time we received data in each connection (in fact, > > don't we already do that?) and compare those. > > using the timestamp is less effective as that only compared the receiving time > of two connections, not equivalent to the connection receiving data most > recently. I mean something like this: if (connection->last_data_received() != best_connection->last_data_received()) { return connection->last_data_received() > best_connection->last_received_data(); } Then we don't have to store the last connection with data received, and it's more clear. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:233: return CompareConnectionCandidates(best_connection, new_connection) < 0; On 2016/06/16 01:13:08, honghaiz3 wrote: > On 2016/06/15 21:59:53, pthatcher1 wrote: > > I think we should make all the logic very similar where it's comparison based > > and the comparison have clear precedence: > > > > writable_state > receiving_timestamp > nomination_flag > network cost > > priority > > As I mentioned in another comment, if the current connection is nominated, we > don't want to switch simply based on connection sorting. I'm mostly suggesting we make the logic clear. Switching based on connection sorting is basically what we already do if both are nominated (as it will be with aggressive nomination). https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:782: } On 2016/06/16 23:03:49, honghaiz3 wrote: > On 2016/06/15 21:59:53, pthatcher1 wrote: > > Can this just be this? > > > > if (!ShouldSwitchSelectedConnection(best_connection, conn) { > > LOG(LS_INFO) << "Not switching the best connection on controlled side yet:"; > > return; > > } > > > > SwitchBestConnectionTo(conn); > > > > By the way, I think SwitchBestConnectionTo should be renamed > > "SwitchSelectedConnection" and should log that the switch happened, and should > > do RequestSort() rather than having it logged here and RequestSort here. > Done. > Regarding the logging, I think the benefit of logging here is that when you > debug, you know where/why the connection is switched. What about RequestSort()? Shouldn't we do that in SwitchSelectedConnection()? https://codereview.webrtc.org/2069493002/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:323: } As mentioned, I think this is more clear and correct: if (best_connection == nullptr) { return true; } if (connection->nominated() != best_connection->nominated()) { return connection->nominated(); } https://codereview.webrtc.org/2069493002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:324: if (new_connection == last_nominated_connection_ && I think we shouldn't change the behavior based on which candidate pair was nominated last, because that's essentially random and will make this behave like it currently does with the bug (switches too much). https://codereview.webrtc.org/2069493002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:327: } As mentioned, I think this is more robust: if (connection->last_data_received() != best_connection->last_data_received()) { return connection->last_data_received() > best_connection->last_received_data(); } https://codereview.webrtc.org/2069493002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1129: LOG(LS_INFO) << "Switching selected connection after sorting: " Can we please unify the two ShouldSwitch methods into one? https://codereview.webrtc.org/2069493002/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2069493002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:261: bool ShouldSwitchOnNominatedOrDataReceived(Connection* new_connection) const; I still think ShouldSwitchSelectedCandidate makes more sense. We don't need why to switch in the method name.
I have made all logic for Switching connection the same now. Sorting also used the same logic except without the rtt improvement margin. Also fixing an issue that some connections may be pruned before it is actually selected by the controlling side. A small but subtle change. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:216: bool ShouldSwitchOnNominatedOrDataReceived( On 2016/06/17 00:27:11, pthatcher1 wrote: > On 2016/06/16 23:03:48, honghaiz3 wrote: > > On 2016/06/15 21:59:53, pthatcher1 wrote: > > > I think this should be called ShouldSwitchSelectedConnection and should be > > like > > > it is in Taylor's CL (a member method), but with modified logic. > > > > I don't think it should use the current ShouldSwitch because this is based on > > nomination. If we do that, it may caused Connection keeping on switching if > the > > priority computation of two sides are ever different. > > That will only happen if the controlling side isn't sending media, in which case > there's only one candidate pair being used anyway. > > And I think this would simplify our code quite a bit: the logic for switching is > always the same regardless of when or why we decide to check if we should > switch. Done. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:224: } On 2016/06/17 00:27:11, pthatcher1 wrote: > True. "if (!wrtible()) { return false; }" is more simple. Changed this logic to just compare the write/receiving state. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:227: } On 2016/06/17 00:27:11, pthatcher1 wrote: > On 2016/06/16 23:03:49, honghaiz3 wrote: > > On 2016/06/15 21:59:53, pthatcher1 wrote: > > > But the new_connection might not be nominated either, so we should probably > > > compare nominated flags like we compare writable states. > > > > This method is primarily called when it is nominated. its state must be > > nominated except when it is called with data receiving. In the latter case, we > > shall switch anyway. > > If the controlling side isn't aggressive and none of the connections are > nominated, this logic becomes "select the last writable connection that calls > this method". And we assume this will only be called when those things make > sense. > > That's pretty subtle. It would prefer it it were more explicit. Something like > this: > > if (best_connection == nullptr) { > return true; > } > > if (connection->nominated() != best_connection->nominated()) { > return connection->nominated(); > } > > Done. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:231: } On 2016/06/17 00:27:11, pthatcher1 wrote: > On 2016/06/16 23:03:48, honghaiz3 wrote: > > On 2016/06/15 21:59:53, pthatcher1 wrote: > > > Perhaps instead of recording the last conn that received data, we should > > record > > > the timestamp of the last time we received data in each connection (in fact, > > > don't we already do that?) and compare those. > > > > using the timestamp is less effective as that only compared the receiving time > > of two connections, not equivalent to the connection receiving data most > > recently. > > I mean something like this: > > if (connection->last_data_received() != best_connection->last_data_received()) { > return connection->last_data_received() > > best_connection->last_received_data(); > } > > Then we don't have to store the last connection with data received, and it's > more clear. Done. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:233: return CompareConnectionCandidates(best_connection, new_connection) < 0; On 2016/06/17 00:27:11, pthatcher1 wrote: > On 2016/06/16 01:13:08, honghaiz3 wrote: > > On 2016/06/15 21:59:53, pthatcher1 wrote: > > > I think we should make all the logic very similar where it's comparison > based > > > and the comparison have clear precedence: > > > > > > writable_state > receiving_timestamp > nomination_flag > network cost > > > priority > > > > As I mentioned in another comment, if the current connection is nominated, we > > don't want to switch simply based on connection sorting. > > I'm mostly suggesting we make the logic clear. Switching based on connection > sorting is basically what we already do if both are nominated (as it will be > with aggressive nomination). Done. https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2069493002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2530: reset_num_selected_candidate_pair_switch(); On 2016/06/15 21:59:53, pthatcher1 wrote: > Why not have reset return the last value. Then you can just do this: > > EXPECT_EQ(1, reset_selected_candidate_pair_switches()); Done. https://codereview.webrtc.org/2069493002/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:323: } On 2016/06/17 00:27:11, pthatcher1 wrote: > As mentioned, I think this is more clear and correct: > > if (best_connection == nullptr) { > return true; > } > > if (connection->nominated() != best_connection->nominated()) { > return connection->nominated(); > } Done. https://codereview.webrtc.org/2069493002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:324: if (new_connection == last_nominated_connection_ && On 2016/06/17 00:27:11, pthatcher1 wrote: > I think we shouldn't change the behavior based on which candidate pair was > nominated last, because that's essentially random and will make this behave like > it currently does with the bug (switches too much). Done. https://codereview.webrtc.org/2069493002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:327: } On 2016/06/17 00:27:11, pthatcher1 wrote: > As mentioned, I think this is more robust: > > if (connection->last_data_received() != best_connection->last_data_received()) { > return connection->last_data_received() > > best_connection->last_received_data(); > } Done. https://codereview.webrtc.org/2069493002/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2069493002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:261: bool ShouldSwitchOnNominatedOrDataReceived(Connection* new_connection) const; On 2016/06/17 00:27:11, pthatcher1 wrote: > I still think ShouldSwitchSelectedCandidate makes more sense. We don't need why > to switch in the method name. Done.
That looks so much better. Just some minor things now. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:230: int P2PTransportChannel::CompareConnections( Can you call this CompareConnectionIncludingStates? https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:246: int P2PTransportChannel::CompareConnectionsBase( Can you call this CompareConnectionsExcludingStates? https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:282: bool P2PTransportChannel::ShouldSwitchConnection( Can you call this ShouldSwitchSelectedConnection? https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:285: RTC_DCHECK(selected_connection_ != new_connection); I think it would simply be better to do this: if (!new_connection || selcted_connection_ == new_connection) { return false; } https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1104: ShouldSwitchConnection(top_connection)) { Can you move this line into ShouldSwitchSelectedConnection? if (selected_connection_ == connection) { return false; } And then this would simplify to: if (ShouldSwitchSelectedConnection(top_connection) { ... } https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1110: // Controlled side can prune only if the selected connection has been Controlled side => The controlled side https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1142: Connection* P2PTransportChannel::selected_nominated_connection() const { Since this is only called once, I suggest we inline it as: if (ice_role_ == ICEROLE_CONTROLLING || (selected_connection_ && selected_connection->nominated())) { PruneConnections(); } https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1571: if (ShouldSwitchConnection(connection)) { Can you move this line into ShouldSwitchConnection? if (selected_connection_ == connection) { return false; } And then this would simplify to: if (ice_role_ == ICEROLE_CONTROLLED && ShouldSwitchSelectedConnection(connection)) { ... } https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:270: // connection need to be pruned. need to be => needs to be
https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:256: if (a->last_data_received() > b->last_data_received()) { What happens if the controlled side stops sending data (call is put on hold)? Then we would be stuck with one connection, right? https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:295: // Lastly, switch only if rtt has improved by a margin. Odd to say "lastly" if there's no "firstly". Maybe above there should be a comment saying "First, compare based on write state, and candidates". https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:742: // Now we have selected the selected connection, time to prune other existing "Now that we have selected a connection, it's time to prune..." https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1174: (CompareConnectionsBase(premier, conn) >= 0)) { Why can't you just use CompareConnections here? Isn't |premier| guaranteed to have a better or equal write state than |conn|, since connections are always sorted by the time this function is called? Also, this change means that the controlled side will prune connections based on which last received packets... This seems wrong to me. The RTT could change, making the controlling side choose a different connection. Or a race condition could result in the wrong connection being pruned when ICE is first starting. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1181: // Track the selected connection, and let listeners know. Maybe "Change the" instead of "Track the". https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1304: (selected_connection_->port()->Network() == network)) Nit: As long as you're changing this code, can you add {}s https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:271: // Returns a positive value if |a| is better than |b| Missing period. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:272: int CompareConnectionsBase(const cricket::Connection* a, I think this method can be removed. But if it's kept in, it should have a better name, such as CompareConnectionsIgnoringWriteState. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1118: // The caller should have the best connection connected to the peer reflexive s/best connection/selected connnection/g https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2512: TEST_F(P2PTransportChannelPingTest, TestDonotSwitchConnectionTooFrequently) { Rename test to ControlledAgentDoesntSwitchConnectionTooFrequently https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2573: } This test's expectations are good but it's testing way too many things. I think there should be separate tests for: 1. First nominated connection is selected. 2. Higher priority nominated connection is selected. 3. Connection on which data was last received is selected. 4. Connection is selected once it becomes writable. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1055: if (write_state_ == STATE_WRITE_TIMEOUT && (just_nominated || !pruned_)) { Why is the connection only restored if it's "just_nominated"? The controlling agent could switch connections back and forth over the course of a session due to RTT, right?
Also, can you fix the formatting of the CL description? And subheading "2)" includes two conditions, so it can probably be split into "2)" and "3)"
Description was changed from ========== Do not switch best connection on the controlled side too frequently due to the nomination from the controlling side. On the controlled side, Switch the best connection if 1) a new connection is writable,and 2) it was nominated and the media is received on it. BUG= ========== to ========== Do not switch best connection on the controlled side too frequently due to the nomination from the controlling side. We now use a single rule to determine connection switch on the controlled side. The rule is to select the new best connection based on the following order: 1. writable/receiving/connected state. 2. nominated 3. last time receiving data packet. 4. priority. 5. latency (rtt) BUG= ==========
I think Taylor is right that we need to be careful with when we prune, and not to include the receiving media state. That seems like something we really don't want to break (pruning too much), and I'm surprised we don't have more unit tests around it. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:256: if (a->last_data_received() > b->last_data_received()) { On 2016/06/21 18:33:25, Taylor Brandstetter wrote: > What happens if the controlled side stops sending data (call is put on hold)? > Then we would be stuck with one connection, right? We could still switch to a higher priority candidate pair, but generally, yes, you're right. And that's exactly what the RFC says to do. And it's exactly why our follow-up to this is to implement the renomination draft: so we can safely switch selected candidate pairs without relying on media being sent. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:295: // Lastly, switch only if rtt has improved by a margin. On 2016/06/21 18:33:25, Taylor Brandstetter wrote: > Odd to say "lastly" if there's no "firstly". Maybe above there should be a > comment saying "First, compare based on write state, and candidates". I think that's what the big comment before the method says. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1174: (CompareConnectionsBase(premier, conn) >= 0)) { On 2016/06/21 18:33:25, Taylor Brandstetter wrote: > Why can't you just use CompareConnections here? Isn't |premier| guaranteed to > have a better or equal write state than |conn|, since connections are always > sorted by the time this function is called? I think this is so we don't prune a better candidate pair just because it's not writable yet. Otherwise, the first writable candidate pair would always prune of all the other candidate pairs, even if we'd rather have one of the others once they become writable. > > Also, this change means that the controlled side will prune connections based on > which last received packets... This seems wrong to me. The RTT could change, > making the controlling side choose a different connection. Or a race condition > could result in the wrong connection being pruned when ICE is first starting. Good catch! Perhaps instead of having CompareConnections and CompareConnectionsBase, we should just have CompareConnections for sorting and then have a separate method just for ShouldPrune(conn, premier), which should take into account the writable and nominated state of premier, but not the writable and nominated state of conn, and not the receiving state of either. And if don't have a unit test that checks that we don't prune a connection until a better one is writable and nominated, then we really should add one. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:961: // because receiving data will increase its priority in the pruning process. I don't understand this comment. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:965: ResetWriteStateAndUnprune(); So the change is to unprune when we received media on a nominated pair? Why would we need to do that? Is that just so that we unprune backup candidate pairs on the controlled side when the controlling side switches to one? If so, is this just here until we're fully switched over to "renomination"? Also, can the comment reflect that such logic is only for backup candidate pairs of aggressively controlling peers? Finally, why did we not need this logic before? https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1055: if (write_state_ == STATE_WRITE_TIMEOUT && (just_nominated || !pruned_)) { On 2016/06/21 18:33:26, Taylor Brandstetter wrote: > Why is the connection only restored if it's "just_nominated"? The controlling > agent could switch connections back and forth over the course of a session due > to RTT, right? Yeah, I don't understand the "just nominated" logic either. If we don't prune until a candidate pair is nominated and writable, then the fact that the candidate pair was pruned means a better candidate pair was writable and nominated. So why would be unprune this candidate pair, which we know to be worse? It seems contrary to the concept of pruning when the controlling side is aggressive. We're just going to prune it again once the better candidate pair is nominated again.
Patchset #6 (id:220001) has been deleted
PTAL. https://codereview.webrtc.org/2069493002/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1129: LOG(LS_INFO) << "Switching selected connection after sorting: " On 2016/06/17 00:27:11, pthatcher1 wrote: > Can we please unify the two ShouldSwitch methods into one? Done. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:230: int P2PTransportChannel::CompareConnections( On 2016/06/21 07:16:53, pthatcher1 wrote: > Can you call this CompareConnectionIncludingStates? Done. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:246: int P2PTransportChannel::CompareConnectionsBase( On 2016/06/21 07:16:53, pthatcher1 wrote: > Can you call this CompareConnectionsExcludingStates? Done. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:256: if (a->last_data_received() > b->last_data_received()) { On 2016/06/21 18:33:25, Taylor Brandstetter wrote: > What happens if the controlled side stops sending data (call is put on hold)? > Then we would be stuck with one connection, right? If the controlling side stops sending data, the controlled side won't switch until writable/receiving state changes. We discussed this and felt it might be OK to have asymmetric path if the controlling side is not sending data. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:256: if (a->last_data_received() > b->last_data_received()) { On 2016/06/22 06:36:34, pthatcher1 wrote: > On 2016/06/21 18:33:25, Taylor Brandstetter wrote: > > What happens if the controlled side stops sending data (call is put on hold)? > > Then we would be stuck with one connection, right? > > We could still switch to a higher priority candidate pair, but generally, yes, > you're right. And that's exactly what the RFC says to do. > > And it's exactly why our follow-up to this is to implement the renomination > draft: so we can safely switch selected candidate pairs without relying on media > being sent. Acknowledged. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:282: bool P2PTransportChannel::ShouldSwitchConnection( On 2016/06/21 07:16:53, pthatcher1 wrote: > Can you call this ShouldSwitchSelectedConnection? Done. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:285: RTC_DCHECK(selected_connection_ != new_connection); On 2016/06/21 07:16:52, pthatcher1 wrote: > I think it would simply be better to do this: > > if (!new_connection || selcted_connection_ == new_connection) { > return false; > } Done. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:295: // Lastly, switch only if rtt has improved by a margin. On 2016/06/21 18:33:25, Taylor Brandstetter wrote: > Odd to say "lastly" if there's no "firstly". Maybe above there should be a > comment saying "First, compare based on write state, and candidates". Revised the comments. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:295: // Lastly, switch only if rtt has improved by a margin. On 2016/06/22 06:36:34, pthatcher1 wrote: > On 2016/06/21 18:33:25, Taylor Brandstetter wrote: > > Odd to say "lastly" if there's no "firstly". Maybe above there should be a > > comment saying "First, compare based on write state, and candidates". > > I think that's what the big comment before the method says. Acknowledged. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:742: // Now we have selected the selected connection, time to prune other existing On 2016/06/21 18:33:25, Taylor Brandstetter wrote: > "Now that we have selected a connection, it's time to prune..." Done. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1104: ShouldSwitchConnection(top_connection)) { On 2016/06/21 07:16:53, pthatcher1 wrote: > Can you move this line into ShouldSwitchSelectedConnection? > > if (selected_connection_ == connection) { > return false; > } > > And then this would simplify to: > > if (ShouldSwitchSelectedConnection(top_connection) { > ... > } Done. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1110: // Controlled side can prune only if the selected connection has been On 2016/06/21 07:16:53, pthatcher1 wrote: > Controlled side => The controlled side Done. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1142: Connection* P2PTransportChannel::selected_nominated_connection() const { On 2016/06/21 07:16:53, pthatcher1 wrote: > Since this is only called once, I suggest we inline it as: > > if (ice_role_ == ICEROLE_CONTROLLING || (selected_connection_ && > selected_connection->nominated())) { > PruneConnections(); > } Done. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1174: (CompareConnectionsBase(premier, conn) >= 0)) { On 2016/06/21 18:33:25, Taylor Brandstetter wrote: > Why can't you just use CompareConnections here? Isn't |premier| guaranteed to > have a better or equal write state than |conn|, since connections are always > sorted by the time this function is called? > > Also, this change means that the controlled side will prune connections based on > which last received packets... This seems wrong to me. The RTT could change, > making the controlling side choose a different connection. Or a race condition > could result in the wrong connection being pruned when ICE is first starting. CompareConnections (now CompareConnectionsIncludingStates) will first compare the write/receiving/connected states. Basically you don't want a high-priority connection is pruned simply because it does not writable yet. Pruning it means setting it write_timeout state. It may not get a chance to be pinged again. Yes it could happen if a connection received data later it may get pruned. But we have the issue of incorrectly pruning connections any way, see my comments in SortConnections (near the end). To mitigate this, once a connection received data or stun ping with nomination, we un-prune it. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1174: (CompareConnectionsBase(premier, conn) >= 0)) { On 2016/06/22 06:36:33, pthatcher1 wrote: > On 2016/06/21 18:33:25, Taylor Brandstetter wrote: > > Why can't you just use CompareConnections here? Isn't |premier| guaranteed to > > have a better or equal write state than |conn|, since connections are always > > sorted by the time this function is called? > > I think this is so we don't prune a better candidate pair just because it's not > writable yet. Otherwise, the first writable candidate pair would always prune > of all the other candidate pairs, even if we'd rather have one of the others > once they become writable. > > > > > Also, this change means that the controlled side will prune connections based > on > > which last received packets... This seems wrong to me. The RTT could change, > > making the controlling side choose a different connection. Or a race condition > > could result in the wrong connection being pruned when ICE is first starting. > > Good catch! Perhaps instead of having CompareConnections and > CompareConnectionsBase, we should just have CompareConnections for sorting and > then have a separate method just for ShouldPrune(conn, premier), which should > take into account the writable and nominated state of premier, but not the > writable and nominated state of conn, and not the receiving state of either. > > And if don't have a unit test that checks that we don't prune a connection until > a better one is writable and nominated, then we really should add one. Done. I just use the CompareConnectionCandidates to determine whether we should prune. The same as before. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1181: // Track the selected connection, and let listeners know. On 2016/06/21 18:33:25, Taylor Brandstetter wrote: > Maybe "Change the" instead of "Track the". Done. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1304: (selected_connection_->port()->Network() == network)) On 2016/06/21 18:33:25, Taylor Brandstetter wrote: > Nit: As long as you're changing this code, can you add {}s Done. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1571: if (ShouldSwitchConnection(connection)) { On 2016/06/21 07:16:53, pthatcher1 wrote: > Can you move this line into ShouldSwitchConnection? > > if (selected_connection_ == connection) { > return false; > } > > And then this would simplify to: > > if (ice_role_ == ICEROLE_CONTROLLED && > ShouldSwitchSelectedConnection(connection)) { > ... > } Done. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:270: // connection need to be pruned. On 2016/06/21 07:16:53, pthatcher1 wrote: > need to be => needs to be Done. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:271: // Returns a positive value if |a| is better than |b| On 2016/06/21 18:33:26, Taylor Brandstetter wrote: > Missing period. Done. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:272: int CompareConnectionsBase(const cricket::Connection* a, On 2016/06/21 18:33:26, Taylor Brandstetter wrote: > I think this method can be removed. But if it's kept in, it should have a better > name, such as CompareConnectionsIgnoringWriteState. Done. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1118: // The caller should have the best connection connected to the peer reflexive On 2016/06/21 18:33:26, Taylor Brandstetter wrote: > s/best connection/selected connnection/g Done. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2512: TEST_F(P2PTransportChannelPingTest, TestDonotSwitchConnectionTooFrequently) { On 2016/06/21 18:33:26, Taylor Brandstetter wrote: > Rename test to ControlledAgentDoesntSwitchConnectionTooFrequently Done. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2573: } On 2016/06/21 18:33:26, Taylor Brandstetter wrote: > This test's expectations are good but it's testing way too many things. I think > there should be separate tests for: > > 1. First nominated connection is selected. > 2. Higher priority nominated connection is selected. > 3. Connection on which data was last received is selected. > 4. Connection is selected once it becomes writable. I break this into three, making each of them smaller. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:961: // because receiving data will increase its priority in the pruning process. On 2016/06/22 06:36:34, pthatcher1 wrote: > I don't understand this comment. Revised. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:965: ResetWriteStateAndUnprune(); On 2016/06/22 06:36:34, pthatcher1 wrote: > So the change is to unprune when we received media on a nominated pair? Why > would we need to do that? Is that just so that we unprune backup candidate > pairs on the controlled side when the controlling side switches to one? If so, > is this just here until we're fully switched over to "renomination"? Also, can > the comment reflect that such logic is only for backup candidate pairs of > aggressively controlling peers? Finally, why did we not need this logic before? > I think there is a bug in the existing code that sometimes a connection used by the controlling side may be pruned before it is selected by the controlled side. Consider the case that two connections having the same candidate priority. Whichever first becomes writable will make the other one pruned in the controlled side but the other one may be the one chosen by the controlling side. Yes we do have the logic on the controlled side that it only start to prune only if it selected connection is nominated but the condition may not be so much useful because with aggressive nomination, it nominated every connection before the best connection becomes writable. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1055: if (write_state_ == STATE_WRITE_TIMEOUT && (just_nominated || !pruned_)) { On 2016/06/21 18:33:26, Taylor Brandstetter wrote: > Why is the connection only restored if it's "just_nominated"? The controlling > agent could switch connections back and forth over the course of a session due > to RTT, right? I think you are right. I may have over-thought this. https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1055: if (write_state_ == STATE_WRITE_TIMEOUT && (just_nominated || !pruned_)) { On 2016/06/22 06:36:34, pthatcher1 wrote: > On 2016/06/21 18:33:26, Taylor Brandstetter wrote: > > Why is the connection only restored if it's "just_nominated"? The controlling > > agent could switch connections back and forth over the course of a session due > > to RTT, right? > > Yeah, I don't understand the "just nominated" logic either. If we don't prune > until a candidate pair is nominated and writable, then the fact that the > candidate pair was pruned means a better candidate pair was writable and > nominated. So why would be unprune this candidate pair, which we know to be > worse? It seems contrary to the concept of pruning when the controlling side is > aggressive. We're just going to prune it again once the better candidate pair > is nominated again. See my reply in the above comments.
lgtm https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1174: (CompareConnectionsBase(premier, conn) >= 0)) { On 2016/06/22 06:36:33, pthatcher1 wrote: > On 2016/06/21 18:33:25, Taylor Brandstetter wrote: > > Why can't you just use CompareConnections here? Isn't |premier| guaranteed to > > have a better or equal write state than |conn|, since connections are always > > sorted by the time this function is called? > > I think this is so we don't prune a better candidate pair just because it's not > writable yet. Otherwise, the first writable candidate pair would always prune > of all the other candidate pairs, even if we'd rather have one of the others > once they become writable. Ah yes, that's obvious now, my mistake. https://codereview.webrtc.org/2069493002/diff/260001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2069493002/diff/260001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2901: EXPECT_TRUE(!conn2->pruned()); EXPECT_FALSE?
https://codereview.webrtc.org/2069493002/diff/260001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2069493002/diff/260001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2901: EXPECT_TRUE(!conn2->pruned()); On 2016/06/22 16:12:23, Taylor Brandstetter wrote: > EXPECT_FALSE? Done.
It all looks good, except I still don't understand or don't like (or both) the unpruning behavior in port.cc. I don't understand why we need it, and I don't understand how it would work well with aggressive nomination. https://codereview.webrtc.org/2069493002/diff/280001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/280001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:251: } Can you make this match the style Taylor had in his CL, like this? if (a->nominated() && !b->nominated()) { return a_is_better; } if (!a->nominated() && b->nominated()) { return b_is_better; } https://codereview.webrtc.org/2069493002/diff/280001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2069493002/diff/280001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:962: // been nominated by the controlling side. Perhaps the wording could be thus: If writability timed out, it could be either because the connection was pruned or because checks are failing. If it's the later (not pruned), then reset the connection state to the INIT state since clearly we can receive data on this connection. If it's the former (pruned) and the connection was nominated, do the same, but also unprune the connection. https://codereview.webrtc.org/2069493002/diff/280001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:966: ResetWriteStateAndUnprune(); Even though I corrected the comment, I don't understand why this is the behavior that we want. With aggressive nomination *all* of the connections will be nominated, which means reset the write state for *all* connections that receive media. So basically the rule would be "unprune if we receive media from the controlling side". Do we really want that? If so, can we save it for a future CL? I'd rather not block this CL on trying to get this part right. And even if we do want this behavior, it would be more clear as this: if (pruned_ && ice_role_ == ICE_CONTROLLED) { // If controlling side is sending on pruned candidate pair; we should probably unprune. Unprune(); } if (!pruned_ && (write_state_ == STATE_WRITE_TIMEOUT)) { set_write_state(STATE_WRITE_INIT); } But even then, what prevents the candidate pair from getting re-pruned shortly after this? https://codereview.webrtc.org/2069493002/diff/280001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1043: bool nominated = msg->GetByteString(STUN_ATTR_USE_CANDIDATE) != nullptr; You are making the variable, so the outer |nominated| will always be false. https://codereview.webrtc.org/2069493002/diff/280001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1054: } When the controlling side is aggressive *all* the candidate pairs will be nominated, so we'd basically be unpruning any candidate pairs that the controlling is pinging, which means we never prune until we stop getting pings. Again, this change doesn't seem necessary for this CL, so could we leave it out and discuss it later? Also, with the current code, nomianted is always false, so it's never triggered anyway. And it's still passing the unit tests :).
Removed the part for un-pruning. Leave it at a separate CL. https://codereview.webrtc.org/2069493002/diff/280001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/280001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:251: } On 2016/06/22 19:18:37, pthatcher1 wrote: > Can you make this match the style Taylor had in his CL, like this? > > if (a->nominated() && !b->nominated()) { > return a_is_better; > } > if (!a->nominated() && b->nominated()) { > return b_is_better; > } We should do this when we merge the two CLs, so we don't have to duplicate the work now. If it is not fixed after merging, will do this in a separate CL. https://codereview.webrtc.org/2069493002/diff/280001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2069493002/diff/280001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:962: // been nominated by the controlling side. On 2016/06/22 19:18:37, pthatcher1 wrote: > Perhaps the wording could be thus: > > If writability timed out, it could be either because the connection was pruned > or because checks are failing. If it's the later (not pruned), then reset the > connection state to the INIT state since clearly we can receive data on this > connection. If it's the former (pruned) and the connection was nominated, do > the same, but also unprune the connection. All changes here are removed. Acknowledged. https://codereview.webrtc.org/2069493002/diff/280001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:966: ResetWriteStateAndUnprune(); On 2016/06/22 19:18:37, pthatcher1 wrote: > Even though I corrected the comment, I don't understand why this is the behavior > that we want. With aggressive nomination *all* of the connections will be > nominated, which means reset the write state for *all* connections that receive > media. So basically the rule would be "unprune if we receive media from the > controlling side". Do we really want that? If so, can we save it for a future > CL? I'd rather not block this CL on trying to get this part right. > > And even if we do want this behavior, it would be more clear as this: > if (pruned_ && ice_role_ == ICE_CONTROLLED) { > // If controlling side is sending on pruned candidate pair; we should > probably unprune. > Unprune(); > } > if (!pruned_ && (write_state_ == STATE_WRITE_TIMEOUT)) { > set_write_state(STATE_WRITE_INIT); > } > > > But even then, what prevents the candidate pair from getting re-pruned shortly > after this? Acknowledged. https://codereview.webrtc.org/2069493002/diff/280001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1043: bool nominated = msg->GetByteString(STUN_ATTR_USE_CANDIDATE) != nullptr; On 2016/06/22 19:18:37, pthatcher1 wrote: > You are making the variable, so the outer |nominated| will always be false. Sorry for the mistake. That means the Unprune is indeed not so much useful (the issue will still happen in some corner cases). https://codereview.webrtc.org/2069493002/diff/280001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1054: } On 2016/06/22 19:18:37, pthatcher1 wrote: > When the controlling side is aggressive *all* the candidate pairs will be > nominated, so we'd basically be unpruning any candidate pairs that the > controlling is pinging, which means we never prune until we stop getting pings. > Again, this change doesn't seem necessary for this CL, so could we leave it out > and discuss it later? > > Also, with the current code, nomianted is always false, so it's never triggered > anyway. And it's still passing the unit tests :). Acknowledged.
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 Link to the patchset: https://codereview.webrtc.org/2069493002/#ps320001 (title: "Changed a comment about pruning.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2069493002/320001
The CQ bit was unchecked by honghaiz@webrtc.org
Use a_is_better, b_is_better to replace 1 and -1 for better readability.
lgtm https://codereview.webrtc.org/2069493002/diff/340001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/340001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:262: } These could use the a_is_better, b_is_better.
https://codereview.webrtc.org/2069493002/diff/340001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2069493002/diff/340001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:262: } On 2016/06/22 22:32:32, pthatcher1 wrote: > These could use the a_is_better, b_is_better. Done.
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/2069493002/#ps420001 (title: "Merge branch 'master' into nominate_on_controlled_side")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2069493002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was checked by honghaiz@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2069493002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
Message was sent while issue was closed.
Description was changed from ========== Do not switch best connection on the controlled side too frequently due to the nomination from the controlling side. We now use a single rule to determine connection switch on the controlled side. The rule is to select the new best connection based on the following order: 1. writable/receiving/connected state. 2. nominated 3. last time receiving data packet. 4. priority. 5. latency (rtt) BUG= ========== to ========== Do not switch best connection on the controlled side too frequently due to the nomination from the controlling side. We now use a single rule to determine connection switch on the controlled side. The rule is to select the new best connection based on the following order: 1. writable/receiving/connected state. 2. nominated 3. last time receiving data packet. 4. priority. 5. latency (rtt) BUG= R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/572b094128c4def38dd9f21a4df9e1dd72787fe9 Cr-Commit-Position: refs/heads/master@{#13274} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/572b094128c4def38dd9f21a4df9e1dd72787fe9 Cr-Commit-Position: refs/heads/master@{#13274} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
