Reset the BWE when the network changes

Reset the BWE when the network changes.
Currently "Resetting the BWE" does nothing yet. This CL passes the correct signaling to the bandwidth estimator.

BUG=
R=pthatcher@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/cc411c0599b038cf3f2cc027482bedc19e066166

[honghaiz3, 2016-03-16 20:08:05]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2016-03-16 20:16:14]

Patchset #1 (id:20001) has been deleted

[honghaiz3, 2016-03-16 20:21:48]

honghaiz@webrtc.org changed reviewers:
+ holmer@google.com, pthatcher@webrtc.org

[honghaiz3, 2016-03-16 20:21:48]

This CL only resets the BWEstimate when local network changes, although it
leaves space for remote network changes. 

Peter, I use the signal name NetworkChanged instead of NetworkIdsChanged as we
discussed as I realize that Network is equivalent to Network id in our
implementation. Let me know if you still prefer the name NetworkIdsChanged. 

I will add a few tests in parallel.

[pthatcher1, 2016-03-16 21:22:03]

This looks like a good start.  I think we could change it a little bit to make
it more flexible for future additions.

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/base/network.h#new...
webrtc/base/network.h:343: int id() const { return
static_cast<int>(std::hash<std::string>()(key_)); }
We should probably make this a uint16_t or uint32_t to make it clear what the
range is.  We can't have this thing return a value outside the range that the
STUN attribute could support. I think I'd prefer uint16_t so we could have it
fit into the same STUN attribute as network-cost.

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/call.h#newcode138
webrtc/call.h:138: virtual void OnNetworkChanged(int local_net_id, int
remote_net_id) = 0;
To save ourselves trouble later when we want to add more information here (such
as 2g/3g/4g/wifi, or other info), I think it would make sense to pass a struct. 
Perhaps something like:

struct NetworkRoute {
  // False if there is no route currently.
  bool connected;
  // Unset if either there is no route (in which case !connected)
  // or the network ID is unknown.
  Optional<int> local_network_id;
  Optional<int> remote_network_id;
}

And then have could include old and new and save the handler from storing the
old:

OnNetworkRouteChanged(const NetworkRoute& old_route, const NetworkRoute&
new_route);

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:173: int remote_network_id_ = 0;
If the signal has both old and new route infos, we don't have to store these.

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:583: remote_net_id == remote_network_id_) {
If we use NetworkRoute structs, we should also check if they are connected or
not.

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:593: void Call::ResetBweBitrates() {
If !new_route.connected, we probably want to do something more drastic with the
BWE.  I'm not sure what, though, so perhaps just a TODO for now.

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:599: config_.bitrate_config.max_bitrate_bps);
Does this really restart the BWE?

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

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1162: }
I think we could use a helper method called GetNetworkRoute(const Connection*
conn) like this:

NetworkRoute route;
if (!conn) {
  route.connected = false;
} else {
  route.local_network_id = new Optional<int>(conn->port()->Network()->id());
  // TODO: Signal remote network ID.
  // route.remote_net_id = conn->remote_candidate->network_id();
}
return route;


And then we can have the code here be:

NetworkRoute old_route = GetNetworkRoute(old_conn);
NetworkRoute new_route = GetNetworkRoute(new_conn);
if (old_route != new_route) {
  SignalNetworkRouteChanged(this, old_route, new_route);
}

Of course, we need to make sure to implement operator== and operator!=

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/p2p/quic/quictrans...
File webrtc/p2p/quic/quictransportchannel.h (right):

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/p2p/quic/quictrans...
webrtc/p2p/quic/quictransportchannel.h:227: void OnRouteChange(TransportChannel*
channel, const Candidate& candidate);
Can you mark this one as deprecated and something we will remove as soon as
chromoting isn't using it?

[honghaiz3, 2016-03-17 02:45:04]

Patchset #2 (id:60001) has been deleted

[honghaiz3, 2016-03-17 03:38:27]

