From aa4f100225e86723e75497aaf2d510588dcb9851 Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Fri, 7 Dec 2018 18:47:26 +0100 Subject: [PATCH] Adds trial to fall back to probe rate if ack rate is missing. Bug: webrtc:9718 Change-Id: I7b6e1d3c051e67b97f6de1ec95e84631af9c5b0d Reviewed-on: https://webrtc-review.googlesource.com/c/113600 Commit-Queue: Sebastian Jansson Reviewed-by: Niels Moller Cr-Commit-Position: refs/heads/master@{#25953} --- .../goog_cc/goog_cc_network_control.cc | 11 +++++++---- .../goog_cc/goog_cc_network_control.h | 1 + .../goog_cc/goog_cc_network_control_slowtest.cc | 12 +++++------- .../goog_cc/probe_bitrate_estimator.cc | 5 +++++ .../goog_cc/probe_bitrate_estimator.h | 5 +++++ 5 files changed, 23 insertions(+), 11 deletions(-) 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 c47922cdaa..c99a6c97e5 100644 --- a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc +++ b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc @@ -136,6 +136,8 @@ GoogCcNetworkController::GoogCcNetworkController(RtcEventLog* event_log, safe_reset_acknowledged_rate_("ack"), use_stable_bandwidth_estimate_( field_trial::IsEnabled("WebRTC-Bwe-StableBandwidthEstimate")), + fall_back_to_probe_rate_( + field_trial::IsEnabled("WebRTC-Bwe-ProbeRateFallback")), probe_controller_(new ProbeController()), congestion_window_pushback_controller_( MaybeInitalizeCongestionWindowPushbackController()), @@ -486,9 +488,6 @@ NetworkControlUpdate GoogCcNetworkController::OnTransportPacketsFeedback( acknowledged_bitrate_estimator_->IncomingPacketFeedbackVector( received_feedback_vector); auto acknowledged_bitrate = acknowledged_bitrate_estimator_->bitrate(); - bandwidth_estimation_->SetAcknowledgedRate(acknowledged_bitrate, - report.feedback_time); - bandwidth_estimation_->IncomingPacketFeedbackVector(report); for (const auto& feedback : received_feedback_vector) { if (feedback.pacing_info.probe_cluster_id != PacedPacketInfo::kNotAProbe) { probe_bitrate_estimator_->HandleProbeAndEstimateBitrate(feedback); @@ -496,7 +495,11 @@ NetworkControlUpdate GoogCcNetworkController::OnTransportPacketsFeedback( } absl::optional probe_bitrate = probe_bitrate_estimator_->FetchAndResetLastEstimatedBitrate(); - + if (fall_back_to_probe_rate_ && !acknowledged_bitrate) + acknowledged_bitrate = probe_bitrate_estimator_->last_estimate(); + bandwidth_estimation_->SetAcknowledgedRate(acknowledged_bitrate, + report.feedback_time); + bandwidth_estimation_->IncomingPacketFeedbackVector(report); DelayBasedBwe::Result result; result = delay_based_bwe_->IncomingPacketFeedbackVector( received_feedback_vector, acknowledged_bitrate, probe_bitrate, 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 94aad4c2ee..057ed121fa 100644 --- a/modules/congestion_controller/goog_cc/goog_cc_network_control.h +++ b/modules/congestion_controller/goog_cc/goog_cc_network_control.h @@ -72,6 +72,7 @@ class GoogCcNetworkController : public NetworkControllerInterface { FieldTrialFlag safe_reset_on_route_change_; FieldTrialFlag safe_reset_acknowledged_rate_; const bool use_stable_bandwidth_estimate_; + const bool fall_back_to_probe_rate_; const std::unique_ptr probe_controller_; const std::unique_ptr 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 70a5a3c5c2..e892664e81 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 @@ -74,12 +74,10 @@ TEST(GoogCcNetworkControllerTest, CutsHighRateInSafeResetTrial) { EXPECT_NEAR(client->send_bandwidth().kbps(), kStartRate.kbps(), 30); } -// This test is flaky because probing on route change can trigger overuse -// without having any acknowledged rate, causing a 50% backoff from the probe -// rate. -// TODO(srte): Add a fix for the above problem and enable this test. -TEST(GoogCcNetworkControllerTest, DISABLED_DetectsHighRateInSafeResetTrial) { - ScopedFieldTrials trial("WebRTC-Bwe-SafeResetOnRouteChange/Enabled,ack/"); +TEST(GoogCcNetworkControllerTest, DetectsHighRateInSafeResetTrial) { + ScopedFieldTrials trial( + "WebRTC-Bwe-SafeResetOnRouteChange/Enabled,ack/" + "WebRTC-Bwe-ProbeRateFallback/Enabled/"); const DataRate kInitialLinkCapacity = DataRate::kbps(200); const DataRate kNewLinkCapacity = DataRate::kbps(800); const DataRate kStartRate = DataRate::kbps(300); @@ -113,7 +111,7 @@ TEST(GoogCcNetworkControllerTest, DISABLED_DetectsHighRateInSafeResetTrial) { EXPECT_NEAR(client->send_bandwidth().kbps(), kInitialLinkCapacity.kbps(), 50); // However, probing should have made us detect the higher rate. s.RunFor(TimeDelta::ms(2000)); - EXPECT_NEAR(client->send_bandwidth().kbps(), kNewLinkCapacity.kbps(), 200); + EXPECT_GT(client->send_bandwidth().kbps(), kNewLinkCapacity.kbps() - 300); } } // namespace test diff --git a/modules/congestion_controller/goog_cc/probe_bitrate_estimator.cc b/modules/congestion_controller/goog_cc/probe_bitrate_estimator.cc index 7ba2fb5a13..380e7d30c2 100644 --- a/modules/congestion_controller/goog_cc/probe_bitrate_estimator.cc +++ b/modules/congestion_controller/goog_cc/probe_bitrate_estimator.cc @@ -166,6 +166,7 @@ int ProbeBitrateEstimator::HandleProbeAndEstimateBitrate( event_log_->Log( absl::make_unique(cluster_id, res)); } + last_estimate_ = DataRate::bps(res); estimated_bitrate_bps_ = res; return *estimated_bitrate_bps_; } @@ -179,6 +180,10 @@ ProbeBitrateEstimator::FetchAndResetLastEstimatedBitrate() { return absl::nullopt; } +absl::optional ProbeBitrateEstimator::last_estimate() const { + return last_estimate_; +} + void ProbeBitrateEstimator::EraseOldClusters(int64_t timestamp_ms) { for (auto it = clusters_.begin(); it != clusters_.end();) { if (it->second.last_receive_ms < timestamp_ms) { diff --git a/modules/congestion_controller/goog_cc/probe_bitrate_estimator.h b/modules/congestion_controller/goog_cc/probe_bitrate_estimator.h index ae83ed380f..ce4eb99126 100644 --- a/modules/congestion_controller/goog_cc/probe_bitrate_estimator.h +++ b/modules/congestion_controller/goog_cc/probe_bitrate_estimator.h @@ -14,6 +14,8 @@ #include #include +#include "absl/types/optional.h" +#include "api/units/data_rate.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" namespace webrtc { @@ -30,6 +32,8 @@ class ProbeBitrateEstimator { absl::optional FetchAndResetLastEstimatedBitrate(); + absl::optional last_estimate() const; + private: struct AggregatedCluster { int num_probes = 0; @@ -48,6 +52,7 @@ class ProbeBitrateEstimator { std::map clusters_; RtcEventLog* const event_log_; absl::optional estimated_bitrate_bps_; + absl::optional last_estimate_; }; } // namespace webrtc