From 87f3a14fdbc02a8d0b9e73b01f08402cfa7c87b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Terelius?= Date: Fri, 1 Dec 2017 19:03:43 +0000 Subject: [PATCH] Revert "Now calculates RTT in SendSideCongestionController." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 8c319e951a6c59212e23af858a4c51d28b4eedc1. Reason for revert: Increase in dropped frames and decreased send bandwidth in perf tests. Original change's description: > Now calculates RTT in SendSideCongestionController. > > Moved calculation of round trip time from transport feedback adapter to send side congestion > controller. This reduces the role of the transport specific transport feedback adapter and > gives more power to the congestion controller to decide how the feedback rtt should be > calculated and used. > > Bug: webrtc:8415 > Change-Id: I7878d9fb32c3f4ed11993a6f39e6d9c69fab190a > Reviewed-on: https://webrtc-review.googlesource.com/27980 > Reviewed-by: Stefan Holmer > Commit-Queue: Sebastian Jansson > Cr-Commit-Position: refs/heads/master@{#20973} TBR=terelius@webrtc.org,stefan@webrtc.org,srte@webrtc.org Change-Id: I993d00de7171a163a41b486d68b9255fd5c0f5da No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:8415 Reviewed-on: https://webrtc-review.googlesource.com/28300 Reviewed-by: Björn Terelius Commit-Queue: Björn Terelius Cr-Commit-Position: refs/heads/master@{#20984} --- .../include/send_side_congestion_controller.h | 5 --- .../send_side_congestion_controller.cc | 37 ++++--------------- .../transport_feedback_adapter.cc | 20 ++++++++++ .../transport_feedback_adapter.h | 4 ++ 4 files changed, 31 insertions(+), 35 deletions(-) diff --git a/modules/congestion_controller/include/send_side_congestion_controller.h b/modules/congestion_controller/include/send_side_congestion_controller.h index ac70d6066e..b33fad0d30 100644 --- a/modules/congestion_controller/include/send_side_congestion_controller.h +++ b/modules/congestion_controller/include/send_side_congestion_controller.h @@ -11,7 +11,6 @@ #ifndef MODULES_CONGESTION_CONTROLLER_INCLUDE_SEND_SIDE_CONGESTION_CONTROLLER_H_ #define MODULES_CONGESTION_CONTROLLER_INCLUDE_SEND_SIDE_CONGESTION_CONTROLLER_H_ -#include #include #include @@ -137,11 +136,7 @@ class SendSideCongestionController : public CallStatsObserver, const std::unique_ptr probe_controller_; const std::unique_ptr retransmission_rate_limiter_; TransportFeedbackAdapter transport_feedback_adapter_; - rtc::CriticalSection network_state_lock_; - std::deque feedback_rtts_ RTC_GUARDED_BY(network_state_lock_); - rtc::Optional min_feedback_rtt_ms_ - RTC_GUARDED_BY(network_state_lock_); uint32_t last_reported_bitrate_bps_ RTC_GUARDED_BY(network_state_lock_); uint8_t last_reported_fraction_loss_ RTC_GUARDED_BY(network_state_lock_); int64_t last_reported_rtt_ RTC_GUARDED_BY(network_state_lock_); diff --git a/modules/congestion_controller/send_side_congestion_controller.cc b/modules/congestion_controller/send_side_congestion_controller.cc index 5bee61f01d..ccca692712 100644 --- a/modules/congestion_controller/send_side_congestion_controller.cc +++ b/modules/congestion_controller/send_side_congestion_controller.cc @@ -315,36 +315,11 @@ void SendSideCongestionController::AddPacket( void SendSideCongestionController::OnTransportFeedback( const rtcp::TransportFeedback& feedback) { RTC_DCHECK_RUNS_SERIALIZED(&worker_race_); - int64_t feedback_time_ms = clock_->TimeInMilliseconds(); - transport_feedback_adapter_.OnTransportFeedback(feedback); - std::vector feedback_vector = - transport_feedback_adapter_.GetTransportFeedbackVector(); + std::vector feedback_vector = ReceivedPacketFeedbackVector( + transport_feedback_adapter_.GetTransportFeedbackVector()); SortPacketFeedbackVector(&feedback_vector); - int64_t feedback_rtt = -1; - for (const auto& packet_feedback : feedback_vector) { - if (packet_feedback.send_time_ms != PacketFeedback::kNoSendTime && - packet_feedback.arrival_time_ms != PacketFeedback::kNotReceived) { - int64_t rtt = feedback_time_ms - packet_feedback.send_time_ms; - // max() is used to account for feedback being delayed by the - // receiver. - feedback_rtt = std::max(rtt, feedback_rtt); - } - } - if (feedback_rtt > -1) { - rtc::CritScope cs(&network_state_lock_); - feedback_rtts_.push_back(feedback_rtt); - const size_t kFeedbackRttWindow = 32; - if (feedback_rtts_.size() > kFeedbackRttWindow) - feedback_rtts_.pop_front(); - min_feedback_rtt_ms_.emplace( - *std::min_element(feedback_rtts_.begin(), feedback_rtts_.end())); - } - - std::vector received_feedback_vector = - ReceivedPacketFeedbackVector(feedback_vector); - bool currently_in_alr = pacer_->GetApplicationLimitedRegionStartTime().has_value(); if (was_in_alr_ && !currently_in_alr) { @@ -377,20 +352,22 @@ void SendSideCongestionController::LimitOutstandingBytes( size_t num_outstanding_bytes) { RTC_DCHECK(in_cwnd_experiment_); rtc::CritScope lock(&network_state_lock_); + rtc::Optional min_rtt_ms = + transport_feedback_adapter_.GetMinFeedbackLoopRtt(); // No valid RTT. Could be because send-side BWE isn't used, in which case // we don't try to limit the outstanding packets. - if (!min_feedback_rtt_ms_) + if (!min_rtt_ms) return; const size_t kMinCwndBytes = 2 * 1500; size_t max_outstanding_bytes = - std::max((*min_feedback_rtt_ms_ + accepted_queue_ms_) * + std::max((*min_rtt_ms + accepted_queue_ms_) * last_reported_bitrate_bps_ / 1000 / 8, kMinCwndBytes); RTC_LOG(LS_INFO) << clock_->TimeInMilliseconds() << " Outstanding bytes: " << num_outstanding_bytes << " pacer queue: " << pacer_->QueueInMs() << " max outstanding: " << max_outstanding_bytes; - RTC_LOG(LS_INFO) << "Feedback rtt: " << *min_feedback_rtt_ms_ + RTC_LOG(LS_INFO) << "Feedback rtt: " << *min_rtt_ms << " Bitrate: " << last_reported_bitrate_bps_; pause_pacer_ = num_outstanding_bytes > max_outstanding_bytes; } diff --git a/modules/congestion_controller/transport_feedback_adapter.cc b/modules/congestion_controller/transport_feedback_adapter.cc index 89ddce0032..0989875df9 100644 --- a/modules/congestion_controller/transport_feedback_adapter.cc +++ b/modules/congestion_controller/transport_feedback_adapter.cc @@ -131,6 +131,7 @@ std::vector TransportFeedbackAdapter::GetPacketFeedbackVector( return packet_feedback_vector; } packet_feedback_vector.reserve(feedback.GetPacketStatusCount()); + int64_t feedback_rtt = -1; { rtc::CritScope cs(&lock_); size_t failed_lookups = 0; @@ -160,6 +161,12 @@ std::vector TransportFeedbackAdapter::GetPacketFeedbackVector( ++failed_lookups; if (packet_feedback.local_net_id == local_net_id_ && packet_feedback.remote_net_id == remote_net_id_) { + if (packet_feedback.send_time_ms >= 0) { + int64_t rtt = now_ms - packet_feedback.send_time_ms; + // max() is used to account for feedback being delayed by the + // receiver. + feedback_rtt = std::max(rtt, feedback_rtt); + } packet_feedback_vector.push_back(packet_feedback); } @@ -171,6 +178,14 @@ std::vector TransportFeedbackAdapter::GetPacketFeedbackVector( << " packet" << (failed_lookups > 1 ? "s" : "") << ". Send time history too small?"; } + if (feedback_rtt > -1) { + feedback_rtts_.push_back(feedback_rtt); + const size_t kFeedbackRttWindow = 32; + if (feedback_rtts_.size() > kFeedbackRttWindow) + feedback_rtts_.pop_front(); + min_feedback_rtt_.emplace( + *std::min_element(feedback_rtts_.begin(), feedback_rtts_.end())); + } } return packet_feedback_vector; } @@ -191,6 +206,11 @@ TransportFeedbackAdapter::GetTransportFeedbackVector() const { return last_packet_feedback_vector_; } +rtc::Optional TransportFeedbackAdapter::GetMinFeedbackLoopRtt() const { + rtc::CritScope cs(&lock_); + return min_feedback_rtt_; +} + size_t TransportFeedbackAdapter::GetOutstandingBytes() const { rtc::CritScope cs(&lock_); return send_time_history_.GetOutstandingBytes(local_net_id_, remote_net_id_); diff --git a/modules/congestion_controller/transport_feedback_adapter.h b/modules/congestion_controller/transport_feedback_adapter.h index bb3a1f79d0..008bab87f1 100644 --- a/modules/congestion_controller/transport_feedback_adapter.h +++ b/modules/congestion_controller/transport_feedback_adapter.h @@ -11,6 +11,7 @@ #ifndef MODULES_CONGESTION_CONTROLLER_TRANSPORT_FEEDBACK_ADAPTER_H_ #define MODULES_CONGESTION_CONTROLLER_TRANSPORT_FEEDBACK_ADAPTER_H_ +#include #include #include "modules/remote_bitrate_estimator/include/send_time_history.h" @@ -46,6 +47,7 @@ class TransportFeedbackAdapter { // to the CongestionController interface. void OnTransportFeedback(const rtcp::TransportFeedback& feedback); std::vector GetTransportFeedbackVector() const; + rtc::Optional GetMinFeedbackLoopRtt() const; void SetTransportOverhead(int transport_overhead_bytes_per_packet); @@ -67,6 +69,8 @@ class TransportFeedbackAdapter { std::vector last_packet_feedback_vector_; uint16_t local_net_id_ RTC_GUARDED_BY(&lock_); uint16_t remote_net_id_ RTC_GUARDED_BY(&lock_); + std::deque feedback_rtts_ RTC_GUARDED_BY(&lock_); + rtc::Optional min_feedback_rtt_ RTC_GUARDED_BY(&lock_); rtc::CriticalSection observers_lock_; std::vector observers_