Add PeerConnectionInterface::UpdateCallBitrate with call tests.

webrtc/call/call_unittest.cc

Index: webrtc/call/call_unittest.cc
diff --git a/webrtc/call/call_unittest.cc b/webrtc/call/call_unittest.cc
index 27d23298112f13b4f4fec673a6a5d54550366040..d10c5e42b591bce04882c49d9257f016e270001a 100644
--- a/webrtc/call/call_unittest.cc
+++ b/webrtc/call/call_unittest.cc
@@ -11,11 +11,14 @@
 #include <list>
 #include <map>
 #include <memory>
+#include <utility>
 
+#include "webrtc/base/ptr_util.h"
 #include "webrtc/call/audio_state.h"
 #include "webrtc/call/call.h"
 #include "webrtc/logging/rtc_event_log/rtc_event_log.h"
 #include "webrtc/modules/audio_mixer/audio_mixer_impl.h"
+#include "webrtc/modules/congestion_controller/include/send_side_congestion_controller.h"
 #include "webrtc/test/gtest.h"
 #include "webrtc/test/mock_audio_decoder_factory.h"
 #include "webrtc/test/mock_transport.h"
@@ -305,4 +308,173 @@ TEST(CallTest, MultipleFlexfecReceiveStreamsProtectingSingleVideoStream) {
   }
 }
 
+// TODO(zstein): This probably belongs in webrtc/modules/congestion_controller/
+class MockSendSideCongestionController : public SendSideCongestionController {
+ public:
+  MockSendSideCongestionController(const Clock* clock,
+                                   Observer* observer,
+                                   RtcEventLog* event_log,
+                                   PacketRouter* packet_router)
+      : SendSideCongestionController(clock,
+                                     observer,
+                                     event_log,
+                                     packet_router) {}
+
+  MOCK_METHOD3(SetBweBitrates, void(int, int, int));
+};
+
+class FakeRtpTransportController : public RtpTransportControllerSendInterface {
+ public:
+  explicit FakeRtpTransportController(PacketRouter* r,
+                                      SendSideCongestionController* cc)
+      : packet_router_(r), send_side_cc_(cc) {}
+
+  PacketRouter* packet_router() override { return packet_router_; }
+
+  SendSideCongestionController* send_side_cc() override {
+    return send_side_cc_;
+  }
+
+  TransportFeedbackObserver* transport_feedback_observer() override {
+    return send_side_cc_;
+  }
+
+  RtpPacketSender* packet_sender() override { return nullptr; }
+
+ private:
+  PacketRouter* packet_router_;
+  SendSideCongestionController* send_side_cc_;
+};
+
+class CallBitrateTest : public testing::Test {
+ public:
+  void SetUp() {
+    Clock* clock = Clock::GetRealTimeClock();  // TODO(zstein): test clock?
+    mock_cc_ =
+        rtc::MakeUnique<testing::NiceMock<MockSendSideCongestionController>>(
+            clock, nullptr, &event_log_, &packet_router_);
+  }
+
+  void SetCall() {

[Taylor_Brandstetter, 2017/04/26 15:46:11]

nit: "CreateCall"?

[Zach Stein, 2017/05/04 22:32:44]

Done.

+    Call::Config config(&event_log_);
+    SetCall(config);
+  }
+
+  void SetCall(const Call::Config& config) {
+    std::unique_ptr<RtpTransportControllerSendInterface> fake_controller =
+        rtc::MakeUnique<FakeRtpTransportController>(&packet_router_,
+                                                    mock_cc_.get());
+    call_.reset(Call::Create(config, std::move(fake_controller)));
+  }
+
+  std::unique_ptr<testing::NiceMock<MockSendSideCongestionController>> mock_cc_;
+  std::unique_ptr<Call> call_;
+
+  webrtc::RtcEventLogNullImpl event_log_;
+
+ private:
+  PacketRouter packet_router_;  // TODO(zstein): fake?
+};
+
+using testing::_;
+
+TEST_F(CallBitrateTest, MinMaskPreferred) {
+  SetCall();
+  Call::Config::BitrateConfigMask mask;
+  mask.min_bitrate_bps = rtc::Optional<int>(1234);
+
+  EXPECT_CALL(*mock_cc_, SetBweBitrates(*mask.min_bitrate_bps, _, _));
+  EXPECT_TRUE(call_->SetBitrateConfigMask(mask).ok());
+}
+
+TEST_F(CallBitrateTest, StartMaskPreferred) {
+  SetCall();
+  Call::Config::BitrateConfigMask mask;
+  mask.start_bitrate_bps = rtc::Optional<int>(1234);
+
+  EXPECT_CALL(*mock_cc_, SetBweBitrates(_, *mask.start_bitrate_bps, _));
+  EXPECT_TRUE(call_->SetBitrateConfigMask(mask).ok());
+}

[Taylor_Brandstetter, 2017/04/26 15:46:11]

Could also test setting the same "start_bitrate_bps" twice and ensuring
SetBweBitrates is called twice (the "force_update") logic.

[Zach Stein, 2017/05/04 22:32:44]

Great idea, thanks.

