Test and fix for huge bwe drop after alr state.

webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h

 /*
  *  Copyright (c) 2017 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.
  */
 
 #ifndef WEBRTC_MODULES_CONGESTION_CONTROLLER_ACKNOWLEDGE_BITRATE_ESTIMATOR_H_
 #define WEBRTC_MODULES_CONGESTION_CONTROLLER_ACKNOWLEDGE_BITRATE_ESTIMATOR_H_
 
+#include <memory>
 #include <vector>
 
+#include "webrtc/base/function_view.h"

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

Remove this? You don't appear to use it.

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

Done.

 #include "webrtc/base/optional.h"
+#include "webrtc/modules/congestion_controller/bitrate_estimator.h"
 
 namespace webrtc {
 
 struct PacketFeedback;
 
-// Computes a bayesian estimate of the throughput given acks containing
-// the arrival time and payload size. Samples which are far from the current
-// estimate or are based on few packets are given a smaller weight, as they
-// are considered to be more likely to have been caused by, e.g., delay spikes
-// unrelated to congestion.
+using BitrateEstimatorCreator =
+    std::function<std::unique_ptr<BitrateEstimator>()>;

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

Hmm. I wonder if it wouldn't be simpler (in the sense of making the code easier
to read) with an interface instead:

  class BitrateEstimatorCreator {
   public:
    virtual ~BitrateEstimatorCreator() = default;
    virtual std::unique_ptr<BitrateEstimator> CreateBitrateEstimator() = 0;
  };

And have the AcknowledgedBitrateEstimator constructor take a
std::unique_ptr<BitrateEstimatorCreator>.

It would be more lines of code at each call site, though. How bad would that be?

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

Ok I went for that solution.
Looks fine to me :)

+
 class AcknowledgedBitrateEstimator {

[minyue-webrtc, 2017/06/12 10:08:07]

shouldn't the file name be called acknowledge"d"_bitrate_estimator?

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

Done.

  public:
-  AcknowledgedBitrateEstimator();
+  AcknowledgedBitrateEstimator(
+      BitrateEstimatorCreator&& bitrate_estimator_creator = nullptr);

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

The style guide says to not use rvalue ref arguments. Pass by value instead?

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

Pass now a std::unique_ptr<BitrateEstimatorCreator> 

 
   void IncomingPacketFeedbackVector(
-      const std::vector<PacketFeedback>& packet_feedback_vector);
+      const std::vector<PacketFeedback>& packet_feedback_vector,
+      bool alr_state);

[minyue-webrtc, 2017/06/12 10:08:07]

what is alr? good to mention it in a comment

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

alr = ApplicationLimitedRegion

I'm not sure if we should mention this at every point we use it. It feels to me
this is kind of domain based knowledge, and its ok to use it like that. But I
don't have a strong opinion on that. 

I general i like to do as less comment as possible. :)
 

   rtc::Optional<uint32_t> bitrate_bps() const;
 
  private:
-  void Update(int64_t now_ms, int bytes);
-  float UpdateWindow(int64_t now_ms, int bytes, int rate_window_ms);
+  bool ShouldSkipPreAlrPacket(const PacketFeedback& packet);
+  bool HasLeftAlrState(bool alr_state) const;
+  void MaybeResetBitrateEstimator(bool alr_state);
 
-  int sum_;
-  int64_t current_win_ms_;
-  int64_t prev_time_ms_;
-  float bitrate_estimate_;
-  float bitrate_estimate_var_;
+  bool last_alr_state_;
+  rtc::Optional<int64_t> left_alr_state_ms_;
+  BitrateEstimatorCreator bitrate_estimator_creator_;
+  std::unique_ptr<BitrateEstimator> bitrate_estimator_;
 };
 
 }  // namespace webrtc
 
 #endif  // WEBRTC_MODULES_CONGESTION_CONTROLLER_ACKNOWLEDGE_BITRATE_ESTIMATOR_H_