Missing packet send time should not cause BWE backoff.
The removed coded causes problems if the same RTCP packet is forwarded
to the congestion controller multiple times.
Bug: webrtc:10125
Change-Id: I659d8f8f3ce3c643710156fa81176ceeaedd714a
Reviewed-on: https://webrtc-review.googlesource.com/c/114165
Commit-Queue: Björn Terelius <terelius@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26016}diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe.cc b/modules/congestion_controller/goog_cc/delay_based_bwe.cc
index 4894c98..a76adb0 100644
--- a/modules/congestion_controller/goog_cc/delay_based_bwe.cc
+++ b/modules/congestion_controller/goog_cc/delay_based_bwe.cc
@@ -46,8 +46,6 @@
constexpr double kDefaultTrendlineSmoothingCoeff = 0.9;
constexpr double kDefaultTrendlineThresholdGain = 4.0;
-constexpr int kMaxConsecutiveFailedLookups = 5;
-
const char kBweWindowSizeInPacketsExperiment[] =
"WebRTC-BweWindowSizeInPackets";
@@ -94,7 +92,6 @@
: kDefaultTrendlineWindowSize),
trendline_smoothing_coeff_(kDefaultTrendlineSmoothingCoeff),
trendline_threshold_gain_(kDefaultTrendlineThresholdGain),
- consecutive_delayed_feedbacks_(0),
prev_bitrate_(DataRate::Zero()),
prev_state_(BandwidthUsage::kBwNormal) {
RTC_LOG(LS_INFO)
@@ -147,43 +144,12 @@
}
if (delayed_feedback) {
- Timestamp arrival_time = Timestamp::PlusInfinity();
- if (packet_feedback_vector.back().arrival_time_ms > 0)
- arrival_time =
- Timestamp::ms(packet_feedback_vector.back().arrival_time_ms);
- return OnDelayedFeedback(arrival_time);
-
- } else {
- consecutive_delayed_feedbacks_ = 0;
- return MaybeUpdateEstimate(acked_bitrate, probe_bitrate,
- recovered_from_overuse, at_time);
+ // TODO(bugs.webrtc.org/10125): Design a better mechanism to safe-guard
+ // against building very large network queues.
+ return Result();
}
- return Result();
-}
-
-DelayBasedBwe::Result DelayBasedBwe::OnDelayedFeedback(Timestamp receive_time) {
- ++consecutive_delayed_feedbacks_;
- if (consecutive_delayed_feedbacks_ >= kMaxConsecutiveFailedLookups) {
- consecutive_delayed_feedbacks_ = 0;
- return OnLongFeedbackDelay(receive_time);
- }
- return Result();
-}
-
-DelayBasedBwe::Result DelayBasedBwe::OnLongFeedbackDelay(
- Timestamp arrival_time) {
- // Estimate should always be valid since a start bitrate always is set in the
- // Call constructor. An alternative would be to return an empty Result here,
- // or to estimate the throughput based on the feedback we received.
- RTC_DCHECK(rate_control_.ValidEstimate());
- rate_control_.SetEstimate(rate_control_.LatestEstimate() / 2, arrival_time);
- Result result;
- result.updated = true;
- result.probe = false;
- result.target_bitrate = rate_control_.LatestEstimate();
- RTC_LOG(LS_WARNING) << "Long feedback delay detected, reducing BWE to "
- << ToString(result.target_bitrate);
- return result;
+ return MaybeUpdateEstimate(acked_bitrate, probe_bitrate,
+ recovered_from_overuse, at_time);
}
void DelayBasedBwe::IncomingPacketFeedback(
diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe.h b/modules/congestion_controller/goog_cc/delay_based_bwe.h
index 616c5b0..ead60f3 100644
--- a/modules/congestion_controller/goog_cc/delay_based_bwe.h
+++ b/modules/congestion_controller/goog_cc/delay_based_bwe.h
@@ -49,7 +49,6 @@
absl::optional<DataRate> acked_bitrate,
absl::optional<DataRate> probe_bitrate,
Timestamp at_time);
- Result OnDelayedFeedback(Timestamp receive_time);
void OnRttUpdate(TimeDelta avg_rtt);
bool LatestEstimate(std::vector<uint32_t>* ssrcs, DataRate* bitrate) const;
void SetStartBitrate(DataRate start_bitrate);
@@ -60,7 +59,6 @@
friend class GoogCcStatePrinter;
void IncomingPacketFeedback(const PacketFeedback& packet_feedback,
Timestamp at_time);
- Result OnLongFeedbackDelay(Timestamp arrival_time);
Result MaybeUpdateEstimate(absl::optional<DataRate> acked_bitrate,
absl::optional<DataRate> probe_bitrate,
bool request_probe,
@@ -81,7 +79,6 @@
size_t trendline_window_size_;
double trendline_smoothing_coeff_;
double trendline_threshold_gain_;
- int consecutive_delayed_feedbacks_;
DataRate prev_bitrate_;
BandwidthUsage prev_state_;
diff --git a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc
index 6de143d..aa0b789 100644
--- a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc
+++ b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc
@@ -397,15 +397,9 @@
NetworkControlUpdate GoogCcNetworkController::OnTransportPacketsFeedback(
TransportPacketsFeedback report) {
if (report.packet_feedbacks.empty()) {
- DelayBasedBwe::Result result = delay_based_bwe_->OnDelayedFeedback(
- report.sendless_arrival_times.back());
- NetworkControlUpdate update;
- if (result.updated) {
- bandwidth_estimation_->UpdateDelayBasedEstimate(report.feedback_time,
- result.target_bitrate);
- MaybeTriggerOnNetworkChanged(&update, report.feedback_time);
- }
- return update;
+ // TODO(bugs.webrtc.org/10125): Design a better mechanism to safe-guard
+ // against building very large network queues.
+ return NetworkControlUpdate();
}
if (congestion_window_pushback_controller_) {
diff --git a/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc b/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc
index 62be521..340a093 100644
--- a/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc
+++ b/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc
@@ -245,74 +245,6 @@
update = controller_->OnProcessInterval(DefaultInterval());
}
-// Estimated bitrate reduced when the feedbacks arrive with such a long delay,
-// that the send-time-history no longer holds the feedbacked packets.
-TEST_F(GoogCcNetworkControllerTest, LongFeedbackDelays) {
- TargetBitrateTrackingSetup();
- const webrtc::PacedPacketInfo kPacingInfo0(0, 5, 2000);
- const webrtc::PacedPacketInfo kPacingInfo1(1, 8, 4000);
- const int64_t kFeedbackTimeoutMs = 60001;
- const int kMaxConsecutiveFailedLookups = 5;
- for (int i = 0; i < kMaxConsecutiveFailedLookups; ++i) {
- std::vector<PacketResult> packets;
- packets.push_back(CreateResult(i * 100, 2 * i * 100, 1500, kPacingInfo0));
- packets.push_back(
- CreateResult(i * 100 + 10, 2 * i * 100 + 10, 1500, kPacingInfo0));
- packets.push_back(
- CreateResult(i * 100 + 20, 2 * i * 100 + 20, 1500, kPacingInfo0));
- packets.push_back(
- CreateResult(i * 100 + 30, 2 * i * 100 + 30, 1500, kPacingInfo1));
- packets.push_back(
- CreateResult(i * 100 + 40, 2 * i * 100 + 40, 1500, kPacingInfo1));
-
- TransportPacketsFeedback feedback;
- for (PacketResult& packet : packets) {
- controller_->OnSentPacket(packet.sent_packet);
- feedback.sendless_arrival_times.push_back(packet.receive_time);
- }
-
- feedback.feedback_time = packets[0].receive_time;
- AdvanceTimeMilliseconds(kFeedbackTimeoutMs);
- SentPacket later_packet;
- later_packet.send_time = Timestamp::ms(kFeedbackTimeoutMs + i * 200 + 40);
- later_packet.size = DataSize::bytes(1500);
- later_packet.pacing_info = kPacingInfo1;
- controller_->OnSentPacket(later_packet);
-
- OnUpdate(controller_->OnTransportPacketsFeedback(feedback));
- }
- OnUpdate(controller_->OnProcessInterval(DefaultInterval()));
-
- EXPECT_EQ(kInitialBitrateKbps / 2, target_bitrate_->kbps());
-
- // Test with feedback that isn't late enough to time out.
- {
- std::vector<PacketResult> packets;
- packets.push_back(CreateResult(100, 200, 1500, kPacingInfo0));
- packets.push_back(CreateResult(110, 210, 1500, kPacingInfo0));
- packets.push_back(CreateResult(120, 220, 1500, kPacingInfo0));
- packets.push_back(CreateResult(130, 230, 1500, kPacingInfo1));
- packets.push_back(CreateResult(140, 240, 1500, kPacingInfo1));
-
- for (const PacketResult& packet : packets)
- controller_->OnSentPacket(packet.sent_packet);
-
- TransportPacketsFeedback feedback;
- feedback.feedback_time = packets[0].receive_time;
- feedback.packet_feedbacks = packets;
-
- AdvanceTimeMilliseconds(kFeedbackTimeoutMs - 1);
-
- SentPacket later_packet;
- later_packet.send_time = Timestamp::ms(kFeedbackTimeoutMs + 240);
- later_packet.size = DataSize::bytes(1500);
- later_packet.pacing_info = kPacingInfo1;
- controller_->OnSentPacket(later_packet);
-
- OnUpdate(controller_->OnTransportPacketsFeedback(feedback));
- }
-}
-
// Bandwidth estimation is updated when feedbacks are received.
// Feedbacks which show an increasing delay cause the estimation to be reduced.
TEST_F(GoogCcNetworkControllerTest, UpdatesDelayBasedEstimate) {
diff --git a/modules/congestion_controller/send_side_congestion_controller_unittest.cc b/modules/congestion_controller/send_side_congestion_controller_unittest.cc
index 5f58f21..0f37e0e 100644
--- a/modules/congestion_controller/send_side_congestion_controller_unittest.cc
+++ b/modules/congestion_controller/send_side_congestion_controller_unittest.cc
@@ -344,97 +344,6 @@
20 * kInitialBitrateBps);
}
-// Estimated bitrate reduced when the feedbacks arrive with such a long delay,
-// that the send-time-history no longer holds the feedbacked packets.
-TEST_F(LegacySendSideCongestionControllerTest, LongFeedbackDelays) {
- TargetBitrateTrackingSetup();
-
- const int64_t kFeedbackTimeoutMs = 60001;
- const int kMaxConsecutiveFailedLookups = 5;
- for (int i = 0; i < kMaxConsecutiveFailedLookups; ++i) {
- std::vector<PacketFeedback> packets;
- packets.push_back(
- PacketFeedback(i * 100, 2 * i * 100, 0, 1500, kPacingInfo0));
- packets.push_back(
- PacketFeedback(i * 100 + 10, 2 * i * 100 + 10, 1, 1500, kPacingInfo0));
- packets.push_back(
- PacketFeedback(i * 100 + 20, 2 * i * 100 + 20, 2, 1500, kPacingInfo0));
- packets.push_back(
- PacketFeedback(i * 100 + 30, 2 * i * 100 + 30, 3, 1500, kPacingInfo1));
- packets.push_back(
- PacketFeedback(i * 100 + 40, 2 * i * 100 + 40, 4, 1500, kPacingInfo1));
-
- for (const PacketFeedback& packet : packets)
- OnSentPacket(packet);
-
- rtcp::TransportFeedback feedback;
- feedback.SetBase(packets[0].sequence_number,
- packets[0].arrival_time_ms * 1000);
-
- for (const PacketFeedback& packet : packets) {
- EXPECT_TRUE(feedback.AddReceivedPacket(packet.sequence_number,
- packet.arrival_time_ms * 1000));
- }
-
- feedback.Build();
-
- clock_.AdvanceTimeMilliseconds(kFeedbackTimeoutMs);
- PacketFeedback later_packet(kFeedbackTimeoutMs + i * 100 + 40,
- kFeedbackTimeoutMs + i * 200 + 40, 5, 1500,
- kPacingInfo1);
- OnSentPacket(later_packet);
-
- controller_->OnTransportFeedback(feedback);
-
- // Check that packets have timed out.
- for (PacketFeedback& packet : packets) {
- packet.send_time_ms = PacketFeedback::kNoSendTime;
- packet.payload_size = 0;
- packet.pacing_info = PacedPacketInfo();
- }
- ComparePacketFeedbackVectors(packets,
- controller_->GetTransportFeedbackVector());
- }
-
- controller_->Process();
-
- EXPECT_EQ(kInitialBitrateBps / 2, target_bitrate_bps_);
-
- // Test with feedback that isn't late enough to time out.
- {
- std::vector<PacketFeedback> packets;
- packets.push_back(PacketFeedback(100, 200, 0, 1500, kPacingInfo0));
- packets.push_back(PacketFeedback(110, 210, 1, 1500, kPacingInfo0));
- packets.push_back(PacketFeedback(120, 220, 2, 1500, kPacingInfo0));
- packets.push_back(PacketFeedback(130, 230, 3, 1500, kPacingInfo1));
- packets.push_back(PacketFeedback(140, 240, 4, 1500, kPacingInfo1));
-
- for (const PacketFeedback& packet : packets)
- OnSentPacket(packet);
-
- rtcp::TransportFeedback feedback;
- feedback.SetBase(packets[0].sequence_number,
- packets[0].arrival_time_ms * 1000);
-
- for (const PacketFeedback& packet : packets) {
- EXPECT_TRUE(feedback.AddReceivedPacket(packet.sequence_number,
- packet.arrival_time_ms * 1000));
- }
-
- feedback.Build();
-
- clock_.AdvanceTimeMilliseconds(kFeedbackTimeoutMs - 1);
- PacketFeedback later_packet(kFeedbackTimeoutMs + 140,
- kFeedbackTimeoutMs + 240, 5, 1500,
- kPacingInfo1);
- OnSentPacket(later_packet);
-
- controller_->OnTransportFeedback(feedback);
- ComparePacketFeedbackVectors(packets,
- controller_->GetTransportFeedbackVector());
- }
-}
-
// Bandwidth estimation is updated when feedbacks are received.
// Feedbacks which show an increasing delay cause the estimation to be reduced.
TEST_F(LegacySendSideCongestionControllerTest, UpdatesDelayBasedEstimate) {
