From fb8fc5391e097dc4c2f89b3a38724f5fa109d8f9 Mon Sep 17 00:00:00 2001 From: Stefan Holmer Date: Fri, 22 Apr 2016 15:48:23 +0200 Subject: [PATCH] Improve the behavior when the BWE times out and when we have too little data to determine the incoming bitrate. This is done by changing the RateStatistics so that it resets its window when the accumulator is empty. It also keeps a dynamic window, so that the rates computed before a full window worth of data has been received will be computed over a smaller window. This means that the rate will be closer to the true rate, but with a higher variance. BUG=webrtc:5773 R=perkj@webrtc.org, sprang@webrtc.org Review URL: https://codereview.webrtc.org/1908893003 . Cr-Commit-Position: refs/heads/master@{#12470} --- webrtc/base/OWNERS | 2 + webrtc/base/rate_statistics.cc | 17 +++-- webrtc/base/rate_statistics_unittest.cc | 69 ++++++++++++++----- .../remote_bitrate_estimator_abs_send_time.cc | 3 +- ...itrate_estimator_abs_send_time_unittest.cc | 6 +- ...itrate_estimator_single_stream_unittest.cc | 2 +- .../video_coding/bitrate_adjuster_unittest.cc | 2 +- 7 files changed, 71 insertions(+), 30 deletions(-) diff --git a/webrtc/base/OWNERS b/webrtc/base/OWNERS index 2f400904c6..b5d02af6b8 100644 --- a/webrtc/base/OWNERS +++ b/webrtc/base/OWNERS @@ -15,4 +15,6 @@ per-file *.gyp=* per-file *.gypi=* per-file BUILD.gn=kjellander@webrtc.org +per-file rate_statistics*=sprang@webrtc.org +per-file rate_statistics*=stefan@webrtc.org diff --git a/webrtc/base/rate_statistics.cc b/webrtc/base/rate_statistics.cc index 8db2851e68..6529aa1f7a 100644 --- a/webrtc/base/rate_statistics.cc +++ b/webrtc/base/rate_statistics.cc @@ -10,7 +10,9 @@ #include "webrtc/base/rate_statistics.h" -#include +#include + +#include "webrtc/base/checks.h" namespace webrtc { @@ -20,7 +22,7 @@ RateStatistics::RateStatistics(uint32_t window_size_ms, float scale) accumulated_count_(0), oldest_time_(0), oldest_index_(0), - scale_(scale / (num_buckets_ - 1)) {} + scale_(scale) {} RateStatistics::~RateStatistics() {} @@ -42,7 +44,7 @@ void RateStatistics::Update(size_t count, int64_t now_ms) { EraseOld(now_ms); int now_offset = static_cast(now_ms - oldest_time_); - assert(now_offset < num_buckets_); + RTC_DCHECK_LT(now_offset, num_buckets_); int index = oldest_index_ + now_offset; if (index >= num_buckets_) { index -= num_buckets_; @@ -53,18 +55,20 @@ void RateStatistics::Update(size_t count, int64_t now_ms) { uint32_t RateStatistics::Rate(int64_t now_ms) { EraseOld(now_ms); - return static_cast(accumulated_count_ * scale_ + 0.5f); + float scale = scale_ / (now_ms - oldest_time_ + 1); + return static_cast(accumulated_count_ * scale + 0.5f); } void RateStatistics::EraseOld(int64_t now_ms) { int64_t new_oldest_time = now_ms - num_buckets_ + 1; if (new_oldest_time <= oldest_time_) { + if (accumulated_count_ == 0) + oldest_time_ = now_ms; return; } - while (oldest_time_ < new_oldest_time) { size_t count_in_oldest_bucket = buckets_[oldest_index_]; - assert(accumulated_count_ >= count_in_oldest_bucket); + RTC_DCHECK_GE(accumulated_count_, count_in_oldest_bucket); accumulated_count_ -= count_in_oldest_bucket; buckets_[oldest_index_] = 0; if (++oldest_index_ >= num_buckets_) { @@ -74,6 +78,7 @@ void RateStatistics::EraseOld(int64_t now_ms) { if (accumulated_count_ == 0) { // This guarantees we go through all the buckets at most once, even if // |new_oldest_time| is far greater than |oldest_time_|. + new_oldest_time = now_ms; break; } } diff --git a/webrtc/base/rate_statistics_unittest.cc b/webrtc/base/rate_statistics_unittest.cc index 0270253d5e..9702da0699 100644 --- a/webrtc/base/rate_statistics_unittest.cc +++ b/webrtc/base/rate_statistics_unittest.cc @@ -8,6 +8,8 @@ * be found in the AUTHORS file in the root of the source tree. */ +#include + #include "testing/gtest/include/gtest/gtest.h" #include "webrtc/base/rate_statistics.h" @@ -15,9 +17,11 @@ namespace { using webrtc::RateStatistics; +const int64_t kWindowMs = 500; + class RateStatisticsTest : public ::testing::Test { protected: - RateStatisticsTest() : stats_(500, 8000) {} + RateStatisticsTest() : stats_(kWindowMs, 8000) {} RateStatistics stats_; }; @@ -26,8 +30,9 @@ TEST_F(RateStatisticsTest, TestStrictMode) { // Should be initialized to 0. EXPECT_EQ(0u, stats_.Rate(now_ms)); stats_.Update(1500, now_ms); - // Expecting 24 kbps given a 500 ms window with one 1500 bytes packet. - EXPECT_EQ(24000u, stats_.Rate(now_ms)); + // Expecting 1200 kbps since the window is initially kept small and grows as + // we have more data. + EXPECT_EQ(12000000u, stats_.Rate(now_ms)); stats_.Reset(); // Expecting 0 after init. EXPECT_EQ(0u, stats_.Rate(now_ms)); @@ -37,12 +42,12 @@ TEST_F(RateStatisticsTest, TestStrictMode) { } // Approximately 1200 kbps expected. Not exact since when packets // are removed we will jump 10 ms to the next packet. - if (now_ms > 0 && now_ms % 500 == 0) { - EXPECT_NEAR(1200000u, stats_.Rate(now_ms), 24000u); + if (now_ms > 0 && now_ms % kWindowMs == 0) { + EXPECT_NEAR(1200000u, stats_.Rate(now_ms), 22000u); } now_ms += 1; } - now_ms += 500; + now_ms += kWindowMs; // The window is 2 seconds. If nothing has been received for that time // the estimate should be 0. EXPECT_EQ(0u, stats_.Rate(now_ms)); @@ -54,25 +59,26 @@ TEST_F(RateStatisticsTest, IncreasingThenDecreasingBitrate) { // Expecting 0 after init. uint32_t bitrate = stats_.Rate(now_ms); EXPECT_EQ(0u, bitrate); + const uint32_t kExpectedBitrate = 8000000; // 1000 bytes per millisecond until plateau is reached. + int prev_error = kExpectedBitrate; while (++now_ms < 10000) { stats_.Update(1000, now_ms); - uint32_t new_bitrate = stats_.Rate(now_ms); - if (new_bitrate != bitrate) { - // New bitrate must be higher than previous one. - EXPECT_GT(new_bitrate, bitrate); - } else { - // Plateau reached, 8000 kbps expected. - EXPECT_NEAR(8000000u, bitrate, 80000u); - break; - } - bitrate = new_bitrate; + bitrate = stats_.Rate(now_ms); + int error = kExpectedBitrate - bitrate; + error = std::abs(error); + // Expect the estimation error to decrease as the window is extended. + EXPECT_LE(error, prev_error + 1); + prev_error = error; } + // Window filled, expect to be close to 8000000. + EXPECT_EQ(kExpectedBitrate, bitrate); + // 1000 bytes per millisecond until 10-second mark, 8000 kbps expected. while (++now_ms < 10000) { stats_.Update(1000, now_ms); bitrate = stats_.Rate(now_ms); - EXPECT_NEAR(8000000u, bitrate, 80000u); + EXPECT_EQ(kExpectedBitrate, bitrate); } // Zero bytes per millisecond until 0 is reached. while (++now_ms < 20000) { @@ -94,4 +100,33 @@ TEST_F(RateStatisticsTest, IncreasingThenDecreasingBitrate) { EXPECT_EQ(0u, stats_.Rate(now_ms)); } } + +TEST_F(RateStatisticsTest, ResetAfterSilence) { + int64_t now_ms = 0; + stats_.Reset(); + // Expecting 0 after init. + uint32_t bitrate = stats_.Rate(now_ms); + EXPECT_EQ(0u, bitrate); + const uint32_t kExpectedBitrate = 8000000; + // 1000 bytes per millisecond until the window has been filled. + int prev_error = kExpectedBitrate; + while (++now_ms < 10000) { + stats_.Update(1000, now_ms); + bitrate = stats_.Rate(now_ms); + int error = kExpectedBitrate - bitrate; + error = std::abs(error); + // Expect the estimation error to decrease as the window is extended. + EXPECT_LE(error, prev_error + 1); + prev_error = error; + } + // Window filled, expect to be close to 8000000. + EXPECT_EQ(kExpectedBitrate, bitrate); + + now_ms += kWindowMs + 1; + EXPECT_EQ(0u, stats_.Rate(now_ms)); + stats_.Update(1000, now_ms); + // We expect one sample of 1000 bytes, and that the bitrate is measured over + // 1 ms, i.e., 8 * 1000 / 0.001 = 8000000. + EXPECT_EQ(kExpectedBitrate, stats_.Rate(now_ms)); +} } // namespace diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc index 5bd0627afe..8ad60aea65 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc @@ -176,8 +176,7 @@ RemoteBitrateEstimatorAbsSendTime::ProcessClusters(int64_t now_ms) { std::min(best_it->GetSendBitrateBps(), best_it->GetRecvBitrateBps()); // Make sure that a probe sent on a lower bitrate than our estimate can't // reduce the estimate. - if (IsBitrateImproving(probe_bitrate_bps) && - probe_bitrate_bps > static_cast(incoming_bitrate_.Rate(now_ms))) { + if (IsBitrateImproving(probe_bitrate_bps)) { LOG(LS_INFO) << "Probe successful, sent at " << best_it->GetSendBitrateBps() << " bps, received at " << best_it->GetRecvBitrateBps() diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time_unittest.cc b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time_unittest.cc index 8f57bbb58b..a57fcb5114 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time_unittest.cc +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time_unittest.cc @@ -35,15 +35,15 @@ TEST_F(RemoteBitrateEstimatorAbsSendTimeTest, RateIncreaseReordering) { } TEST_F(RemoteBitrateEstimatorAbsSendTimeTest, RateIncreaseRtpTimestamps) { - RateIncreaseRtpTimestampsTestHelper(1232); + RateIncreaseRtpTimestampsTestHelper(1229); } TEST_F(RemoteBitrateEstimatorAbsSendTimeTest, CapacityDropOneStream) { - CapacityDropTestHelper(1, false, 633); + CapacityDropTestHelper(1, false, 667); } TEST_F(RemoteBitrateEstimatorAbsSendTimeTest, CapacityDropOneStreamWrap) { - CapacityDropTestHelper(1, true, 633); + CapacityDropTestHelper(1, true, 667); } TEST_F(RemoteBitrateEstimatorAbsSendTimeTest, CapacityDropTwoStreamsWrap) { diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream_unittest.cc b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream_unittest.cc index 6fd0ad11b5..97e3abaa32 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream_unittest.cc +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream_unittest.cc @@ -47,7 +47,7 @@ TEST_F(RemoteBitrateEstimatorSingleTest, CapacityDropOneStreamWrap) { } TEST_F(RemoteBitrateEstimatorSingleTest, CapacityDropTwoStreamsWrap) { - CapacityDropTestHelper(2, true, 767); + CapacityDropTestHelper(2, true, 600); } TEST_F(RemoteBitrateEstimatorSingleTest, CapacityDropThreeStreamsWrap) { diff --git a/webrtc/modules/video_coding/bitrate_adjuster_unittest.cc b/webrtc/modules/video_coding/bitrate_adjuster_unittest.cc index 1d14ee3160..1421082aeb 100644 --- a/webrtc/modules/video_coding/bitrate_adjuster_unittest.cc +++ b/webrtc/modules/video_coding/bitrate_adjuster_unittest.cc @@ -86,7 +86,7 @@ TEST_F(BitrateAdjusterTest, VaryingBitrates) { SimulateBitrateBps(actual_bitrate_bps); VerifyAdjustment(); adjusted_bitrate_bps = adjuster_.GetAdjustedBitrateBps(); - EXPECT_LT(adjusted_bitrate_bps, last_adjusted_bitrate_bps); + EXPECT_LE(adjusted_bitrate_bps, last_adjusted_bitrate_bps); last_adjusted_bitrate_bps = adjusted_bitrate_bps; // After two cycles we should've stabilized and hit the lower bound. EXPECT_EQ(GetTargetBitrateBpsPct(kMinAdjustedBitratePct),