+
+TEST_F(CallBitrateTest, SmallerMaskMaxUsed) {
+  Call::Config config(&event_log_);
+  config.bitrate_config.max_bitrate_bps =
+      config.bitrate_config.start_bitrate_bps + 2000;
+  SetCall(config);
+
+  Call::Config::BitrateConfigMask mask;
+  mask.max_bitrate_bps =
+      rtc::Optional<int>(config.bitrate_config.start_bitrate_bps + 1000);
+
+  EXPECT_CALL(*mock_cc_, SetBweBitrates(_, _, *mask.max_bitrate_bps));
+  EXPECT_TRUE(call_->SetBitrateConfigMask(mask).ok());
+}
+
+TEST_F(CallBitrateTest, SmallerConfigMaxUsed) {
+  Call::Config config(&event_log_);
+  config.bitrate_config.max_bitrate_bps =
+      config.bitrate_config.start_bitrate_bps + 1000;
+  SetCall(config);
+
+  Call::Config::BitrateConfigMask mask;
+  mask.max_bitrate_bps =
+      rtc::Optional<int>(config.bitrate_config.start_bitrate_bps + 2000);
+
+  // Expect no calls because nothing changes
+  EXPECT_CALL(*mock_cc_, SetBweBitrates(_, _, _)).Times(0);
+  EXPECT_TRUE(call_->SetBitrateConfigMask(mask).ok());
+}
+
+TEST_F(CallBitrateTest, MaskStartLessThanConfigMinFails) {

[Taylor_Brandstetter, 2017/04/26 15:46:11]

I'm not sure if it should work this way. Since there's no way to get the current
config from the PC API level, how can an application prevent these errors? I'd
suggest just clamping the values if there's an inconsistency after applying the
mask.

[Zach Stein, 2017/05/04 22:32:43]

What do you think about having PeerConnection::SetBitrate return the clamped
values it computes instead of an RTCError? If we just clamp the values, the
client might be surprised that the parameters are not being respected when we
say everything is okay (and I'm not sure it is even easy to discover that the
parameters aren't being respected).

[Taylor Brandstetter, 2017/05/05 08:04:32]

That's a good idea. RTCError was actually meant to be used for things like this,
when errors need to provide additional structured information.

However, it's still inconsistent that calling SetBitrateConfig then
SetBitrateConfigMask will return something with the clamped values, but calling
them in the reverse order will silently succeed. How does the application know
the parameters aren't being respected in that case? Surfacing the information
from SetRemoteDescription would be a little strange.

[Zach Stein, 2017/05/09 00:41:26]

I was thinking that inconsistency was "less bad" since we want to discourage
people from setting these values via SetRemoteDescription in the long term. It
still seems most natural to me that the call to
PC::SetBitrate/SetBitrateConfigMask should only be applied to the current remote
description instead of kept around as a global config (in which case clamping
makes less sense), but you have more established intuitions here than I do. So
the idea of returning the clamped values was just to make it more obvious in the
API that the values were clamped. It would be inconsistent with
SetRemoteDescription, but the behavior would be the same and there is no obvious
way to surface the information there...

I am planning on going forward with clamping and not returning anything from
PC::SetBitrate (we could decide to return the clamped values later instead),
unless you think we should consider this more now.

[Taylor Brandstetter, 2017/05/09 15:51:54]

That sounds good to me. I think this is what's most consistent with existing
WebRTC APIs.

+  Call::Config config(&event_log_);
+  config.bitrate_config.min_bitrate_bps = 2000;
+  SetCall(config);
+
+  Call::Config::BitrateConfigMask mask;
+  mask.start_bitrate_bps = rtc::Optional<int>(1000);
+
+  EXPECT_FALSE(call_->SetBitrateConfigMask(mask).ok());
+}
+
+TEST_F(CallBitrateTest, MaskMaxLessThanConfigStartFails) {
+  Call::Config config(&event_log_);
+  config.bitrate_config.start_bitrate_bps = 2000;
+  SetCall(config);
+
+  Call::Config::BitrateConfigMask mask;
+  mask.max_bitrate_bps = rtc::Optional<int>(1000);
+
+  EXPECT_FALSE(call_->SetBitrateConfigMask(mask).ok());
+}
+
+TEST_F(CallBitrateTest, MaskMaxLessThanConfigMinFails) {
+  Call::Config config(&event_log_);
+  config.bitrate_config.min_bitrate_bps = 2000;
+  SetCall(config);
+
+  Call::Config::BitrateConfigMask mask;
+  mask.max_bitrate_bps = rtc::Optional<int>(1000);
+
+  EXPECT_FALSE(call_->SetBitrateConfigMask(mask).ok());
+}
+
+// TODO(zstein): Delete or rename.
+TEST_F(CallBitrateTest, TaylorCR) {
+  SetCall();
+
+  Call::Config::BitrateConfig config1;
+  config1.min_bitrate_bps = 0;
+  config1.start_bitrate_bps = 1000;
+  config1.max_bitrate_bps = -1;
+
+  Call::Config::BitrateConfig config2;
+  config2.min_bitrate_bps = 0;
+  config2.start_bitrate_bps = -1;
+  config2.max_bitrate_bps = -1;
+
+  EXPECT_CALL(*mock_cc_, SetBweBitrates(0, 1000, -1));
+  call_->SetBitrateConfig(config1);
+  call_->SetBitrateConfig(config2);
+}
+
 }  // namespace webrtc