From 24779d82292e57fdec11bbb540ce4137b07416ee Mon Sep 17 00:00:00 2001 From: Bjorn Terelius Date: Thu, 13 Dec 2018 13:29:46 +0100 Subject: [PATCH] Missing packet send time should not cause BWE backoff. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Sebastian Jansson Cr-Commit-Position: refs/heads/master@{#26016} --- .../goog_cc/delay_based_bwe.cc | 44 +-------- .../goog_cc/delay_based_bwe.h | 3 - .../goog_cc/goog_cc_network_control.cc | 12 +-- .../goog_cc_network_control_unittest.cc | 68 -------------- ...end_side_congestion_controller_unittest.cc | 91 ------------------- 5 files changed, 8 insertions(+), 210 deletions(-) diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe.cc b/modules/congestion_controller/goog_cc/delay_based_bwe.cc index 4894c9863b..a76adb01c6 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 size_t kDefaultTrendlineWindowSize = 20; constexpr double kDefaultTrendlineSmoothingCoeff = 0.9; constexpr double kDefaultTrendlineThresholdGain = 4.0; -constexpr int kMaxConsecutiveFailedLookups = 5; - const char kBweWindowSizeInPacketsExperiment[] = "WebRTC-BweWindowSizeInPackets"; @@ -94,7 +92,6 @@ DelayBasedBwe::DelayBasedBwe(RtcEventLog* event_log) : 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 @@ DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector( } 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 616c5b0896..ead60f3012 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 @@ class DelayBasedBwe { absl::optional acked_bitrate, absl::optional probe_bitrate, Timestamp at_time); - Result OnDelayedFeedback(Timestamp receive_time); void OnRttUpdate(TimeDelta avg_rtt); bool LatestEstimate(std::vector* ssrcs, DataRate* bitrate) const; void SetStartBitrate(DataRate start_bitrate); @@ -60,7 +59,6 @@ class DelayBasedBwe { friend class GoogCcStatePrinter; void IncomingPacketFeedback(const PacketFeedback& packet_feedback, Timestamp at_time); - Result OnLongFeedbackDelay(Timestamp arrival_time); Result MaybeUpdateEstimate(absl::optional acked_bitrate, absl::optional probe_bitrate, bool request_probe, @@ -81,7 +79,6 @@ class DelayBasedBwe { 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 6de143d1df..aa0b78969f 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::OnTransportLossReport( 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 62be521425..340a093979 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 @@ TEST_F(GoogCcNetworkControllerTest, ProbeOnRouteChange) { 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 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 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 5f58f21cdf..0f37e0eb3c 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 @@ TEST_F(LegacySendSideCongestionControllerTest, ProbeOnRouteChange) { 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 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 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) {