From eb680eac5dad102a4df698dff9e7aec974a97d98 Mon Sep 17 00:00:00 2001 From: philipel Date: Wed, 17 Aug 2016 11:11:59 +0200 Subject: [PATCH] CongestionController::SetBweBitrates may now trigger probing. BUG=webrtc:5859 R=stefan@webrtc.org Review URL: https://codereview.webrtc.org/2246403002 . Cr-Commit-Position: refs/heads/master@{#13791} --- .../congestion_controller.cc | 20 +++++++++++++++++++ .../include/congestion_controller.h | 1 + webrtc/modules/pacing/bitrate_prober.cc | 7 ++++--- webrtc/modules/pacing/bitrate_prober.h | 2 +- .../modules/pacing/bitrate_prober_unittest.cc | 6 +++--- webrtc/modules/pacing/paced_sender.cc | 7 +++++-- webrtc/modules/pacing/paced_sender.h | 2 ++ .../modules/pacing/paced_sender_unittest.cc | 6 ++++++ 8 files changed, 42 insertions(+), 9 deletions(-) diff --git a/webrtc/modules/congestion_controller/congestion_controller.cc b/webrtc/modules/congestion_controller/congestion_controller.cc index 88bc907136..68c306ce10 100644 --- a/webrtc/modules/congestion_controller/congestion_controller.cc +++ b/webrtc/modules/congestion_controller/congestion_controller.cc @@ -171,6 +171,7 @@ CongestionController::CongestionController( remote_estimator_proxy_(clock_, packet_router_.get()), transport_feedback_adapter_(bitrate_controller_.get(), clock_), min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps), + max_bitrate_bps_(0), last_reported_bitrate_bps_(0), last_reported_fraction_loss_(0), last_reported_rtt_(0), @@ -200,6 +201,7 @@ CongestionController::CongestionController( remote_estimator_proxy_(clock_, packet_router_.get()), transport_feedback_adapter_(bitrate_controller_.get(), clock_), min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps), + max_bitrate_bps_(0), last_reported_bitrate_bps_(0), last_reported_fraction_loss_(0), last_reported_rtt_(0), @@ -214,6 +216,8 @@ void CongestionController::Init() { new DelayBasedBwe(&transport_feedback_adapter_, clock_)); transport_feedback_adapter_.GetBitrateEstimator()->SetMinBitrate( min_bitrate_bps_); + pacer_->CreateProbeCluster(900000, 6); + pacer_->CreateProbeCluster(1800000, 5); } void CongestionController::SetBweBitrates(int min_bitrate_bps, @@ -224,6 +228,21 @@ void CongestionController::SetBweBitrates(int min_bitrate_bps, min_bitrate_bps, max_bitrate_bps); + { + // Only do probing if: + // - we are mid-call, which we consider to be if + // |last_reported_bitrate_bps_| != 0, and + // - the current bitrate is lower than the new |max_bitrate_bps|, and + // - we actually want to increase the |max_bitrate_bps_|. + rtc::CritScope cs(&critsect_); + if (last_reported_bitrate_bps_ != 0 && + last_reported_bitrate_bps_ < static_cast(max_bitrate_bps) && + max_bitrate_bps > max_bitrate_bps_) { + pacer_->CreateProbeCluster(max_bitrate_bps, 5); + } + } + max_bitrate_bps_ = max_bitrate_bps; + if (remote_bitrate_estimator_) remote_bitrate_estimator_->SetMinBitrate(min_bitrate_bps); min_bitrate_bps_ = min_bitrate_bps; @@ -241,6 +260,7 @@ void CongestionController::ResetBweAndBitrates(int bitrate_bps, bitrate_controller_->ResetBitrates(bitrate_bps, min_bitrate_bps, max_bitrate_bps); min_bitrate_bps_ = min_bitrate_bps; + max_bitrate_bps_ = max_bitrate_bps; // TODO(honghaiz): Recreate this object once the remote bitrate estimator is // no longer exposed outside CongestionController. if (remote_bitrate_estimator_) diff --git a/webrtc/modules/congestion_controller/include/congestion_controller.h b/webrtc/modules/congestion_controller/include/congestion_controller.h index a0531cc27e..b70899d84b 100644 --- a/webrtc/modules/congestion_controller/include/congestion_controller.h +++ b/webrtc/modules/congestion_controller/include/congestion_controller.h @@ -124,6 +124,7 @@ class CongestionController : public CallStatsObserver, public Module { RemoteEstimatorProxy remote_estimator_proxy_; TransportFeedbackAdapter transport_feedback_adapter_; int min_bitrate_bps_; + int max_bitrate_bps_; rtc::CriticalSection critsect_; uint32_t last_reported_bitrate_bps_ GUARDED_BY(critsect_); uint8_t last_reported_fraction_loss_ GUARDED_BY(critsect_); diff --git a/webrtc/modules/pacing/bitrate_prober.cc b/webrtc/modules/pacing/bitrate_prober.cc index 4b2a977bbe..e2cea73305 100644 --- a/webrtc/modules/pacing/bitrate_prober.cc +++ b/webrtc/modules/pacing/bitrate_prober.cc @@ -59,12 +59,13 @@ void BitrateProber::OnIncomingPacket(size_t packet_size) { // Don't initialize probing unless we have something large enough to start // probing. if (probing_state_ == ProbingState::kInactive && + !clusters_.empty() && packet_size >= PacedSender::kMinProbePacketSize) { probing_state_ = ProbingState::kActive; } } -void BitrateProber::ProbeAtBitrate(uint32_t bitrate_bps, int num_packets) { +void BitrateProber::CreateProbeCluster(int bitrate_bps, int num_packets) { ProbeCluster cluster; cluster.max_probe_packets = num_packets; cluster.probe_bitrate_bps = bitrate_bps; @@ -85,8 +86,8 @@ void BitrateProber::ResetState() { std::queue clusters; clusters.swap(clusters_); while (!clusters.empty()) { - ProbeAtBitrate(clusters.front().probe_bitrate_bps, - clusters.front().max_probe_packets); + CreateProbeCluster(clusters.front().probe_bitrate_bps, + clusters.front().max_probe_packets); clusters.pop(); } // If its enabled, reset to inactive. diff --git a/webrtc/modules/pacing/bitrate_prober.h b/webrtc/modules/pacing/bitrate_prober.h index fc0e13fb67..cfeb4972f9 100644 --- a/webrtc/modules/pacing/bitrate_prober.h +++ b/webrtc/modules/pacing/bitrate_prober.h @@ -38,7 +38,7 @@ class BitrateProber { // Create a cluster used to probe for |bitrate_bps| with |num_packets| number // of packets. - void ProbeAtBitrate(uint32_t bitrate_bps, int num_packets); + void CreateProbeCluster(int bitrate_bps, int num_packets); // Returns the number of milliseconds until the next packet should be sent to // get accurate probing. diff --git a/webrtc/modules/pacing/bitrate_prober_unittest.cc b/webrtc/modules/pacing/bitrate_prober_unittest.cc index 1b2a321573..422bd27b68 100644 --- a/webrtc/modules/pacing/bitrate_prober_unittest.cc +++ b/webrtc/modules/pacing/bitrate_prober_unittest.cc @@ -21,8 +21,8 @@ TEST(BitrateProberTest, VerifyStatesAndTimeBetweenProbes) { int64_t now_ms = 0; EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms)); - prober.ProbeAtBitrate(900000, 6); - prober.ProbeAtBitrate(1800000, 5); + prober.CreateProbeCluster(900000, 6); + prober.CreateProbeCluster(1800000, 5); EXPECT_FALSE(prober.IsProbing()); prober.OnIncomingPacket(1000); @@ -60,7 +60,7 @@ TEST(BitrateProberTest, DoesntProbeWithoutRecentPackets) { int64_t now_ms = 0; EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms)); - prober.ProbeAtBitrate(900000, 6); + prober.CreateProbeCluster(900000, 6); EXPECT_FALSE(prober.IsProbing()); prober.OnIncomingPacket(1000); diff --git a/webrtc/modules/pacing/paced_sender.cc b/webrtc/modules/pacing/paced_sender.cc index bb071f1656..b0b86ee40e 100644 --- a/webrtc/modules/pacing/paced_sender.cc +++ b/webrtc/modules/pacing/paced_sender.cc @@ -261,12 +261,15 @@ PacedSender::PacedSender(Clock* clock, PacketSender* packet_sender) packets_(new paced_sender::PacketQueue(clock)), packet_counter_(0) { UpdateBytesPerInterval(kMinPacketLimitMs); - prober_->ProbeAtBitrate(900000, 6); - prober_->ProbeAtBitrate(1800000, 5); } PacedSender::~PacedSender() {} +void PacedSender::CreateProbeCluster(int bitrate_bps, int num_packets) { + CriticalSectionScoped cs(critsect_.get()); + prober_->CreateProbeCluster(bitrate_bps, num_packets); +} + void PacedSender::Pause() { LOG(LS_INFO) << "PacedSender paused."; CriticalSectionScoped cs(critsect_.get()); diff --git a/webrtc/modules/pacing/paced_sender.h b/webrtc/modules/pacing/paced_sender.h index c904d4b6b7..cb9b84930b 100644 --- a/webrtc/modules/pacing/paced_sender.h +++ b/webrtc/modules/pacing/paced_sender.h @@ -71,6 +71,8 @@ class PacedSender : public Module, public RtpPacketSender { virtual ~PacedSender(); + void CreateProbeCluster(int bitrate_bps, int num_packets); + // Temporarily pause all sending. void Pause(); diff --git a/webrtc/modules/pacing/paced_sender_unittest.cc b/webrtc/modules/pacing/paced_sender_unittest.cc index 354b9b5a23..861452567f 100644 --- a/webrtc/modules/pacing/paced_sender_unittest.cc +++ b/webrtc/modules/pacing/paced_sender_unittest.cc @@ -110,6 +110,8 @@ class PacedSenderTest : public ::testing::Test { srand(0); // Need to initialize PacedSender after we initialize clock. send_bucket_.reset(new PacedSender(&clock_, &callback_)); + send_bucket_->CreateProbeCluster(900000, 6); + send_bucket_->CreateProbeCluster(1800000, 5); // Default to bitrate probing disabled for testing purposes. Probing tests // have to enable probing, either by creating a new PacedSender instance or // by calling SetProbingEnabled(true). @@ -814,6 +816,8 @@ TEST_F(PacedSenderTest, ProbingWithInitialFrame) { expected_deltas + kNumDeltas); PacedSenderProbing callback(expected_deltas_list, &clock_); send_bucket_.reset(new PacedSender(&clock_, &callback)); + send_bucket_->CreateProbeCluster(900000, 6); + send_bucket_->CreateProbeCluster(1800000, 5); send_bucket_->SetEstimatedBitrate(kInitialBitrateBps); for (int i = 0; i < kNumPackets; ++i) { @@ -844,6 +848,8 @@ TEST_F(PacedSenderTest, ProbingWithTooSmallInitialFrame) { expected_deltas + kNumDeltas); PacedSenderProbing callback(expected_deltas_list, &clock_); send_bucket_.reset(new PacedSender(&clock_, &callback)); + send_bucket_->CreateProbeCluster(900000, 6); + send_bucket_->CreateProbeCluster(1800000, 5); send_bucket_->SetEstimatedBitrate(kInitialBitrateBps); for (int i = 0; i < kNumPackets - 5; ++i) {