Test and fix for huge bwe drop after alr state.

webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc

Index: webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc
diff --git a/webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc b/webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc
index 9df378c64b8f24165703eb738e5078b6b7db5fb3..8f38d666a12c0cbf201544a510b31687bcd6637c 100644
--- a/webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc
+++ b/webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc
@@ -10,103 +10,67 @@
 
 #include "webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h"
 
-#include <cmath>
+#include <utility>
 
+#include "webrtc/base/ptr_util.h"
+#include "webrtc/base/timeutils.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 
 namespace webrtc {
 
 namespace {
-constexpr int kInitialRateWindowMs = 500;
-constexpr int kRateWindowMs = 150;
-
 bool IsInSendTimeHistory(const PacketFeedback& packet) {
   return packet.send_time_ms >= 0;
 }
-
 }  // namespace
 
-AcknowledgedBitrateEstimator::AcknowledgedBitrateEstimator()
-    : sum_(0),
-      current_win_ms_(0),
-      prev_time_ms_(-1),
-      bitrate_estimate_(-1.0f),
-      bitrate_estimate_var_(50.0f) {}
+AcknowledgedBitrateEstimator::AcknowledgedBitrateEstimator(
+    BitrateEstimatorCreator&& bitrate_estimator_creator)
+    : last_alr_state_(false),
+      bitrate_estimator_creator_(bitrate_estimator_creator
+                                     ? std::move(bitrate_estimator_creator)
+                                     : &rtc::MakeUnique<BitrateEstimator>),

[kwiberg-webrtc, 2017/06/08 23:00:29]

Can you remove this fallback, and force all callers to pass a non-empty bitrate
estimator creator? That makes the interface between AcknowledgedBitrateEstimator
and its callers simpler---at the cost of adding some complexity at the call
sites---which is usually a net win.

[tschumi, 2017/06/12 11:33:27]

Done.

But I'm not completely sure if the added complexity is worth the better
readability of the ctor. 

WDYT ?

+      bitrate_estimator_(bitrate_estimator_creator_()) {}
 
 void AcknowledgedBitrateEstimator::IncomingPacketFeedbackVector(
-    const std::vector<PacketFeedback>& packet_feedback_vector) {
+    const std::vector<PacketFeedback>& packet_feedback_vector,
+    bool alr_state) {
   RTC_DCHECK(std::is_sorted(packet_feedback_vector.begin(),
                             packet_feedback_vector.end(),
                             PacketFeedbackComparator()));
+  MaybeResetBitrateEstimator(alr_state);
   for (const auto& packet : packet_feedback_vector) {
-    if (IsInSendTimeHistory(packet))
-      Update(packet.arrival_time_ms, packet.payload_size);
+    if (IsInSendTimeHistory(packet) && !ShouldSkipPreAlrPacket(packet))
+      bitrate_estimator_->Update(packet.arrival_time_ms, packet.payload_size);
   }
 }
 
-void AcknowledgedBitrateEstimator::Update(int64_t now_ms, int bytes) {
-  int rate_window_ms = kRateWindowMs;
-  // We use a larger window at the beginning to get a more stable sample that
-  // we can use to initialize the estimate.
-  if (bitrate_estimate_ < 0.f)
-    rate_window_ms = kInitialRateWindowMs;
-  float bitrate_sample = UpdateWindow(now_ms, bytes, rate_window_ms);
-  if (bitrate_sample < 0.0f)
-    return;
-  if (bitrate_estimate_ < 0.0f) {
-    // This is the very first sample we get. Use it to initialize the estimate.
-    bitrate_estimate_ = bitrate_sample;
-    return;
-  }
-  // Define the sample uncertainty as a function of how far away it is from the
-  // current estimate.
-  float sample_uncertainty =
-      10.0f * std::abs(bitrate_estimate_ - bitrate_sample) / bitrate_estimate_;
-  float sample_var = sample_uncertainty * sample_uncertainty;
-  // Update a bayesian estimate of the rate, weighting it lower if the sample
-  // uncertainty is large.
-  // The bitrate estimate uncertainty is increased with each update to model
-  // that the bitrate changes over time.
-  float pred_bitrate_estimate_var = bitrate_estimate_var_ + 5.f;
-  bitrate_estimate_ = (sample_var * bitrate_estimate_ +
-                       pred_bitrate_estimate_var * bitrate_sample) /
-                      (sample_var + pred_bitrate_estimate_var);
-  bitrate_estimate_var_ = sample_var * pred_bitrate_estimate_var /
-                          (sample_var + pred_bitrate_estimate_var);
+rtc::Optional<uint32_t> AcknowledgedBitrateEstimator::bitrate_bps() const {
+  return bitrate_estimator_->bitrate_bps();
 }
 
-float AcknowledgedBitrateEstimator::UpdateWindow(int64_t now_ms,
-                                                 int bytes,
-                                                 int rate_window_ms) {
-  // Reset if time moves backwards.
-  if (now_ms < prev_time_ms_) {
-    prev_time_ms_ = -1;
-    sum_ = 0;
-    current_win_ms_ = 0;
-  }
-  if (prev_time_ms_ >= 0) {
-    current_win_ms_ += now_ms - prev_time_ms_;
-    // Reset if nothing has been received for more than a full window.
-    if (now_ms - prev_time_ms_ > rate_window_ms) {
-      sum_ = 0;
-      current_win_ms_ %= rate_window_ms;
+bool AcknowledgedBitrateEstimator::ShouldSkipPreAlrPacket(

[philipel, 2017/06/09 14:49:53]

WDYT about renaming this to SentInAlr?

[tschumi, 2017/06/12 11:33:28]

Hmm "SentInAlr" tells just parts of what this function try's to do.
What about "SentBeforeLeftAlr" ? 

+    const PacketFeedback& packet) {
+  if (left_alr_state_ms_) {
+    if (*left_alr_state_ms_ > packet.send_time_ms) {
+      return true;
+    } else {
+      left_alr_state_ms_.reset();
     }
   }
-  prev_time_ms_ = now_ms;
-  float bitrate_sample = -1.0f;
-  if (current_win_ms_ >= rate_window_ms) {
-    bitrate_sample = 8.0f * sum_ / static_cast<float>(rate_window_ms);
-    current_win_ms_ -= rate_window_ms;
-    sum_ = 0;
-  }
-  sum_ += bytes;
-  return bitrate_sample;
+  return false;
 }
 
-rtc::Optional<uint32_t> AcknowledgedBitrateEstimator::bitrate_bps() const {
-  if (bitrate_estimate_ < 0.f)
-    return rtc::Optional<uint32_t>();
-  return rtc::Optional<uint32_t>(bitrate_estimate_ * 1000);
+bool AcknowledgedBitrateEstimator::HasLeftAlrState(bool alr_state) const {
+  return last_alr_state_ && !alr_state;
+}
+
+void AcknowledgedBitrateEstimator::MaybeResetBitrateEstimator(bool alr_state) {
+  if (HasLeftAlrState(alr_state)) {

[philipel, 2017/06/09 14:49:53]

If you only expect HasLeftAlrState to be called from here, remove it and replace
it with something like:
bool has_left_alr = last_alr_state_ && !alr_state;

[tschumi, 2017/06/12 11:33:28]

I assumed that the compiler will inline this anyway. So we would end-up with
more or less the same code. 
And in terms of code readability, I would go for the "if
(HasLeftAlrState(alr_state))"
WDYT ?

+    bitrate_estimator_ = bitrate_estimator_creator_();

[kwiberg-webrtc, 2017/06/08 23:00:29]

Hmm. If BitrateEstimator had a Reset() method, you wouldn't have to have a
creator object at all. Is that a viable approach? (That's what we ended up doing
with AudioEncoder, and it made our lives much simpler.)

[tschumi, 2017/06/12 11:33:28]

It would be much simpler. Having a reset function can be a pain as well. Since
you have to make sure the all the states are handled the right way. 

So for now i would go with this no reset approach :) 

+    left_alr_state_ms_ = rtc::Optional<int64_t>(rtc::TimeMillis());
+  }
+  last_alr_state_ = alr_state;
 }
 
 }  // namespace webrtc