Add PacketRouter::SetMaxDesiredReceiveBitrate for application limited receive bandwidth

webrtc/modules/pacing/packet_router_unittest.cc

Index: webrtc/modules/pacing/packet_router_unittest.cc
diff --git a/webrtc/modules/pacing/packet_router_unittest.cc b/webrtc/modules/pacing/packet_router_unittest.cc
index de3206688a5e98bec0a833ab0480fa854430fec4..e90a25ecc2d737139f48571eae800e508ee54e21 100644
--- a/webrtc/modules/pacing/packet_router_unittest.cc
+++ b/webrtc/modules/pacing/packet_router_unittest.cc
@@ -20,14 +20,6 @@
 #include "webrtc/test/gmock.h"
 #include "webrtc/test/gtest.h"
 
-using ::testing::_;
-using ::testing::AnyNumber;
-using ::testing::Field;
-using ::testing::NiceMock;
-using ::testing::Return;
-using ::testing::ReturnPointee;
-using ::testing::SaveArg;
-
 namespace webrtc {
 
 // TODO(eladalon): Restructure and/or replace the existing monolithic tests
@@ -36,6 +28,18 @@ namespace webrtc {
 // (I'm not removing any tests during CL, so as to demonstrate no regressions.)
 
 namespace {
+
+using ::testing::_;

[eladalon, 2017/08/03 12:59:02]

Is this move meaningfull? We're inside of a .cc file, so this shouldn't really
matter, I'm guessing? But perhaps it's a general policy to keep things to the
minimum possible scope?

[danilchap, 2017/08/03 14:16:19]

yes, it is general policy, this style might help to avoid hard-to find name
collisions without introducing downsides.
moved it because extended it.

[eladalon, 2017/08/04 08:32:49]

Acknowledged.

+using ::testing::AnyNumber;
+using ::testing::AtLeast;
+using ::testing::Field;
+using ::testing::Gt;
+using ::testing::Le;
+using ::testing::NiceMock;
+using ::testing::Return;
+using ::testing::ReturnPointee;
+using ::testing::SaveArg;
+
 constexpr int kProbeMinProbes = 5;
 constexpr int kProbeMinBytes = 1000;
 
@@ -562,6 +566,128 @@ TEST(PacketRouterRembTest, OnlyOneRembForRepeatedOnReceiveBitrateChanged) {
   packet_router.RemoveSendRtpModule(&rtp);
 }
 
+TEST(PacketRouterRembTest, SetMaxEstimatedBandwidthLimitsSetREMBData) {
+  rtc::ScopedFakeClock clock;
+  PacketRouter packet_router;
+  clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
+  NiceMock<MockRtpRtcpWithRembTracking> remb_sender;
+  packet_router.AddSendRtpModule(&remb_sender, /*remb_candidate=*/true);

[eladalon, 2017/08/03 12:59:02]

nit: Prefer keeping files self-consistent, and either use |constexpr bool
remb_candidate| everywhere (current implementation) or nowhere (possible change
by you). I personally prefer the former (everywhere), but either would be better
than a mix of the two.

[danilchap, 2017/08/03 14:16:19]

changed to keep old style for consistency, thanks.

+  ASSERT_TRUE(remb_sender.REMB());
+
+  const uint32_t cap_bitrate = 100000;
+  EXPECT_CALL(remb_sender, SetREMBData(Le(cap_bitrate), _)).Times(AtLeast(1));
+  EXPECT_CALL(remb_sender, SetREMBData(Gt(cap_bitrate), _)).Times(0);

[eladalon, 2017/08/03 12:59:02]

On the one hand, you illuminate the intention of your test. On the other,
extremely wrong behavior of the code under test could still be perceived as
legal - suppose the bitrate is capped at 0, 10, or anything else that's way
below cap_bitrate. That would clearly be wrong, but it would pass. IMHO, it
would be better to EXPECT exactly what we expect here.

[danilchap, 2017/08/03 14:16:19]

Acknowledged.
I do that in test below, where there are no OnReceiveBitrateChanged calls.
Here I prefer to test exactly what test promise and what is the main feature of
the function.

+
+  const std::vector<uint32_t> ssrcs = {1234};
+  packet_router.SetMaxEstimatedBandwidth(cap_bitrate);
+  packet_router.OnReceiveBitrateChanged(ssrcs, cap_bitrate + 5000);
+  clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
+  packet_router.OnReceiveBitrateChanged(ssrcs, cap_bitrate - 5000);
+  // Cleanup.

[eladalon, 2017/08/03 12:59:02]

nit #1: Empty line before the cleanup block.
nit #2: "Cleanup" everywhere or "test tear-down" everywhere, imho, would be
better than a mix.

[danilchap, 2017/08/03 14:16:19]

Done.

+  packet_router.RemoveSendRtpModule(&remb_sender);
+}
+
+TEST(PacketRouterRembTest,
+     SetMaxEstimatedBandwidthTriggersRembWhenMoreRestrictive) {
+  rtc::ScopedFakeClock clock;
+  PacketRouter packet_router;
+  clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
+  NiceMock<MockRtpRtcpWithRembTracking> remb_sender;
+  packet_router.AddSendRtpModule(&remb_sender, /*remb_candidate=*/true);
+  ASSERT_TRUE(remb_sender.REMB());
+
+  const std::vector<uint32_t> ssrcs = {1234};
+  EXPECT_CALL(remb_sender, SetREMBData(150000, _));
+  packet_router.OnReceiveBitrateChanged(ssrcs, 150000);
+
+  EXPECT_CALL(remb_sender, SetREMBData(100000, _));
+  packet_router.SetMaxEstimatedBandwidth(100000);
+  // Cleanup.
+  packet_router.RemoveSendRtpModule(&remb_sender);
+}
+
+TEST(PacketRouterRembTest,
+     SetMaxEstimatedBandwidthDoesNotTriggerRembWhenAsRestrictive) {
+  rtc::ScopedFakeClock clock;
+  PacketRouter packet_router;
+  clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
+  NiceMock<MockRtpRtcpWithRembTracking> remb_sender;
+  packet_router.AddSendRtpModule(&remb_sender, /*remb_candidate=*/true);
+  ASSERT_TRUE(remb_sender.REMB());
+
+  const uint32_t measured_bitrate_bps = 150000;

[eladalon, 2017/08/03 12:59:02]

nit: constexpr

[danilchap, 2017/08/03 14:16:19]

not sure why is it better in this case,
checked tests around - some use constexpr, some const, some neither.

personally when it doesn't matter prefer to use constexpr for values that never
every really need to change (e.g. number of bits in byte),
but keep const for values that can have different value, but just locally happen
to be same.

[eladalon, 2017/08/04 08:32:49]

That could have been a good convention, but there are problems with it.
1. The natural problem with introducing new conventions - not everybody uses it,
so it can't be relied on.
2. As you no-doubt know, constexpr/const are not interchangeable for the code,
so we can't choose between them purely to fit the convention. |const| is needed
whenever you want an address, for example; constexpr whenever you want things to
be set in compile-time.

I prefer using constexpr whenever possible. A good compiler would probably not
even need either keyword in the kind of cases we're discussing at the moment
(local variables in a function - the compiler can see everything we can see, and
optimize better than us), but a developer seeing constexpr would immediately
know that the address of the const in question is never used - and that's
independent of convention.

But excuse this long discussion; I guess it's not really important, one way or
the other.

[danilchap, 2017/08/04 10:51:23]

Acknowledged.

+  const std::vector<uint32_t> ssrcs = {1234};
+  EXPECT_CALL(remb_sender, SetREMBData(measured_bitrate_bps, _));
+  packet_router.OnReceiveBitrateChanged(ssrcs, measured_bitrate_bps);
+
+  EXPECT_CALL(remb_sender, SetREMBData(measured_bitrate_bps, _)).Times(0);
+  packet_router.SetMaxEstimatedBandwidth(measured_bitrate_bps);

[eladalon, 2017/08/03 12:59:02]

Suggestion - if you created another const(expr), called it "max...", and set it
to be equal to |measured_bitrate_bps|, nothing would change in the binary, but
it would be clear this is not a typo.

[danilchap, 2017/08/03 14:16:18]

Done.

+  // Cleanup.
+  packet_router.RemoveSendRtpModule(&remb_sender);
+}
+
+TEST(PacketRouterRembTest,
+     SetMaxEstimatedBandwidthDoesNotTriggerRembWhenLessRestrictive) {
+  rtc::ScopedFakeClock clock;
+  PacketRouter packet_router;
+  clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
+  NiceMock<MockRtpRtcpWithRembTracking> remb_sender;
+  packet_router.AddSendRtpModule(&remb_sender, /*remb_candidate=*/true);
+  ASSERT_TRUE(remb_sender.REMB());
+
+  const uint32_t measured_bitrate_bps = 150000;
+  const std::vector<uint32_t> ssrcs = {1234};
+  EXPECT_CALL(remb_sender, SetREMBData(measured_bitrate_bps, _));
+  packet_router.OnReceiveBitrateChanged(ssrcs, measured_bitrate_bps);
+
+  EXPECT_CALL(remb_sender, SetREMBData(measured_bitrate_bps + 500, _)).Times(0);
+  packet_router.SetMaxEstimatedBandwidth(measured_bitrate_bps + 500);
+  // Cleanup.
+  packet_router.RemoveSendRtpModule(&remb_sender);
+}
+
+TEST(PacketRouterRembTest,
+     SetMaxEstimatedBandwidthTriggersRembWhenNoRecentMeasure) {
+  rtc::ScopedFakeClock clock;
+  PacketRouter packet_router;
+  clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
+  NiceMock<MockRtpRtcpWithRembTracking> remb_sender;
+  packet_router.AddSendRtpModule(&remb_sender, /*remb_candidate=*/true);
+  ASSERT_TRUE(remb_sender.REMB());
+
+  const uint32_t measured_bitrate_bps = 150000;
+  const std::vector<uint32_t> ssrcs = {1234};
+  EXPECT_CALL(remb_sender, SetREMBData(measured_bitrate_bps, _));
+  packet_router.OnReceiveBitrateChanged(ssrcs, measured_bitrate_bps);
+  clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
+
+  EXPECT_CALL(remb_sender, SetREMBData(measured_bitrate_bps + 500, _));
+  packet_router.SetMaxEstimatedBandwidth(measured_bitrate_bps + 500);
+  // Cleanup.
+  packet_router.RemoveSendRtpModule(&remb_sender);
+}
+
+TEST(PacketRouterRembTest, SetMaxEstimatedBandwidthTriggersRembWhenNoMeasures) {

[eladalon, 2017/08/03 12:59:02]

So this is intentional?
1. Might be good to document this in PacketRouter.
2. Do we mean to let this override the minimum time between REMB messages?

[danilchap, 2017/08/03 14:16:18]

1. It sort of implicit - if you set a limit how much you can receive, you expect
sender will be notified about the limit.
I've update documentation a bit. Any suggestion how I can explain it better?
2. Yes, if application apply the limit, it likely want to propagate it asap, but
do not want to give those promises in the documentation, e.g. SetREMBData
doesn't actually sends a remb, it can have own delay and own throttle.

[eladalon, 2017/08/04 08:32:49]

Acknowledged.
I've suggested a rephrasing next to the function's documentation.

+  rtc::ScopedFakeClock clock;
+  PacketRouter packet_router;
+  clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
+  NiceMock<MockRtpRtcpWithRembTracking> remb_sender;
+  packet_router.AddSendRtpModule(&remb_sender, /*remb_candidate=*/true);
+  ASSERT_TRUE(remb_sender.REMB());
+
+  // Set cap.
+  EXPECT_CALL(remb_sender, SetREMBData(100000, _)).Times(1);
+  packet_router.SetMaxEstimatedBandwidth(100000);
+  // Increase cap.
+  EXPECT_CALL(remb_sender, SetREMBData(200000, _)).Times(1);
+  packet_router.SetMaxEstimatedBandwidth(200000);
+  // Decrease cap.
+  EXPECT_CALL(remb_sender, SetREMBData(150000, _)).Times(1);
+  packet_router.SetMaxEstimatedBandwidth(150000);
+  // Cleanup.
+  packet_router.RemoveSendRtpModule(&remb_sender);
+}
+
 // Only register receiving modules and make sure we fallback to trigger a REMB
 // packet on this one.
 TEST(PacketRouterRembTest, NoSendingRtpModule) {