New api struct BitrateSettings.

Replaces both BitrateConstraintsMask and
PeerConnectionInterface::BitrateParameters. The latter is kept
temporarily for backwards compatibility.

Bug: None
Change-Id: Ibe1d043f2a76e56ff67809774e9c0f5e0ec9e00f
Reviewed-on: https://webrtc-review.googlesource.com/74020
Commit-Queue: Niels Moller <nisse@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23148}

api/BUILD.gn

@@ -108,6 +108,7 @@ rtc_static_library("libjingle_peerconnection_api") {
     ":video_frame_api",
     "audio:audio_mixer_api",
     "audio_codecs:audio_codecs_api",
+    "transport:bitrate_settings",
 
     # Basically, don't add stuff here. You might break sensitive downstream
     # targets like pnacl. API should not depend on anything outside of this

api/peerconnectioninterface.h

@@ -90,6 +90,7 @@
 #include "api/setremotedescriptionobserverinterface.h"
 #include "api/stats/rtcstatscollectorcallback.h"
 #include "api/statstypes.h"
+#include "api/transport/bitrate_settings.h"
 #include "api/turncustomizer.h"
 #include "api/umametrics.h"
 #include "logging/rtc_event_log/rtc_event_log_factory_interface.h"
@@ -994,7 +995,24 @@ class PeerConnectionInterface : public rtc::RefCountInterface {
   //
   // Setting |current_bitrate_bps| will reset the current bitrate estimate
   // to the provided value.
-  virtual RTCError SetBitrate(const BitrateParameters& bitrate) = 0;
+  virtual RTCError SetBitrate(const BitrateSettings& bitrate) {
+    BitrateParameters bitrate_parameters;
+    bitrate_parameters.min_bitrate_bps = bitrate.min_bitrate_bps;
+    bitrate_parameters.current_bitrate_bps = bitrate.start_bitrate_bps;
+    bitrate_parameters.max_bitrate_bps = bitrate.max_bitrate_bps;
+    return SetBitrate(bitrate_parameters);
+  }
+
+  // TODO(nisse): Deprecated - use version above. These two default
+  // implementations require subclasses to implement one or the other
+  // of the methods.
+  virtual RTCError SetBitrate(const BitrateParameters& bitrate_parameters) {
+    BitrateSettings bitrate;
+    bitrate.min_bitrate_bps = bitrate_parameters.min_bitrate_bps;
+    bitrate.start_bitrate_bps = bitrate_parameters.current_bitrate_bps;
+    bitrate.max_bitrate_bps = bitrate_parameters.max_bitrate_bps;
+    return SetBitrate(bitrate);
+  }
 
   // Sets current strategy. If not set default WebRTC allocator will be used.
   // May be changed during an active session. The strategy

api/peerconnectionproxy.h

@@ -135,7 +135,7 @@ BEGIN_SIGNALING_PROXY_MAP(PeerConnection)
   PROXY_METHOD1(void, SetAudioPlayout, bool)
   PROXY_METHOD1(void, SetAudioRecording, bool)
   PROXY_METHOD1(void, RegisterUMAObserver, UMAObserver*)
-  PROXY_METHOD1(RTCError, SetBitrate, const BitrateParameters&);
+  PROXY_METHOD1(RTCError, SetBitrate, const BitrateSettings&);
   PROXY_METHOD1(void,
                 SetBitrateAllocationStrategy,
                 std::unique_ptr<rtc::BitrateAllocationStrategy>);

api/transport/BUILD.gn

@@ -0,0 +1,20 @@
+# Copyright (c) 2018 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.
+
+import("../../webrtc.gni")
+
+rtc_source_set("bitrate_settings") {
+  visibility = [ "*" ]
+  sources = [
+    "bitrate_settings.cc",
+    "bitrate_settings.h",
+  ]
+  deps = [
+    "..:optional",
+  ]
+}

api/transport/bitrate_settings.cc

@@ -8,11 +8,12 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
-#include "call/bitrate_constraints.h"
+#include "api/transport/bitrate_settings.h"
 
 namespace webrtc {
-BitrateConstraintsMask::BitrateConstraintsMask() = default;
-BitrateConstraintsMask::~BitrateConstraintsMask() = default;
-BitrateConstraintsMask::BitrateConstraintsMask(const BitrateConstraintsMask&) =
-    default;
+
+BitrateSettings::BitrateSettings() = default;
+BitrateSettings::~BitrateSettings() = default;
+BitrateSettings::BitrateSettings(const BitrateSettings&) = default;
+
 }  // namespace webrtc

api/transport/bitrate_settings.h

@@ -0,0 +1,35 @@
+/*
+ *  Copyright (c) 2018 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.
+ */
+
+#ifndef API_TRANSPORT_BITRATE_SETTINGS_H_
+#define API_TRANSPORT_BITRATE_SETTINGS_H_
+
+#include "api/optional.h"
+
+namespace webrtc {
+
+// Configuration of send bitrate. The |start_bitrate_bps| value is
+// used for multiple purposes, both as a prior in the bandwidth
+// estimator, and for initial configuration of the encoder. We may
+// want to create separate apis for those, and use a smaller struct
+// with only the min and max constraints.
+struct BitrateSettings {
+  BitrateSettings();
+  ~BitrateSettings();
+  BitrateSettings(const BitrateSettings&);
+  // 0 <= min <= start <= max should hold for set parameters.
+  rtc::Optional<int> min_bitrate_bps;
+  rtc::Optional<int> start_bitrate_bps;
+  rtc::Optional<int> max_bitrate_bps;
+};
+
+}  // namespace webrtc
+
+#endif  // API_TRANSPORT_BITRATE_SETTINGS_H_

call/BUILD.gn

@@ -50,7 +50,6 @@ rtc_source_set("call_interfaces") {
 # when interfaces have stabilized. See also TODO for |mock_rtp_interfaces|.
 rtc_source_set("rtp_interfaces") {
   sources = [
-    "bitrate_constraints.cc",
     "bitrate_constraints.h",
     "rtcp_packet_sink_interface.h",
     "rtp_config.cc",
@@ -62,6 +61,7 @@ rtc_source_set("rtp_interfaces") {
   deps = [
     "../api:array_view",
     "../api:optional",
+    "../api/transport:bitrate_settings",
     "../rtc_base:rtc_base_approved",
   ]
 }
@@ -122,6 +122,7 @@ rtc_source_set("bitrate_configurator") {
   ]
   deps = [
     ":rtp_interfaces",
+    "../api/transport:bitrate_settings",
     "../rtc_base:checks",
     "../rtc_base:rtc_base_approved",
   ]

call/bitrate_constraints.h

@@ -13,10 +13,8 @@
 
 #include <algorithm>
 
-#include "api/optional.h"
-
 namespace webrtc {
-// TODO(srte): BitrateConstraints and BitrateConstraintsMask should be merged.
+// TODO(srte): BitrateConstraints and BitrateSettings should be merged.
 // Both represent the same kind data, but are using different default
 // initializer and representation of unset values.
 struct BitrateConstraints {
@@ -28,18 +26,6 @@ struct BitrateConstraints {
   static constexpr int kDefaultStartBitrateBps = 300000;
 };
 
-// BitrateConstraintsMask is used for the local client's bitrate preferences.
-// Semantically it carries the same kind of information as BitrateConstraints,
-// but is used in a slightly different way.
-struct BitrateConstraintsMask {
-  BitrateConstraintsMask();
-  ~BitrateConstraintsMask();
-  BitrateConstraintsMask(const BitrateConstraintsMask&);
-  rtc::Optional<int> min_bitrate_bps;
-  rtc::Optional<int> start_bitrate_bps;
-  rtc::Optional<int> max_bitrate_bps;
-};
-
 // Like std::min, but considers non-positive values to be unset.
 template <typename T>
 static T MinPositive(T a, T b) {

call/rtp_bitrate_configurator.cc

@@ -58,7 +58,7 @@ RtpBitrateConfigurator::UpdateWithSdpParameters(
 
 rtc::Optional<BitrateConstraints>
 RtpBitrateConfigurator::UpdateWithClientPreferences(
-    const BitrateConstraintsMask& bitrate_mask) {
+    const BitrateSettings& bitrate_mask) {
   bitrate_config_mask_ = bitrate_mask;
   return UpdateConstraints(bitrate_mask.start_bitrate_bps);
 }

call/rtp_bitrate_configurator.h

@@ -11,6 +11,7 @@
 #ifndef CALL_RTP_BITRATE_CONFIGURATOR_H_
 #define CALL_RTP_BITRATE_CONFIGURATOR_H_
 
+#include "api/transport/bitrate_settings.h"
 #include "call/bitrate_constraints.h"
 #include "rtc_base/constructormagic.h"
 
@@ -41,7 +42,7 @@ class RtpBitrateConfigurator {
   // Update the bitrate configuration
   // The optional return value is set with new configuration if it was updated.
   rtc::Optional<BitrateConstraints> UpdateWithClientPreferences(
-      const BitrateConstraintsMask& bitrate_mask);
+      const BitrateSettings& bitrate_mask);
 
  private:
   // Applies update to the BitrateConstraints cached in |config_|, resetting
@@ -55,7 +56,7 @@ class RtpBitrateConfigurator {
 
   // The config mask set by SetClientBitratePreferences.
   // 0 <= min <= start <= max
-  BitrateConstraintsMask bitrate_config_mask_;
+  BitrateSettings bitrate_config_mask_;
 
   // The config set by SetSdpBitrateParameters.
   // min >= 0, start != 0, max == -1 || max > 0

call/rtp_bitrate_configurator_unittest.cc

@@ -35,7 +35,7 @@ class RtpBitrateConfiguratorTest : public testing::Test {
       EXPECT_EQ(result->max_bitrate_bps, max_bitrate_bps);
   }
 
-  void UpdateMaskMatches(BitrateConstraintsMask bitrate_mask,
+  void UpdateMaskMatches(BitrateSettings bitrate_mask,
                          rtc::Optional<int> min_bitrate_bps,
                          rtc::Optional<int> start_bitrate_bps,
                          rtc::Optional<int> max_bitrate_bps) {
@@ -120,13 +120,13 @@ TEST_F(RtpBitrateConfiguratorTest,
 }
 
 TEST_F(RtpBitrateConfiguratorTest, BiggerMaskMinUsed) {
-  BitrateConstraintsMask mask;
+  BitrateSettings mask;
   mask.min_bitrate_bps = 1234;
   UpdateMaskMatches(mask, *mask.min_bitrate_bps, nullopt, nullopt);
 }
 
 TEST_F(RtpBitrateConfiguratorTest, BiggerConfigMinUsed) {
-  BitrateConstraintsMask mask;
+  BitrateSettings mask;
   mask.min_bitrate_bps = 1000;
   UpdateMaskMatches(mask, 1000, nullopt, nullopt);
 
@@ -137,7 +137,7 @@ TEST_F(RtpBitrateConfiguratorTest, BiggerConfigMinUsed) {
 
 // The last call to set start should be used.
 TEST_F(RtpBitrateConfiguratorTest, LatestStartMaskPreferred) {
-  BitrateConstraintsMask mask;
+  BitrateSettings mask;
   mask.start_bitrate_bps = 1300;
   UpdateMaskMatches(mask, nullopt, *mask.start_bitrate_bps, nullopt);
 
@@ -153,7 +153,7 @@ TEST_F(RtpBitrateConfiguratorTest, SmallerMaskMaxUsed) {
   bitrate_config.max_bitrate_bps = bitrate_config.start_bitrate_bps + 2000;
   configurator_.reset(new RtpBitrateConfigurator(bitrate_config));
 
-  BitrateConstraintsMask mask;
+  BitrateSettings mask;
   mask.max_bitrate_bps = bitrate_config.start_bitrate_bps + 1000;
 
   UpdateMaskMatches(mask, nullopt, nullopt, *mask.max_bitrate_bps);
@@ -164,7 +164,7 @@ TEST_F(RtpBitrateConfiguratorTest, SmallerConfigMaxUsed) {
   bitrate_config.max_bitrate_bps = bitrate_config.start_bitrate_bps + 1000;
   configurator_.reset(new RtpBitrateConfigurator(bitrate_config));
 
-  BitrateConstraintsMask mask;
+  BitrateSettings mask;
   mask.max_bitrate_bps = bitrate_config.start_bitrate_bps + 2000;
 
   // Expect no return because nothing changes
@@ -176,7 +176,7 @@ TEST_F(RtpBitrateConfiguratorTest, MaskStartLessThanConfigMinClamped) {
   bitrate_config.min_bitrate_bps = 2000;
   configurator_.reset(new RtpBitrateConfigurator(bitrate_config));
 
-  BitrateConstraintsMask mask;
+  BitrateSettings mask;
   mask.start_bitrate_bps = 1000;
   UpdateMaskMatches(mask, 2000, 2000, nullopt);
 }
@@ -186,7 +186,7 @@ TEST_F(RtpBitrateConfiguratorTest, MaskStartGreaterThanConfigMaxClamped) {
   bitrate_config.start_bitrate_bps = 2000;
   configurator_.reset(new RtpBitrateConfigurator(bitrate_config));
 
-  BitrateConstraintsMask mask;
+  BitrateSettings mask;
   mask.max_bitrate_bps = 1000;
 
   UpdateMaskMatches(mask, nullopt, -1, 1000);
@@ -197,14 +197,14 @@ TEST_F(RtpBitrateConfiguratorTest, MaskMinGreaterThanConfigMaxClamped) {
   bitrate_config.min_bitrate_bps = 2000;
   configurator_.reset(new RtpBitrateConfigurator(bitrate_config));
 
-  BitrateConstraintsMask mask;
+  BitrateSettings mask;
   mask.max_bitrate_bps = 1000;
 
   UpdateMaskMatches(mask, 1000, nullopt, 1000);
 }
 
 TEST_F(RtpBitrateConfiguratorTest, SettingMaskStartForcesUpdate) {
-  BitrateConstraintsMask mask;
+  BitrateSettings mask;
   mask.start_bitrate_bps = 1000;
 
   // Config should be returned twice with the same params since
@@ -242,7 +242,7 @@ TEST_F(RtpBitrateConfiguratorTest,
   UpdateConfigMatches(config, nullopt, nullopt, 2000);
 
   // Reduce effective max to 1000 with the mask.
-  BitrateConstraintsMask mask;
+  BitrateSettings mask;
   mask.max_bitrate_bps = 1000;
   UpdateMaskMatches(mask, nullopt, nullopt, 1000);
 
@@ -255,7 +255,7 @@ TEST_F(RtpBitrateConfiguratorTest,
 // When the "start bitrate" mask is removed, new config shouldn't be returned
 // again, since nothing's changing.
 TEST_F(RtpBitrateConfiguratorTest, NewConfigNotReturnedWhenStartMaskRemoved) {
-  BitrateConstraintsMask mask;
+  BitrateSettings mask;
   mask.start_bitrate_bps = 1000;
   UpdateMaskMatches(mask, 0, 1000, -1);
 
@@ -263,11 +263,11 @@ TEST_F(RtpBitrateConfiguratorTest, NewConfigNotReturnedWhenStartMaskRemoved) {
   EXPECT_FALSE(configurator_->UpdateWithClientPreferences(mask).has_value());
 }
 
-// Test that if a new config is returned after BitrateConstraintsMask applies a
+// Test that if a new config is returned after BitrateSettings applies a
 // "start" value, the new config won't return that start value a
 // second time.
 TEST_F(RtpBitrateConfiguratorTest, NewConfigAfterBitrateConfigMaskWithStart) {
-  BitrateConstraintsMask mask;
+  BitrateSettings mask;
   mask.start_bitrate_bps = 1000;
   UpdateMaskMatches(mask, 0, 1000, -1);
 
@@ -288,7 +288,7 @@ TEST_F(RtpBitrateConfiguratorTest,
   configurator_.reset(new RtpBitrateConfigurator(bitrate_config));
 
   // Set min to 2000; it is clamped to the max (1000).
-  BitrateConstraintsMask mask;
+  BitrateSettings mask;
   mask.min_bitrate_bps = 2000;
   UpdateMaskMatches(mask, 1000, -1, 1000);
 

call/rtp_transport_controller_send.cc

@@ -248,7 +248,7 @@ void RtpTransportControllerSend::SetSdpBitrateParameters(
 }
 
 void RtpTransportControllerSend::SetClientBitratePreferences(
-    const BitrateConstraintsMask& preferences) {
+    const BitrateSettings& preferences) {
   rtc::Optional<BitrateConstraints> updated =
       bitrate_configurator_.UpdateWithClientPreferences(preferences);
   if (updated.has_value()) {

call/rtp_transport_controller_send.h

@@ -81,7 +81,7 @@ class RtpTransportControllerSend final
 
   void SetSdpBitrateParameters(const BitrateConstraints& constraints) override;
   void SetClientBitratePreferences(
-      const BitrateConstraintsMask& preferences) override;
+      const BitrateSettings& preferences) override;
 
  private:
   const Clock* const clock_;

call/rtp_transport_controller_send_interface.h

@@ -16,6 +16,7 @@
 #include <string>
 
 #include "api/optional.h"
+#include "api/transport/bitrate_settings.h"
 #include "call/bitrate_constraints.h"
 
 namespace rtc {
@@ -108,7 +109,7 @@ class RtpTransportControllerSendInterface {
   virtual void SetSdpBitrateParameters(
       const BitrateConstraints& constraints) = 0;
   virtual void SetClientBitratePreferences(
-      const BitrateConstraintsMask& preferences) = 0;
+      const BitrateSettings& preferences) = 0;
 };
 
 }  // namespace webrtc

call/test/mock_rtp_transport_controller_send.h

@@ -51,7 +51,7 @@ class MockRtpTransportControllerSend
   MOCK_METHOD1(OnSentPacket, void(const rtc::SentPacket&));
   MOCK_METHOD1(SetSdpBitrateParameters, void(const BitrateConstraints&));
   MOCK_METHOD1(SetClientBitratePreferences,
-               void(const BitrateConstraintsMask&));
+               void(const BitrateSettings&));
 };
 }  // namespace webrtc
 #endif  // CALL_TEST_MOCK_RTP_TRANSPORT_CONTROLLER_SEND_H_

pc/peerconnection.cc

@@ -3019,33 +3019,33 @@ void PeerConnection::SetMetricObserver_n(UMAObserver* observer) {
   }
 }
 
-RTCError PeerConnection::SetBitrate(const BitrateParameters& bitrate) {
+RTCError PeerConnection::SetBitrate(const BitrateSettings& bitrate) {
   if (!worker_thread()->IsCurrent()) {
     return worker_thread()->Invoke<RTCError>(
-        RTC_FROM_HERE, rtc::Bind(&PeerConnection::SetBitrate, this, bitrate));
+        RTC_FROM_HERE, [&](){ return SetBitrate(bitrate); });
   }
 
-  const bool has_min = static_cast<bool>(bitrate.min_bitrate_bps);
-  const bool has_current = static_cast<bool>(bitrate.current_bitrate_bps);
-  const bool has_max = static_cast<bool>(bitrate.max_bitrate_bps);
+  const bool has_min = bitrate.min_bitrate_bps.has_value();
+  const bool has_start = bitrate.start_bitrate_bps.has_value();
+  const bool has_max = bitrate.max_bitrate_bps.has_value();
   if (has_min && *bitrate.min_bitrate_bps < 0) {
     LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER,
                          "min_bitrate_bps <= 0");
   }
-  if (has_current) {
-    if (has_min && *bitrate.current_bitrate_bps < *bitrate.min_bitrate_bps) {
+  if (has_start) {
+    if (has_min && *bitrate.start_bitrate_bps < *bitrate.min_bitrate_bps) {
       LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER,
-                           "current_bitrate_bps < min_bitrate_bps");
-    } else if (*bitrate.current_bitrate_bps < 0) {
+                           "start_bitrate_bps < min_bitrate_bps");
+    } else if (*bitrate.start_bitrate_bps < 0) {
       LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER,
                            "curent_bitrate_bps < 0");
     }
   }
   if (has_max) {
-    if (has_current &&
-        *bitrate.max_bitrate_bps < *bitrate.current_bitrate_bps) {
+    if (has_start &&
+        *bitrate.max_bitrate_bps < *bitrate.start_bitrate_bps) {
       LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER,
-                           "max_bitrate_bps < current_bitrate_bps");
+                           "max_bitrate_bps < start_bitrate_bps");
     } else if (has_min && *bitrate.max_bitrate_bps < *bitrate.min_bitrate_bps) {
       LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER,
                            "max_bitrate_bps < min_bitrate_bps");
@@ -3055,13 +3055,8 @@ RTCError PeerConnection::SetBitrate(const BitrateParameters& bitrate) {
     }
   }
 
-  BitrateConstraintsMask mask;
-  mask.min_bitrate_bps = bitrate.min_bitrate_bps;
-  mask.start_bitrate_bps = bitrate.current_bitrate_bps;
-  mask.max_bitrate_bps = bitrate.max_bitrate_bps;
-
   RTC_DCHECK(call_.get());
-  call_->GetTransportControllerSend()->SetClientBitratePreferences(mask);
+  call_->GetTransportControllerSend()->SetClientBitratePreferences(bitrate);
 
   return RTCError::OK();
 }

pc/peerconnection.h

@@ -177,7 +177,7 @@ class PeerConnection : public PeerConnectionInternal,
 
   void RegisterUMAObserver(UMAObserver* observer) override;
 
-  RTCError SetBitrate(const BitrateParameters& bitrate) override;
+  RTCError SetBitrate(const BitrateSettings& bitrate) override;
 
   void SetBitrateAllocationStrategy(
       std::unique_ptr<rtc::BitrateAllocationStrategy>

pc/peerconnectioninterface_unittest.cc

@@ -3772,7 +3772,7 @@ TEST_P(PeerConnectionInterfaceTest,
   CreatePeerConnection(config, nullptr);
 }
 
-// The current bitrate from Call's BitrateConstraintsMask is currently clamped
+// The current bitrate from BitrateSettings is currently clamped
 // by Call's BitrateConstraints, which comes from the SDP or a default value.
 // This test checks that a call to SetBitrate with a current bitrate that will
 // be clamped succeeds.

pc/test/fakepeerconnectionbase.h

@@ -196,7 +196,7 @@ class FakePeerConnectionBase : public PeerConnectionInternal {
 
   void RegisterUMAObserver(UMAObserver* observer) override {}
 
-  RTCError SetBitrate(const BitrateParameters& bitrate) override {
+  RTCError SetBitrate(const BitrateSettings& bitrate) override {
     return RTCError(RTCErrorType::UNSUPPORTED_OPERATION, "Not implemented");
   }