Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1577233006:
Implement Turn/Turn first logic for connection selection. (Closed)
|
Created:
4 years, 11 months ago by guoweis_webrtc Modified:
4 years, 9 months ago Reviewers:
pthatcher1 CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionImplement Turn/Turn first logic for connection selection.
This feature is off by default and can be turned on by setting IceConfig. When turned on, we'll choose a Turn/Turn (UDP takes higher priroity) over the other types of connections while no any connection is writable. However, when there is best connection or there is pending triggered check, those will take higher priority.
BUG=webrtc:4591
Committed: https://crrev.com/36f0137fd51e3a557087c3332dd47dcd5956ae4a
Cr-Commit-Position: refs/heads/master@{#11850}
Patch Set 1 #Patch Set 2 : remove unneeded change. #Patch Set 3 : fix a bug #
Total comments: 14
Patch Set 4 : create Connections class while maintaining the funcitonality. #Patch Set 5 : Add the turn/turn first functionality #Patch Set 6 : finish the prioritization of udp turn/turn over tcp turn/turn #Patch Set 7 : Fix an issue when none of the connections are pinged yet. #Patch Set 8 : Fix a comment. #Patch Set 9 : Fix test issues #
Total comments: 46
Patch Set 10 : address comments. #Patch Set 11 : missed to rename a var #
Total comments: 10
Patch Set 12 : address comments. #Patch Set 13 : fix a typo #Patch Set 14 : Fix test cases #
Messages
Total messages: 55 (29 generated)
Description was changed from ========== Add Turn/Turn first test cases. BUG= ========== to ========== Add Turn/Turn first test cases. BUG=webrtc:4591 ==========
Description was changed from ========== Add Turn/Turn first test cases. BUG=webrtc:4591 ========== to ========== Add Turn/Turn first test cases. Now they are all disabled. The implementation in the code will follow. BUG=webrtc:4591 ==========
guoweis@webrtc.org changed reviewers: + pthatcher@webrtc.org
PTAL.
These are very well written. Mostly minor things. https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2419: // Test that Relay/Relay candidate pair will be pinged when no any other pairs Please use either "candidate pair" or "connection" everywhere (be consistent). I'd prefer "candidate pair", but "connection" is more consistent with the rest of the code. https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2419: // Test that Relay/Relay candidate pair will be pinged when no any other pairs no any other => no other https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2465: // been pinged even if the connection didn't form the earliest. if the connection didn't form the earliest => even if the Relay/Relay connection wasn't the first to be pinged in the first round. https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2489: // Relay/Relay should be the first since it's not pinged before. it's not => it hasn't been https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2530: // Local/Local should be the first since it's not pinged before. it's not => it hasn't been https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2544: // Test the ping sequence is UDP Relay/Relay followed by TCP Relay/Relay, Hmm... that's a good question. Do we really want TCP Relay/Relay before other UDP pairs? I'd like to hear what Justin thinks about that. https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2565: ch.AddRemoteCandidate(CreateRelayCandidate("1.1.1.1", 1, 1)); Can you add a non-relay candidate also, to be sure we don't prioritize host->host, etc over relay->relay?
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
PTAL. https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2419: // Test that Relay/Relay candidate pair will be pinged when no any other pairs On 2016/01/14 22:40:01, pthatcher1 wrote: > Please use either "candidate pair" or "connection" everywhere (be consistent). > I'd prefer "candidate pair", but "connection" is more consistent with the rest > of the code. Done. https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2419: // Test that Relay/Relay candidate pair will be pinged when no any other pairs On 2016/01/14 22:40:01, pthatcher1 wrote: > no any other => no other Done. https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2465: // been pinged even if the connection didn't form the earliest. On 2016/01/14 22:40:01, pthatcher1 wrote: > if the connection didn't form the earliest => even if the Relay/Relay connection > wasn't the first to be pinged in the first round. Done. https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2489: // Relay/Relay should be the first since it's not pinged before. On 2016/01/14 22:40:01, pthatcher1 wrote: > it's not => it hasn't been Done. https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2530: // Local/Local should be the first since it's not pinged before. On 2016/01/14 22:40:01, pthatcher1 wrote: > it's not => it hasn't been Done. https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2565: ch.AddRemoteCandidate(CreateRelayCandidate("1.1.1.1", 1, 1)); On 2016/01/14 22:40:01, pthatcher1 wrote: > Can you add a non-relay candidate also, to be sure we don't prioritize > host->host, etc over relay->relay? This was tested in the first test case, isn't it?
Description was changed from ========== Add Turn/Turn first test cases. Now they are all disabled. The implementation in the code will follow. BUG=webrtc:4591 ========== to ========== Implement Turn/Turn first logic for connection selection. This feature is off by default and can be turned on by setting IceConfig. When turned on, we'll choose a Turn/Turn (UDP takes higher priroity) over the other types of connections while no any connection is writable. However, when there is best connection or there is pending triggered check, those will take higher priority. BUG=webrtc:4591 ==========
lgtm https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2565: ch.AddRemoteCandidate(CreateRelayCandidate("1.1.1.1", 1, 1)); On 2016/01/15 17:47:52, guoweis_webrtc wrote: > On 2016/01/14 22:40:01, pthatcher1 wrote: > > Can you add a non-relay candidate also, to be sure we don't prioritize > > host->host, etc over relay->relay? > > This was tested in the first test case, isn't it? Yes, good point.
un-lgtm? Whoops, I hit the wrong button. I meant to wait until the code is written :). On 2016/01/15 20:10:34, pthatcher1 wrote: > lgtm > > https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... > File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): > > https://codereview.webrtc.org/1577233006/diff/40001/webrtc/p2p/base/p2ptransp... > webrtc/p2p/base/p2ptransportchannel_unittest.cc:2565: > ch.AddRemoteCandidate(CreateRelayCandidate("1.1.1.1", 1, 1)); > On 2016/01/15 17:47:52, guoweis_webrtc wrote: > > On 2016/01/14 22:40:01, pthatcher1 wrote: > > > Can you add a non-relay candidate also, to be sure we don't prioritize > > > host->host, etc over relay->relay? > > > > This was tested in the first test case, isn't it? > > Yes, good point.
The CQ bit was checked by guoweis@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1577233006/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1577233006/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/12098)
The CQ bit was checked by guoweis@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1577233006/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1577233006/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/12026)
On 2016/01/22 00:30:38, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > linux_asan on tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/12026) Ping.
https://codereview.webrtc.org/1577233006/diff/200001/talk/app/webrtc/peerconn... File talk/app/webrtc/peerconnectioninterface.h (right): https://codereview.webrtc.org/1577233006/diff/200001/talk/app/webrtc/peerconn... talk/app/webrtc/peerconnectioninterface.h:262: bool ping_most_likely_candidate_pair_first; I better name might be prioritize_most_likely_ice_candidate_pairs_. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:402: << connections_.most_likely_first(); This might be better named prioritize_most_likely_to_work https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:411: return config; Instead of converting between the struct and the member variables, can we just store a struct internally as the member variable? https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1274: } Why is this needed? Can't connection_ just handle it? https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1280: connections_.MarkConnectionPinged(best_connection_); I don't think FindNextPingableConnection should do the logic of causing the candidate pair to be marked pinged. It should only be marked pinged when the ping is actually sent. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1431: : channel_(channel) {} I don't like how the Connections class depends on the TC and the TC depends on the Connections. It just seems too complex. I looked though how the Connections class uses TC, and it looks like everything comes down to best_connection_ (and maybe state_ == completed, but that's derived from connections_ and best_connection_). So, I think we should either pass in best_connection_ to FindNextPingableConnection or make it possible to keep FindNextPingableConnection in TC and expose whatever needs to be exposed from Connections. Or, we could just have TC store both pinged_connections_ and unpinged_connections_, and then have a connections() method that returns an iterator that iterates over both. That way, we just have to implement an iterator that can cover both rather than a whole collection class. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1463: pinged_connections_.clear(); Shouldn't this happen after MarkConnectionPinged results in an empty unpinged_connections_? https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1484: } I think having a separate FindOldestConnectionNeedingTriggeredCheck would be nice. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1489: MarkConnectionPinged(oldest_needing_triggered_check); Again, I don't like that just finding it marks it pinged. It should only be marked pinged once it's actually pinged. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1521: bool conn1_turn_turn = IsTurnTurn(conn1); rr1 would be nicer, shorter name. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1526: if (conn1_turn_turn && conn2_turn_turn) { This would be more readable as: bool rr1 == IsRelayRelay(conn1); bool rr2 == IsRelayRelay(conn2); if (rr1 && !rr2) { return conn1; } else if (rr2 && !rr1) { return conn2; } else if (rr1 && rr2) { bool udp1 = IsUdp(conn1); bool udp2 = IsUdp(conn2); if (udp1 && !udp2) { return conn1; } else if (udp2 && udp1) { return conn2; } } And, actually, it would be nice to put that in a separate MostLikelyToWork method. if (prioritize_most_likely_to_work_) { Connection* most_likely_to_work_conn = MostLikelyToWork(conn1, conn2); if (most_likely_to_work_conn) { return most_likely_to_work_conn; } } https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1542: } And similarly, we could make a method for LeastRecentlyPinged: Conneciton* conn = LeastRecentlyPinged(conn1, conn2); if (conn) { return conn; } https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1552: bool P2PTransportChannel::Connections::IsTurnTurn(Connection* conn) { IsRelayToRelay would be a more accurate name. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:193: class Connections : public std::vector<Connection*> { This seems a little crazy. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1770: cricket::Candidate CreateLocalCandidate(const std::string& ip, We ought to call this CreateHostCandidate. LOCAL_PORT_TYPE should be renamed to CANDIDATE_TYPE_HOST. But that's for another day. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2364: class P2PTransportChannelTurnTurnFirstTest Can you call this P2PTransportChannelMostLikelyToWorkFirstTest? https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2384: cricket::P2PTransportChannel& StartChannel() { Can we call this StartTransportChannel()? And it's not clear from the name that it's prioritizing most likely to work candidate pairs. Would it make sense to add that to the name, or to pass in a variable? https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2430: // Test that Relay/Relay connection will be pinged when no other connections Can you say "pinged first" instead of just "pinged", like you do in the other comments? https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2430: // Test that Relay/Relay connection will be pinged when no other connections Relay/Relay connection => Relay/Relay connections Here and in other comments like this. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2434: TestTurnTurnFirstWhenNothingPingedYet) { The comment says Relay/Relay, but the variable name says Turn/Turn. Can we pick one? I think RelayRelay would be slightly better. Here and in other places. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2469: WAIT(false, cricket::MAX_CURRENT_STRONG_DELAY + 100); We have to wait 1 second for each of these tests? I'm not a fan of that. I'd prefer instead to put the delay into IceConfig and make it shorter for the unit tests (50ms?). Then the default could be the constant. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2513: // before we re-ping Relay/Relay connection. connection => connections again https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/transport.h File webrtc/p2p/base/transport.h (right): https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/transpor... webrtc/p2p/base/transport.h:150: bool ping_most_likely_candidate_pair_first = false; And here, the name could be the same but without "ice" in it: prioritize_most_likely_candidate_pairs
Patchset #10 (id:220001) has been deleted
Patchset #11 (id:260001) has been deleted
Patchset #11 (id:280001) has been deleted
Patchset #11 (id:300001) has been deleted
Patchset #10 (id:240001) has been deleted
PTAL. https://codereview.webrtc.org/1577233006/diff/200001/talk/app/webrtc/peerconn... File talk/app/webrtc/peerconnectioninterface.h (right): https://codereview.webrtc.org/1577233006/diff/200001/talk/app/webrtc/peerconn... talk/app/webrtc/peerconnectioninterface.h:262: bool ping_most_likely_candidate_pair_first; On 2016/01/27 19:59:58, pthatcher1 wrote: > I better name might be prioritize_most_likely_ice_candidate_pairs_. Done. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:402: << connections_.most_likely_first(); On 2016/01/27 19:59:58, pthatcher1 wrote: > This might be better named prioritize_most_likely_to_work Done. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:411: return config; On 2016/01/27 19:59:58, pthatcher1 wrote: > Instead of converting between the struct and the member variables, can we just > store a struct internally as the member variable? Done. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1274: } On 2016/01/27 19:59:58, pthatcher1 wrote: > Why is this needed? Can't connection_ just handle it? Done. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1280: connections_.MarkConnectionPinged(best_connection_); On 2016/01/27 19:59:58, pthatcher1 wrote: > I don't think FindNextPingableConnection should do the logic of causing the > candidate pair to be marked pinged. It should only be marked pinged when the > ping is actually sent. Removed this function. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1431: : channel_(channel) {} On 2016/01/27 19:59:58, pthatcher1 wrote: > I don't like how the Connections class depends on the TC and the TC depends on > the Connections. It just seems too complex. I looked though how the > Connections class uses TC, and it looks like everything comes down to > best_connection_ (and maybe state_ == completed, but that's derived from > connections_ and best_connection_). > > So, I think we should either pass in best_connection_ to > FindNextPingableConnection or make it possible to keep > FindNextPingableConnection in TC and expose whatever needs to be exposed from > Connections. > > Or, we could just have TC store both pinged_connections_ and > unpinged_connections_, and then have a connections() method that returns an > iterator that iterates over both. That way, we just have to implement an > iterator that can cover both rather than a whole collection class. Agree. Removed Connections and move these 2 to TC. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1463: pinged_connections_.clear(); On 2016/01/27 19:59:58, pthatcher1 wrote: > Shouldn't this happen after MarkConnectionPinged results in an empty > unpinged_connections_? If the unpinged_connections has only unpingable connections, it's virtually empty. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1484: } On 2016/01/27 19:59:58, pthatcher1 wrote: > I think having a separate FindOldestConnectionNeedingTriggeredCheck would be > nice. Done. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1489: MarkConnectionPinged(oldest_needing_triggered_check); On 2016/01/27 19:59:58, pthatcher1 wrote: > Again, I don't like that just finding it marks it pinged. It should only be > marked pinged once it's actually pinged. Removed. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1521: bool conn1_turn_turn = IsTurnTurn(conn1); On 2016/01/27 19:59:58, pthatcher1 wrote: > rr1 would be nicer, shorter name. Done. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1526: if (conn1_turn_turn && conn2_turn_turn) { On 2016/01/27 19:59:58, pthatcher1 wrote: > This would be more readable as: > > bool rr1 == IsRelayRelay(conn1); > bool rr2 == IsRelayRelay(conn2); > if (rr1 && !rr2) { > return conn1; > } else if (rr2 && !rr1) { > return conn2; > } else if (rr1 && rr2) { > bool udp1 = IsUdp(conn1); > bool udp2 = IsUdp(conn2); > if (udp1 && !udp2) { > return conn1; > } else if (udp2 && udp1) { > return conn2; > } > } > > And, actually, it would be nice to put that in a separate MostLikelyToWork > method. > > if (prioritize_most_likely_to_work_) { > Connection* most_likely_to_work_conn = MostLikelyToWork(conn1, conn2); > if (most_likely_to_work_conn) { > return most_likely_to_work_conn; > } > } > > > Done. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1542: } On 2016/01/27 19:59:58, pthatcher1 wrote: > And similarly, we could make a method for LeastRecentlyPinged: > > Conneciton* conn = LeastRecentlyPinged(conn1, conn2); > if (conn) { > return conn; > } Done. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1552: bool P2PTransportChannel::Connections::IsTurnTurn(Connection* conn) { On 2016/01/27 19:59:58, pthatcher1 wrote: > IsRelayToRelay would be a more accurate name. Done. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:193: class Connections : public std::vector<Connection*> { On 2016/01/27 19:59:58, pthatcher1 wrote: > This seems a little crazy. Removed the class. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1770: cricket::Candidate CreateLocalCandidate(const std::string& ip, On 2016/01/27 19:59:59, pthatcher1 wrote: > We ought to call this CreateHostCandidate. LOCAL_PORT_TYPE should be renamed to > CANDIDATE_TYPE_HOST. But that's for another day. Done. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2364: class P2PTransportChannelTurnTurnFirstTest On 2016/01/27 19:59:59, pthatcher1 wrote: > Can you call this P2PTransportChannelMostLikelyToWorkFirstTest? Done. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2384: cricket::P2PTransportChannel& StartChannel() { On 2016/01/27 19:59:59, pthatcher1 wrote: > Can we call this StartTransportChannel()? > > And it's not clear from the name that it's prioritizing most likely to work > candidate pairs. Would it make sense to add that to the name, or to pass in a > variable? Done. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2430: // Test that Relay/Relay connection will be pinged when no other connections On 2016/01/27 19:59:59, pthatcher1 wrote: > Relay/Relay connection => Relay/Relay connections > > Here and in other comments like this. Done. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2430: // Test that Relay/Relay connection will be pinged when no other connections On 2016/01/27 19:59:59, pthatcher1 wrote: > Can you say "pinged first" instead of just "pinged", like you do in the other > comments? Done. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2434: TestTurnTurnFirstWhenNothingPingedYet) { On 2016/01/27 19:59:59, pthatcher1 wrote: > The comment says Relay/Relay, but the variable name says Turn/Turn. Can we pick > one? I think RelayRelay would be slightly better. Here and in other places. Done. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2469: WAIT(false, cricket::MAX_CURRENT_STRONG_DELAY + 100); On 2016/01/27 19:59:59, pthatcher1 wrote: > We have to wait 1 second for each of these tests? I'm not a fan of that. I'd > prefer instead to put the delay into IceConfig and make it shorter for the unit > tests (50ms?). Then the default could be the constant. Done. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2513: // before we re-ping Relay/Relay connection. On 2016/01/27 19:59:59, pthatcher1 wrote: > connection => connections again Done. https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/transport.h File webrtc/p2p/base/transport.h (right): https://codereview.webrtc.org/1577233006/diff/200001/webrtc/p2p/base/transpor... webrtc/p2p/base/transport.h:150: bool ping_most_likely_candidate_pair_first = false; On 2016/01/27 19:59:59, pthatcher1 wrote: > And here, the name could be the same but without "ice" in it: > prioritize_most_likely_candidate_pairs Done.
The CQ bit was checked by guoweis@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1577233006/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1577233006/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Mostly style, but I think we should not have FindNext mutate things. That's probably the only major thing left. Thanks for doing this. It looks much better without the subclass of std::vector. https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:252: false, Can you put a comment of what these are so it's easier to read? https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:441: const IceConfig& P2PTransportChannel::GetIceConfig() const { Can you make this just config()? https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1317: unpinged_connections_.erase(conn_to_ping); I think it would be more correct to move this to OnCheckAndPing: Connection* conn = FindNextPingableConnection(); if (conn) { PingConnection(conn); if (conn && pinged_connections_.insert(conn_to_ping).second) { unpinged_connections_.erase(conn_to_ping); } } In other words, FindNext shouldn't change the pinged state, Ping should. https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1525: } I think this should also happen in OnCheckAndPing. Whenever we move something out of unpinged_connections_, then check if it's "empty". https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1575: Connection* P2PTransportChannel::SelectMorePingableConnection( To keep up with your naming conventions, this should be MostPingable
On 2016/03/02 20:54:36, pthatcher1 wrote: > Mostly style, but I think we should not have FindNext mutate things. That's > probably the only major thing left. > > Thanks for doing this. It looks much better without the subclass of > std::vector. > > https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... > File webrtc/p2p/base/p2ptransportchannel.cc (right): > > https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:252: false, > Can you put a comment of what these are so it's easier to read? > > https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:441: const IceConfig& > P2PTransportChannel::GetIceConfig() const { > Can you make this just config()? > > https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:1317: > unpinged_connections_.erase(conn_to_ping); > I think it would be more correct to move this to OnCheckAndPing: > > Connection* conn = FindNextPingableConnection(); > if (conn) { > PingConnection(conn); > if (conn && pinged_connections_.insert(conn_to_ping).second) { > unpinged_connections_.erase(conn_to_ping); > } > } > > In other words, FindNext shouldn't change the pinged state, Ping should. > > https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:1525: } > I think this should also happen in OnCheckAndPing. Whenever we move something > out of unpinged_connections_, then check if it's "empty". > > https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:1575: Connection* > P2PTransportChannel::SelectMorePingableConnection( > To keep up with your naming conventions, this should be MostPingable PTAL.
https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:252: false, On 2016/03/02 20:54:36, pthatcher1 wrote: > Can you put a comment of what these are so it's easier to read? Done. https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:441: const IceConfig& P2PTransportChannel::GetIceConfig() const { On 2016/03/02 20:54:36, pthatcher1 wrote: > Can you make this just config()? Done. https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1317: unpinged_connections_.erase(conn_to_ping); On 2016/03/02 20:54:36, pthatcher1 wrote: > I think it would be more correct to move this to OnCheckAndPing: > > Connection* conn = FindNextPingableConnection(); > if (conn) { > PingConnection(conn); > if (conn && pinged_connections_.insert(conn_to_ping).second) { > unpinged_connections_.erase(conn_to_ping); > } > } > > In other words, FindNext shouldn't change the pinged state, Ping should. > Done. https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1525: } On 2016/03/02 20:54:36, pthatcher1 wrote: > I think this should also happen in OnCheckAndPing. Whenever we move something > out of unpinged_connections_, then check if it's "empty". I put it here on purpose such that if we get a new connection after we ping the last existing one, the new one will have high priority. https://codereview.webrtc.org/1577233006/diff/340001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1575: Connection* P2PTransportChannel::SelectMorePingableConnection( On 2016/03/02 20:54:36, pthatcher1 wrote: > To keep up with your naming conventions, this should be MostPingable Done.
lgtm
The CQ bit was checked by guoweis@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1577233006/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1577233006/360001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by guoweis@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1577233006/#ps380001 (title: "fix a typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1577233006/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1577233006/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_ubsan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/175) linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) win_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/13160) win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/12890)
Patchset #14 (id:400001) has been deleted
The CQ bit was checked by guoweis@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1577233006/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1577233006/420001
The CQ bit was unchecked by guoweis@webrtc.org
The CQ bit was checked by guoweis@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1577233006/#ps420001 (title: "Fix test cases")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1577233006/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1577233006/420001
Message was sent while issue was closed.
Description was changed from ========== Implement Turn/Turn first logic for connection selection. This feature is off by default and can be turned on by setting IceConfig. When turned on, we'll choose a Turn/Turn (UDP takes higher priroity) over the other types of connections while no any connection is writable. However, when there is best connection or there is pending triggered check, those will take higher priority. BUG=webrtc:4591 ========== to ========== Implement Turn/Turn first logic for connection selection. This feature is off by default and can be turned on by setting IceConfig. When turned on, we'll choose a Turn/Turn (UDP takes higher priroity) over the other types of connections while no any connection is writable. However, when there is best connection or there is pending triggered check, those will take higher priority. BUG=webrtc:4591 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== Implement Turn/Turn first logic for connection selection. This feature is off by default and can be turned on by setting IceConfig. When turned on, we'll choose a Turn/Turn (UDP takes higher priroity) over the other types of connections while no any connection is writable. However, when there is best connection or there is pending triggered check, those will take higher priority. BUG=webrtc:4591 ========== to ========== Implement Turn/Turn first logic for connection selection. This feature is off by default and can be turned on by setting IceConfig. When turned on, we'll choose a Turn/Turn (UDP takes higher priroity) over the other types of connections while no any connection is writable. However, when there is best connection or there is pending triggered check, those will take higher priority. BUG=webrtc:4591 Committed: https://crrev.com/36f0137fd51e3a557087c3332dd47dcd5956ae4a Cr-Commit-Position: refs/heads/master@{#11850} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/36f0137fd51e3a557087c3332dd47dcd5956ae4a Cr-Commit-Position: refs/heads/master@{#11850} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
