Suppress REMB in bitrate ctrl if it seems lika a short network glitch.

BUG=4082
R=stefan@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/37369004

git-svn-id: http://webrtc.googlecode.com/svn/trunk@7948 4adac7df-926f-26a2-2b94-8c16560cd09d

webrtc/modules/bitrate_controller/BUILD.gn

@@ -13,6 +13,8 @@ source_set("bitrate_controller") {
     "bitrate_controller_impl.cc",
     "bitrate_controller_impl.h",
     "include/bitrate_controller.h",
+    "remb_suppressor.cc",
+    "remb_suppressor.h",
     "send_side_bandwidth_estimation.cc",
     "send_side_bandwidth_estimation.h",
   ]

webrtc/modules/bitrate_controller/bitrate_controller.gypi

@@ -18,6 +18,8 @@
         'bitrate_controller_impl.cc',
         'bitrate_controller_impl.h',
         'include/bitrate_controller.h',
+        'remb_suppressor.cc',
+        'remb_suppressor.h',
         'send_side_bandwidth_estimation.cc',
         'send_side_bandwidth_estimation.h',
       ],

webrtc/modules/bitrate_controller/bitrate_controller_impl.cc

@@ -83,7 +83,8 @@ BitrateController* BitrateController::CreateBitrateController(
   return new BitrateControllerImpl(clock, enforce_min_bitrate);
 }
 
-BitrateControllerImpl::BitrateControllerImpl(Clock* clock, bool enforce_min_bitrate)
+BitrateControllerImpl::BitrateControllerImpl(Clock* clock,
+                                             bool enforce_min_bitrate)
     : clock_(clock),
       last_bitrate_update_ms_(clock_->TimeInMilliseconds()),
       critsect_(CriticalSectionWrapper::CreateCriticalSection()),
@@ -96,7 +97,9 @@ BitrateControllerImpl::BitrateControllerImpl(Clock* clock, bool enforce_min_bitr
       last_rtt_ms_(0),
       last_enforce_min_bitrate_(!enforce_min_bitrate_),
       bitrate_observers_modified_(false),
-      last_reserved_bitrate_bps_(0) {}
+      last_reserved_bitrate_bps_(0),
+      remb_suppressor_(new RembSuppressor(clock)) {
+}
 
 BitrateControllerImpl::~BitrateControllerImpl() {
   BitrateObserverConfList::iterator it = bitrate_observers_.begin();
@@ -219,6 +222,9 @@ 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();
 }
@@ -378,4 +384,14 @@ 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

webrtc/modules/bitrate_controller/bitrate_controller_impl.h

@@ -21,6 +21,7 @@
 #include <map>
 #include <utility>
 
+#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"
 #include "webrtc/system_wrappers/interface/scoped_ptr.h"
@@ -49,6 +50,11 @@ class BitrateControllerImpl : public BitrateController {
   virtual int64_t TimeUntilNextProcess() OVERRIDE;
   virtual int32_t Process() OVERRIDE;
 
+  // Current bitrate actually being sent.
+  virtual void SetBitrateSent(uint32_t bitrate_sent_bps) OVERRIDE;
+
+  virtual void SetCodecMode(webrtc::VideoCodecMode mode) OVERRIDE;
+
  private:
   class RtcpBandwidthObserverImpl;
 
@@ -127,6 +133,7 @@ class BitrateControllerImpl : public BitrateController {
   bool last_enforce_min_bitrate_ GUARDED_BY(*critsect_);
   bool bitrate_observers_modified_ GUARDED_BY(*critsect_);
   uint32_t last_reserved_bitrate_bps_ GUARDED_BY(*critsect_);
+  scoped_ptr<RembSuppressor> remb_suppressor_ GUARDED_BY(*critsect_);
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(BitrateControllerImpl);
 };

webrtc/modules/bitrate_controller/include/bitrate_controller.h

@@ -77,6 +77,10 @@ class BitrateController : public Module {
   virtual void EnforceMinBitrate(bool enforce_min_bitrate) = 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_

webrtc/modules/bitrate_controller/remb_suppressor.cc

@@ -0,0 +1,121 @@
+/*
+ *  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 <math.h>
+
+#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<uint32_t>(last_remb_bps_ * kMaxBweDropRatio + 0.5)) {
+    if (bitrate_sent_bps_ <
+        static_cast<uint32_t>(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

webrtc/modules/bitrate_controller/remb_suppressor.h

@@ -0,0 +1,54 @@
+/*
+ *  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_

webrtc/modules/bitrate_controller/remb_suppressor_unittest.cc

@@ -0,0 +1,112 @@
+/*
+ *  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 <algorithm>
+#include <vector>
+
+#include "webrtc/modules/bitrate_controller/remb_suppressor.h"
+
+class TestSuppressor : public webrtc::RembSuppressor {
+ public:
+  explicit TestSuppressor(webrtc::Clock* clock) : RembSuppressor(clock) {}
+  virtual ~TestSuppressor() {}
+
+  virtual 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<uint32_t>(bitrate * 0.44);
+  EXPECT_TRUE(NewRemb(bitrate));
+
+  while (bitrate < start_bitrate) {
+    EXPECT_TRUE(NewRemb(bitrate));
+    bitrate = static_cast<uint32_t>(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));
+}

webrtc/modules/modules.gyp

@@ -191,6 +191,7 @@
             'audio_processing/utility/delay_estimator_unittest.cc',
             'audio_processing/utility/ring_buffer_unittest.cc',
             'bitrate_controller/bitrate_controller_unittest.cc',
+            'bitrate_controller/remb_suppressor_unittest.cc',
             'bitrate_controller/send_side_bandwidth_estimation_unittest.cc',
             'desktop_capture/desktop_and_cursor_composer_unittest.cc',
             'desktop_capture/desktop_region_unittest.cc',

webrtc/video_engine/vie_encoder.cc

@@ -389,6 +389,7 @@ int32_t ViEEncoder::SetEncoder(const webrtc::VideoCodec& video_codec) {
                                           video_codec.minBitrate * 1000,
                                           kTransmissionMaxBitrateMultiplier *
                                           video_codec.maxBitrate * 1000);
+  bitrate_controller_->SetCodecMode(video_codec.mode);
 
   CriticalSectionScoped crit(data_cs_.get());
   int pad_up_to_bitrate_kbps = video_codec.startBitrate;
@@ -755,6 +756,7 @@ int32_t ViEEncoder::ProtectionRequest(
 
 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);