PTAL. It is a little different from what we discussed.
As I realize we can use a null pointer instead of optional for null value. 
Once it is a pointer, then we can use Connection instead of creating a new
CandidatePair struct because  it will not matter that Connection may contain
some unnecessary information that may not be needed. Plus, Connection should
contain all possible information we will need.
WDYT?

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/base/network.h#new...
webrtc/base/network.h:343: int id() const { return
static_cast<int>(std::hash<std::string>()(key_)); }
On 2016/03/16 21:22:02, pthatcher1 wrote:
> We should probably make this a uint16_t or uint32_t to make it clear what the
> range is.  We can't have this thing return a value outside the range that the
> STUN attribute could support. I think I'd prefer uint16_t so we could have it
> fit into the same STUN attribute as network-cost.

Done.

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/call.h#newcode138
webrtc/call.h:138: virtual void OnNetworkChanged(int local_net_id, int
remote_net_id) = 0;
On 2016/03/16 21:22:03, pthatcher1 wrote:
> To save ourselves trouble later when we want to add more information here
(such
> as 2g/3g/4g/wifi, or other info), I think it would make sense to pass a
struct. 
> Perhaps something like:
> 
> struct NetworkRoute {
>   // False if there is no route currently.
>   bool connected;
>   // Unset if either there is no route (in which case !connected)
>   // or the network ID is unknown.
>   Optional<int> local_network_id;
>   Optional<int> remote_network_id;
> }
> 
> And then have could include old and new and save the handler from storing the
> old:
> 
> OnNetworkRouteChanged(const NetworkRoute& old_route, const NetworkRoute&
> new_route);
I now just pass the Connection (CandidatePair), which should include every
information we need.

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:173: int remote_network_id_ = 0;
On 2016/03/16 21:22:03, pthatcher1 wrote:
> If the signal has both old and new route infos, we don't have to store these.

Keep this as we have discussed.

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:583: remote_net_id == remote_network_id_) {
On 2016/03/16 21:22:03, pthatcher1 wrote:
> If we use NetworkRoute structs, we should also check if they are connected or
> not.
We can now use the null pointer for best connection being deleted.

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:593: void Call::ResetBweBitrates() {
On 2016/03/16 21:22:03, pthatcher1 wrote:
> If !new_route.connected, we probably want to do something more drastic with
the
> BWE.  I'm not sure what, though, so perhaps just a TODO for now.

Done. Add TODO.

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:599: config_.bitrate_config.max_bitrate_bps);
On 2016/03/16 21:22:03, pthatcher1 wrote:
> Does this really restart the BWE?  
Actually It may not. Maybe we can leave this to BWE experts.

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

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1162: }
On 2016/03/16 21:22:03, pthatcher1 wrote:
> I think we could use a helper method called GetNetworkRoute(const Connection*
> conn) like this:
> 
> NetworkRoute route;
> if (!conn) {
>   route.connected = false;
> } else {
>   route.local_network_id = new Optional<int>(conn->port()->Network()->id());
>   // TODO: Signal remote network ID.
>   // route.remote_net_id = conn->remote_candidate->network_id();
> }
> return route;
> 
> 
> And then we can have the code here be:
> 
> NetworkRoute old_route = GetNetworkRoute(old_conn);
> NetworkRoute new_route = GetNetworkRoute(new_conn);
> if (old_route != new_route) {
>   SignalNetworkRouteChanged(this, old_route, new_route);
> }
> 
> Of course, we need to make sure to implement operator== and operator!=
Instead of Optional and CandidatePair, I just pass the pointer of
best_connection_, 
It can check if the best_connection is null and it should include every possible
information we will need, and it is sort of equivalent to SelectedCandidatePair
but without the new struct.

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/p2p/quic/quictrans...
File webrtc/p2p/quic/quictransportchannel.h (right):

https://codereview.webrtc.org/1803063004/diff/40001/webrtc/p2p/quic/quictrans...
webrtc/p2p/quic/quictransportchannel.h:227: void OnRouteChange(TransportChannel*
channel, const Candidate& candidate);
On 2016/03/16 21:22:03, pthatcher1 wrote:
> Can you mark this one as deprecated and something we will remove as soon as
> chromoting isn't using it?
I marked the SignalRouteChange to be deprecated.

[honghaiz3, 2016-03-17 03:39:56]

https://codereview.webrtc.org/1803063004/diff/80001/webrtc/base/networkmonitor.h
File webrtc/base/networkmonitor.h (right):

