From c610e26be55e312d2e1ecd6ba3fb649d17afa181 Mon Sep 17 00:00:00 2001 From: Christoffer Rodbro Date: Tue, 8 Jan 2019 10:49:19 +0100 Subject: [PATCH] Include pacing buffer size in congestion window. Bug: webrtc:10171 Change-Id: I9e21880a8b6f325415b62397081c301ee904f2ea Reviewed-on: https://webrtc-review.googlesource.com/c/116068 Reviewed-by: Niels Moller Reviewed-by: Sebastian Jansson Commit-Queue: Christoffer Rodbro Cr-Commit-Position: refs/heads/master@{#26175} --- api/transport/network_types.cc | 4 +++ api/transport/network_types.h | 4 +++ call/rtp_transport_controller_send.cc | 4 +++ call/rtp_transport_controller_send.h | 1 + .../congestion_window_pushback_controller.cc | 23 ++++++++++--- .../congestion_window_pushback_controller.h | 7 ++-- .../goog_cc/goog_cc_network_control.cc | 4 +++ .../goog_cc_network_control_slowtest.cc | 32 +++++++++++++++++++ modules/pacing/paced_sender.cc | 5 +++ modules/pacing/paced_sender.h | 2 ++ 10 files changed, 80 insertions(+), 6 deletions(-) diff --git a/api/transport/network_types.cc b/api/transport/network_types.cc index 80214de1a2..acc9c2b099 100644 --- a/api/transport/network_types.cc +++ b/api/transport/network_types.cc @@ -81,4 +81,8 @@ bool PacedPacketInfo::operator==(const PacedPacketInfo& rhs) const { probe_cluster_min_bytes == rhs.probe_cluster_min_bytes; } +ProcessInterval::ProcessInterval() = default; +ProcessInterval::ProcessInterval(const ProcessInterval&) = default; +ProcessInterval::~ProcessInterval() = default; + } // namespace webrtc diff --git a/api/transport/network_types.h b/api/transport/network_types.h index 43778caaac..e3880b9729 100644 --- a/api/transport/network_types.h +++ b/api/transport/network_types.h @@ -202,7 +202,11 @@ struct NetworkControlUpdate { // Process control struct ProcessInterval { + ProcessInterval(); + ProcessInterval(const ProcessInterval&); + ~ProcessInterval(); Timestamp at_time = Timestamp::PlusInfinity(); + absl::optional pacer_queue; }; } // namespace webrtc diff --git a/call/rtp_transport_controller_send.cc b/call/rtp_transport_controller_send.cc index f667ca366b..9f173b1ad1 100644 --- a/call/rtp_transport_controller_send.cc +++ b/call/rtp_transport_controller_send.cc @@ -130,6 +130,8 @@ RtpTransportControllerSend::RtpTransportControllerSend( !field_trial::IsEnabled("WebRTC-Bwe-NoFeedbackReset")), send_side_bwe_with_overhead_( webrtc::field_trial::IsEnabled("WebRTC-SendSideBwe-WithOverhead")), + add_pacing_to_cwin_( + field_trial::IsEnabled("WebRTC-AddPacingToCongestionWindowPushback")), transport_overhead_bytes_per_packet_(0), network_available_(false), packet_feedback_available_(false), @@ -600,6 +602,8 @@ void RtpTransportControllerSend::UpdateControllerWithTimeInterval() { RTC_DCHECK(controller_); ProcessInterval msg; msg.at_time = Timestamp::ms(clock_->TimeInMilliseconds()); + if (add_pacing_to_cwin_) + msg.pacer_queue = DataSize::bytes(pacer_.QueueSizeBytes()); PostUpdates(controller_->OnProcessInterval(msg)); } diff --git a/call/rtp_transport_controller_send.h b/call/rtp_transport_controller_send.h index d644e740f7..5b14a73617 100644 --- a/call/rtp_transport_controller_send.h +++ b/call/rtp_transport_controller_send.h @@ -174,6 +174,7 @@ class RtpTransportControllerSend final const bool reset_feedback_on_route_change_; const bool send_side_bwe_with_overhead_; + const bool add_pacing_to_cwin_; // Transport overhead is written by OnNetworkRouteChanged and read by // AddPacket. // TODO(srte): Remove atomic when feedback adapter runs on task queue. diff --git a/modules/congestion_controller/goog_cc/congestion_window_pushback_controller.cc b/modules/congestion_controller/goog_cc/congestion_window_pushback_controller.cc index b4f1872324..7c681be584 100644 --- a/modules/congestion_controller/goog_cc/congestion_window_pushback_controller.cc +++ b/modules/congestion_controller/goog_cc/congestion_window_pushback_controller.cc @@ -19,6 +19,8 @@ namespace webrtc { +namespace { + // When CongestionWindowPushback is enabled, the pacer is oblivious to // the congestion window. The relation between outstanding data and // the congestion window affects encoder allocations directly. @@ -41,7 +43,11 @@ bool ReadCongestionWindowPushbackExperimentParameter( return false; } -CongestionWindowPushbackController::CongestionWindowPushbackController() { +} // namespace + +CongestionWindowPushbackController::CongestionWindowPushbackController() + : add_pacing_(field_trial::IsEnabled( + "WebRTC-AddPacingToCongestionWindowPushback")) { if (!ReadCongestionWindowPushbackExperimentParameter( &min_pushback_target_bitrate_bps_)) { min_pushback_target_bitrate_bps_ = kDefaultMinPushbackTargetBitrateBps; @@ -50,12 +56,18 @@ CongestionWindowPushbackController::CongestionWindowPushbackController() { CongestionWindowPushbackController::CongestionWindowPushbackController( uint32_t min_pushback_target_bitrate_bps) - : min_pushback_target_bitrate_bps_(min_pushback_target_bitrate_bps) {} + : add_pacing_( + field_trial::IsEnabled("WebRTC-AddPacingToCongestionWindowPushback")), + min_pushback_target_bitrate_bps_(min_pushback_target_bitrate_bps) {} void CongestionWindowPushbackController::UpdateOutstandingData( - size_t outstanding_bytes) { + int64_t outstanding_bytes) { outstanding_bytes_ = outstanding_bytes; } +void CongestionWindowPushbackController::UpdatePacingQueue( + int64_t pacing_bytes) { + pacing_bytes_ = pacing_bytes; +} void CongestionWindowPushbackController::UpdateMaxOutstandingData( size_t max_outstanding_bytes) { @@ -74,8 +86,11 @@ uint32_t CongestionWindowPushbackController::UpdateTargetBitrate( uint32_t bitrate_bps) { if (!current_data_window_ || current_data_window_->IsZero()) return bitrate_bps; + int64_t total_bytes = outstanding_bytes_; + if (add_pacing_) + total_bytes += pacing_bytes_; double fill_ratio = - outstanding_bytes_ / static_cast(current_data_window_->bytes()); + total_bytes / static_cast(current_data_window_->bytes()); if (fill_ratio > 1.5) { encoding_rate_ratio_ *= 0.9; } else if (fill_ratio > 1) { diff --git a/modules/congestion_controller/goog_cc/congestion_window_pushback_controller.h b/modules/congestion_controller/goog_cc/congestion_window_pushback_controller.h index 14af745edf..c6b1023dd4 100644 --- a/modules/congestion_controller/goog_cc/congestion_window_pushback_controller.h +++ b/modules/congestion_controller/goog_cc/congestion_window_pushback_controller.h @@ -29,14 +29,17 @@ class CongestionWindowPushbackController { CongestionWindowPushbackController(); explicit CongestionWindowPushbackController( uint32_t min_pushback_target_bitrate_bps); - void UpdateOutstandingData(size_t outstanding_bytes); + void UpdateOutstandingData(int64_t outstanding_bytes); + void UpdatePacingQueue(int64_t pacing_bytes); void UpdateMaxOutstandingData(size_t max_outstanding_bytes); uint32_t UpdateTargetBitrate(uint32_t bitrate_bps); void SetDataWindow(DataSize data_window); private: absl::optional current_data_window_; - size_t outstanding_bytes_ = 0; + int64_t outstanding_bytes_ = 0; + int64_t pacing_bytes_ = 0; + const bool add_pacing_; uint32_t min_pushback_target_bitrate_bps_; double encoding_rate_ratio_ = 1.0; }; 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 bc7777f956..f0eef4b9af 100644 --- a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc +++ b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc @@ -275,6 +275,10 @@ NetworkControlUpdate GoogCcNetworkController::OnProcessInterval( bandwidth_estimation_->UpdateDelayBasedEstimate( msg.at_time, delay_based_controller_->target_rate()); } + if (congestion_window_pushback_controller_ && msg.pacer_queue) { + congestion_window_pushback_controller_->UpdatePacingQueue( + msg.pacer_queue->bytes()); + } bandwidth_estimation_->UpdateEstimate(msg.at_time); absl::optional start_time_ms = alr_detector_->GetApplicationLimitedRegionStartTime(); diff --git a/modules/congestion_controller/goog_cc/goog_cc_network_control_slowtest.cc b/modules/congestion_controller/goog_cc/goog_cc_network_control_slowtest.cc index e892664e81..0b2b187a35 100644 --- a/modules/congestion_controller/goog_cc/goog_cc_network_control_slowtest.cc +++ b/modules/congestion_controller/goog_cc/goog_cc_network_control_slowtest.cc @@ -114,5 +114,37 @@ TEST(GoogCcNetworkControllerTest, DetectsHighRateInSafeResetTrial) { EXPECT_GT(client->send_bandwidth().kbps(), kNewLinkCapacity.kbps() - 300); } +TEST(GoogCcNetworkControllerTest, + TargetRateReducedOnPacingBufferBuildupInTrial) { + // Configure strict pacing to ensure build-up. + ScopedFieldTrials trial( + "WebRTC-CongestionWindowPushback/Enabled/WebRTC-CwndExperiment/" + "Enabled-100/WebRTC-Video-Pacing/factor:1.0/" + "WebRTC-AddPacingToCongestionWindowPushback/Enabled/"); + + const DataRate kLinkCapacity = DataRate::kbps(1000); + const DataRate kStartRate = DataRate::kbps(1000); + + Scenario s("googcc_unit/pacing_buffer_buildup", true); + auto* net = s.CreateSimulationNode([&](NetworkNodeConfig* c) { + c->simulation.bandwidth = kLinkCapacity; + c->simulation.delay = TimeDelta::ms(50); + }); + // TODO(srte): replace with SimulatedTimeClient when it supports pacing. + auto* client = s.CreateClient("send", [&](CallClientConfig* c) { + c->transport.cc = TransportControllerConfig::CongestionController::kGoogCc; + c->transport.rates.start_rate = kStartRate; + }); + auto* route = s.CreateRoutes(client, {net}, + s.CreateClient("return", CallClientConfig()), + {s.CreateSimulationNode(NetworkNodeConfig())}); + s.CreateVideoStream(route->forward(), VideoStreamConfig()); + // Allow some time for the buffer to build up. + s.RunFor(TimeDelta::seconds(5)); + + // Without trial, pacer delay reaches ~250 ms. + EXPECT_LT(client->GetStats().pacer_delay_ms, 150); +} + } // namespace test } // namespace webrtc diff --git a/modules/pacing/paced_sender.cc b/modules/pacing/paced_sender.cc index f512938557..1a160d854c 100644 --- a/modules/pacing/paced_sender.cc +++ b/modules/pacing/paced_sender.cc @@ -232,6 +232,11 @@ size_t PacedSender::QueueSizePackets() const { return packets_.SizeInPackets(); } +int64_t PacedSender::QueueSizeBytes() const { + rtc::CritScope cs(&critsect_); + return packets_.SizeInBytes(); +} + int64_t PacedSender::FirstSentPacketTimeMs() const { rtc::CritScope cs(&critsect_); return first_sent_packet_ms_; diff --git a/modules/pacing/paced_sender.h b/modules/pacing/paced_sender.h index 7b8ecc1c6a..8ca32c377a 100644 --- a/modules/pacing/paced_sender.h +++ b/modules/pacing/paced_sender.h @@ -13,6 +13,7 @@ #include #include +#include #include #include "absl/types/optional.h" @@ -118,6 +119,7 @@ class PacedSender : public Pacer { virtual int64_t QueueInMs() const; virtual size_t QueueSizePackets() const; + virtual int64_t QueueSizeBytes() const; // Returns the time when the first packet was sent, or -1 if no packet is // sent.