From fe0c90501b17ce81bd3ca209ae6109ee781c47f7 Mon Sep 17 00:00:00 2001 From: stefan Date: Mon, 27 Jul 2015 03:13:32 -0700 Subject: [PATCH] Improve probing by ignoring small packets which otherwise break the mechanism. These small packets are common for H.264 where the first packet of an IDR contains the parameter sets. BUG=4806 Review URL: https://codereview.webrtc.org/1221943002 Cr-Commit-Position: refs/heads/master@{#9639} --- webrtc/modules/pacing/bitrate_prober.cc | 5 ++- webrtc/modules/pacing/bitrate_prober.h | 2 ++ .../remote_bitrate_estimator_abs_send_time.cc | 6 +++- ...itrate_estimator_abs_send_time_unittest.cc | 34 ++++++++++++++++++- 4 files changed, 44 insertions(+), 3 deletions(-) diff --git a/webrtc/modules/pacing/bitrate_prober.cc b/webrtc/modules/pacing/bitrate_prober.cc index 1ed6298ff1..bedb892267 100644 --- a/webrtc/modules/pacing/bitrate_prober.cc +++ b/webrtc/modules/pacing/bitrate_prober.cc @@ -29,6 +29,8 @@ int ComputeDeltaFromBitrate(size_t packet_size, int bitrate_bps) { } } // namespace +const size_t BitrateProber::kMinProbePacketSize = 200; + BitrateProber::BitrateProber() : probing_state_(kDisabled), packet_size_last_send_(0), @@ -88,7 +90,8 @@ int BitrateProber::TimeUntilNextProbe(int64_t now_ms) { // We will send the first probe packet immediately if no packet has been // sent before. int time_until_probe_ms = 0; - if (packet_size_last_send_ > 0 && probing_state_ == kProbing) { + if (packet_size_last_send_ > kMinProbePacketSize && + probing_state_ == kProbing) { int next_delta_ms = ComputeDeltaFromBitrate(packet_size_last_send_, probe_bitrates_.front()); time_until_probe_ms = next_delta_ms - elapsed_time_ms; diff --git a/webrtc/modules/pacing/bitrate_prober.h b/webrtc/modules/pacing/bitrate_prober.h index b3f52afeb6..c26b0d6275 100644 --- a/webrtc/modules/pacing/bitrate_prober.h +++ b/webrtc/modules/pacing/bitrate_prober.h @@ -22,6 +22,8 @@ namespace webrtc { // on being protected by the caller. class BitrateProber { public: + static const size_t kMinProbePacketSize; + BitrateProber(); void SetEnabled(bool enable); 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 e83ac812e3..c5faf1617f 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 @@ -16,6 +16,7 @@ #include "webrtc/base/scoped_ptr.h" #include "webrtc/base/thread_annotations.h" #include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" +#include "webrtc/modules/pacing/bitrate_prober.h" #include "webrtc/system_wrappers/interface/clock.h" #include "webrtc/system_wrappers/interface/critical_section_wrapper.h" #include "webrtc/system_wrappers/interface/logging.h" @@ -268,7 +269,10 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo( uint32_t ts_delta = 0; int64_t t_delta = 0; int size_delta = 0; - // For now only try to detect probes while we don't have a valid estimate. + // For now only try to detect probes while we don't have a valid estimate, and + // make sure the packet was paced. We currently assume that only packets + // larger than 200 bytes are paced by the sender. + was_paced = was_paced && payload_size > BitrateProber::kMinProbePacketSize; if (was_paced && (!remote_rate_.ValidEstimate() || now_ms - first_packet_time_ms_ < kInitialProbingIntervalMs)) { diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time_unittest.cc b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time_unittest.cc index 6f8d6cb2c8..c2137d806f 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time_unittest.cc +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time_unittest.cc @@ -38,7 +38,7 @@ TEST_F(RemoteBitrateEstimatorAbsSendTimeTest, RateIncreaseReordering) { } TEST_F(RemoteBitrateEstimatorAbsSendTimeTest, RateIncreaseRtpTimestamps) { - RateIncreaseRtpTimestampsTestHelper(1089); + RateIncreaseRtpTimestampsTestHelper(1240); } TEST_F(RemoteBitrateEstimatorAbsSendTimeTest, CapacityDropOneStream) { @@ -259,4 +259,36 @@ TEST_F(RemoteBitrateEstimatorAbsSendTimeTest, EXPECT_TRUE(bitrate_observer_->updated()); EXPECT_NEAR(bitrate_observer_->latest_bitrate(), 4000000u, 10000); } + +TEST_F(RemoteBitrateEstimatorAbsSendTimeTest, ProbingIgnoresSmallPackets) { + const int kProbeLength = 5; + int64_t now_ms = clock_.TimeInMilliseconds(); + // Probing with 200 bytes every 10 ms, should be ignored by the probe + // detection. + for (int i = 0; i < kProbeLength; ++i) { + clock_.AdvanceTimeMilliseconds(10); + now_ms = clock_.TimeInMilliseconds(); + IncomingPacket(0, 200, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000), + true); + } + + EXPECT_EQ(0, bitrate_estimator_->Process()); + EXPECT_FALSE(bitrate_observer_->updated()); + + // Followed by a probe with 1000 bytes packets, should be detected as a + // probe. + for (int i = 0; i < kProbeLength; ++i) { + clock_.AdvanceTimeMilliseconds(10); + now_ms = clock_.TimeInMilliseconds(); + IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000), + true); + } + + // Wait long enough so that we can call Process again. + clock_.AdvanceTimeMilliseconds(1000); + + EXPECT_EQ(0, bitrate_estimator_->Process()); + EXPECT_TRUE(bitrate_observer_->updated()); + EXPECT_NEAR(bitrate_observer_->latest_bitrate(), 800000u, 10000); +} } // namespace webrtc