Refactor NACK bitrate allocation

webrtc/modules/congestion_controller/congestion_controller.cc

Index: webrtc/modules/congestion_controller/congestion_controller.cc
diff --git a/webrtc/modules/congestion_controller/congestion_controller.cc b/webrtc/modules/congestion_controller/congestion_controller.cc
index a585e339cb9520d9d88cc73db0853572f98f41fb..444d59b9376fff966c2184e9abec3cd10987506b 100644
--- a/webrtc/modules/congestion_controller/congestion_controller.cc
+++ b/webrtc/modules/congestion_controller/congestion_controller.cc
@@ -24,6 +24,7 @@
 #include "webrtc/modules/remote_bitrate_estimator/include/send_time_history.h"
 #include "webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h"
 #include "webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h"
+#include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 #include "webrtc/modules/utility/include/process_thread.h"
 #include "webrtc/system_wrappers/include/critical_section_wrapper.h"
 #include "webrtc/video/payload_router.h"
@@ -32,6 +33,8 @@ namespace webrtc {
 namespace {
 
 static const uint32_t kTimeOffsetSwitchThreshold = 30;
+static const int64_t kMinRetransmitWindowSizeMs = 30;
+static const int64_t kMaxRetransmitWindowSizeMs = 1000;
 
 class WrappingBitrateEstimator : public RemoteBitrateEstimator {
  public:
@@ -135,6 +138,62 @@ class WrappingBitrateEstimator : public RemoteBitrateEstimator {
 
 }  // namespace
 
+class RateLimiter : public NackRateLimiter {
+ public:
+  RateLimiter(Clock* clock, int64_t max_window_size_ms, uint32_t max_rate_bps)
+      : clock_(clock),
+        current_rate_(max_window_size_ms, RateStatistics::kBpsScale),
+        window_size_ms_(max_window_size_ms),
+        max_rate_bps_(max_rate_bps) {}
+  virtual ~RateLimiter() {}
+
+  // Try to use rate to send bytes. Returns true on success and if so updates
+  // current rate.
+  bool TryUseRate(size_t bytes) override {
+    rtc::CritScope cs(&lock_);
+    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.
+
+      uint32_t bitrate_addition_bps = (bytes * 8 * 1000) / window_size_ms_;
+      if (*current_rate + bitrate_addition_bps > max_rate_bps_)
+        return false;
+    }
+
+    current_rate_.Update(bytes, clock_->TimeInMilliseconds());
+    return true;
+  }
+
+  // Set the maximum bitrate, in bps, that this limiter allows to send.
+  void SetMaxRate(uint32_t max_bitrate_bps) {
+    rtc::CritScope cs(&lock_);
+    max_rate_bps_ = max_bitrate_bps;

[danilchap, 2016/06/28 14:26:17]

maybe rename max_rate_bps_ to max_bitrate_bps_
or max_bitrate_bps to max_rate_bps
to make names look more consistant.

[sprang_webrtc, 2016/07/04 09:33:03]

Done.

+  }
+
+  // Set the window size over which to measure the current bitrate.
+  // For retransmissions, this is typically the RTT.
+  void SetWindowSize(int64_t window_size_ms) {
+    rtc::CritScope cs(&lock_);
+    window_size_ms_ = window_size_ms;
+    current_rate_.SetWindowSize(window_size_ms, clock_->TimeInMilliseconds());
+  }
+
+ private:
+  Clock* const clock_;
+  rtc::CriticalSection lock_;
+  RateStatistics current_rate_ GUARDED_BY(lock_);
+  int64_t window_size_ms_ GUARDED_BY(lock_);
+  uint32_t max_rate_bps_ GUARDED_BY(lock_);
+
+  RTC_DISALLOW_COPY_AND_ASSIGN(RateLimiter);

[danilchap, 2016/06/28 14:26:17]

RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(RateLimiter) since you are not using default
constructor too.

[sprang_webrtc, 2016/07/04 09:33:03]

Done.

+};
+
 CongestionController::CongestionController(
     Clock* clock,
     BitrateObserver* bitrate_observer,
@@ -147,6 +206,10 @@ CongestionController::CongestionController(
           new WrappingBitrateEstimator(remote_bitrate_observer, clock_)),
       bitrate_controller_(
           BitrateController::CreateBitrateController(clock_, bitrate_observer)),
+      nack_rate_limiter_(
+          new RateLimiter(clock,
+                          kMaxRetransmitWindowSizeMs,
+                          RemoteBitrateEstimator::kDefaultMinBitrateBps)),
       remote_estimator_proxy_(clock_, packet_router_.get()),
       transport_feedback_adapter_(bitrate_controller_.get(), clock_),
       min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps),
@@ -168,6 +231,10 @@ CongestionController::CongestionController(
       remote_bitrate_estimator_(
           new WrappingBitrateEstimator(remote_bitrate_observer, clock_)),
       bitrate_controller_(BitrateController::CreateBitrateController(clock_)),
+      nack_rate_limiter_(
+          new RateLimiter(clock,
+                          kMaxRetransmitWindowSizeMs,

[danilchap, 2016/06/28 14:26:17]

RateLimiter is local class, and window size/bitrate are initialized with same
constants. May be use this constants inside RateLimiter constructor class
instead of passing them as parameters. Would make CongestionController
constructors cleaner.

[sprang_webrtc, 2016/07/04 09:33:03]

True. This was a leftover from a partial refactoring.

+                          RemoteBitrateEstimator::kDefaultMinBitrateBps)),
       remote_estimator_proxy_(clock_, packet_router_.get()),
       transport_feedback_adapter_(bitrate_controller_.get(), clock_),
       min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps),
@@ -193,6 +260,10 @@ CongestionController::CongestionController(
       // Constructed last as this object calls the provided callback on
       // construction.
       bitrate_controller_(BitrateController::CreateBitrateController(clock_)),
+      nack_rate_limiter_(
+          new RateLimiter(clock,
+                          kMaxRetransmitWindowSizeMs,
+                          RemoteBitrateEstimator::kDefaultMinBitrateBps)),
       remote_estimator_proxy_(clock_, packet_router_.get()),
       transport_feedback_adapter_(bitrate_controller_.get(), clock_),
       min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps),
@@ -257,6 +328,10 @@ CongestionController::GetTransportFeedbackObserver() {
   return &transport_feedback_adapter_;
 }
 
+NackRateLimiter* CongestionController::GetNackRateLimiter() {
+  return nack_rate_limiter_.get();
+}
+
 void CongestionController::SetAllocatedSendBitrateLimits(
     int min_send_bitrate_bps,
     int max_padding_bitrate_bps) {
@@ -288,6 +363,14 @@ void CongestionController::OnSentPacket(const rtc::SentPacket& sent_packet) {
 void CongestionController::OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) {
   remote_bitrate_estimator_->OnRttUpdate(avg_rtt_ms, max_rtt_ms);
   transport_feedback_adapter_.OnRttUpdate(avg_rtt_ms, max_rtt_ms);
+
+  int64_t nack_window_size = max_rtt_ms;

[danilchap, 2016/06/28 14:26:17]

nack_window_size_ms

[sprang_webrtc, 2016/07/04 09:33:03]

Done.

+  if (nack_window_size > kMaxRetransmitWindowSizeMs) {
+    nack_window_size = kMaxRetransmitWindowSizeMs;
+  } else if (nack_window_size < kMinRetransmitWindowSizeMs) {
+    nack_window_size = kMinRetransmitWindowSizeMs;
+  }
+  nack_rate_limiter_->SetWindowSize(nack_window_size);
 }
 
 int64_t CongestionController::TimeUntilNextProcess() {
@@ -312,8 +395,10 @@ void CongestionController::MaybeTriggerOnNetworkChanged() {
   int64_t rtt;
   bool estimate_changed = bitrate_controller_->GetNetworkParameters(
       &bitrate_bps, &fraction_loss, &rtt);
-  if (estimate_changed)
+  if (estimate_changed) {
     pacer_->SetEstimatedBitrate(bitrate_bps);
+    nack_rate_limiter_->SetMaxRate(bitrate_bps);
+  }
 
   bitrate_bps = IsNetworkDown() || IsSendQueueFull() ? 0 : bitrate_bps;