Add PeerConnectionInterface::UpdateCallBitrate.

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 162 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;
+
+  // 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,
                                   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 121 matching lines @@
   VieRemb remb_;
   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.
+  Config::BitrateConfigMask bitrate_config_mask_;
+
+  // The config set by SetBitrateConfig.
+  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 33 matching lines @@
       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),
       transport_send_(std::move(transport_send)),
       remb_(clock_),
       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);
   RTC_DCHECK_GT(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 502 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) {
+  }
+
+  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) {
   RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());
   switch (media) {
     case MediaType::AUDIO:
       audio_network_state_ = state;
       break;
     case MediaType::VIDEO:
       video_network_state_ = state;
@@ skipping 392 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