Add PeerConnectionInterface::UpdateCallBitrate with call tests.

webrtc/call/call.cc

Index: webrtc/call/call.cc
diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc
index e594dcc6cadce83560ac48f325c60c4637a566d2..17dc3689b7563a72cc86ad7883a21744c6eb290a 100644
--- a/webrtc/call/call.cc
+++ b/webrtc/call/call.cc
@@ -142,6 +142,9 @@ class Call : public webrtc::Call,
   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,
@@ -188,6 +191,17 @@ class Call : public webrtc::Call,
   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_;
@@ -282,6 +296,14 @@ class Call : public webrtc::Call,
   // 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
@@ -337,7 +359,8 @@ Call::Call(const Call::Config& config,
       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);
@@ -862,29 +885,99 @@ 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) {