Add PacketRouter::SetMaxDesiredReceiveBitrate for application limited receive bandwidth

webrtc/modules/pacing/packet_router.cc

Index: webrtc/modules/pacing/packet_router.cc
diff --git a/webrtc/modules/pacing/packet_router.cc b/webrtc/modules/pacing/packet_router.cc
index 64e1aeacc1cde5277be14adf30b2bf1e8d855ad0..e3b870343e8c304ea56bc483b628b482de69256f 100644
--- a/webrtc/modules/pacing/packet_router.cc
+++ b/webrtc/modules/pacing/packet_router.cc
@@ -10,6 +10,9 @@
 
 #include "webrtc/modules/pacing/packet_router.h"
 
+#include <algorithm>
+#include <limits>
+
 #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h"
@@ -18,10 +21,17 @@
 #include "webrtc/rtc_base/timeutils.h"
 
 namespace webrtc {
+namespace {
+

[eladalon, 2017/08/03 12:40:52]

nit: I'd have preferred with an empty line between the two "namespace" lines,
but then without the empty lines around the "constexpr" line. Of course, this is
just my own preference.

[danilchap, 2017/08/03 13:25:33]

Acknowledged.

+constexpr int kRembSendIntervalMs = 200;
+
+}  // namespace
 
 PacketRouter::PacketRouter()
     : last_remb_time_ms_(rtc::TimeMillis()),
       last_send_bitrate_bps_(0),
+      bitrate_bps_(0),
+      max_bitrate_bps_(std::numeric_limits<uint32_t>::max()),

[eladalon, 2017/08/03 12:40:52]

Suggestion:
  max_bitrate_bps_(std::numeric_limits<decltype(max_bitrate_bps_)>::max()),
1. This way, nobody can change the type only in the .h file and introduce an
error.
2. Also, the reader doesn't need to check if uint32_t is the actual type, or if
the value has a special meaning.

[danilchap, 2017/08/03 13:25:33]

Done.

       active_remb_module_(nullptr),
       transport_seq_(0) {}
 
@@ -142,8 +152,6 @@ uint16_t PacketRouter::AllocateSequenceNumber() {
 
 void PacketRouter::OnReceiveBitrateChanged(const std::vector<uint32_t>& ssrcs,
                                            uint32_t bitrate_bps) {
-  const int kRembSendIntervalMs = 200;
-
   // % threshold for if we should send a new REMB asap.
   const uint32_t kSendThresholdPercent = 97;
 
@@ -173,10 +181,26 @@ void PacketRouter::OnReceiveBitrateChanged(const std::vector<uint32_t>& ssrcs,
     // a module to actually send it.
     last_remb_time_ms_ = now_ms;
     last_send_bitrate_bps_ = bitrate_bps;
+    // Cap the value to send in remb with configured value.
+    bitrate_bps = std::min(bitrate_bps, max_bitrate_bps_);
   }
   SendRemb(bitrate_bps, ssrcs);
 }
 
+void PacketRouter::SetMaxEstimatedBandwidth(uint32_t bitrate_bps) {
+  {
+    rtc::CritScope lock(&remb_crit_);
+    max_bitrate_bps_ = bitrate_bps;
+    if (last_send_bitrate_bps_ > 0 &&
+        rtc::TimeMillis() - last_remb_time_ms_ < kRembSendIntervalMs &&

[eladalon, 2017/08/03 12:40:52]

1. This is a bit confusing; it would be more natural, IMHO, to suppress the REMB
if (a || b || c), where B is "not enough time has passed since the last sent
REMB."
2. The way it's written now, if SetMaxEstimatedBandwidth() is called from, say,
the ctor, wouldn't the (last_send_bitrate_bps_ > 0) fail, causing us to *not*
supress the REMB, and therefore send out an REMB before we have any valid
estimate? (It could be that it's suppressed from inside SendRemb then, but it's
suboptimal to call SendRemb at all.)

[danilchap, 2017/08/03 13:25:33]

1. Recent measure bitrate below the cap seems like the only reason not to send
remb. can you please expand what a and c in your suggestion?
For now I swapped conditions to match order of the words in the comment below.

Alternatively it can be simplified by changing last_remb_time_ms_ to be infinite
past initially instead of current time. then 'recent' check would include 'had
measurement'.

2. This function explicitly expected to send remb before any measurements are
done (last_send_bitrate_bps_ > 0 is there exactly for that feature). In
particular it expect to work with send side bwe where no measurements will be
provided.

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

1. |a| was |last_send_bitrate_bps_ > 0|, etc. But if you really intended to send
REMB whenever SetMaxEstimatedBandwidth() was called, except for very specific
cases where you'd suppress, then never mind.
2. I had initially suspected this to be a bug, until I had seen you testing for
it in the unit-tests.
3. This means calling SetMaxEstimatedBandwidth() too often might produce too
many REMBs. I wonder if we could not just let the next expected REMB be sent
normally, instead of triggering an early REMB? Even if
SetMaxEstimatedBandwidth() being called too often is highly unlikely, not
sending the REMB from yet another code-path would reduce complexity. Wdyt?

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

to reduce complexity it might make sense to move all remb throttling to rtcp
module from packet router: it has more knowledge if throttling is desired. But
that is out of scope of this CL.
This CL introduce new feature trying not to change any behavior if it is not
used and with minimalistic prod code change.

+        last_send_bitrate_bps_ <= max_bitrate_bps_) {
+      // Recent measured bitrate is already below the cap.
+      return;
+    }
+  }
+  SendRemb(bitrate_bps, /*ssrcs=*/{});
+}
+
 bool PacketRouter::SendRemb(uint32_t bitrate_bps,
                             const std::vector<uint32_t>& ssrcs) {
   rtc::CritScope lock(&modules_crit_);