Add PeerConnectionInterface::UpdateCallBitrate with call tests.

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 124 matching lines @@
                                const uint8_t* packet,
                                size_t length,
                                const PacketTime& packet_time) override;
 
   // Implements RecoveredPacketReceiver.
   bool OnRecoveredPacket(const uint8_t* packet, size_t length) override;
 
   void SetBitrateConfig(
       const webrtc::Call::Config::BitrateConfig& bitrate_config) override;
 
+  void SetBitrateConfigMask(
+      const webrtc::Call::Config::BitrateConfigMask& bitrate_config) override;
+
   void SignalChannelNetworkState(MediaType media, NetworkState state) override;
 
   void OnTransportOverheadChanged(MediaType media,
                                   int transport_overhead_per_packet) override;
 
   void OnNetworkRouteChanged(const std::string& transport_name,
                              const rtc::NetworkRoute& network_route) override;
 
   void OnSentPacket(const rtc::SentPacket& sent_packet) override;
 
@@ skipping 26 matching lines @@
                                                   size_t length,
                                                   const PacketTime& packet_time)
       SHARED_LOCKS_REQUIRED(receive_crit_);
 
   void UpdateSendHistograms(int64_t first_sent_packet_ms)
       EXCLUSIVE_LOCKS_REQUIRED(&bitrate_crit_);
   void UpdateReceiveHistograms();
   void UpdateHistograms();
   void UpdateAggregateNetworkState();
 
+  // These values only get set if there is actually something to change.
+  struct BitrateUpdate {
+    rtc::Optional<int> min_bitrate_bps;
+    rtc::Optional<int> start_bitrate_bps;
+    rtc::Optional<int> max_bitrate_bps;
+  };
+
+  // Applies update to the BitrateConfig cached in |config_| and updates the
+  // congestion controller if update is not empty.
+  void UpdateCurrentBitrateConfig(const BitrateUpdate& update);
+
   Clock* const clock_;
 
   const int num_cpu_cores_;
   const std::unique_ptr<ProcessThread> module_process_thread_;
   const std::unique_ptr<ProcessThread> pacer_thread_;
   const std::unique_ptr<CallStats> call_stats_;
   const std::unique_ptr<BitrateAllocator> bitrate_allocator_;
   Call::Config config_;
   rtc::ThreadChecker configuration_thread_checker_;
 
@@ skipping 74 matching lines @@
   std::unique_ptr<RtpTransportControllerSendInterface> transport_send_;
   ReceiveSideCongestionController receive_side_cc_;
   const std::unique_ptr<SendDelayStats> video_send_delay_stats_;
   const int64_t start_ms_;
   // TODO(perkj): |worker_queue_| is supposed to replace
   // |module_process_thread_|.
   // |worker_queue| is defined last to ensure all pending tasks are cancelled
   // and deleted before any other members.
   rtc::TaskQueue worker_queue_;
 
+  // The config mask set by SetBitrateConfigMask.
+  // 0 <= min <= start <= max
+  Config::BitrateConfigMask bitrate_config_mask_;
+
+  // The config set by SetBitrateConfig.
+  // min >= 0, start != 0, max == -1 || max > 0
+  Config::BitrateConfig base_bitrate_config_;
+
   RTC_DISALLOW_COPY_AND_ASSIGN(Call);
 };
 }  // namespace internal
 
 std::string Call::Stats::ToString(int64_t time_ms) const {
   std::stringstream ss;
   ss << "Call stats: " << time_ms << ", {";
   ss << "send_bw_bps: " << send_bandwidth_bps << ", ";
   ss << "recv_bw_bps: " << recv_bandwidth_bps << ", ";
   ss << "max_pad_bps: " << max_padding_bitrate_bps << ", ";
@@ skipping 35 matching lines @@
       received_audio_bytes_per_second_counter_(clock_, nullptr, true),
       received_video_bytes_per_second_counter_(clock_, nullptr, true),
       received_rtcp_bytes_per_second_counter_(clock_, nullptr, true),
       min_allocated_send_bitrate_bps_(0),
       configured_max_padding_bitrate_bps_(0),
       estimated_send_bitrate_kbps_counter_(clock_, nullptr, true),
       pacer_bitrate_kbps_counter_(clock_, nullptr, true),
       receive_side_cc_(clock_, transport_send->packet_router()),
       video_send_delay_stats_(new SendDelayStats(clock_)),
       start_ms_(clock_->TimeInMilliseconds()),
-      worker_queue_("call_worker_queue") {
+      worker_queue_("call_worker_queue"),
+      base_bitrate_config_(config.bitrate_config) {
   RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());
   RTC_DCHECK(config.event_log != nullptr);
   RTC_DCHECK_GE(config.bitrate_config.min_bitrate_bps, 0);
   RTC_DCHECK_GE(config.bitrate_config.start_bitrate_bps,
                 config.bitrate_config.min_bitrate_bps);
   if (config.bitrate_config.max_bitrate_bps != -1) {
     RTC_DCHECK_GE(config.bitrate_config.max_bitrate_bps,
                   config.bitrate_config.start_bitrate_bps);
   }
   Trace::CreateTrace();
@@ skipping 504 matching lines @@
     rtc::CritScope cs(&bitrate_crit_);
     stats.max_padding_bitrate_bps = configured_max_padding_bitrate_bps_;
   }
   return stats;
 }
 
 void Call::SetBitrateConfig(
     const webrtc::Call::Config::BitrateConfig& bitrate_config) {
   TRACE_EVENT0("webrtc", "Call::SetBitrateConfig");
   RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());
