From 3326d0ac8232d6e9f6cd696b63f88bd817b0ec62 Mon Sep 17 00:00:00 2001 From: ivoc Date: Fri, 4 Nov 2016 09:48:38 -0700 Subject: [PATCH] Revert of Change TWCC send interval to reduce overhead on low BW situations. (patchset #10 id:200001 of https://codereview.webrtc.org/2381833003/ ) Reason for revert: Breaks internal tests. Original issue's description: > Change TWCC send interval to reduce overhead on low BW situations. > > BUG=webrtc:6442 > > Committed: https://crrev.com/861e9374640eaa37ba5d905e3e0971df04b4fc9e > Cr-Commit-Position: refs/heads/master@{#14910} TBR=minyue@webrtc.org,stefan@webrtc.org,michaelt@webrtc.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=webrtc:6442 Review-Url: https://codereview.webrtc.org/2468413009 Cr-Commit-Position: refs/heads/master@{#14933} --- .../congestion_controller.cc | 1 - .../remote_estimator_proxy.cc | 34 ++++--------------- .../remote_estimator_proxy.h | 6 +--- .../remote_estimator_proxy_unittest.cc | 33 +----------------- 4 files changed, 8 insertions(+), 66 deletions(-) diff --git a/webrtc/modules/congestion_controller/congestion_controller.cc b/webrtc/modules/congestion_controller/congestion_controller.cc index 63d7db9fa8..f7e7e5625a 100644 --- a/webrtc/modules/congestion_controller/congestion_controller.cc +++ b/webrtc/modules/congestion_controller/congestion_controller.cc @@ -347,7 +347,6 @@ void CongestionController::MaybeTriggerOnNetworkChanged() { if (HasNetworkParametersToReportChanged(bitrate_bps, fraction_loss, rtt)) { observer_->OnNetworkChanged(bitrate_bps, fraction_loss, rtt); - remote_estimator_proxy_.OnBitrateChanged(bitrate_bps); } } diff --git a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc index 85ef1dd16c..b08b30a8ce 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc +++ b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc @@ -11,7 +11,6 @@ #include "webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h" #include -#include #include "webrtc/base/checks.h" #include "webrtc/base/logging.h" @@ -23,10 +22,8 @@ namespace webrtc { // TODO(sprang): Tune these! +const int RemoteEstimatorProxy::kDefaultProcessIntervalMs = 50; const int RemoteEstimatorProxy::kBackWindowMs = 500; -const int RemoteEstimatorProxy::kMinSendIntervalMs = 50; -const int RemoteEstimatorProxy::kMaxSendIntervalMs = 250; -const int RemoteEstimatorProxy::kDefaultSendIntervalMs = 100; // The maximum allowed value for a timestamp in milliseconds. This is lower // than the numerical limit since we often convert to microseconds. @@ -40,8 +37,7 @@ RemoteEstimatorProxy::RemoteEstimatorProxy(Clock* clock, last_process_time_ms_(-1), media_ssrc_(0), feedback_sequence_(0), - window_start_seq_(-1), - send_interval_ms_(kDefaultSendIntervalMs) {} + window_start_seq_(-1) {} RemoteEstimatorProxy::~RemoteEstimatorProxy() {} @@ -72,12 +68,11 @@ bool RemoteEstimatorProxy::LatestEstimate(std::vector* ssrcs, } int64_t RemoteEstimatorProxy::TimeUntilNextProcess() { + int64_t now = clock_->TimeInMilliseconds(); int64_t time_until_next = 0; - if (last_process_time_ms_ != -1) { - rtc::CritScope cs(&lock_); - int64_t now = clock_->TimeInMilliseconds(); - if (now - last_process_time_ms_ < send_interval_ms_) - time_until_next = (last_process_time_ms_ + send_interval_ms_ - now); + if (last_process_time_ms_ != -1 && + now - last_process_time_ms_ < kDefaultProcessIntervalMs) { + time_until_next = (last_process_time_ms_ + kDefaultProcessIntervalMs - now); } return time_until_next; } @@ -97,23 +92,6 @@ void RemoteEstimatorProxy::Process() { } } -void RemoteEstimatorProxy::OnBitrateChanged(int bitrate_bps) { - // TwccReportSize = Ipv4(20B) + UDP(8B) + SRTP(10B) + - // AverageTwccReport(30B) - // TwccReport size at 50ms interval is 24 byte. - // TwccReport size at 250ms interval is 36 byte. - // AverageTwccReport = (TwccReport(50ms) + TwccReport(250ms)) / 2 - constexpr int kTwccReportSize = 20 + 8 + 10 + 30; - - // Let TWCC reports occupy 5% of total bandwidth. - rtc::CritScope cs(&lock_); - send_interval_ms_ = std::min( - std::max(static_cast( - kTwccReportSize * 8.0 * 1000.0 / (0.05 * bitrate_bps) + 0.5), - kMinSendIntervalMs), - kMaxSendIntervalMs); -} - void RemoteEstimatorProxy::OnPacketArrival(uint16_t sequence_number, int64_t arrival_time) { if (arrival_time < 0 || arrival_time > kMaxTimeMs) { diff --git a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h index 71099bf9ed..127886300d 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h +++ b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h @@ -47,11 +47,8 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator { void SetMinBitrate(int min_bitrate_bps) override {} int64_t TimeUntilNextProcess() override; void Process() override; - void OnBitrateChanged(int bitrate); - static const int kMinSendIntervalMs; - static const int kMaxSendIntervalMs; - static const int kDefaultSendIntervalMs; + static const int kDefaultProcessIntervalMs; static const int kBackWindowMs; private: @@ -71,7 +68,6 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator { int64_t window_start_seq_ GUARDED_BY(&lock_); // Map unwrapped seq -> time. std::map packet_arrival_times_ GUARDED_BY(&lock_); - int64_t send_interval_ms_ GUARDED_BY(&lock_); }; } // namespace webrtc diff --git a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc index 56001d266e..9821568260 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc +++ b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc @@ -42,7 +42,7 @@ class RemoteEstimatorProxyTest : public ::testing::Test { void Process() { clock_.AdvanceTimeMilliseconds( - RemoteEstimatorProxy::kDefaultSendIntervalMs); + RemoteEstimatorProxy::kDefaultProcessIntervalMs); proxy_.Process(); } @@ -350,35 +350,4 @@ TEST_F(RemoteEstimatorProxyTest, RemovesTimestampsOutOfScope) { Process(); } -TEST_F(RemoteEstimatorProxyTest, TimeUntilNextProcessIsZeroBeforeFirstProcess) { - EXPECT_EQ(0, proxy_.TimeUntilNextProcess()); -} - -TEST_F(RemoteEstimatorProxyTest, TimeUntilNextProcessIsDefaultOnUnkownBitrate) { - Process(); - EXPECT_EQ(RemoteEstimatorProxy::kDefaultSendIntervalMs, - proxy_.TimeUntilNextProcess()); -} - -TEST_F(RemoteEstimatorProxyTest, TimeUntilNextProcessIsMinIntervalOn300kbps) { - Process(); - proxy_.OnBitrateChanged(300000); - EXPECT_EQ(RemoteEstimatorProxy::kMinSendIntervalMs, - proxy_.TimeUntilNextProcess()); -} - -TEST_F(RemoteEstimatorProxyTest, TimeUntilNextProcessIsMaxIntervalOn20kbps) { - Process(); - proxy_.OnBitrateChanged(20000); - EXPECT_EQ(RemoteEstimatorProxy::kMaxSendIntervalMs, - proxy_.TimeUntilNextProcess()); -} - -TEST_F(RemoteEstimatorProxyTest, TwccReportsUse5PercentOfAvailableBandwidth) { - Process(); - proxy_.OnBitrateChanged(80000); - // 80kbps * 0.05 = TwccReportSize(68B * 8b/B) * 1000ms / SendInterval(136ms) - EXPECT_EQ(136, proxy_.TimeUntilNextProcess()); -} - } // namespace webrtc