Refactor NACK bitrate allocation

webrtc/base/rate_limiter.cc

Index: webrtc/base/rate_limiter.cc
diff --git a/webrtc/base/rate_limiter.cc b/webrtc/base/rate_limiter.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a3011c2cec8f42434b2395d26b47746b37783845
--- /dev/null
+++ b/webrtc/base/rate_limiter.cc
@@ -0,0 +1,62 @@
+/*
+ *  Copyright (c) 2016 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.
+ */
+
+#include "webrtc/base/rate_limiter.h"
+#include "webrtc/system_wrappers/include/clock.h"
+
+namespace webrtc {
+
+RateLimiter::RateLimiter(Clock* clock, int64_t max_retransmission_window_ms)
+    : clock_(clock),
+      current_rate_(max_retransmission_window_ms, RateStatistics::kBpsScale),
+      window_size_ms_(max_retransmission_window_ms),
+      max_rate_bps_(std::numeric_limits<uint32_t>::max()) {}
+
+RateLimiter::~RateLimiter() {}
+
+// Try to use rate to send bytes. Returns true on success and if so updates
+// current rate.

[tommi, 2016/07/07 12:48:46]

keep the function level documentation in the header

[sprang_webrtc, 2016/07/07 13:46:50]

Done.

+bool RateLimiter::TryUseRate(size_t packet_size_bytes) {
+  rtc::CritScope cs(&lock_);

[tommi, 2016/07/07 12:48:46]

can you add documentation about threading? Why not single threaded for instance?
On what threads do we expect the method to be called when not the constructing
thread?

[sprang_webrtc, 2016/07/07 13:46:50]

In the initial use case, TryUseRate() will be called from the network thread,
SetWindowSize() from module processing thread (rtcp module) and SetMaxRate from
the bandwidth estimator (unsure which thread).

[tommi, 2016/07/08 08:52:20]

Thanks, that's useful information.  What about putting that here as a comment? 
Knowing usage patterns like that can allow us to make improvements such as
having unlocked reads on the only thread/execution context that modifies a
variable.

[sprang_webrtc, 2016/07/08 11:46:58]

Done.

+  rtc::Optional<uint32_t> current_rate =
+      current_rate_.Rate(clock_->TimeInMilliseconds());
+  if (current_rate) {
+    // If there is a current rate, check if adding bytes would cause maximum
+    // bitrate target to be exceeded. If there is NOT a valid current rate,
+    // allow allocating rate even if target is exceeded. This prevents
+    // problems
+    // at very low rates, where for instance retransmissions would never be
+    // allowed due to too high bitrate caused by a single packet.
+
+    size_t bitrate_addition_bps =
+        (packet_size_bytes * 8 * 1000) / window_size_ms_;
+    if (*current_rate + bitrate_addition_bps > max_rate_bps_)
+      return false;
+  }
+
+  current_rate_.Update(packet_size_bytes, clock_->TimeInMilliseconds());

[tommi, 2016/07/07 12:48:46]

calling out to a different implementation while holding a lock. Would be good to
avoid that.

[sprang_webrtc, 2016/07/07 13:46:50]

I looked into bolting rate limiting logic onto RateStatistics, but didn't like
it. Also really don't want to duplicate the RateStatistics logic here, so
settled for this wrapper around the rate tracker instead.
I guess we could just move this class into the same file as RateStatistics,
but...

[tommi, 2016/07/08 08:52:20]

Might make sense at some point in the future.  This approach is a good step in
the right direction.

[sprang_webrtc, 2016/07/08 11:46:58]

Acknowledged.

+  return true;
+}
+
+// Set the maximum bitrate, in bps, that this limiter allows to send.
+void RateLimiter::SetMaxRate(uint32_t max_rate_bps) {
+  rtc::CritScope cs(&lock_);
+  max_rate_bps_ = max_rate_bps;

[tommi, 2016/07/07 12:48:46]

If this variable is changed on the same thread as TryUseRate is called on, we
don't need a lock.  If we do need a lock, please document what will be going on.
(great if we could dcheck somehow what we expect to be happening)

[sprang_webrtc, 2016/07/07 13:46:50]

For the initial use case at least, SetMaxRate() and TryUseRate() will not be
called from the same thread. Same for SetWindowSize().

+}
+
+// Set the window size over which to measure the current bitrate.
+// For retransmissions, this is typically the RTT.
+void RateLimiter::SetWindowSize(int64_t window_size_ms) {

[tommi, 2016/07/07 12:48:46]

same for this method. keep the method level documentation in the header,
document the threading model well.

[sprang_webrtc, 2016/07/07 13:46:50]

Done.

+  rtc::CritScope cs(&lock_);
+  window_size_ms_ = window_size_ms;
+  current_rate_.SetWindowSize(window_size_ms, clock_->TimeInMilliseconds());

[tommi, 2016/07/07 12:48:46]

calling into a different implementation while holding a lock... does this
SetWindowSize grab a lock by any chance?

[sprang_webrtc, 2016/07/07 13:46:50]

Nope. RateStatistics does not use any locks.

[tommi, 2016/07/08 08:52:20]

Thanks.  I took a closer look at how these classes are related (I didn't have
time to do that yesterday unfortuantely), so this makes a lot more sense to me
now.

+}
+
+}  // namespace webrtc