Reset the BWE when the network changes

webrtc/call/call.cc

Index: webrtc/call/call.cc
diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc
index 5f9eb3fd93312ad657c6f9c19ce6eb1c758836a1..9cafd62444f6589eb66565ae57c25c7468902989 100644
--- a/webrtc/call/call.cc
+++ b/webrtc/call/call.cc
@@ -89,6 +89,10 @@ class Call : public webrtc::Call, public PacketReceiver,
 
   void SignalChannelNetworkState(MediaType media, NetworkState state) override;
 
+  void OnNetworkRouteChanged(
+      const std::string& transport_name,
+      const cricket::NetworkRoute& network_route) override;
+
   void OnSentPacket(const rtc::SentPacket& sent_packet) override;
 
   // Implements BitrateObserver.
@@ -102,7 +106,6 @@ class Call : public webrtc::Call, public PacketReceiver,
                             const uint8_t* packet,
                             size_t length,
                             const PacketTime& packet_time);
-
   void ConfigureSync(const std::string& sync_group)
       EXCLUSIVE_LOCKS_REQUIRED(receive_crit_);
 
@@ -170,6 +173,9 @@ class Call : public webrtc::Call, public PacketReceiver,
   int64_t pacer_bitrate_sum_kbits_ GUARDED_BY(&bitrate_crit_);
   int64_t num_bitrate_updates_ GUARDED_BY(&bitrate_crit_);
 
+  std::map<std::string, cricket::NetworkRoute> network_routes_;
+  cricket::NetworkRoute last_changed_route_;

[pthatcher1, 2016/03/24 18:46:36]

Either last_changed_network_route_ or routes_, to be consistent.  

I slightly prefer last_changed_network_route_.

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

Done.

+
   VieRemb remb_;
   const std::unique_ptr<CongestionController> congestion_controller_;
 
@@ -591,6 +597,37 @@ void Call::SignalChannelNetworkState(MediaType media, NetworkState state) {
   }
 }
 
+void Call::OnNetworkRouteChanged(const std::string& transport_name,
+                                 const cricket::NetworkRoute& network_route) {
+  RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());
+  // Check if the network route is connected.
+  if (!network_route.connected) {
+    LOG(LS_INFO) << "Transport channel " << transport_name

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

Might as well just say "Transport" instead of "Transport channel".  Once we
remove non-muxed RTCP, they will be the same thing.

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

Done.

+                 << " is disconnected";
+    // TODO(honghaiz): Perhaps it should stop sending until a connected route is
+    // established.
+    return;
+  }
+  // When using un-bundled transport channels, we need to compare the new route
+  // against |last_changed_route_| and the network route on its own
+  // transport channel. If it is equivalent to either of them, there is no need
+  // to reset BWE.
+  bool route_changed = network_route != last_changed_route_;
+  if (network_routes_.size() > 1) {

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

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);

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

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? 

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

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]

Done.

+    route_changed &= (network_route != network_routes_[transport_name]);
+  }
+  if (route_changed) {
+    LOG(LS_INFO) << "Network route changed on transport " << transport_name
+                 << ": new local network id " << network_route.local_network_id
+                 << " new remote network id "
+                 << network_route.remote_network_id;
+    congestion_controller_->ResetBweBitrates(
+        config_.bitrate_config.start_bitrate_bps);
+  }
+  network_routes_[transport_name] = network_route;
+  last_changed_route_ = network_route;
+}
+
 void Call::UpdateAggregateNetworkState() {
   RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());