Reset the BWE when the network changes

webrtc/call/call.cc

 /*
  *  Copyright (c) 2013 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 71 matching lines @@
   DeliveryStatus DeliverPacket(MediaType media_type,
                                const uint8_t* packet,
                                size_t length,
                                const PacketTime& packet_time) override;
 
   void SetBitrateConfig(
       const webrtc::Call::Config::BitrateConfig& bitrate_config) override;
 
   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.
   void OnNetworkChanged(uint32_t bitrate_bps, uint8_t fraction_loss,
                         int64_t rtt_ms) override;
 
  private:
   DeliveryStatus DeliverRtcp(MediaType media_type, const uint8_t* packet,
                              size_t length);
   DeliveryStatus DeliverRtp(MediaType media_type,
                             const uint8_t* packet,
                             size_t length,
                             const PacketTime& packet_time);
-
   void ConfigureSync(const std::string& sync_group)
       EXCLUSIVE_LOCKS_REQUIRED(receive_crit_);
 
   VoiceEngine* voice_engine() {
     internal::AudioState* audio_state =
         static_cast<internal::AudioState*>(config_.audio_state.get());
     if (audio_state)
       return audio_state->voice_engine();
     else
       return nullptr;
@@ skipping 47 matching lines @@
   int64_t last_rtp_packet_received_ms_;
   int64_t first_packet_sent_ms_;
 
   // TODO(holmer): Remove this lock once BitrateController no longer calls
   // OnNetworkChanged from multiple threads.
   rtc::CriticalSection bitrate_crit_;
   int64_t estimated_send_bitrate_sum_kbits_ GUARDED_BY(&bitrate_crit_);
   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_;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(Call);
 };
 }  // namespace internal
 
 Call* Call::Create(const Call::Config& config) {
   return new internal::Call(config);
 }
@@ skipping 401 matching lines @@
     ReadLockScoped read_lock(*receive_crit_);
     for (auto& kv : audio_receive_ssrcs_) {
       kv.second->SignalNetworkState(audio_network_state_);
     }
     for (auto& kv : video_receive_ssrcs_) {
       kv.second->SignalNetworkState(video_network_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());
 
   bool have_audio = false;
   bool have_video = false;
   {
     ReadLockScoped read_lock(*send_crit_);
     if (audio_send_ssrcs_.size() > 0)
       have_audio = true;
     if (video_send_ssrcs_.size() > 0)
@@ skipping 194 matching lines @@
   // thread. Then this check can be enabled.
   // RTC_DCHECK(!configuration_thread_checker_.CalledOnValidThread());
   if (RtpHeaderParser::IsRtcp(packet, length))
     return DeliverRtcp(media_type, packet, length);
 
   return DeliverRtp(media_type, packet, length, packet_time);
 }
 
 }  // namespace internal
 }  // namespace webrtc