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 182 matching lines @@
                                const uint8_t* packet,
                                size_t length,
                                const PacketTime& packet_time) override;
 
   // Implements RecoveredPacketReceiver.
   void 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();
 
+  // Applies update to the BitrateConfig cached in |config_|, restarting
+  // bandwidth estimation from |new_start| if set.
+  void UpdateCurrentBitrateConfig(const rtc::Optional<int>& new_start);
+
   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 75 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") {
-  RTC_DCHECK_RUN_ON(&configuration_thread_checker_);
+      worker_queue_("call_worker_queue"),
+      base_bitrate_config_(config.bitrate_config) {
+  RTC_DCHECK(&configuration_thread_checker_);
   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();
   transport_send->send_side_cc()->RegisterNetworkObserver(this);
@@ skipping 487 matching lines @@
     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_RUN_ON(&configuration_thread_checker_);
   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.
+  }
+
+  rtc::Optional<int> new_start;
+  // Only update the "start" bitrate if it's set, and different from the old
+  // value. In practice, this value comes from the x-google-start-bitrate codec
+  // parameter in SDP, and setting the same remote description twice shouldn't
+  // restart bandwidth estimation.
+  if (bitrate_config.start_bitrate_bps != -1 &&
+      bitrate_config.start_bitrate_bps !=
+          base_bitrate_config_.start_bitrate_bps) {
+    new_start.emplace(bitrate_config.start_bitrate_bps);
+  }
+  base_bitrate_config_ = bitrate_config;
+  UpdateCurrentBitrateConfig(new_start);
+}
+
+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(mask.start_bitrate_bps);
+}
+
+void Call::UpdateCurrentBitrateConfig(const rtc::Optional<int>& new_start) {
+  Config::BitrateConfig updated;
+  updated.min_bitrate_bps =
+      std::max(bitrate_config_mask_.min_bitrate_bps.value_or(0),
+               base_bitrate_config_.min_bitrate_bps);
+
+  updated.max_bitrate_bps =
+      cricket::MinPositive(bitrate_config_mask_.max_bitrate_bps.value_or(-1),
+                           base_bitrate_config_.max_bitrate_bps);
+
+  // If the combined min ends up greater than the combined max, the max takes
+  // priority.
+  if (updated.max_bitrate_bps != -1 &&
+      updated.min_bitrate_bps > updated.max_bitrate_bps) {
+    updated.min_bitrate_bps = updated.max_bitrate_bps;
+  }
+
+  // If these's nothing to update (min/max unchanged, no new bandwidth

[holmer, 2017/05/31 07:46:33]

there's

[Zach Stein, 2017/06/01 03:37:49]

Done.

+  // estimation start value), return early.
+  if (updated.min_bitrate_bps == config_.bitrate_config.min_bitrate_bps &&
+      updated.max_bitrate_bps == config_.bitrate_config.max_bitrate_bps &&
+      !new_start) {
+    LOG(LS_VERBOSE) << "WebRTC.Call.UpdateCurrentBitrateConfig: "
+                    << "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);
-  transport_send_->send_side_cc()->SetBweBitrates(
-      bitrate_config.min_bitrate_bps, bitrate_config.start_bitrate_bps,
-      bitrate_config.max_bitrate_bps);
+
+  if (new_start) {
+    // Clamp start by min and max.
+    updated.start_bitrate_bps = cricket::MinPositive(

[holmer, 2017/05/31 07:46:34]

I haven't seen us use the cricket namespace here before. Should it be moved to
the rtc namespace and be put under webrtc/base?

[Zach Stein, 2017/06/01 03:37:49]

MinPositive is currently defined in mediachannel.h and used by
webrtcvideonengine and webrtcvideoengine2 (which are in the cricket namespace).
webrtc/base/mathutils.h looks most suitable to me, but that file doesn't
currently use a namespace... Mind if I consider this in a later CL? Seems out of
scope here (unless you see something natural to do).

[kwiberg-webrtc, 2017/06/01 07:50:25]

Can either of you explain what MinPositive is supposed to do? The definition
looks like this:

template <typename T>
static T MinPositive(T a, T b) {
  if (a <= 0) {
    return b;
  }
  if (b <= 0) {
    return a;
  }
  return std::min(a, b);
}

IOW, if both are positive, it returns the smaller one; if one is negative and
one is positive, it returns the positive one (that is, the greater one); and if
both are negative, it returns the second one regardless of which number is
greater.

This behavior seems really complex, and I can't think of a situation where it
would be useful.

[Taylor Brandstetter, 2017/06/01 16:24:43]

It's useful in situations where you're taking the minimum of two maximums, and
"-1" means "no maximum set". Which does occur here and some other places.

If we changed these maximums to use "rtc::Optional", MinPositive could become
"MinSet" or "MinNonNull" and then maybe it would make more sense.

[stefan-webrtc, 2017/06/01 16:32:47]

I'd prefer not using the cricket namespace here, so in that case I'd rather land
this with code duplication until we have moved it to a different namespace.

[Zach Stein, 2017/06/01 20:12:44]

Peter pointed out that this could also be moved to the webrtc namespace in
call.h. Does that make sense to you (for a future CL) or do you think somewhere
in webrtc/base is better?

[kwiberg-webrtc, 2017/06/01 22:55:44]

Ah, OK. Negative values are "not set", and if given two negative values we don't
care which one we return, because they all just mean "not set".

I agree that Optional would make such logic easier to follow...

+        std::max(*new_start, updated.min_bitrate_bps), updated.max_bitrate_bps);
+  } else {
+    updated.start_bitrate_bps = -1;
+  }
+
+  LOG(INFO) << "WebRTC.Call.UpdateCurrentBitrateConfig: "
+            << "calling SetBweBitrates with args (" << updated.min_bitrate_bps
+            << ", " << updated.start_bitrate_bps << ", "
+            << updated.max_bitrate_bps << ")";
+  transport_send_->send_side_cc()->SetBweBitrates(updated.min_bitrate_bps,
+                                                  updated.start_bitrate_bps,
+                                                  updated.max_bitrate_bps);
+  config_.bitrate_config = updated;
 }
 
 void Call::SignalChannelNetworkState(MediaType media, NetworkState state) {
   RTC_DCHECK_RUN_ON(&configuration_thread_checker_);
   switch (media) {
     case MediaType::AUDIO:
       audio_network_state_ = state;
       break;
     case MediaType::VIDEO:
       video_network_state_ = state;
@@ skipping 369 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