Add PeerConnectionInterface::UpdateCallBitrate.

webrtc/call/call.cc

Index: webrtc/call/call.cc
diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc
index e9838d9baad0acdadde8f33ddc12bb863068cc92..226388fcc3a018363cfc2766abd862126b5777d9 100644
--- a/webrtc/call/call.cc
+++ b/webrtc/call/call.cc
@@ -180,6 +180,14 @@ 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;
+
+  // Updates the BitrateConfig cached in |config_| by combining
+  // |base_bitrate_config_| and |bitrate_config_mask_| and udpates the

[Taylor Brandstetter, 2017/04/19 01:06:54]

Typo, "udpates"

[Zach Stein, 2017/04/20 20:48:00]

Done.

+  // congestion controller if the combined config changed.
+  void UpdateCurrentBitrateConfig();
+
   void SignalChannelNetworkState(MediaType media, NetworkState state) override;
 
   void OnTransportOverheadChanged(MediaType media,
@@ -321,6 +329,12 @@ class Call : public webrtc::Call,
   // and deleted before any other members.
   rtc::TaskQueue worker_queue_;
 
+  // The config mask set by SetBitrateConfigMask.
+  Config::BitrateConfigMask bitrate_config_mask_;
+
+  // The config set by SetBitrateConfig.
+  Config::BitrateConfig base_bitrate_config_;
+
   RTC_DISALLOW_COPY_AND_ASSIGN(Call);
 };
 }  // namespace internal
@@ -374,7 +388,8 @@ Call::Call(const Call::Config& config,
       receive_side_cc_(clock_, &remb_, 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);
@@ -897,29 +912,84 @@ 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) {
+  }
+
+  const bool same_min =
+      base_bitrate_config_.min_bitrate_bps == bitrate_config.min_bitrate_bps;
+  const bool same_start = base_bitrate_config_.start_bitrate_bps ==
+                          bitrate_config.start_bitrate_bps;
+  const bool same_max =
+      base_bitrate_config_.max_bitrate_bps == bitrate_config.max_bitrate_bps;
+  if (same_min && (bitrate_config.start_bitrate_bps <= 0 || same_start) &&
+      same_max) {
     // Nothing new to set, early abort to avoid encoder reconfigurations.
     return;
   }
-  config_.bitrate_config.min_bitrate_bps = bitrate_config.min_bitrate_bps;
+
+  base_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);
-  transport_send_->send_side_cc()->SetBweBitrates(
-      bitrate_config.min_bitrate_bps, bitrate_config.start_bitrate_bps,
-      bitrate_config.max_bitrate_bps);
+  if (bitrate_config.start_bitrate_bps > 0) {
+    base_bitrate_config_.start_bitrate_bps = bitrate_config.start_bitrate_bps;
+  }
+  base_bitrate_config_.max_bitrate_bps = bitrate_config.max_bitrate_bps;
+
+  UpdateCurrentBitrateConfig();
+}
+
+void Call::SetBitrateConfigMask(
+    const webrtc::Call::Config::BitrateConfigMask& mask) {
+  TRACE_EVENT0("webrtc", "Call::SetBitrateConfigMask");
+  RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());
+
+  bitrate_config_mask_ = mask;
+
+  UpdateCurrentBitrateConfig();
+}
+
+void Call::UpdateCurrentBitrateConfig() {
+  Config::BitrateConfig masked;
+  masked.min_bitrate_bps = bitrate_config_mask_.min_bitrate_bps.value_or(
+      base_bitrate_config_.min_bitrate_bps);
+  masked.start_bitrate_bps = bitrate_config_mask_.start_bitrate_bps.value_or(
+      base_bitrate_config_.start_bitrate_bps);
+
+  const rtc::Optional<int> mask_max = bitrate_config_mask_.max_bitrate_bps;

[Taylor Brandstetter, 2017/04/19 01:06:54]

nit: Can be const ref.

[Zach Stein, 2017/04/20 20:48:00]

Done.

+  const int base_max = base_bitrate_config_.max_bitrate_bps;
+  const bool mask_has_max = static_cast<bool>(mask_max);
+  const bool base_has_max = base_max != -1;
+  if (mask_has_max && base_has_max) {
+    masked.max_bitrate_bps = std::min(*mask_max, base_max);
+  } else if (mask_has_max) {
+    masked.max_bitrate_bps = *mask_max;
+  } else if (base_has_max) {
+    masked.max_bitrate_bps = base_max;
+  } else {
+    masked.max_bitrate_bps = -1;
+  }

[Taylor Brandstetter, 2017/04/19 01:06:53]

nit: I think this code could be simplified. Example:

masked.max_bitrate_bps = MinPositive(
    base_bitrate_config_.max_bitrate_bps
    base_bitrate_config_mask_.max_bitrate_bps.value_or(-1));

Though, if you think this is less readable, what you have is good.

[Zach Stein, 2017/04/20 20:48:00]

Didn't know about MinPositive (defined in mediachannel.h). I also considered:

if (mask_has_max && base_has_max)
  std::min(*mask_max, base_max);
else
  masked_max.value_or(base_max);

But I think what I have is most readable.

[Taylor Brandstetter, 2017/04/21 10:14:07]

Acknowledged.

+
+  Config::BitrateConfig& current = config_.bitrate_config;
+  if (masked.min_bitrate_bps == current.min_bitrate_bps &&
+      masked.start_bitrate_bps == current.start_bitrate_bps &&

[Taylor Brandstetter, 2017/04/19 01:06:54]

Same comment as before: shouldn't this be checking the same conditions as in
SetBitrateConfig, on line 928? In fact, those checks should just be moved here,
since having them in both places would be redundant. 

[Zach Stein, 2017/04/20 20:48:00]

I was avoiding changing the behavior of SetBitrateConfig when
SetBitrateConfigMask is not called since I don't see where it is tested.
The idea here was to avoid calling SetBweBitrates if there is no mask or the
mask sets the same value as was set by SetBitrateConfig (BitrateParameters
enforces 0 <= min <= current <= max for set parameters - I should add that
constraint to BitrateConfigMask).
I will revisit changing SetBitrateConfig.

[Taylor Brandstetter, 2017/04/21 10:14:07]

But I believe it does change the behavior of SetBitrateConfig when
SetBitrateConfigMask isn't called.

Before, calling it with {0, 1000, -1}, then {0, -1, -1} would only call
SetBweBitrates once. But now it will call it twice.

I think ultimately what you want is to not call SetBweBitrates when nothing's
changing. And "start <= 0" means "start's not changing", so it should be
included in the boolean expression.

Then the boolean expression here becomes the same as the one in
SetBitrateConfig, so the one in SetBitrateConfig is redundant and can be
removed.

+      masked.max_bitrate_bps == current.max_bitrate_bps) {
+    return;
+  }
+
+  RTC_DCHECK_GT(masked.min_bitrate_bps, 0);
+  RTC_DCHECK(masked.start_bitrate_bps <= 0 ||
+             masked.start_bitrate_bps >= masked.min_bitrate_bps);
+  RTC_DCHECK(masked.max_bitrate_bps == -1 ||
+             masked.max_bitrate_bps >= masked.start_bitrate_bps);
+
+  current = masked;
+  transport_send_->send_side_cc()->SetBweBitrates(current.min_bitrate_bps,
+                                                  current.start_bitrate_bps,
+                                                  current.max_bitrate_bps);
 }
 
 void Call::SignalChannelNetworkState(MediaType media, NetworkState state) {