Reimplemeted "Test and fix for huge bwe drop after alr state"

webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc

+/*
+ *  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.
+ */
+
+#include "webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h"
+
+#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 {
+bool IsInSendTimeHistory(const PacketFeedback& packet) {
+  return packet.send_time_ms >= 0;
+}
+}  // namespace
+
+AcknowledgedBitrateEstimator::AcknowledgedBitrateEstimator()
+    : AcknowledgedBitrateEstimator(rtc::MakeUnique<BitrateEstimator>()) {}
+
+AcknowledgedBitrateEstimator::AcknowledgedBitrateEstimator(
+    std::unique_ptr<BitrateEstimator> bitrate_estimator)
+    : bitrate_estimator_(std::move(bitrate_estimator)) {}
+
+void AcknowledgedBitrateEstimator::IncomingPacketFeedbackVector(
+    const std::vector<PacketFeedback>& packet_feedback_vector,
+    bool currently_in_alr) {
+  RTC_DCHECK(std::is_sorted(packet_feedback_vector.begin(),
+                            packet_feedback_vector.end(),
+                            PacketFeedbackComparator()));
+  CheckAlrEndTime(currently_in_alr);
+  for (const auto& packet : packet_feedback_vector) {
+    if (IsInSendTimeHistory(packet)) {
+      MaybeExpectFastRateChange(packet.send_time_ms);
+      bitrate_estimator_->Update(packet.arrival_time_ms, packet.payload_size);
+    }
+  }
+}
+
+rtc::Optional<uint32_t> AcknowledgedBitrateEstimator::bitrate_bps() const {
+  return bitrate_estimator_->bitrate_bps();
+}
+
+bool AcknowledgedBitrateEstimator::AlrEnded(bool currently_in_alr) const {
+  return was_in_alr_time_ms_ && !currently_in_alr;
+}
+
+void AcknowledgedBitrateEstimator::CheckAlrEndTime(bool currently_in_alr) {

[terelius, 2017/07/11 20:54:19]

How about
bool currently_in_alr = pacer->GetAlrStartTime().has_value()
if (was_in_alr && !currently_in_alr) {
  ack_bitrate_estimator.SetAlrExitTime(now_ms)
}
was_in_alr = currently_in_alr
in the congestion controller?

That way we wouldn't have to pass currently_in_alr to every function in this
class. In the future, we could also reuse the same logic to set the alr exit
time of the probe_controller.

[tschumi, 2017/07/13 09:05:07]

Impl. according to offline discussion. 

+  if (AlrEnded(currently_in_alr)) {
+    alr_ended_time_ms_ = was_in_alr_time_ms_;
+  }
+  was_in_alr_time_ms_ = currently_in_alr
+                            ? rtc::Optional<int64_t>(rtc::TimeMillis())
+                            : rtc::Optional<int64_t>();
+}
+
+void AcknowledgedBitrateEstimator::MaybeExpectFastRateChange(

[terelius, 2017/07/11 20:54:19]

Maybe inline this function? (It is important that we reset alr_ended_time_ms_,
but that is not obvious from the function name. Hence, I have to jump into the
function to look at what it does. Personally, I find this more  distracting than
adding 3 lines to the function above.)

[tschumi, 2017/07/13 09:05:07]

:)
I prefer it this way.

+    int64_t packet_send_time_ms) {
+  if (alr_ended_time_ms_ && packet_send_time_ms > *alr_ended_time_ms_) {
+    bitrate_estimator_->ExpectFastRateChange();
+    alr_ended_time_ms_.reset();
+  }
+}
+
+}  // namespace webrtc