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 cached |masked_bitrate_config_| struct and updates the
+  // congestion controller if the masked config changed.
+  void UpdateMaskedBitrateConfig();
+
   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 mask is kept so it can be re-applied if SetBitrateConfig is called.
+  Config::BitrateConfigMask bitrate_config_mask_;
+
+  // This is the BitrateConfig given to the congestion controller. We cache it
+  // here to avoid resets in the congestion controller when nothing has actually
+  // changed.
+  Config::BitrateConfig masked_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"),
+      masked_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_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.
     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);
-  transport_send_->send_side_cc()->SetBweBitrates(
-      bitrate_config.min_bitrate_bps, bitrate_config.start_bitrate_bps,
+
+  UpdateMaskedBitrateConfig();
+}
+
+void Call::SetBitrateConfigMask(
+    const webrtc::Call::Config::BitrateConfigMask& mask) {
+  TRACE_EVENT0("webrtc", "Call::UpdateBitrateConfig");
+  RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());
+
+  const bool has_min = static_cast<bool>(mask.min_bitrate_bps);
+  const bool has_start = static_cast<bool>(mask.start_bitrate_bps);
+  const bool has_max = static_cast<bool>(mask.max_bitrate_bps);
+  if (has_min) {
+    RTC_DCHECK_GE(*mask.min_bitrate_bps, 0);

[Taylor Brandstetter, 2017/04/12 01:33:41]

In general we don't assert as a result of bad input at the API level. Could the
method either return an error (see "RTCError"), or do the clamping I've seen
elsewhere with BitrateConfigs?

[Zach Stein, 2017/04/18 22:54:50]

Done.

+  }
+  if (has_start) {
+    RTC_DCHECK_GE(*mask.start_bitrate_bps, has_min ? *mask.min_bitrate_bps : 0);
+  }
+  if (has_max) {
+    RTC_DCHECK_GE(*mask.max_bitrate_bps,
+                  has_start ? *mask.start_bitrate_bps
+                            : (has_min ? *mask.min_bitrate_bps : 0));
+  }
+
+  bitrate_config_mask_ = mask;
+
+  UpdateMaskedBitrateConfig();
+}
+
+void Call::UpdateMaskedBitrateConfig() {
+  Config::BitrateConfig new_masked;
+  const Config::BitrateConfig& bitrate_config = config_.bitrate_config;

[Taylor Brandstetter, 2017/04/12 01:33:42]

It seems unintuitive that config_.bitrate_config isn't the one actually being
used. I'd prefer reversing it, personally, and storing the "from SDP"
BitrateConfig in a separate member variable.

[Zach Stein, 2017/04/18 22:54:50]

Done.

+
+  new_masked.min_bitrate_bps = bitrate_config_mask_.min_bitrate_bps.value_or(
+      bitrate_config.min_bitrate_bps);
+  new_masked.start_bitrate_bps =
+      bitrate_config_mask_.start_bitrate_bps.value_or(
+          bitrate_config.start_bitrate_bps);
+  new_masked.max_bitrate_bps = bitrate_config_mask_.max_bitrate_bps.value_or(
       bitrate_config.max_bitrate_bps);

[Taylor Brandstetter, 2017/04/12 01:33:42]

I'm not sure the "mask" should completely override the max value that came from
SDP. In general, the PeerConnection APIs operate within the envelope negotiated
by SDP. So if a "b=AS" bandwidth limit is present, it should be honored even if
the application called SetCallBitrate. Meaning, take the minimum of the two
maximums here.

[Zach Stein, 2017/04/13 00:26:36]

We don't know that bitrate_config.max_bitrate_bps was set by the SDP here - it
might have come from a codec param (but maybe that is the code that should be
changed).

[Taylor Brandstetter, 2017/04/13 22:16:13]

The codec params also come from SDP (from "a=fmtp" lines). I do understand your
point that both "x-google-max-bitrate" and "b=AS" end up condensed to a single
value, but "x-google-max-bitrate" is non-standard, and more of a corner case, so
we should optimize for the "b=AS" case if anything.

[Zach Stein, 2017/04/18 22:54:50]

I didn't realize that the codec params also came from SDP.

+
+  if (new_masked.min_bitrate_bps != masked_bitrate_config_.min_bitrate_bps ||
+      new_masked.start_bitrate_bps !=
+          masked_bitrate_config_.start_bitrate_bps ||

[Taylor Brandstetter, 2017/04/12 01:33:42]

Should this be the same condition as in SetBitrateConfig, including "||
bitrate_config.start_bitrate_bps <= 0"? May be worth making a helper function.

[Zach Stein, 2017/04/18 22:54:50]

Done.

+      new_masked.max_bitrate_bps != masked_bitrate_config_.max_bitrate_bps) {
+    RTC_DCHECK_GT(new_masked.min_bitrate_bps, 0);
+    RTC_DCHECK_GT(new_masked.start_bitrate_bps, new_masked.min_bitrate_bps);
+    RTC_DCHECK(new_masked.max_bitrate_bps == -1 ||
+               new_masked.max_bitrate_bps > new_masked.start_bitrate_bps);
+
+    masked_bitrate_config_ = new_masked;
+    transport_send_->send_side_cc()->SetBweBitrates(
+        masked_bitrate_config_.min_bitrate_bps,
+        masked_bitrate_config_.start_bitrate_bps,
+        masked_bitrate_config_.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