From f00497c5732b99c42f66023a5b0940c7418d14ed Mon Sep 17 00:00:00 2001 From: stefan Date: Fri, 27 Jan 2017 02:27:33 -0800 Subject: [PATCH] Improve bitrate probing for the audio-only case. This means that smaller probe packets will be allowed at lower bitrates. BUG=webrtc:7043 Review-Url: https://codereview.webrtc.org/2650393002 Cr-Commit-Position: refs/heads/master@{#16317} --- .../delay_based_bwe_unittest.cc | 4 +--- .../congestion_controller/probe_controller.cc | 2 +- webrtc/modules/pacing/alr_detector.cc | 4 ++-- webrtc/modules/pacing/alr_detector_unittest.cc | 12 ++++++------ webrtc/modules/pacing/bitrate_prober.cc | 13 +++++++++---- webrtc/modules/pacing/bitrate_prober_unittest.cc | 6 +----- webrtc/modules/pacing/paced_sender.h | 2 -- .../remote_bitrate_estimator_abs_send_time.cc | 3 ++- 8 files changed, 22 insertions(+), 24 deletions(-) diff --git a/webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc b/webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc index d4b3a72a3f..4f0829ac62 100644 --- a/webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc +++ b/webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc @@ -57,9 +57,7 @@ TEST_F(DelayBasedBweTest, ProbeDetectionNonPacedPackets) { IncomingFeedback(now_ms, now_ms, seq_num++, 1000, 0); // Non-paced packet, arriving 5 ms after. clock_.AdvanceTimeMilliseconds(5); - IncomingFeedback(now_ms, now_ms, seq_num++, - PacedSender::kMinProbePacketSize + 1, - PacketInfo::kNotAProbe); + IncomingFeedback(now_ms, now_ms, seq_num++, 100, PacketInfo::kNotAProbe); } EXPECT_TRUE(bitrate_observer_.updated()); diff --git a/webrtc/modules/congestion_controller/probe_controller.cc b/webrtc/modules/congestion_controller/probe_controller.cc index 373b268d2c..72d0e9ed0d 100644 --- a/webrtc/modules/congestion_controller/probe_controller.cc +++ b/webrtc/modules/congestion_controller/probe_controller.cc @@ -164,7 +164,7 @@ void ProbeController::SetEstimatedBitrate(int64_t bitrate_bps) { // it ramps up from bitrate_bps. if (state_ == State::kProbingComplete && pacer_->GetApplicationLimitedRegionStartTime() && - bitrate_bps < estimated_bitrate_bps_ / 2 && + bitrate_bps < 2 * estimated_bitrate_bps_ / 3 && (now_ms - last_alr_probing_time_) > kAlrProbingIntervalMinMs) { LOG(LS_INFO) << "Detected big BW drop in ALR, start probe."; // Track how often we probe in response to BW drop in ALR. diff --git a/webrtc/modules/pacing/alr_detector.cc b/webrtc/modules/pacing/alr_detector.cc index b31d4482bb..9eeca26012 100644 --- a/webrtc/modules/pacing/alr_detector.cc +++ b/webrtc/modules/pacing/alr_detector.cc @@ -23,8 +23,8 @@ constexpr int kMeasurementPeriodMs = 500; // kAlrStartUsagePercent and ends when it raises above kAlrEndUsagePercent. // NOTE: This is intentionally conservative at the moment until BW adjustments // of application limited region is fine tuned. -constexpr int kAlrStartUsagePercent = 30; -constexpr int kAlrEndUsagePercent = 50; +constexpr int kAlrStartUsagePercent = 60; +constexpr int kAlrEndUsagePercent = 70; } // namespace diff --git a/webrtc/modules/pacing/alr_detector_unittest.cc b/webrtc/modules/pacing/alr_detector_unittest.cc index c6165915d0..3c91277223 100644 --- a/webrtc/modules/pacing/alr_detector_unittest.cc +++ b/webrtc/modules/pacing/alr_detector_unittest.cc @@ -61,12 +61,12 @@ TEST_F(AlrDetectorTest, AlrDetection) { SimulateOutgoingTraffic(500, 20); EXPECT_TRUE(alr_detector_.GetApplicationLimitedRegionStartTime()); - // Verify that we remain in ALR state while usage is still below 50%. - SimulateOutgoingTraffic(500, 40); + // Verify that we remain in ALR state while usage is still below 70%. + SimulateOutgoingTraffic(500, 69); EXPECT_TRUE(alr_detector_.GetApplicationLimitedRegionStartTime()); - // Verify that ALR ends when usage is above 50%. - SimulateOutgoingTraffic(500, 60); + // Verify that ALR ends when usage is above 70%. + SimulateOutgoingTraffic(500, 75); EXPECT_FALSE(alr_detector_.GetApplicationLimitedRegionStartTime()); } @@ -85,8 +85,8 @@ TEST_F(AlrDetectorTest, ShortSpike) { SimulateOutgoingTraffic(200, 20); EXPECT_TRUE(alr_detector_.GetApplicationLimitedRegionStartTime()); - // ALR ends when usage is above 50%. - SimulateOutgoingTraffic(500, 60); + // ALR ends when usage is above 70%. + SimulateOutgoingTraffic(500, 75); EXPECT_FALSE(alr_detector_.GetApplicationLimitedRegionStartTime()); } diff --git a/webrtc/modules/pacing/bitrate_prober.cc b/webrtc/modules/pacing/bitrate_prober.cc index bdf48fda1d..37bdecf3b4 100644 --- a/webrtc/modules/pacing/bitrate_prober.cc +++ b/webrtc/modules/pacing/bitrate_prober.cc @@ -36,6 +36,11 @@ constexpr int kMaxProbeDelayMs = 3; // Number of times probing is retried before the cluster is dropped. constexpr int kMaxRetryAttempts = 3; +// The min probe packet size is scaled with the bitrate we're probing at. +// This defines the max min probe packet size, meaning that on high bitrates +// we have a min probe packet size of 200 bytes. +constexpr size_t kMinProbePacketSize = 200; + } // namespace BitrateProber::BitrateProber() @@ -64,9 +69,9 @@ bool BitrateProber::IsProbing() const { 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) { + if (probing_state_ == ProbingState::kInactive && !clusters_.empty() && + packet_size >= + std::min(RecommendedMinProbeSize(), kMinProbePacketSize)) { // Send next probe right away. next_probe_time_ms_ = -1; probing_state_ = ProbingState::kActive; @@ -136,7 +141,7 @@ int BitrateProber::CurrentClusterId() const { // feasible. size_t BitrateProber::RecommendedMinProbeSize() const { RTC_DCHECK(!clusters_.empty()); - return clusters_.front().bitrate_bps * 2 * kMinProbeDeltaMs / (8 * 1000); + return clusters_.front().bitrate_bps * 3 * kMinProbeDeltaMs / (8 * 1000); } void BitrateProber::ProbeSent(int64_t now_ms, size_t bytes) { diff --git a/webrtc/modules/pacing/bitrate_prober_unittest.cc b/webrtc/modules/pacing/bitrate_prober_unittest.cc index 9ada114625..b5bc889014 100644 --- a/webrtc/modules/pacing/bitrate_prober_unittest.cc +++ b/webrtc/modules/pacing/bitrate_prober_unittest.cc @@ -88,10 +88,6 @@ TEST(BitrateProberTest, DoesntProbeWithoutRecentPackets) { // Let time pass, no large enough packets put into prober. now_ms += 6000; EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms)); - // Insert a small packet, not a candidate for probing. - prober.OnIncomingPacket(100); - EXPECT_FALSE(prober.IsProbing()); - EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms)); // Insert a large-enough packet after downtime while probing should reset to // perform a new probe since the requested one didn't finish. prober.OnIncomingPacket(1000); @@ -147,7 +143,7 @@ TEST(BitrateProberTest, ScaleBytesUsedForProbing) { prober.OnIncomingPacket(kPacketSizeBytes); int bytes_sent = 0; while (bytes_sent < kExpectedBytesSent) { - EXPECT_TRUE(prober.IsProbing()); + ASSERT_TRUE(prober.IsProbing()); prober.ProbeSent(0, kPacketSizeBytes); bytes_sent += kPacketSizeBytes; } diff --git a/webrtc/modules/pacing/paced_sender.h b/webrtc/modules/pacing/paced_sender.h index 50ebb14c57..7fd65111f2 100644 --- a/webrtc/modules/pacing/paced_sender.h +++ b/webrtc/modules/pacing/paced_sender.h @@ -67,8 +67,6 @@ class PacedSender : public Module, public RtpPacketSender { // overshoots from the encoder. static const float kDefaultPaceMultiplier; - static const size_t kMinProbePacketSize = 200; - PacedSender(Clock* clock, PacketSender* packet_sender); virtual ~PacedSender(); diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc index 269a5d117d..7d1b50bf66 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc @@ -286,7 +286,8 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo( // For now only try to detect probes while we don't have a valid estimate. // We currently assume that only packets larger than 200 bytes are paced by // the sender. - if (payload_size > PacedSender::kMinProbePacketSize && + const size_t kMinProbePacketSize = 200; + if (payload_size > kMinProbePacketSize && (!remote_rate_.ValidEstimate() || now_ms - first_packet_time_ms_ < kInitialProbingIntervalMs)) { // TODO(holmer): Use a map instead to get correct order?