Refactor NACK bitrate allocation

webrtc/base/rate_limiter_unittest.cc

Index: webrtc/base/rate_limiter_unittest.cc
diff --git a/webrtc/base/rate_limiter_unittest.cc b/webrtc/base/rate_limiter_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..718b49c3c9f74015a14ad9a568a16301d76d68b4
--- /dev/null
+++ b/webrtc/base/rate_limiter_unittest.cc
@@ -0,0 +1,100 @@
+/*
+ *  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 <algorithm>
+#include <memory>
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "webrtc/base/rate_limiter.h"
+#include "webrtc/system_wrappers/include/clock.h"
+
+namespace webrtc {
+
+class RateLimitTest : public ::testing::Test {

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

since the class that these tests are testing, is currently implemented to be
multithreaded, it would be good to have tests for that.  As is, all the tests
appear to be single threaded.

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

Since all methods are synchronized, and with the same lock, all access into the
class is serialized so I'm not quite sure what you're hoping to accomplish with
a multi threaded test?

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

I'm thinking about it from a regression point of view (as well as correct
behavior I guess). If the implementation is modified, e.g. someone forgets to
grab a lock while accessing a member variable then a multithreaded test running
on tsan could catch that regression.  It sounds like you're reviewing the code
and from inspection assuming that everything is correct.  It might be, but code
review doesn't catch everything.

Generally speaking, if a class is supposed to be multithreaded, I think some
sort of a test that represents multithreaded usage of that class, would be a
good thing.
---
As a more practical question for this cl, would this result in an expected
state?

RateLimiter limiter(...);

void Thread1() {
  int64_t max_rtt_ms = GetMaxRttMs();  // Runs first.
  Sleep(1000);  // Simulate context switch.
  limiter.SetWindowSize(max_rtt_ms);  // Runs second - sets the oldest max.
}

void Thread2() {
  Sleep(1000);  // Simulate context switch.
  int64_t max_rtt_ms = GetMaxRttMs(); // Runs second.
  limiter.SetWindowSize(max_rtt_ms);// Runs first, sets the newest max.
}

StartBothThreads();
WaitForThreadsToExit();
EXPECT_TRUE(WindowSizeIsCorrectAccordingToTheLatestBitrate());


If not and if SetWindowSize() must always be called from the same thread, then
that can be checked.

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

I see. I implemented a test case where each of the public methods are called on
separate threads, in order to catch data race regressions with tsan. wdyt?

+ public:
+  RateLimitTest()
+      : clock_(0), rate_limiter(new RateLimiter(&clock_, kWindowSizeMs)) {}
+  virtual ~RateLimitTest() {}
+
+  void SetUp() override { rate_limiter->SetMaxRate(kMaxRateBps); }
+
+ protected:
+  static constexpr int64_t kWindowSizeMs = 1000;
+  static constexpr uint32_t kMaxRateBps = 100000;
+  // Bytes needed to completely saturate the rate limiter.
+  static constexpr size_t kRateFillingBytes =
+      (kMaxRateBps * kWindowSizeMs) / (8 * 1000);
+  SimulatedClock clock_;
+  std::unique_ptr<RateLimiter> rate_limiter;
+};
+
+TEST_F(RateLimitTest, IncreasingMaxRate) {
+  // Fill rate, extend window to full size.
+  EXPECT_TRUE(rate_limiter->TryUseRate(kRateFillingBytes / 2));
+  clock_.AdvanceTimeMilliseconds(kWindowSizeMs - 1);
+  EXPECT_TRUE(rate_limiter->TryUseRate(kRateFillingBytes / 2));
+
+  // All rate consumed.
+  EXPECT_FALSE(rate_limiter->TryUseRate(1));
+
+  // Double the available rate and fill that too.
+  rate_limiter->SetMaxRate(kMaxRateBps * 2);
+  EXPECT_TRUE(rate_limiter->TryUseRate(kRateFillingBytes));
+
+  // All rate consumed again.
+  EXPECT_FALSE(rate_limiter->TryUseRate(1));
+}
+
+TEST_F(RateLimitTest, DecreasingMaxRate) {
+  // Fill rate, extend window to full size.
+  EXPECT_TRUE(rate_limiter->TryUseRate(kRateFillingBytes / 2));
+  clock_.AdvanceTimeMilliseconds(kWindowSizeMs - 1);
+  EXPECT_TRUE(rate_limiter->TryUseRate(kRateFillingBytes / 2));
+
+  // All rate consumed.
+  EXPECT_FALSE(rate_limiter->TryUseRate(1));
+
+  // Halve the available rate and move window so half of the data falls out.
+  rate_limiter->SetMaxRate(kMaxRateBps / 2);
+  clock_.AdvanceTimeMilliseconds(1);
+
+  // All rate still consumed.
+  EXPECT_FALSE(rate_limiter->TryUseRate(1));
+}
+
+TEST_F(RateLimitTest, ChangingWindowSize) {
+  // Fill rate, extend window to full size.
+  EXPECT_TRUE(rate_limiter->TryUseRate(kRateFillingBytes / 2));
+  clock_.AdvanceTimeMilliseconds(kWindowSizeMs - 1);
+  EXPECT_TRUE(rate_limiter->TryUseRate(kRateFillingBytes / 2));
+
+  // All rate consumed.
+  EXPECT_FALSE(rate_limiter->TryUseRate(1));
+
+  // Decrease window size so half of the data falls out.
+  rate_limiter->SetWindowSize(kWindowSizeMs / 2);
+  // Average rate should still be the same, so rate is still all consumed.
+  EXPECT_FALSE(rate_limiter->TryUseRate(1));
+
+  // Increase window size again. Now the rate is only half used (removed data
+  // points don't come back to life).
+  rate_limiter->SetWindowSize(kWindowSizeMs);
+  EXPECT_TRUE(rate_limiter->TryUseRate(kRateFillingBytes / 2));
+
+  // All rate consumed again.
+  EXPECT_FALSE(rate_limiter->TryUseRate(1));
+}
+
+TEST_F(RateLimitTest, SingleUsageAlwaysOk) {
+  // Using more bytes than can fit in a window is OK for a single packet.
+  EXPECT_TRUE(rate_limiter->TryUseRate(kRateFillingBytes + 1));
+}
+
+}  // namespace webrtc