From 885cf601068bd3fcf9cd1a6c2d0331e7f9d8aaad Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Fri, 23 Nov 2018 14:02:35 +0100 Subject: [PATCH] Moves ProbeBitrateEstimator from DelayBasedBwe. This prepares for providing an additional implementation of delay based rate control. By moving the probe controller, less code will have to be added in the upcoming CL. Bug: webrtc:9718 Change-Id: I64eb2c8f5f7950b6e9d209f110dc0a757c710b4b Reviewed-on: https://webrtc-review.googlesource.com/c/111860 Commit-Queue: Sebastian Jansson Reviewed-by: Christoffer Rodbro Cr-Commit-Position: refs/heads/master@{#25770} --- .../congestion_controller/goog_cc/delay_based_bwe.cc | 12 ++++-------- .../congestion_controller/goog_cc/delay_based_bwe.h | 3 ++- .../goog_cc/delay_based_bwe_unittest.cc | 4 ++-- .../goog_cc/delay_based_bwe_unittest_helper.cc | 12 ++++++++++++ .../goog_cc/delay_based_bwe_unittest_helper.h | 1 + .../goog_cc/goog_cc_network_control.cc | 12 +++++++++++- .../goog_cc/goog_cc_network_control.h | 1 + .../include/send_side_congestion_controller.h | 2 ++ .../send_side_congestion_controller.cc | 9 +++++++++ .../test/estimators/send_side.cc | 7 +++++++ .../test/estimators/send_side.h | 1 + 11 files changed, 52 insertions(+), 12 deletions(-) diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe.cc b/modules/congestion_controller/goog_cc/delay_based_bwe.cc index 7a1ee117bd..4e127dce0f 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe.cc +++ b/modules/congestion_controller/goog_cc/delay_based_bwe.cc @@ -88,7 +88,6 @@ DelayBasedBwe::DelayBasedBwe(RtcEventLog* event_log) delay_detector_(), last_seen_packet_(Timestamp::MinusInfinity()), uma_recorded_(false), - probe_bitrate_estimator_(event_log), trendline_window_size_( webrtc::field_trial::IsEnabled(kBweWindowSizeInPacketsExperiment) ? ReadTrendlineFilterWindowSize() @@ -111,6 +110,7 @@ DelayBasedBwe::~DelayBasedBwe() {} DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector( const std::vector& packet_feedback_vector, absl::optional acked_bitrate, + absl::optional probe_bitrate, Timestamp at_time) { RTC_DCHECK(std::is_sorted(packet_feedback_vector.begin(), packet_feedback_vector.end(), @@ -155,7 +155,8 @@ DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector( } else { consecutive_delayed_feedbacks_ = 0; - return MaybeUpdateEstimate(acked_bitrate, recovered_from_overuse, at_time); + return MaybeUpdateEstimate(acked_bitrate, probe_bitrate, + recovered_from_overuse, at_time); } return Result(); } @@ -221,20 +222,15 @@ void DelayBasedBwe::IncomingPacketFeedback( delay_detector_->Update(t_delta, ts_delta_ms, packet_feedback.arrival_time_ms); } - if (packet_feedback.pacing_info.probe_cluster_id != - PacedPacketInfo::kNotAProbe) { - probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(packet_feedback); - } } DelayBasedBwe::Result DelayBasedBwe::MaybeUpdateEstimate( absl::optional acked_bitrate, + absl::optional probe_bitrate, bool recovered_from_overuse, Timestamp at_time) { Result result; - absl::optional probe_bitrate = - probe_bitrate_estimator_.FetchAndResetLastEstimatedBitrate(); // Currently overusing the bandwidth. if (delay_detector_->State() == BandwidthUsage::kBwOverusing) { if (acked_bitrate && diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe.h b/modules/congestion_controller/goog_cc/delay_based_bwe.h index f7c1964573..616c5b0896 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe.h +++ b/modules/congestion_controller/goog_cc/delay_based_bwe.h @@ -47,6 +47,7 @@ class DelayBasedBwe { Result IncomingPacketFeedbackVector( const std::vector& packet_feedback_vector, absl::optional acked_bitrate, + absl::optional probe_bitrate, Timestamp at_time); Result OnDelayedFeedback(Timestamp receive_time); void OnRttUpdate(TimeDelta avg_rtt); @@ -61,6 +62,7 @@ class DelayBasedBwe { Timestamp at_time); Result OnLongFeedbackDelay(Timestamp arrival_time); Result MaybeUpdateEstimate(absl::optional acked_bitrate, + absl::optional probe_bitrate, bool request_probe, Timestamp at_time); // Updates the current remote rate estimate and returns true if a valid @@ -76,7 +78,6 @@ class DelayBasedBwe { Timestamp last_seen_packet_; bool uma_recorded_; AimdRateControl rate_control_; - ProbeBitrateEstimator probe_bitrate_estimator_; size_t trendline_window_size_; double trendline_smoothing_coeff_; double trendline_threshold_gain_; diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest.cc b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest.cc index 0b2f799864..7de11b1684 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest.cc +++ b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest.cc @@ -31,7 +31,7 @@ constexpr Timestamp kDummyTimestamp = Timestamp::Seconds<1000>(); TEST_F(DelayBasedBweTest, NoCrashEmptyFeedback) { std::vector packet_feedback_vector; bitrate_estimator_->IncomingPacketFeedbackVector( - packet_feedback_vector, absl::nullopt, kDummyTimestamp); + packet_feedback_vector, absl::nullopt, absl::nullopt, kDummyTimestamp); } TEST_F(DelayBasedBweTest, NoCrashOnlyLostFeedback) { @@ -43,7 +43,7 @@ TEST_F(DelayBasedBweTest, NoCrashOnlyLostFeedback) { PacketFeedback::kNoSendTime, 1, 1500, PacedPacketInfo())); bitrate_estimator_->IncomingPacketFeedbackVector( - packet_feedback_vector, absl::nullopt, kDummyTimestamp); + packet_feedback_vector, absl::nullopt, absl::nullopt, kDummyTimestamp); } TEST_F(DelayBasedBweTest, ProbeDetection) { diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.cc b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.cc index 583ce25e85..47288e5fbc 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.cc +++ b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.cc @@ -154,6 +154,7 @@ DelayBasedBweTest::DelayBasedBweTest() clock_(100000000), acknowledged_bitrate_estimator_( absl::make_unique()), + probe_bitrate_estimator_(new ProbeBitrateEstimator(nullptr)), bitrate_estimator_(new DelayBasedBwe(nullptr)), stream_generator_(new test::StreamGenerator(1e6, // Capacity. clock_.TimeInMicroseconds())), @@ -166,6 +167,7 @@ DelayBasedBweTest::DelayBasedBweTest(const std::string& field_trial_string) clock_(100000000), acknowledged_bitrate_estimator_( absl::make_unique()), + probe_bitrate_estimator_(new ProbeBitrateEstimator(nullptr)), bitrate_estimator_(new DelayBasedBwe(nullptr)), stream_generator_(new test::StreamGenerator(1e6, // Capacity. clock_.TimeInMicroseconds())), @@ -198,10 +200,15 @@ void DelayBasedBweTest::IncomingFeedback(int64_t arrival_time_ms, sequence_number, payload_size, pacing_info); std::vector packets; packets.push_back(packet); + if (packet.send_time_ms != PacketFeedback::kNoSendTime && + packet.pacing_info.probe_cluster_id != PacedPacketInfo::kNotAProbe) + probe_bitrate_estimator_->HandleProbeAndEstimateBitrate(packet); + acknowledged_bitrate_estimator_->IncomingPacketFeedbackVector(packets); DelayBasedBwe::Result result = bitrate_estimator_->IncomingPacketFeedbackVector( packets, acknowledged_bitrate_estimator_->bitrate(), + probe_bitrate_estimator_->FetchAndResetLastEstimatedBitrate(), Timestamp::ms(clock_.TimeInMilliseconds())); const uint32_t kDummySsrc = 0; if (result.updated) { @@ -232,12 +239,17 @@ bool DelayBasedBweTest::GenerateAndProcessFrame(uint32_t ssrc, for (auto& packet : packets) { RTC_CHECK_GE(packet.arrival_time_ms + arrival_time_offset_ms_, 0); packet.arrival_time_ms += arrival_time_offset_ms_; + + if (packet.send_time_ms != PacketFeedback::kNoSendTime && + packet.pacing_info.probe_cluster_id != PacedPacketInfo::kNotAProbe) + probe_bitrate_estimator_->HandleProbeAndEstimateBitrate(packet); } acknowledged_bitrate_estimator_->IncomingPacketFeedbackVector(packets); DelayBasedBwe::Result result = bitrate_estimator_->IncomingPacketFeedbackVector( packets, acknowledged_bitrate_estimator_->bitrate(), + probe_bitrate_estimator_->FetchAndResetLastEstimatedBitrate(), Timestamp::ms(clock_.TimeInMilliseconds())); const uint32_t kDummySsrc = 0; if (result.updated) { diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.h b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.h index 7ba18c3675..444e0cd67d 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.h +++ b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.h @@ -172,6 +172,7 @@ class DelayBasedBweTest : public ::testing::Test { SimulatedClock clock_; // Time at the receiver. test::TestBitrateObserver bitrate_observer_; std::unique_ptr acknowledged_bitrate_estimator_; + const std::unique_ptr probe_bitrate_estimator_; std::unique_ptr bitrate_estimator_; std::unique_ptr stream_generator_; int64_t arrival_time_offset_ms_; 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 945a9c2eb4..93fb9e1b44 100644 --- a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc +++ b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc @@ -140,6 +140,7 @@ GoogCcNetworkController::GoogCcNetworkController(RtcEventLog* event_log, bandwidth_estimation_( absl::make_unique(event_log_)), alr_detector_(absl::make_unique()), + probe_bitrate_estimator_(new ProbeBitrateEstimator(event_log)), delay_based_bwe_(new DelayBasedBwe(event_log_)), acknowledged_bitrate_estimator_( absl::make_unique()), @@ -208,6 +209,7 @@ NetworkControlUpdate GoogCcNetworkController::OnNetworkRouteChange( } acknowledged_bitrate_estimator_.reset(new AcknowledgedBitrateEstimator()); + probe_bitrate_estimator_.reset(new ProbeBitrateEstimator(event_log_)); delay_based_bwe_.reset(new DelayBasedBwe(event_log_)); if (msg.constraints.starting_rate) delay_based_bwe_->SetStartBitrate(*msg.constraints.starting_rate); @@ -484,10 +486,18 @@ NetworkControlUpdate GoogCcNetworkController::OnTransportPacketsFeedback( auto acknowledged_bitrate = acknowledged_bitrate_estimator_->bitrate(); bandwidth_estimation_->IncomingPacketFeedbackVector(report, acknowledged_bitrate); + for (const auto& feedback : received_feedback_vector) { + if (feedback.pacing_info.probe_cluster_id != PacedPacketInfo::kNotAProbe) { + probe_bitrate_estimator_->HandleProbeAndEstimateBitrate(feedback); + } + } + absl::optional probe_bitrate = + probe_bitrate_estimator_->FetchAndResetLastEstimatedBitrate(); DelayBasedBwe::Result result; result = delay_based_bwe_->IncomingPacketFeedbackVector( - received_feedback_vector, acknowledged_bitrate, report.feedback_time); + received_feedback_vector, acknowledged_bitrate, probe_bitrate, + report.feedback_time); NetworkControlUpdate update; if (result.updated) { diff --git a/modules/congestion_controller/goog_cc/goog_cc_network_control.h b/modules/congestion_controller/goog_cc/goog_cc_network_control.h index 60a75ad8e4..382f027d5f 100644 --- a/modules/congestion_controller/goog_cc/goog_cc_network_control.h +++ b/modules/congestion_controller/goog_cc/goog_cc_network_control.h @@ -73,6 +73,7 @@ class GoogCcNetworkController : public NetworkControllerInterface { std::unique_ptr bandwidth_estimation_; std::unique_ptr alr_detector_; + std::unique_ptr probe_bitrate_estimator_; std::unique_ptr delay_based_bwe_; std::unique_ptr acknowledged_bitrate_estimator_; diff --git a/modules/congestion_controller/include/send_side_congestion_controller.h b/modules/congestion_controller/include/send_side_congestion_controller.h index 88530a22c1..746e590a59 100644 --- a/modules/congestion_controller/include/send_side_congestion_controller.h +++ b/modules/congestion_controller/include/send_side_congestion_controller.h @@ -152,6 +152,8 @@ class SendSideCongestionController bool pacer_paused_; rtc::CriticalSection bwe_lock_; int min_bitrate_bps_ RTC_GUARDED_BY(bwe_lock_); + std::unique_ptr probe_bitrate_estimator_ + RTC_GUARDED_BY(bwe_lock_); std::unique_ptr delay_based_bwe_ RTC_GUARDED_BY(bwe_lock_); bool in_cwnd_experiment_; int64_t accepted_queue_ms_; diff --git a/modules/congestion_controller/send_side_congestion_controller.cc b/modules/congestion_controller/send_side_congestion_controller.cc index 36e11dcbc1..147f103e78 100644 --- a/modules/congestion_controller/send_side_congestion_controller.cc +++ b/modules/congestion_controller/send_side_congestion_controller.cc @@ -143,6 +143,7 @@ SendSideCongestionController::SendSideCongestionController( pause_pacer_(false), pacer_paused_(false), min_bitrate_bps_(congestion_controller::GetMinBitrateBps()), + probe_bitrate_estimator_(new ProbeBitrateEstimator(event_log_)), delay_based_bwe_(new DelayBasedBwe(event_log_)), in_cwnd_experiment_(CwndExperimentEnabled()), accepted_queue_ms_(kDefaultAcceptedQueueMs), @@ -258,6 +259,7 @@ void SendSideCongestionController::OnNetworkRouteChanged( rtc::CritScope cs(&bwe_lock_); transport_overhead_bytes_per_packet_ = network_route.packet_overhead; min_bitrate_bps_ = min_bitrate_bps; + probe_bitrate_estimator_.reset(new ProbeBitrateEstimator(event_log_)); delay_based_bwe_.reset(new DelayBasedBwe(event_log_)); acknowledged_bitrate_estimator_.reset(new AcknowledgedBitrateEstimator()); if (bitrate_bps > 0) { @@ -416,8 +418,15 @@ void SendSideCongestionController::OnTransportFeedback( DelayBasedBwe::Result result; { rtc::CritScope cs(&bwe_lock_); + for (const auto& packet : feedback_vector) { + if (packet.send_time_ms != PacketFeedback::kNoSendTime && + packet.pacing_info.probe_cluster_id != PacedPacketInfo::kNotAProbe) { + probe_bitrate_estimator_->HandleProbeAndEstimateBitrate(packet); + } + } result = delay_based_bwe_->IncomingPacketFeedbackVector( feedback_vector, acknowledged_bitrate_estimator_->bitrate(), + probe_bitrate_estimator_->FetchAndResetLastEstimatedBitrate(), Timestamp::ms(clock_->TimeInMilliseconds())); } if (result.updated) { diff --git a/modules/remote_bitrate_estimator/test/estimators/send_side.cc b/modules/remote_bitrate_estimator/test/estimators/send_side.cc index 2907563947..289ff70ee2 100644 --- a/modules/remote_bitrate_estimator/test/estimators/send_side.cc +++ b/modules/remote_bitrate_estimator/test/estimators/send_side.cc @@ -32,6 +32,7 @@ SendSideBweSender::SendSideBweSender(int kbps, &event_log_)), acknowledged_bitrate_estimator_( absl::make_unique()), + probe_bitrate_estimator_(new ProbeBitrateEstimator(nullptr)), bwe_(new DelayBasedBwe(nullptr)), feedback_observer_(bitrate_controller_.get()), clock_(clock), @@ -79,8 +80,14 @@ void SendSideBweSender::GiveFeedback(const FeedbackPacket& feedback) { PacketFeedbackComparator()); acknowledged_bitrate_estimator_->IncomingPacketFeedbackVector( packet_feedback_vector); + for (PacketFeedback& packet : packet_feedback_vector) { + if (packet.send_time_ms != PacketFeedback::kNoSendTime && + packet.pacing_info.probe_cluster_id != PacedPacketInfo::kNotAProbe) + probe_bitrate_estimator_->HandleProbeAndEstimateBitrate(packet); + } DelayBasedBwe::Result result = bwe_->IncomingPacketFeedbackVector( packet_feedback_vector, acknowledged_bitrate_estimator_->bitrate(), + probe_bitrate_estimator_->FetchAndResetLastEstimatedBitrate(), Timestamp::ms(clock_->TimeInMilliseconds())); if (result.updated) bitrate_controller_->OnDelayBasedBweResult(result); diff --git a/modules/remote_bitrate_estimator/test/estimators/send_side.h b/modules/remote_bitrate_estimator/test/estimators/send_side.h index 6e939c1f03..5b45e66fbc 100644 --- a/modules/remote_bitrate_estimator/test/estimators/send_side.h +++ b/modules/remote_bitrate_estimator/test/estimators/send_side.h @@ -39,6 +39,7 @@ class SendSideBweSender : public BweSender, public RemoteBitrateObserver { protected: std::unique_ptr bitrate_controller_; std::unique_ptr acknowledged_bitrate_estimator_; + std::unique_ptr probe_bitrate_estimator_; std::unique_ptr bwe_; RtcpBandwidthObserver* feedback_observer_;