fix wrong implementation of AimdRateControl::TimeToReduceFurther.

webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc

 /*
  *  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.
  */
 
@@ skipping 54 matching lines @@
 }
 
 bool AimdRateControl::TimeToReduceFurther(int64_t time_now,
                                           uint32_t incoming_bitrate_bps) const {
   const int64_t bitrate_reduction_interval =
       std::max<int64_t>(std::min<int64_t>(rtt_, 200), 10);
   if (time_now - time_last_bitrate_change_ >= bitrate_reduction_interval) {
     return true;
   }
   if (ValidEstimate()) {
-    const int threshold = static_cast<int>(kWithinIncomingBitrateHysteresis *
-                                           incoming_bitrate_bps);
-    const int bitrate_difference = LatestEstimate() - incoming_bitrate_bps;
-    return bitrate_difference > threshold;
+    const uint32_t threshold = static_cast<uint32_t>
+        (kWithinIncomingBitrateHysteresis * LatestEstimate());
+    return incoming_bitrate_bps > threshold;

[stefan-webrtc, 2016/11/29 15:52:08]

Agree that it's wrong! I think the comment is wrong too, and that this should
say:
return incoming_bitrate_bps < 0.95 * threshold;

The reasoning is that, if the incoming bitrate is even lower than our estimate,
it's probably a good idea to further decrease the estimate.

Could I request that you add a unittest in aimd_rate_control_unittests.cc and
update the comment in the .h file?

[zhengpeng, 2016/11/30 04:33:11]

After reconsideration, I agree that if we are overusing and the actual bitrate
is very close to (or lower than) our lastest estimate, then it may be a good
opportunity to further reduce the bitrate estimate. 

"Hysteresis" implies it was originally intended to detect lags, since actual
changes always happen behind estimate changes.If this is the case, then I guess
it should be
return incoming_bitrate_bps < 1.05 * LatestEstimate;
It gives AimdRateControl early chance to reduce bitrate, which make bitrate
reduction more aggressive.

On the other hand, "return incoming_bitrate_bps < 0.95 * LatestEstimate" seems
more conservative, and the 5% losts the original meaning. 

Anyway, my patch is wrong, though it complies with the comment.
I will follow your suggested modification and re-commit patches.

   }
   return false;
 }
 
 uint32_t AimdRateControl::LatestEstimate() const {
   return current_bitrate_bps_;
 }
 
 uint32_t AimdRateControl::UpdateBandwidthEstimate(int64_t now_ms) {
   current_bitrate_bps_ = ChangeBitrate(
@@ skipping 210 matching lines @@
 }
 
 void AimdRateControl::ChangeRegion(RateControlRegion region) {
   rate_control_region_ = region;
 }
 
 void AimdRateControl::ChangeState(RateControlState new_state) {
   rate_control_state_ = new_state;
 }
 }  // namespace webrtc