From 0bac07a89b58dcafbb27f8a25de3a38ae3a39704 Mon Sep 17 00:00:00 2001 From: terelius Date: Thu, 15 Dec 2016 06:41:39 -0800 Subject: [PATCH] Revert of Avoid precision loss in MedianSlopeEstimator from int64_t -> double conversion (patchset #3 id:40001 of https://codereview.webrtc.org/2578543002/ ) Reason for revert: Multiple definitions of TestEstimator Original issue's description: > Pass arrival time as an int64_t rather than a double to the MedianSlopeEstimator to avoid precision loss. > > Also clean up the unit test. > > BUG=webrtc:6892 > > Committed: https://crrev.com/ebcbcc3b2451f5c4fb07f7b37815bd54f364d057 > Cr-Commit-Position: refs/heads/master@{#15634} TBR=brandtr@webrtc.org,stefan@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=webrtc:6892 Review-Url: https://codereview.webrtc.org/2572353003 Cr-Commit-Position: refs/heads/master@{#15635} --- .../median_slope_estimator.cc | 19 ++-- .../median_slope_estimator.h | 13 +-- .../median_slope_estimator_unittest.cc | 95 +++++++++++++------ 3 files changed, 82 insertions(+), 45 deletions(-) diff --git a/webrtc/modules/congestion_controller/median_slope_estimator.cc b/webrtc/modules/congestion_controller/median_slope_estimator.cc index 328aa6b5d4..488831e173 100644 --- a/webrtc/modules/congestion_controller/median_slope_estimator.cc +++ b/webrtc/modules/congestion_controller/median_slope_estimator.cc @@ -35,15 +35,15 @@ MedianSlopeEstimator::~MedianSlopeEstimator() {} void MedianSlopeEstimator::Update(double recv_delta_ms, double send_delta_ms, - int64_t arrival_time_ms) { + double now_ms) { const double delta_ms = recv_delta_ms - send_delta_ms; ++num_of_deltas_; - if (num_of_deltas_ > kDeltaCounterMax) + if (num_of_deltas_ > kDeltaCounterMax) { num_of_deltas_ = kDeltaCounterMax; + } accumulated_delay_ += delta_ms; - BWE_TEST_LOGGING_PLOT(1, "accumulated_delay_ms", arrival_time_ms, - accumulated_delay_); + BWE_TEST_LOGGING_PLOT(1, "accumulated_delay_ms", now_ms, accumulated_delay_); // If the window is full, remove the |window_size_| - 1 slopes that belong to // the oldest point. @@ -56,7 +56,7 @@ void MedianSlopeEstimator::Update(double recv_delta_ms, } // Add |window_size_| - 1 new slopes. for (auto& old_delay : delay_hist_) { - if (arrival_time_ms - old_delay.time != 0) { + if (now_ms - old_delay.time != 0) { // The C99 standard explicitly states that casts and assignments must // perform the associated conversions. This means that |slope| will be // a 64-bit double even if the division is computed using, e.g., 80-bit @@ -64,21 +64,20 @@ void MedianSlopeEstimator::Update(double recv_delta_ms, // C++11 standard isn't as explicit. Furthermore, there are good reasons // to believe that compilers couldn't perform optimizations that break // this assumption even if they wanted to. - double slope = (accumulated_delay_ - old_delay.delay) / - static_cast(arrival_time_ms - old_delay.time); + double slope = + (accumulated_delay_ - old_delay.delay) / (now_ms - old_delay.time); median_filter_.Insert(slope); // We want to avoid issues with different rounding mode / precision // which we might get if we recomputed the slope when we remove it. old_delay.slopes.push_back(slope); } } - delay_hist_.emplace_back(arrival_time_ms, accumulated_delay_, - window_size_ - 1); + delay_hist_.emplace_back(now_ms, accumulated_delay_, window_size_ - 1); // Recompute the median slope. if (delay_hist_.size() == window_size_) trendline_ = median_filter_.GetPercentileValue(); - BWE_TEST_LOGGING_PLOT(1, "trendline_slope", arrival_time_ms, trendline_); + BWE_TEST_LOGGING_PLOT(1, "trendline_slope", now_ms, trendline_); } } // namespace webrtc diff --git a/webrtc/modules/congestion_controller/median_slope_estimator.h b/webrtc/modules/congestion_controller/median_slope_estimator.h index 76bb60ac81..26d7f6144d 100644 --- a/webrtc/modules/congestion_controller/median_slope_estimator.h +++ b/webrtc/modules/congestion_controller/median_slope_estimator.h @@ -10,14 +10,13 @@ #ifndef WEBRTC_MODULES_CONGESTION_CONTROLLER_MEDIAN_SLOPE_ESTIMATOR_H_ #define WEBRTC_MODULES_CONGESTION_CONTROLLER_MEDIAN_SLOPE_ESTIMATOR_H_ -#include -#include - #include +#include #include #include "webrtc/base/analytics/percentile_filter.h" #include "webrtc/base/constructormagic.h" +#include "webrtc/common_types.h" namespace webrtc { @@ -33,9 +32,7 @@ class MedianSlopeEstimator { // Update the estimator with a new sample. The deltas should represent deltas // between timestamp groups as defined by the InterArrival class. - void Update(double recv_delta_ms, - double send_delta_ms, - int64_t arrival_time_ms); + void Update(double recv_delta_ms, double send_delta_ms, double now_ms); // Returns the estimated trend k multiplied by some gain. // 0 < k < 1 -> the delay increases, queues are filling up @@ -48,11 +45,11 @@ class MedianSlopeEstimator { private: struct DelayInfo { - DelayInfo(int64_t time, double delay, size_t slope_count) + DelayInfo(double time, double delay, size_t slope_count) : time(time), delay(delay) { slopes.reserve(slope_count); } - int64_t time; + double time; double delay; std::vector slopes; }; diff --git a/webrtc/modules/congestion_controller/median_slope_estimator_unittest.cc b/webrtc/modules/congestion_controller/median_slope_estimator_unittest.cc index 8a016d9d87..ef942f143d 100644 --- a/webrtc/modules/congestion_controller/median_slope_estimator_unittest.cc +++ b/webrtc/modules/congestion_controller/median_slope_estimator_unittest.cc @@ -18,55 +18,96 @@ namespace { constexpr size_t kWindowSize = 20; constexpr double kGain = 1; constexpr int64_t kAvgTimeBetweenPackets = 10; -constexpr size_t kPacketCount = 2 * kWindowSize + 1; } // namespace -void TestEstimator(double slope, double jitter_stddev, double tolerance) { +TEST(MedianSlopeEstimator, PerfectLineSlopeOneHalf) { MedianSlopeEstimator estimator(kWindowSize, kGain); - Random random(0x1234567); - int64_t send_times[kPacketCount]; - int64_t recv_times[kPacketCount]; - int64_t send_start_time = random.Rand(1000000); - int64_t recv_start_time = random.Rand(1000000); - for (size_t i = 0; i < kPacketCount; ++i) { - send_times[i] = send_start_time + i * kAvgTimeBetweenPackets; - double latency = i * kAvgTimeBetweenPackets / (1 - slope); - double jitter = random.Gaussian(0, jitter_stddev); - recv_times[i] = recv_start_time + latency + jitter; - } - for (size_t i = 1; i < kPacketCount; ++i) { - double recv_delta = recv_times[i] - recv_times[i - 1]; - double send_delta = send_times[i] - send_times[i - 1]; - estimator.Update(recv_delta, send_delta, recv_times[i]); + Random rand(0x1234567); + double now_ms = rand.Rand() * 10000; + for (size_t i = 1; i < 2 * kWindowSize; i++) { + double send_delta = rand.Rand() * 2 * kAvgTimeBetweenPackets; + double recv_delta = 2 * send_delta; + now_ms += recv_delta; + estimator.Update(recv_delta, send_delta, now_ms); if (i < kWindowSize) EXPECT_NEAR(estimator.trendline_slope(), 0, 0.001); else - EXPECT_NEAR(estimator.trendline_slope(), slope, tolerance); + EXPECT_NEAR(estimator.trendline_slope(), 0.5, 0.001); } } -TEST(MedianSlopeEstimator, PerfectLineSlopeOneHalf) { - TestEstimator(0.5, 0, 0.001); -} - TEST(MedianSlopeEstimator, PerfectLineSlopeMinusOne) { - TestEstimator(-1, 0, 0.001); + MedianSlopeEstimator estimator(kWindowSize, kGain); + Random rand(0x1234567); + double now_ms = rand.Rand() * 10000; + for (size_t i = 1; i < 2 * kWindowSize; i++) { + double send_delta = rand.Rand() * 2 * kAvgTimeBetweenPackets; + double recv_delta = 0.5 * send_delta; + now_ms += recv_delta; + estimator.Update(recv_delta, send_delta, now_ms); + if (i < kWindowSize) + EXPECT_NEAR(estimator.trendline_slope(), 0, 0.001); + else + EXPECT_NEAR(estimator.trendline_slope(), -1, 0.001); + } } TEST(MedianSlopeEstimator, PerfectLineSlopeZero) { - TestEstimator(0, 0, 0.001); + MedianSlopeEstimator estimator(kWindowSize, kGain); + Random rand(0x1234567); + double now_ms = rand.Rand() * 10000; + for (size_t i = 1; i < 2 * kWindowSize; i++) { + double send_delta = rand.Rand() * 2 * kAvgTimeBetweenPackets; + double recv_delta = send_delta; + now_ms += recv_delta; + estimator.Update(recv_delta, send_delta, now_ms); + EXPECT_NEAR(estimator.trendline_slope(), 0, 0.001); + } } TEST(MedianSlopeEstimator, JitteryLineSlopeOneHalf) { - TestEstimator(0.5, kAvgTimeBetweenPackets / 3.0, 0.01); + MedianSlopeEstimator estimator(kWindowSize, kGain); + Random rand(0x1234567); + double now_ms = rand.Rand() * 10000; + for (size_t i = 1; i < 2 * kWindowSize; i++) { + double send_delta = rand.Rand() * 2 * kAvgTimeBetweenPackets; + double recv_delta = 2 * send_delta + rand.Gaussian(0, send_delta / 3); + now_ms += recv_delta; + estimator.Update(recv_delta, send_delta, now_ms); + if (i < kWindowSize) + EXPECT_NEAR(estimator.trendline_slope(), 0, 0.001); + else + EXPECT_NEAR(estimator.trendline_slope(), 0.5, 0.1); + } } TEST(MedianSlopeEstimator, JitteryLineSlopeMinusOne) { - TestEstimator(-1, kAvgTimeBetweenPackets / 3.0, 0.05); + MedianSlopeEstimator estimator(kWindowSize, kGain); + Random rand(0x1234567); + double now_ms = rand.Rand() * 10000; + for (size_t i = 1; i < 2 * kWindowSize; i++) { + double send_delta = rand.Rand() * 2 * kAvgTimeBetweenPackets; + double recv_delta = 0.5 * send_delta + rand.Gaussian(0, send_delta / 20); + now_ms += recv_delta; + estimator.Update(recv_delta, send_delta, now_ms); + if (i < kWindowSize) + EXPECT_NEAR(estimator.trendline_slope(), 0, 0.001); + else + EXPECT_NEAR(estimator.trendline_slope(), -1, 0.1); + } } TEST(MedianSlopeEstimator, JitteryLineSlopeZero) { - TestEstimator(0, kAvgTimeBetweenPackets / 3.0, 0.02); + MedianSlopeEstimator estimator(kWindowSize, kGain); + Random rand(0x1234567); + double now_ms = rand.Rand() * 10000; + for (size_t i = 1; i < 2 * kWindowSize; i++) { + double send_delta = rand.Rand() * 2 * kAvgTimeBetweenPackets; + double recv_delta = send_delta + rand.Gaussian(0, send_delta / 5); + now_ms += recv_delta; + estimator.Update(recv_delta, send_delta, now_ms); + EXPECT_NEAR(estimator.trendline_slope(), 0, 0.1); + } } } // namespace webrtc