From 8bd2f40a8c25b6c71790e49f202593ce61e09cca Mon Sep 17 00:00:00 2001 From: "sprang@webrtc.org" Date: Mon, 16 Mar 2015 14:11:21 +0000 Subject: [PATCH] Remove code related to REMB suppressor experiment. Stats indicate this isn't helping. Ditching the whole thing. BUG=4082 R=mflodman@webrtc.org Review URL: https://webrtc-codereview.appspot.com/47569004 Cr-Commit-Position: refs/heads/master@{#8734} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8734 4adac7df-926f-26a2-2b94-8c16560cd09d --- webrtc/modules/bitrate_controller/BUILD.gn | 2 - .../bitrate_controller.gypi | 2 - .../bitrate_controller_impl.cc | 16 +-- .../bitrate_controller_impl.h | 9 -- .../include/bitrate_controller.h | 4 - .../bitrate_controller/remb_suppressor.cc | 121 ------------------ .../bitrate_controller/remb_suppressor.h | 54 -------- .../remb_suppressor_unittest.cc | 112 ---------------- webrtc/modules/modules.gyp | 1 - webrtc/video_engine/vie_encoder.cc | 3 - 10 files changed, 1 insertion(+), 323 deletions(-) delete mode 100644 webrtc/modules/bitrate_controller/remb_suppressor.cc delete mode 100644 webrtc/modules/bitrate_controller/remb_suppressor.h delete mode 100644 webrtc/modules/bitrate_controller/remb_suppressor_unittest.cc diff --git a/webrtc/modules/bitrate_controller/BUILD.gn b/webrtc/modules/bitrate_controller/BUILD.gn index af3b8dc3e..f7f67b899 100644 --- a/webrtc/modules/bitrate_controller/BUILD.gn +++ b/webrtc/modules/bitrate_controller/BUILD.gn @@ -15,8 +15,6 @@ source_set("bitrate_controller") { "bitrate_controller_impl.h", "include/bitrate_allocator.h", "include/bitrate_controller.h", - "remb_suppressor.cc", - "remb_suppressor.h", "send_side_bandwidth_estimation.cc", "send_side_bandwidth_estimation.h", "send_time_history.cc", diff --git a/webrtc/modules/bitrate_controller/bitrate_controller.gypi b/webrtc/modules/bitrate_controller/bitrate_controller.gypi index e1c083bc4..a0c2fc92f 100644 --- a/webrtc/modules/bitrate_controller/bitrate_controller.gypi +++ b/webrtc/modules/bitrate_controller/bitrate_controller.gypi @@ -20,8 +20,6 @@ 'bitrate_controller_impl.h', 'include/bitrate_controller.h', 'include/bitrate_allocator.h', - 'remb_suppressor.cc', - 'remb_suppressor.h', 'send_side_bandwidth_estimation.cc', 'send_side_bandwidth_estimation.h', 'send_time_history.cc', diff --git a/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc b/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc index 477276b3b..d6f64fbd2 100644 --- a/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc +++ b/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc @@ -93,8 +93,7 @@ BitrateControllerImpl::BitrateControllerImpl(Clock* clock, last_bitrate_bps_(0), last_fraction_loss_(0), last_rtt_ms_(0), - last_reserved_bitrate_bps_(0), - remb_suppressor_(new RembSuppressor(clock)) { + last_reserved_bitrate_bps_(0) { } BitrateControllerImpl::~BitrateControllerImpl() { @@ -127,9 +126,6 @@ void BitrateControllerImpl::SetReservedBitrate(uint32_t reserved_bitrate_bps) { void BitrateControllerImpl::OnReceivedEstimatedBitrate(uint32_t bitrate) { { CriticalSectionScoped cs(critsect_); - if (remb_suppressor_->SuppresNewRemb(bitrate)) { - return; - } bandwidth_estimation_.UpdateReceiverEstimate(bitrate); } MaybeTriggerOnNetworkChanged(); @@ -208,14 +204,4 @@ bool BitrateControllerImpl::AvailableBandwidth(uint32_t* bandwidth) const { return false; } -void BitrateControllerImpl::SetBitrateSent(uint32_t bitrate_sent_bps) { - CriticalSectionScoped cs(critsect_); - remb_suppressor_->SetBitrateSent(bitrate_sent_bps); -} - -void BitrateControllerImpl::SetCodecMode(webrtc::VideoCodecMode mode) { - CriticalSectionScoped cs(critsect_); - remb_suppressor_->SetEnabled(mode == kScreensharing); -} - } // namespace webrtc diff --git a/webrtc/modules/bitrate_controller/bitrate_controller_impl.h b/webrtc/modules/bitrate_controller/bitrate_controller_impl.h index f089be9d6..dec4afad8 100644 --- a/webrtc/modules/bitrate_controller/bitrate_controller_impl.h +++ b/webrtc/modules/bitrate_controller/bitrate_controller_impl.h @@ -21,7 +21,6 @@ #include #include "webrtc/base/scoped_ptr.h" -#include "webrtc/modules/bitrate_controller/remb_suppressor.h" #include "webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h" #include "webrtc/system_wrappers/interface/critical_section_wrapper.h" @@ -45,11 +44,6 @@ class BitrateControllerImpl : public BitrateController { int64_t TimeUntilNextProcess() override; int32_t Process() override; - // Current bitrate actually being sent. - void SetBitrateSent(uint32_t bitrate_sent_bps) override; - - void SetCodecMode(webrtc::VideoCodecMode mode) override; - private: class RtcpBandwidthObserverImpl; @@ -75,15 +69,12 @@ class BitrateControllerImpl : public BitrateController { CriticalSectionWrapper* critsect_; SendSideBandwidthEstimation bandwidth_estimation_ GUARDED_BY(*critsect_); - bool enforce_min_bitrate_ GUARDED_BY(*critsect_); uint32_t reserved_bitrate_bps_ GUARDED_BY(*critsect_); uint32_t last_bitrate_bps_ GUARDED_BY(*critsect_); uint8_t last_fraction_loss_ GUARDED_BY(*critsect_); int64_t last_rtt_ms_ GUARDED_BY(*critsect_); - bool last_enforce_min_bitrate_ GUARDED_BY(*critsect_); uint32_t last_reserved_bitrate_bps_ GUARDED_BY(*critsect_); - rtc::scoped_ptr remb_suppressor_ GUARDED_BY(*critsect_); DISALLOW_IMPLICIT_CONSTRUCTORS(BitrateControllerImpl); }; diff --git a/webrtc/modules/bitrate_controller/include/bitrate_controller.h b/webrtc/modules/bitrate_controller/include/bitrate_controller.h index c12dbebcb..7303d069a 100644 --- a/webrtc/modules/bitrate_controller/include/bitrate_controller.h +++ b/webrtc/modules/bitrate_controller/include/bitrate_controller.h @@ -60,10 +60,6 @@ class BitrateController : public Module { virtual bool AvailableBandwidth(uint32_t* bandwidth) const = 0; virtual void SetReservedBitrate(uint32_t reserved_bitrate_bps) = 0; - - virtual void SetBitrateSent(uint32_t bitrate_sent_bps) = 0; - - virtual void SetCodecMode(webrtc::VideoCodecMode mode) = 0; }; } // namespace webrtc #endif // WEBRTC_MODULES_BITRATE_CONTROLLER_INCLUDE_BITRATE_CONTROLLER_H_ diff --git a/webrtc/modules/bitrate_controller/remb_suppressor.cc b/webrtc/modules/bitrate_controller/remb_suppressor.cc deleted file mode 100644 index f65837a04..000000000 --- a/webrtc/modules/bitrate_controller/remb_suppressor.cc +++ /dev/null @@ -1,121 +0,0 @@ -/* - * Copyright (c) 2014 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 "webrtc/modules/bitrate_controller/remb_suppressor.h" - -#include - -#include "webrtc/system_wrappers/interface/field_trial.h" - -namespace webrtc { - -// If BWE falls more than this fraction from one REMB to the next, -// classify this as a glitch. -static const double kMaxBweDropRatio = 0.45; - -// If we are sending less then this fraction of the last REMB when a glitch -// is detected, start suppressing REMB. -static const double kMinSendBitrateFraction = 0.75; - -// Minimum fractional BWE growth per second needed to keep suppressing. -static const double kMinGrowth = 0.015; - -RembSuppressor::RembSuppressor(Clock* clock) - : enabled_(false), - clock_(clock), - last_remb_bps_(0), - bitrate_sent_bps_(0), - last_remb_ignored_bps_(0), - last_remb_ignore_time_ms_(0), - remb_silence_start_(0) { -} - -RembSuppressor::~RembSuppressor() { -} - -bool RembSuppressor::SuppresNewRemb(uint32_t bitrate_bps) { - if (!Enabled()) - return false; - - if (remb_silence_start_ == 0) { - // Not currently suppressing. Check if there is a bit rate drop - // significant enough to warrant suppression. - return StartSuppressing(bitrate_bps); - } - - // Check if signs point to recovery, otherwise back off suppression. - if (!ContinueSuppressing(bitrate_bps)) { - remb_silence_start_ = 0; - last_remb_ignored_bps_ = 0; - last_remb_ignore_time_ms_ = 0; - return false; - } - return true; -} - -bool RembSuppressor::StartSuppressing(uint32_t bitrate_bps) { - if (bitrate_bps < - static_cast(last_remb_bps_ * kMaxBweDropRatio + 0.5)) { - if (bitrate_sent_bps_ < - static_cast(last_remb_bps_ * kMinSendBitrateFraction + 0.5)) { - int64_t now = clock_->TimeInMilliseconds(); - remb_silence_start_ = now; - last_remb_ignore_time_ms_ = now; - last_remb_ignored_bps_ = bitrate_bps; - return true; - } - } - last_remb_bps_ = bitrate_bps; - return false; -} - -bool RembSuppressor::ContinueSuppressing(uint32_t bitrate_bps) { - int64_t now = clock_->TimeInMilliseconds(); - - if (bitrate_bps >= last_remb_bps_) { - // We have fully recovered, stop suppressing! - return false; - } - - // If exactly the same REMB, we probably don't have a new estimate. Keep on - // suppressing. However, if REMB is going down or just not increasing fast - // enough (kMinGrowth = 0.015 => REMB should increase by at least 1.5% / s) - // it looks like the link capacity has actually deteriorated and we are - // currently over-utilizing; back off. - if (bitrate_bps != last_remb_ignored_bps_) { - double delta_t = (now - last_remb_ignore_time_ms_) / 1000.0; - double min_increase = pow(1.0 + kMinGrowth, delta_t); - if (bitrate_bps < last_remb_ignored_bps_ * min_increase) { - return false; - } - } - - last_remb_ignored_bps_ = bitrate_bps; - last_remb_ignore_time_ms_ = now; - - return true; -} - -void RembSuppressor::SetBitrateSent(uint32_t bitrate_bps) { - bitrate_sent_bps_ = bitrate_bps; -} - -bool RembSuppressor::Enabled() { - return enabled_; -} - -void RembSuppressor::SetEnabled(bool enabled) { - enabled_ = enabled && - webrtc::field_trial::FindFullName( - "WebRTC-ConditionalRembSuppression") == "Enabled"; -} - -} // namespace webrtc diff --git a/webrtc/modules/bitrate_controller/remb_suppressor.h b/webrtc/modules/bitrate_controller/remb_suppressor.h deleted file mode 100644 index 987f97fde..000000000 --- a/webrtc/modules/bitrate_controller/remb_suppressor.h +++ /dev/null @@ -1,54 +0,0 @@ -/* - * Copyright (c) 2014 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. - * - * Usage: this class will register multiple RtcpBitrateObserver's one at each - * RTCP module. It will aggregate the results and run one bandwidth estimation - * and push the result to the encoder via VideoEncoderCallback. - */ - -#ifndef THIRD_PARTY_WEBRTC_MODULES_BITRATE_CONTROLLER_REMB_SUPPRESSOR_H_ -#define THIRD_PARTY_WEBRTC_MODULES_BITRATE_CONTROLLER_REMB_SUPPRESSOR_H_ - -#include "webrtc/modules/rtp_rtcp/interface/rtp_rtcp_defines.h" -#include "webrtc/system_wrappers/interface/clock.h" - -namespace webrtc { - -class RembSuppressor { - public: - explicit RembSuppressor(Clock* clock); - virtual ~RembSuppressor(); - - // Check whether this new REMB value should be suppressed. - bool SuppresNewRemb(uint32_t bitrate_bps); - // Update the current bitrate actually being sent. - void SetBitrateSent(uint32_t bitrate_bps); - // Turn suppression on or off. - void SetEnabled(bool enabled); - - protected: - virtual bool Enabled(); - - private: - bool StartSuppressing(uint32_t bitrate_bps); - bool ContinueSuppressing(uint32_t bitrate_bps); - - bool enabled_; - Clock* const clock_; - uint32_t last_remb_bps_; - uint32_t bitrate_sent_bps_; - uint32_t last_remb_ignored_bps_; - uint32_t last_remb_ignore_time_ms_; - int64_t remb_silence_start_; - - DISALLOW_COPY_AND_ASSIGN(RembSuppressor); -}; - -} // namespace webrtc -#endif // THIRD_PARTY_WEBRTC_MODULES_BITRATE_CONTROLLER_REMB_SUPPRESSOR_H_ diff --git a/webrtc/modules/bitrate_controller/remb_suppressor_unittest.cc b/webrtc/modules/bitrate_controller/remb_suppressor_unittest.cc deleted file mode 100644 index 834ec5fe0..000000000 --- a/webrtc/modules/bitrate_controller/remb_suppressor_unittest.cc +++ /dev/null @@ -1,112 +0,0 @@ -/* - * Copyright (c) 2014 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 "testing/gtest/include/gtest/gtest.h" - -#include -#include - -#include "webrtc/modules/bitrate_controller/remb_suppressor.h" - -class TestSuppressor : public webrtc::RembSuppressor { - public: - explicit TestSuppressor(webrtc::Clock* clock) : RembSuppressor(clock) {} - virtual ~TestSuppressor() {} - - bool Enabled() override { return true; } -}; - -class RembSuppressorTest : public ::testing::Test { - protected: - RembSuppressorTest() : clock_(0), suppressor_(&clock_) {} - ~RembSuppressorTest() {} - - virtual void SetUp() {} - - virtual void TearDown() {} - - bool NewRemb(uint32_t bitrate_bps) { - suppressor_.SetBitrateSent(bitrate_bps); - bool suppress = suppressor_.SuppresNewRemb(bitrate_bps); - // Default one REMB per second. - clock_.AdvanceTimeMilliseconds(1000); - return suppress; - } - - webrtc::SimulatedClock clock_; - TestSuppressor suppressor_; -}; - -TEST_F(RembSuppressorTest, Basic) { - // Never true on first sample. - EXPECT_FALSE(NewRemb(50000)); - // Some rampup. - EXPECT_FALSE(NewRemb(55000)); - EXPECT_FALSE(NewRemb(60500)); - EXPECT_FALSE(NewRemb(66550)); - EXPECT_FALSE(NewRemb(73250)); - - // Reached limit, some fluctuation ok. - EXPECT_FALSE(NewRemb(72100)); - EXPECT_FALSE(NewRemb(75500)); - EXPECT_FALSE(NewRemb(69250)); - EXPECT_FALSE(NewRemb(73250)); -} - -TEST_F(RembSuppressorTest, RecoveryTooSlow) { - // Never true on first sample. - EXPECT_FALSE(NewRemb(50000)); - - // Large drop. - EXPECT_TRUE(NewRemb(22499)); - - // No new estimate, still suppressing. - EXPECT_TRUE(NewRemb(22499)); - - // Too little increase - stop suppressing. - EXPECT_FALSE(NewRemb(22835)); -} - -TEST_F(RembSuppressorTest, RembDownDurinSupression) { - // Never true on first sample. - EXPECT_FALSE(NewRemb(50000)); - - // Large drop. - EXPECT_TRUE(NewRemb(22499)); - - // Remb is not allowed to fall. - EXPECT_FALSE(NewRemb(22498)); -} - -TEST_F(RembSuppressorTest, GlitchWithRecovery) { - const uint32_t start_bitrate = 300000; - uint32_t bitrate = start_bitrate; - // Never true on first sample. - EXPECT_FALSE(NewRemb(bitrate)); - - bitrate = static_cast(bitrate * 0.44); - EXPECT_TRUE(NewRemb(bitrate)); - - while (bitrate < start_bitrate) { - EXPECT_TRUE(NewRemb(bitrate)); - bitrate = static_cast(bitrate * 1.10); - } - - EXPECT_FALSE(NewRemb(bitrate)); -} - -TEST_F(RembSuppressorTest, BitrateSent) { - // Never true on first sample. - EXPECT_FALSE(NewRemb(50000)); - - // Only suppress large drop, if we are not sending at full capacity. - suppressor_.SetBitrateSent(37500); - EXPECT_FALSE(suppressor_.SuppresNewRemb(22499)); -} diff --git a/webrtc/modules/modules.gyp b/webrtc/modules/modules.gyp index de21ae66a..128380953 100644 --- a/webrtc/modules/modules.gyp +++ b/webrtc/modules/modules.gyp @@ -187,7 +187,6 @@ 'audio_processing/utility/delay_estimator_unittest.cc', 'bitrate_controller/bitrate_allocator_unittest.cc', 'bitrate_controller/bitrate_controller_unittest.cc', - 'bitrate_controller/remb_suppressor_unittest.cc', 'bitrate_controller/send_side_bandwidth_estimation_unittest.cc', 'bitrate_controller/send_time_history_unittest.cc', 'desktop_capture/desktop_and_cursor_composer_unittest.cc', diff --git a/webrtc/video_engine/vie_encoder.cc b/webrtc/video_engine/vie_encoder.cc index f86beef4f..f71149f29 100644 --- a/webrtc/video_engine/vie_encoder.cc +++ b/webrtc/video_engine/vie_encoder.cc @@ -408,8 +408,6 @@ int32_t ViEEncoder::SetEncoder(const webrtc::VideoCodec& video_codec) { &new_bwe_max_bps); bitrate_controller_->SetMinMaxBitrate(new_bwe_min_bps, new_bwe_max_bps); - bitrate_controller_->SetCodecMode(video_codec.mode); - int pad_up_to_bitrate_bps = GetPaddingNeededBps(1000 * video_codec.startBitrate); paced_sender_->UpdateBitrate( @@ -796,7 +794,6 @@ int32_t ViEEncoder::SendData( int32_t ViEEncoder::SendStatistics(const uint32_t bit_rate, const uint32_t frame_rate) { - bitrate_controller_->SetBitrateSent(bit_rate); CriticalSectionScoped cs(callback_cs_.get()); if (codec_observer_) { codec_observer_->OutgoingRate(channel_id_, frame_rate, bit_rate);