From bf8a2c94ce32d64da2fae5c21c727555f2036585 Mon Sep 17 00:00:00 2001 From: philipel Date: Wed, 10 Aug 2016 19:00:39 +0200 Subject: [PATCH] Probe bitrate estimator correction. Since the interval between the timestamps does not include the send/receive time of the last/first packet we correct the interval by adding the average of the interval between probing packets. BUG=webrtc:5859 R=mflodman@webrtc.org, stefan@webrtc.org Review URL: https://codereview.webrtc.org/2224173003 . Cr-Commit-Position: refs/heads/master@{#13719} --- webrtc/modules/BUILD.gn | 1 + .../probe_bitrate_estimator.cc | 19 +++++++++--- .../probe_bitrate_estimator.h | 1 + ...cc => probe_bitrate_estimator_unittest.cc} | 29 ++++++++++--------- webrtc/modules/modules.gyp | 2 +- 5 files changed, 33 insertions(+), 19 deletions(-) rename webrtc/modules/congestion_controller/{probe_bitrate_controller_unittest.cc => probe_bitrate_estimator_unittest.cc} (85%) diff --git a/webrtc/modules/BUILD.gn b/webrtc/modules/BUILD.gn index f1cd14b3fa..e3e92a1417 100644 --- a/webrtc/modules/BUILD.gn +++ b/webrtc/modules/BUILD.gn @@ -225,6 +225,7 @@ if (rtc_include_tests) { "congestion_controller/delay_based_bwe_unittest.cc", "congestion_controller/delay_based_bwe_unittest_helper.cc", "congestion_controller/delay_based_bwe_unittest_helper.h", + "congestion_controller/probe_bitrate_estimator_unittest.cc", "media_file/media_file_unittest.cc", "module_common_types_unittest.cc", "pacing/bitrate_prober_unittest.cc", diff --git a/webrtc/modules/congestion_controller/probe_bitrate_estimator.cc b/webrtc/modules/congestion_controller/probe_bitrate_estimator.cc index 4a63625291..561fde9391 100644 --- a/webrtc/modules/congestion_controller/probe_bitrate_estimator.cc +++ b/webrtc/modules/congestion_controller/probe_bitrate_estimator.cc @@ -32,6 +32,10 @@ ProbingResult::ProbingResult() : bps(kNoEstimate), timestamp(0) {} ProbingResult::ProbingResult(int bps, int64_t timestamp) : bps(bps), timestamp(timestamp) {} +bool ProbingResult::valid() const { + return bps != kNoEstimate; +} + ProbeBitrateEstimator::ProbeBitrateEstimator() : last_valid_cluster_id_(0) {} ProbingResult ProbeBitrateEstimator::PacketFeedback( @@ -52,7 +56,7 @@ ProbingResult ProbeBitrateEstimator::PacketFeedback( std::min(cluster->first_receive_ms, packet_info.arrival_time_ms); cluster->last_receive_ms = std::max(cluster->last_receive_ms, packet_info.arrival_time_ms); - cluster->size += packet_info.payload_size; + cluster->size += packet_info.payload_size * 8; cluster->num_probes += 1; // Clean up old clusters. @@ -62,10 +66,18 @@ ProbingResult ProbeBitrateEstimator::PacketFeedback( if (cluster->num_probes < kMinNumProbesValidCluster) return ProbingResult(); - int send_interval_ms = cluster->last_send_ms - cluster->first_send_ms; - int receive_interval_ms = + float send_interval_ms = cluster->last_send_ms - cluster->first_send_ms; + float receive_interval_ms = cluster->last_receive_ms - cluster->first_receive_ms; + // Since the send/receive interval does not include the send/receive time of + // the last/first packet we expand the interval by the average inverval + // between the probing packets. + float interval_correction = + static_cast(cluster->num_probes) / (cluster->num_probes - 1); + send_interval_ms *= interval_correction; + receive_interval_ms *= interval_correction; + if (send_interval_ms == 0 || receive_interval_ms == 0) { LOG(LS_INFO) << "Probing unsuccessful, invalid send/receive interval" << " [cluster id: " << packet_info.probe_cluster_id @@ -74,7 +86,6 @@ ProbingResult ProbeBitrateEstimator::PacketFeedback( return ProbingResult(); } - float send_bps = static_cast(cluster->size) / send_interval_ms * 1000; float receive_bps = static_cast(cluster->size) / receive_interval_ms * 1000; diff --git a/webrtc/modules/congestion_controller/probe_bitrate_estimator.h b/webrtc/modules/congestion_controller/probe_bitrate_estimator.h index 2aea59c09c..7f0c4569c9 100644 --- a/webrtc/modules/congestion_controller/probe_bitrate_estimator.h +++ b/webrtc/modules/congestion_controller/probe_bitrate_estimator.h @@ -23,6 +23,7 @@ struct ProbingResult { ProbingResult(); ProbingResult(int bps, int64_t timestamp); + bool valid() const; int bps; int64_t timestamp; diff --git a/webrtc/modules/congestion_controller/probe_bitrate_controller_unittest.cc b/webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc similarity index 85% rename from webrtc/modules/congestion_controller/probe_bitrate_controller_unittest.cc rename to webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc index 40beb0f2f3..e3bbd5a227 100644 --- a/webrtc/modules/congestion_controller/probe_bitrate_controller_unittest.cc +++ b/webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc @@ -24,12 +24,13 @@ class TestProbeBitrateEstimator : public ::testing::Test { TestProbeBitrateEstimator() : probe_bitrate_estimator_() {} void AddPacketFeedback(int probe_cluster_id, - size_t size, + size_t size_bytes, int64_t send_time_ms, int64_t arrival_time_ms) { - PacketInfo info(arrival_time_ms, send_time_ms, 0, size, probe_cluster_id); + PacketInfo info(arrival_time_ms, send_time_ms, 0, size_bytes, + probe_cluster_id); ProbingResult res = probe_bitrate_estimator_.PacketFeedback(info); - if (res.bps != ProbingResult::kNoEstimate) + if (res.valid()) results_.emplace_back(res.bps, res.timestamp); } @@ -48,18 +49,18 @@ TEST_F(TestProbeBitrateEstimator, OneCluster) { AddPacketFeedback(0, 1000, 0, 10); AddPacketFeedback(0, 1000, 10, 20); AddPacketFeedback(0, 1000, 20, 30); - AddPacketFeedback(0, 1000, 40, 50); + AddPacketFeedback(0, 1000, 30, 40); - CheckResult(0, 100000, 10, 50); + CheckResult(0, 800000, 10, 40); } TEST_F(TestProbeBitrateEstimator, FastReceive) { AddPacketFeedback(0, 1000, 0, 15); AddPacketFeedback(0, 1000, 10, 30); - AddPacketFeedback(0, 1000, 20, 40); - AddPacketFeedback(0, 1000, 40, 50); + AddPacketFeedback(0, 1000, 20, 35); + AddPacketFeedback(0, 1000, 30, 40); - CheckResult(0, 100000, 10, 50); + CheckResult(0, 800000, 10, 40); } TEST_F(TestProbeBitrateEstimator, TooFastReceive) { @@ -75,9 +76,9 @@ TEST_F(TestProbeBitrateEstimator, SlowReceive) { AddPacketFeedback(0, 1000, 0, 10); AddPacketFeedback(0, 1000, 10, 40); AddPacketFeedback(0, 1000, 20, 70); - AddPacketFeedback(0, 1000, 40, 110); + AddPacketFeedback(0, 1000, 30, 85); - CheckResult(0, 40000, 10, 110); + CheckResult(0, 320000, 10, 85); } TEST_F(TestProbeBitrateEstimator, BurstReceive) { @@ -96,15 +97,15 @@ TEST_F(TestProbeBitrateEstimator, MultipleClusters) { AddPacketFeedback(0, 1000, 40, 60); AddPacketFeedback(0, 1000, 50, 60); - CheckResult(0, 80000, 10, 60); - CheckResult(1, 100000, 10, 60); + CheckResult(0, 480000, 10, 60); + CheckResult(1, 640000, 10, 60); AddPacketFeedback(1, 1000, 60, 70); AddPacketFeedback(1, 1000, 65, 77); AddPacketFeedback(1, 1000, 70, 84); AddPacketFeedback(1, 1000, 75, 90); - CheckResult(2, 200000, 10, 90); + CheckResult(2, 1200000, 10, 90); } TEST_F(TestProbeBitrateEstimator, OldProbe) { @@ -117,7 +118,7 @@ TEST_F(TestProbeBitrateEstimator, OldProbe) { AddPacketFeedback(1, 1000, 70, 84); AddPacketFeedback(1, 1000, 75, 90); - CheckResult(0, 200000, 10, 90); + CheckResult(0, 1200000, 10, 90); AddPacketFeedback(0, 1000, 40, 60); diff --git a/webrtc/modules/modules.gyp b/webrtc/modules/modules.gyp index f3b986f12b..c0f9504d08 100644 --- a/webrtc/modules/modules.gyp +++ b/webrtc/modules/modules.gyp @@ -277,7 +277,7 @@ 'congestion_controller/delay_based_bwe_unittest.cc', 'congestion_controller/delay_based_bwe_unittest_helper.cc', 'congestion_controller/delay_based_bwe_unittest_helper.h', - 'congestion_controller/probe_bitrate_controller_unittest.cc', + 'congestion_controller/probe_bitrate_estimator_unittest.cc', 'media_file/media_file_unittest.cc', 'module_common_types_unittest.cc', 'pacing/bitrate_prober_unittest.cc',