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
+
+std::unique_ptr<BitrateEstimator> BitrateEstimatorCreator::Create() const {
+  return rtc::MakeUnique<BitrateEstimator>();
+}
+
+AcknowledgedBitrateEstimator::AcknowledgedBitrateEstimator(
+    std::unique_ptr<BitrateEstimatorCreator> bitrate_estimator_creator)
+    : last_alr_state_(false),
+      bitrate_estimator_creator_(
+          bitrate_estimator_creator
+              ? std::move(bitrate_estimator_creator)
+              : rtc::MakeUnique<BitrateEstimatorCreator>()),
+      bitrate_estimator_(bitrate_estimator_creator_->Create()) {}
+
+void AcknowledgedBitrateEstimator::IncomingPacketFeedbackVector(
+    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) && !SentInAlrState(packet))
+      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::SentInAlrState(

[terelius, 2017/06/13 14:03:20]

I'd also prefer a more descriptive name for this function. Or just manually
inline it.

[tschumi, 2017/06/14 07:47:44]

Renamed it to "SentBeforeAlrEnded"

+    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();

[terelius, 2017/06/13 14:03:20]

Do we really want to forget that we have left ALR just because we received a
packet that was sent after the change?? If packets are acknowledged out of order
we might still receive packets that were sent in ALR. Or we assume that we never
reorder packets for more than 1 RTT?

[tschumi, 2017/06/14 07:47:44]

I'm not sure if it's worth to add additional complexity here to handle the case
of unordered packets.

+    }
+  }
+  return false;
+}
+
+bool AcknowledgedBitrateEstimator::HasLeftAlrState(bool alr_state) const {
+  return last_alr_state_ && !alr_state;

[terelius, 2017/06/13 14:03:20]

Maybe call them was_in_alr and currently_in_alr? Adding "_state" seems redundant
for a boolean, and "Application Limited Region state" sounds strage to me.

[tschumi, 2017/06/14 07:47:43]

Acknowledged.

+}
+
+void AcknowledgedBitrateEstimator::MaybeResetBitrateEstimator(bool alr_state) {
+  if (HasLeftAlrState(alr_state)) {

[terelius, 2017/06/13 14:03:20]

Manually inline this function?

[tschumi, 2017/06/14 07:47:43]

Don't you think the function name adds additional value in terms of readability
?
Renamed it to "AlrEnded" according to "SentBeforeAlrEnded".

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