https://codereview.webrtc.org/1803063004/diff/80001/webrtc/base/networkmonito...
webrtc/base/networkmonitor.h:49: virtual ~NetworkBinderInterface(){};
Added the change here to make the compiler happy when this is included in
call.cc.

[stefan-webrtc, 2016-03-17 12:16:21]

Description was changed from

==========
When the local network changes, reset the BWE estimation in the call.

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

to

==========
When the local network changes, reset the BWE estimation in the call.

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

[stefan-webrtc, 2016-03-17 12:16:21]

stefan@webrtc.org changed reviewers:
+ stefan@webrtc.org
- holmer@google.com

[stefan-webrtc, 2016-03-17 12:48:19]

https://codereview.webrtc.org/1803063004/diff/80001/webrtc/base/networkmonitor.h
File webrtc/base/networkmonitor.h (right):

https://codereview.webrtc.org/1803063004/diff/80001/webrtc/base/networkmonito...
webrtc/base/networkmonitor.h:49: virtual ~NetworkBinderInterface(){};
On 2016/03/17 03:39:56, honghaiz3 wrote:
> Added the change here to make the compiler happy when this is included in
> call.cc.

space between () and {}

https://codereview.webrtc.org/1803063004/diff/80001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1803063004/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2337: void
WebRtcVoiceMediaChannel::OnBestConnectionChanged(
What will happen when we're not using BUNDLE? It's probably not a good thing
that voice tears down the BWE if only the voice connection has changed.

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

https://codereview.webrtc.org/1803063004/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1160: 
Remove empty line

[pbos-webrtc, 2016-03-17 14:09:22]

pbos@webrtc.org changed reviewers:
+ pbos@webrtc.org

[pbos-webrtc, 2016-03-17 14:09:22]

https://codereview.webrtc.org/1803063004/diff/80001/webrtc/base/networkmonitor.h
File webrtc/base/networkmonitor.h (right):

https://codereview.webrtc.org/1803063004/diff/80001/webrtc/base/networkmonito...
webrtc/base/networkmonitor.h:49: virtual ~NetworkBinderInterface(){};
On 2016/03/17 12:48:19, stefan-webrtc (holmer) wrote:
> On 2016/03/17 03:39:56, honghaiz3 wrote:
> > Added the change here to make the compiler happy when this is included in
> > call.cc.
> 
> space between () and {}

and no ;

then run git cl format

[honghaiz3, 2016-03-23 19:54:13]

Description was changed from

==========
When the local network changes, reset the BWE estimation in the call.

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

to

==========
Reset the BWE when the network changes. 
Currently "Resetting the BWE" does nothing yet. This CL passes the correct
signaling to the bandwidth estimator.

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

[honghaiz3, 2016-03-23 19:55:40]

Thanks! PTAL.

https://codereview.webrtc.org/1803063004/diff/80001/webrtc/base/networkmonitor.h
File webrtc/base/networkmonitor.h (right):

https://codereview.webrtc.org/1803063004/diff/80001/webrtc/base/networkmonito...
webrtc/base/networkmonitor.h:49: virtual ~NetworkBinderInterface(){};
On 2016/03/17 14:09:22, pbos-webrtc wrote:
> On 2016/03/17 12:48:19, stefan-webrtc (holmer) wrote:
> > On 2016/03/17 03:39:56, honghaiz3 wrote:
> > > Added the change here to make the compiler happy when this is included in
> > > call.cc.
> > 
> > space between () and {}
> 
> and no ;
> 
> then run git cl format
Done. Thanks!
I forgot to run that before uploading this change.

https://codereview.webrtc.org/1803063004/diff/80001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1803063004/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2337: void
WebRtcVoiceMediaChannel::OnBestConnectionChanged(
On 2016/03/17 12:48:19, stefan-webrtc (holmer) wrote:
> What will happen when we're not using BUNDLE? It's probably not a good thing
> that voice tears down the BWE if only the voice connection has changed.
This is a very good point.
We are using one BWE for all media channels, right?
Then the choices are
1) reset BWE if one media connection's network changed, or 
2) reset BWE if all media connections's networks changed. 
Is option 2) better?
In most cases, the media connection should be using the same network anyway, so
option 1 and 2 may not have big differences.

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

