Test and fix for huge bwe drop after alr state.

webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h

Index: webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h
diff --git a/webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h b/webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h
index 7a9d669a1cb6a5473f99d77d30152cacc71e4955..6261978278f928f2a130a5cd184c48d28fc239a1 100644
--- a/webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h
+++ b/webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h
@@ -11,36 +11,39 @@
 #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);
-
-  int sum_;
-  int64_t current_win_ms_;
-  int64_t prev_time_ms_;
-  float bitrate_estimate_;
-  float bitrate_estimate_var_;
+  bool ShouldSkipPreAlrPacket(const PacketFeedback& packet);
+  bool HasLeftAlrState(bool alr_state) const;
+  void MaybeResetBitrateEstimator(bool alr_state);
+
+  bool last_alr_state_;
+  rtc::Optional<int64_t> left_alr_state_ms_;
+  BitrateEstimatorCreator bitrate_estimator_creator_;
+  std::unique_ptr<BitrateEstimator> bitrate_estimator_;
 };
 
 }  // namespace webrtc