+
   RTC_DCHECK_GE(bitrate_config.min_bitrate_bps, 0);
-  if (bitrate_config.max_bitrate_bps != -1)
+  RTC_DCHECK_NE(bitrate_config.start_bitrate_bps, 0);
+  if (bitrate_config.max_bitrate_bps != -1) {
     RTC_DCHECK_GT(bitrate_config.max_bitrate_bps, 0);
-  if (config_.bitrate_config.min_bitrate_bps ==
-          bitrate_config.min_bitrate_bps &&
-      (bitrate_config.start_bitrate_bps <= 0 ||
-       config_.bitrate_config.start_bitrate_bps ==
-           bitrate_config.start_bitrate_bps) &&
-      config_.bitrate_config.max_bitrate_bps ==
-          bitrate_config.max_bitrate_bps) {
-    // Nothing new to set, early abort to avoid encoder reconfigurations.
+  }
+
+  BitrateUpdate update;
+
+  const int min = bitrate_config.min_bitrate_bps;
+  if (min != base_bitrate_config_.min_bitrate_bps) {
+    base_bitrate_config_.min_bitrate_bps = min;
+    if (!bitrate_config_mask_.min_bitrate_bps ||
+        min > *bitrate_config_mask_.min_bitrate_bps) {

[Taylor Brandstetter, 2017/05/11 04:16:15]

The code would be simpler if instead of "see if combined value will change, if
so pass it along", the algorithm was "compute new combined value, pass it along
if it's changed".

[Zach Stein, 2017/05/11 21:43:09]

For c->SetBitrateConfig(1,2,3); c->SetBitrateConfig(1,2,4); the second call
currently calls SetBweBitrates(1,2,4), not SetBweBitrates(1,-1,4), so it will
reset bandwidth estimation. I was trying to avoid changing that behavior but
maybe we don't care. I would guess that we want to reset bandwidth estimation if
the network properties changed sufficiently, which we don't have an indicator
for here.

+      update.min_bitrate_bps = rtc::Optional<int>(min);
+    }
+  }
+
+  const int start = bitrate_config.start_bitrate_bps;
+  if (start != -1) {
+    base_bitrate_config_.start_bitrate_bps = start;
+    // TODO(zstein): Do we only want to update if the start rate changed here?

[Taylor Brandstetter, 2017/05/11 04:16:15]

Yes; there should be a test for that too.

[Zach Stein, 2017/05/11 21:43:09]

Done.

Test is in this other CL (the 2 Elides tests):
https://codereview.chromium.org/2870383003/diff/1/webrtc/call/call_unittest.cc

+    update.start_bitrate_bps = rtc::Optional<int>(start);
+  }
+  const int max = bitrate_config.max_bitrate_bps;
+  if (max != -1 && max != base_bitrate_config_.max_bitrate_bps) {
+    base_bitrate_config_.max_bitrate_bps = max;
+    if (!bitrate_config_mask_.max_bitrate_bps ||
+        max < *bitrate_config_mask_.max_bitrate_bps) {
+      update.max_bitrate_bps = rtc::Optional<int>(max);
+    }
+  }
+
+  UpdateCurrentBitrateConfig(update);
+}
+
+void Call::SetBitrateConfigMask(
+    const webrtc::Call::Config::BitrateConfigMask& mask) {
+  TRACE_EVENT0("webrtc", "Call::SetBitrateConfigMask");
+  RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());
+
+  bitrate_config_mask_ = mask;
+  BitrateUpdate update;
+
+  if (mask.min_bitrate_bps &&
+      *mask.min_bitrate_bps > base_bitrate_config_.min_bitrate_bps) {
+    update.min_bitrate_bps = mask.min_bitrate_bps;
+  }
+  update.start_bitrate_bps = mask.start_bitrate_bps;
+  if (mask.max_bitrate_bps &&
+      (base_bitrate_config_.max_bitrate_bps == -1 ||
+       *mask.max_bitrate_bps < base_bitrate_config_.max_bitrate_bps)) {
+    update.max_bitrate_bps = mask.max_bitrate_bps;
+  }
+
+  UpdateCurrentBitrateConfig(update);
+}
+
+void Call::UpdateCurrentBitrateConfig(const BitrateUpdate& update) {
+  if (!update.min_bitrate_bps && !update.start_bitrate_bps &&
+      !update.max_bitrate_bps) {

[Taylor Brandstetter, 2017/05/11 04:16:15]

Getting into more corner-casey territory, but you could get past this check, but
still end up with nothing changed due to the clamping afterwards. Here's a test
for this and a fix: https://codereview.webrtc.org/2875703004/

[Zach Stein, 2017/05/11 21:43:09]

Maybe that is enough of an edge case that we don't care about eliding the call,
but I will take a look at your change.

+    // Nothing to update.
     return;
   }
-  config_.bitrate_config.min_bitrate_bps = bitrate_config.min_bitrate_bps;
-  // Start bitrate of -1 means we should keep the old bitrate, which there is
-  // no point in remembering for the future.
-  if (bitrate_config.start_bitrate_bps > 0)
-    config_.bitrate_config.start_bitrate_bps = bitrate_config.start_bitrate_bps;
-  config_.bitrate_config.max_bitrate_bps = bitrate_config.max_bitrate_bps;
-  RTC_DCHECK_NE(bitrate_config.start_bitrate_bps, 0);
+
+  Config::BitrateConfig updated;
+  const Config::BitrateConfig& current = config_.bitrate_config;
+  updated.min_bitrate_bps =
+      update.min_bitrate_bps.value_or(current.min_bitrate_bps);
+  updated.start_bitrate_bps =
+      update.start_bitrate_bps.value_or(current.start_bitrate_bps);
+  updated.max_bitrate_bps =
+      update.max_bitrate_bps.value_or(current.max_bitrate_bps);
+
+  // TODO(zstein): This clamp logic is different than send_side_cc's (moves min
+  // instead of max, allows start to be greather than min) - check that it is a
+  // sensible difference.
+
+  // Clamp min under max and start under max and above min.
+  if (updated.max_bitrate_bps != -1) {
+    updated.min_bitrate_bps =
+        std::min(updated.min_bitrate_bps, updated.max_bitrate_bps);
+  }
+  updated.start_bitrate_bps =
+      cricket::MinPositive(updated.start_bitrate_bps, updated.max_bitrate_bps);
+  updated.start_bitrate_bps =
+      std::max(updated.start_bitrate_bps, updated.min_bitrate_bps);
+
   transport_send_->send_side_cc()->SetBweBitrates(
-      bitrate_config.min_bitrate_bps, bitrate_config.start_bitrate_bps,
-      bitrate_config.max_bitrate_bps);
+      updated.min_bitrate_bps,
+      update.start_bitrate_bps ? updated.start_bitrate_bps : -1,
+      updated.max_bitrate_bps);
+  config_.bitrate_config = updated;
 }
 
 void Call::SignalChannelNetworkState(MediaType media, NetworkState state) {
   RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());
   switch (media) {
     case MediaType::AUDIO:
       audio_network_state_ = state;
       break;
     case MediaType::VIDEO:
       video_network_state_ = state;
@@ skipping 390 matching lines @@
       (use_send_side_bwe && header.extension.hasTransportSequenceNumber)) {
     receive_side_cc_.OnReceivedPacket(
         packet.arrival_time_ms(), packet.payload_size() + packet.padding_size(),
         header);
   }
 }
 
 }  // namespace internal
 
 }  // namespace webrtc