https://codereview.webrtc.org/1803063004/diff/80001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1160: 
On 2016/03/17 12:48:19, stefan-webrtc (holmer) wrote:
> Remove empty line

Done.

[honghaiz3, 2016-03-24 00:32:21]

Patchset #4 (id:120001) has been deleted

[honghaiz3, 2016-03-24 00:32:31]

Patchset #5 (id:160001) has been deleted

[honghaiz3, 2016-03-24 00:32:38]

Patchset #5 (id:180001) has been deleted

[honghaiz3, 2016-03-24 00:32:59]

Patchset #4 (id:140001) has been deleted

[honghaiz3, 2016-03-24 00:44:26]

Patchset #4 (id:200001) has been deleted

[honghaiz3, 2016-03-24 00:46:53]

Updated the CL to merge with head (signaling network id) and added NetworkRoute
and compare NetworkRoute on each transport channel for unbundled channels.

[honghaiz3, 2016-03-24 00:49:04]

Patchset #4 (id:220001) has been deleted

[honghaiz3, 2016-03-24 00:54:26]

Still need to add tests yet.

[pthatcher1, 2016-03-24 18:46:37]

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/base/networkroute.h
File webrtc/base/networkroute.h (right):

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/base/networkroute...
webrtc/base/networkroute.h:14: namespace cricket {
It seems like this shouldn't be in base/.  We don't have a good place for things
that both network and media code depend on, but I don't think the network code
depends on this.  

It seems like this should go where MediaChannel::NetworkInterface is, which is
mediachannel.h  We can find a better home for it when we refactor
mediachannel.h.

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:177: cricket::NetworkRoute last_changed_route_;
Either last_changed_network_route_ or routes_, to be consistent.  

I slightly prefer last_changed_network_route_.

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:605: LOG(LS_INFO) << "Transport channel " << transport_name
Might as well just say "Transport" instead of "Transport channel".  Once we
remove non-muxed RTCP, they will be the same thing.

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:616: if (network_routes_.size() > 1) {
This isn't quite right in the case where we have audio and then add unbundled
video and the video route changes before the audio one does.  

It looks like you're doing this to avoid a map lookup, but then you do one later
anyway to set the value.  So why not make it into one lookup and avoid the
complexity of size() and last_changed_route?  Something like this:


auto result = network_routes_.insert(std::make_pair(transport_name,
network_route));
bool inserted = result->second;
if (inserted) {
  // The BWE doesn't need to be reset the first time the network connects.
  return;  
}

auto kv = result->first; 
bool changed = (kv.second != network_route)
if (!changed) {
  // The BWE doesn't need to be reset of the route didn't really change.
  return;
}

kv.second = network_route;
LOG(LS_INFO) << "...";
congestion_controller_->ResetBweBitrates(config_.bitrate_config.start_bitrate_bps);

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.cc:642: ASSERT(channel == channel_);
Same with the signal: SignalSelectedConnectionChanged.

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel.h (right):

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.h:220: Connection* best_connection);
I think we should move away from calling these "best connection"s and toward
calling them "selected connection"s, or even "selected candidate pair"s. 

Can you call this OnSelectedConnectionChanged?  And then call it
selected_connection?

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1168: SignalBestConnectionChanged(this,
best_connection_);
It would be nice to call this selected_connection_ or even
selected_candidate_pair_.  Could you at least put a TODO?

[honghaiz3, 2016-03-24 22:37:12]

Tried to add tests on the call.cc, but it turned out that it is pretty
complicated unless we expose internal::Call to tests. So I am going to leave
that out. Let me know if you have different thoughts.

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/base/networkroute.h
File webrtc/base/networkroute.h (right):

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/base/networkroute...
webrtc/base/networkroute.h:14: namespace cricket {
On 2016/03/24 18:46:36, pthatcher1 wrote:
> It seems like this shouldn't be in base/.  We don't have a good place for
things
> that both network and media code depend on, but I don't think the network code
> depends on this.  
> 
> It seems like this should go where MediaChannel::NetworkInterface is, which is
> mediachannel.h  We can find a better home for it when we refactor
> mediachannel.h.

If we do that, call dir has to include webrtc/media, which is not allowed at
this moment. Not sure if it is a good idea to add that dependency.

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:177: cricket::NetworkRoute last_changed_route_;
On 2016/03/24 18:46:36, pthatcher1 wrote:
> Either last_changed_network_route_ or routes_, to be consistent.  
> 
> I slightly prefer last_changed_network_route_.

Done.

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:605: LOG(LS_INFO) << "Transport channel " << transport_name
On 2016/03/24 18:46:37, pthatcher1 wrote:
> Might as well just say "Transport" instead of "Transport channel".  Once we
> remove non-muxed RTCP, they will be the same thing.

Done.

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:616: if (network_routes_.size() > 1) {
On 2016/03/24 18:46:37, pthatcher1 wrote:
> This isn't quite right in the case where we have audio and then add unbundled
> video and the video route changes before the audio one does.  
> 
> It looks like you're doing this to avoid a map lookup, but then you do one
later
> anyway to set the value.  So why not make it into one lookup and avoid the
> complexity of size() and last_changed_route?  Something like this:
> 
> 
> auto result = network_routes_.insert(std::make_pair(transport_name,
> network_route));
> bool inserted = result->second;
> if (inserted) {
>   // The BWE doesn't need to be reset the first time the network connects.
>   return;  
> }
> 
> auto kv = result->first; 
> bool changed = (kv.second != network_route)
> if (!changed) {
>   // The BWE doesn't need to be reset of the route didn't really change.
>   return;
> }
> 
> kv.second = network_route;
> LOG(LS_INFO) << "...";
>
congestion_controller_->ResetBweBitrates(config_.bitrate_config.start_bitrate_bps);

Takes the suggestions. But I think we still need to keep the last_changed_it. 
In the case that there are audio and video channel, and audio switched to a new
network and then video did so too, it should not restart BWE again for the video
channel change. WDYT?

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.cc:642: ASSERT(channel == channel_);
On 2016/03/24 18:46:37, pthatcher1 wrote:
> Same with the signal: SignalSelectedConnectionChanged.

Done.

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel.h (right):

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.h:220: Connection* best_connection);
On 2016/03/24 18:46:37, pthatcher1 wrote:
> I think we should move away from calling these "best connection"s and toward
> calling them "selected connection"s, or even "selected candidate pair"s. 
> 
> Can you call this OnSelectedConnectionChanged?  And then call it
> selected_connection?

Done.

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1168: SignalBestConnectionChanged(this,
best_connection_);
On 2016/03/24 18:46:37, pthatcher1 wrote:
> It would be nice to call this selected_connection_ or even
> selected_candidate_pair_.  Could you at least put a TODO?
I added a TODO here, will make that change in a separate CL.

[pthatcher1, 2016-03-25 21:30:21]

It seems like we at least need unit tests to cover:

1.  That we fire the connection change even in the network code correctly.

2.  That we pass that over to the call object correctly.

It sounds like the hard part is verifying the code in call.cc to make sure it's
correct.  Is that accurate?

If so, can you ask holmer and pbos how they would like it to be tested, while
you right the tests for #1 and #2 above?

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/base/networkroute.h
File webrtc/base/networkroute.h (right):

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/base/networkroute...
webrtc/base/networkroute.h:14: namespace cricket {
On 2016/03/24 22:37:11, honghaiz3 wrote:
> On 2016/03/24 18:46:36, pthatcher1 wrote:
> > It seems like this shouldn't be in base/.  We don't have a good place for
> things
> > that both network and media code depend on, but I don't think the network
code
> > depends on this.  
> > 
> > It seems like this should go where MediaChannel::NetworkInterface is, which
is
> > mediachannel.h  We can find a better home for it when we refactor
> > mediachannel.h.
> 
> If we do that, call dir has to include webrtc/media, which is not allowed at
> this moment. Not sure if it is a good idea to add that dependency. 

Yeah, since the network code doesn't depend on the media code and the media code
doesn't depend on the network code, we need a third place to stick things that
both can depend on.  I hope someday we can have a "here's the interface/structs
the media stack needs from the network" that doesn't depend on the network code
or the media code.  But right now we don't have a place like that.

So, can you at least put a TODO here, something like:

TODO: Make a directory that describes the interfaces and structs that the media
code can rely on and the network code can implement, and both can depend on
that, but not depend on each other.  Then, move this file to that directory.

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:616: if (network_routes_.size() > 1) {
On 2016/03/24 22:37:12, honghaiz3 wrote:
> On 2016/03/24 18:46:37, pthatcher1 wrote:
> > This isn't quite right in the case where we have audio and then add
unbundled
> > video and the video route changes before the audio one does.  
> > 
> > It looks like you're doing this to avoid a map lookup, but then you do one
> later
> > anyway to set the value.  So why not make it into one lookup and avoid the
> > complexity of size() and last_changed_route?  Something like this:
> > 
> > 
> > auto result = network_routes_.insert(std::make_pair(transport_name,
> > network_route));
> > bool inserted = result->second;
> > if (inserted) {
> >   // The BWE doesn't need to be reset the first time the network connects.
> >   return;  
> > }
> > 
> > auto kv = result->first; 
> > bool changed = (kv.second != network_route)
> > if (!changed) {
> >   // The BWE doesn't need to be reset of the route didn't really change.
> >   return;
> > }
> > 
> > kv.second = network_route;
> > LOG(LS_INFO) << "...";
> >
>
congestion_controller_->ResetBweBitrates(config_.bitrate_config.start_bitrate_bps);
> 
> Takes the suggestions. But I think we still need to keep the last_changed_it. 
> In the case that there are audio and video channel, and audio switched to a
new
> network and then video did so too, it should not restart BWE again for the
video
> channel change. WDYT? 

I think this came up on Wednesday, and I liked the idea that several people had
which is to just let it change twice.  It's more simple, and perhaps more
correct.  If they are close in time to each other, then it doesn't really
matter.  If they are not, then we should reset it twice.

[honghaiz3, 2016-03-28 04:03:16]

Added tests for p2ptransportchannel. Still working on the tests for the channel.


Stefan, I wonder if you have suggestions on testings in call.cc. It appears to
me that it is necessary to expose the class internal::Call to the testing class.
Is there a better recommended way?

Thanks.

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/base/networkroute.h
File webrtc/base/networkroute.h (right):

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/base/networkroute...
webrtc/base/networkroute.h:14: namespace cricket {
On 2016/03/25 21:30:21, pthatcher1 wrote:
> On 2016/03/24 22:37:11, honghaiz3 wrote:
> > On 2016/03/24 18:46:36, pthatcher1 wrote:
> > > It seems like this shouldn't be in base/.  We don't have a good place for
> > things
> > > that both network and media code depend on, but I don't think the network
> code
> > > depends on this.  
> > > 
> > > It seems like this should go where MediaChannel::NetworkInterface is,
which
> is
> > > mediachannel.h  We can find a better home for it when we refactor
> > > mediachannel.h.
> > 
> > If we do that, call dir has to include webrtc/media, which is not allowed at
> > this moment. Not sure if it is a good idea to add that dependency. 
> 
> Yeah, since the network code doesn't depend on the media code and the media
code
> doesn't depend on the network code, we need a third place to stick things that
> both can depend on.  I hope someday we can have a "here's the
interface/structs
> the media stack needs from the network" that doesn't depend on the network
code
> or the media code.  But right now we don't have a place like that.
> 
> So, can you at least put a TODO here, something like:
> 
> TODO: Make a directory that describes the interfaces and structs that the
media
> code can rely on and the network code can implement, and both can depend on
> that, but not depend on each other.  Then, move this file to that directory.

Done. Thanks!

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1803063004/diff/260001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:616: if (network_routes_.size() > 1) {
On 2016/03/25 21:30:21, pthatcher1 wrote:
> On 2016/03/24 22:37:12, honghaiz3 wrote:
> > On 2016/03/24 18:46:37, pthatcher1 wrote:
> > > This isn't quite right in the case where we have audio and then add
> unbundled
> > > video and the video route changes before the audio one does.  
> > > 
> > > It looks like you're doing this to avoid a map lookup, but then you do one
> > later
> > > anyway to set the value.  So why not make it into one lookup and avoid the
> > > complexity of size() and last_changed_route?  Something like this:
> > > 
> > > 
> > > auto result = network_routes_.insert(std::make_pair(transport_name,
> > > network_route));
> > > bool inserted = result->second;
> > > if (inserted) {
> > >   // The BWE doesn't need to be reset the first time the network connects.
> > >   return;  
> > > }
> > > 
> > > auto kv = result->first; 
> > > bool changed = (kv.second != network_route)
> > > if (!changed) {
> > >   // The BWE doesn't need to be reset of the route didn't really change.
> > >   return;
> > > }
> > > 
> > > kv.second = network_route;
> > > LOG(LS_INFO) << "...";
> > >
> >
>
congestion_controller_->ResetBweBitrates(config_.bitrate_config.start_bitrate_bps);
> > 
> > Takes the suggestions. But I think we still need to keep the
last_changed_it. 
> > In the case that there are audio and video channel, and audio switched to a
> new
> > network and then video did so too, it should not restart BWE again for the
> video
> > channel change. WDYT? 
> 
> I think this came up on Wednesday, and I liked the idea that several people
had
> which is to just let it change twice.  It's more simple, and perhaps more
> correct.  If they are close in time to each other, then it doesn't really
> matter.  If they are not, then we should reset it twice.

Done.

[pthatcher1, 2016-03-28 16:18:49]

Other than the lack of a little more unit testing, this looks good.

https://codereview.webrtc.org/1803063004/diff/320001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/1803063004/diff/320001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:520: network_route);
Should we have a unit test for this part also?

[honghaiz3, 2016-03-29 01:03:03]

Changed the signal name and added tests for channel.cc. PTAL.

https://codereview.webrtc.org/1803063004/diff/320001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/1803063004/diff/320001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:520: network_route);
On 2016/03/28 16:18:48, pthatcher1 wrote:
> Should we have a unit test for this part also?

Done.

[honghaiz3, 2016-03-29 02:52:12]

Patchset #10 (id:360001) has been deleted

[pthatcher1, 2016-03-29 21:22:56]

lgtm

[honghaiz3, 2016-03-29 22:32:45]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-03-29 22:32:46]

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/1803063004/#ps420001 (title:
"Added TODO for tests.")

[commit-bot: I haz the power, 2016-03-29 22:32:57]

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

[commit-bot: I haz the power, 2016-03-29 22:42:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-29 22:42:49]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/4458)

[honghaiz3, 2016-03-29 23:15:03]

Patchset #12 (id:420001) has been deleted

[honghaiz3, 2016-03-29 23:15:25]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-03-29 23:15:25]

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/1803063004/#ps440001 (title:
" Removed changes in call dir and leave that in a separate CL. ")

[commit-bot: I haz the power, 2016-03-29 23:15:30]

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

[commit-bot: I haz the power, 2016-03-30 00:27:41]

Description was changed from

==========
Reset the BWE when the network changes. 
Currently "Resetting the BWE" does nothing yet. This CL passes the correct
signaling to the bandwidth estimator.

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

to

==========
Reset the BWE when the network changes.
Currently "Resetting the BWE" does nothing yet. This CL passes the correct
signaling to the bandwidth estimator.

BUG=
R=pthatcher@webrtc.org

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

[honghaiz3, 2016-03-30 00:27:42]

Description was changed from

==========
Reset the BWE when the network changes.
Currently "Resetting the BWE" does nothing yet. This CL passes the correct
signaling to the bandwidth estimator.

BUG=
R=pthatcher@webrtc.org

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

to

==========
Reset the BWE when the network changes.
Currently "Resetting the BWE" does nothing yet. This CL passes the correct
signaling to the bandwidth estimator.

BUG=
R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/cc411c0599b038cf3f2cc0274...
==========

[commit-bot: I haz the power, 2016-03-30 00:27:42]

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

[honghaiz3, 2016-03-30 00:27:44]

Committed patchset #12 (id:440001) manually as
cc411c0599b038cf3f2cc027482bedc19e066166 (presubmit successful).