From ebfbc8ebfd0c9ace51353827a85fade550c04353 Mon Sep 17 00:00:00 2001 From: sprang Date: Tue, 10 Jan 2017 01:27:28 -0800 Subject: [PATCH] Revert of Fix BitrateProber to match the requested bitrate more precisely (patchset #4 id:60001 of https://codereview.webrtc.org/2613543003/ ) Reason for revert: Speculative revert. Linux memcheck bot started failing a lot at the time of this cl. Doesn't look related at first glance, but we don't have another lead yet. Original issue's description: > Fix BitrateProber to match the requested bitrate more precisely > > Previously BirateProber was calculating delay between probes based on > the size of the previous probe. Because of that the actual sent bitrate > can deviate greatly from the target value. With this change it uses > total number of bytes in the cluster to estimate delay before each > probe. > > BUG=webrtc:6952 > > Review-Url: https://codereview.webrtc.org/2613543003 > Cr-Commit-Position: refs/heads/master@{#15971} > Committed: https://chromium.googlesource.com/external/webrtc/+/599c5011e7569a269f5521a8c7e6ab930e7adbd5 TBR=philipel@webrtc.org,stefan@webrtc.org,sergeyu@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=webrtc:6952 Review-Url: https://codereview.webrtc.org/2626473004 Cr-Commit-Position: refs/heads/master@{#15979} --- webrtc/modules/pacing/bitrate_prober.cc | 73 +++++++++++-------- webrtc/modules/pacing/bitrate_prober.h | 21 ++---- .../modules/pacing/bitrate_prober_unittest.cc | 46 ++++-------- .../modules/pacing/paced_sender_unittest.cc | 7 +- 4 files changed, 66 insertions(+), 81 deletions(-) diff --git a/webrtc/modules/pacing/bitrate_prober.cc b/webrtc/modules/pacing/bitrate_prober.cc index d4007cc1ad..8e3c5e3547 100644 --- a/webrtc/modules/pacing/bitrate_prober.cc +++ b/webrtc/modules/pacing/bitrate_prober.cc @@ -20,6 +20,9 @@ namespace webrtc { namespace { +// Inactivity threshold above which probing is restarted. +constexpr int kInactivityThresholdMs = 5000; + // A minimum interval between probes to allow scheduling to be feasible. constexpr int kMinProbeDeltaMs = 1; @@ -29,11 +32,18 @@ constexpr int kMinProbePacketsSent = 5; // The minimum probing duration in ms. constexpr int kMinProbeDurationMs = 15; +int ComputeDeltaFromBitrate(size_t probe_size, uint32_t bitrate_bps) { + RTC_CHECK_GT(bitrate_bps, 0); + // Compute the time delta needed to send probe_size bytes at bitrate_bps + // bps. Result is in milliseconds. + return static_cast((1000ll * probe_size * 8) / bitrate_bps); +} } // namespace BitrateProber::BitrateProber() : probing_state_(ProbingState::kDisabled), - next_probe_time_ms_(-1), + probe_size_last_sent_(0), + time_last_probe_sent_ms_(-1), next_cluster_id_(0) { SetEnabled(true); } @@ -60,8 +70,6 @@ void BitrateProber::OnIncomingPacket(size_t packet_size) { if (probing_state_ == ProbingState::kInactive && !clusters_.empty() && packet_size >= PacedSender::kMinProbePacketSize) { - // Send next probe right away. - next_probe_time_ms_ = -1; probing_state_ = ProbingState::kActive; } } @@ -85,6 +93,9 @@ void BitrateProber::CreateProbeCluster(int bitrate_bps) { } void BitrateProber::ResetState() { + time_last_probe_sent_ms_ = -1; + probe_size_last_sent_ = 0; + // Recreate all probing clusters. std::queue clusters; clusters.swap(clusters_); @@ -101,19 +112,36 @@ int BitrateProber::TimeUntilNextProbe(int64_t now_ms) { // Probing is not active or probing is already complete. if (probing_state_ != ProbingState::kActive || clusters_.empty()) return -1; - + // time_last_probe_sent_ms_ of -1 indicates no probes have yet been sent. + int64_t elapsed_time_ms; + if (time_last_probe_sent_ms_ == -1) { + elapsed_time_ms = 0; + } else { + elapsed_time_ms = now_ms - time_last_probe_sent_ms_; + } + // If no probes have been sent for a while, abort current probing and + // reset. + if (elapsed_time_ms > kInactivityThresholdMs) { + ResetState(); + return -1; + } + // We will send the first probe packet immediately if no packet has been + // sent before. int time_until_probe_ms = 0; - if (next_probe_time_ms_ >= 0) { - time_until_probe_ms = next_probe_time_ms_ - now_ms; - // If we have waited more than 3 ms for a new packet we will restart probing - // again later. + if (probe_size_last_sent_ != 0 && probing_state_ == ProbingState::kActive) { + int next_delta_ms = ComputeDeltaFromBitrate(probe_size_last_sent_, + clusters_.front().bitrate_bps); + time_until_probe_ms = next_delta_ms - elapsed_time_ms; + // If we have waited more than 3 ms for a new packet to probe with we will + // consider this probing session over. const int kMaxProbeDelayMs = 3; - if (time_until_probe_ms < -kMaxProbeDelayMs) { - ResetState(); - return -1; + if (next_delta_ms < kMinProbeDeltaMs || + time_until_probe_ms < -kMaxProbeDelayMs) { + probing_state_ = ProbingState::kSuspended; + LOG(LS_INFO) << "Delta too small or missed probing accurately, suspend"; + time_until_probe_ms = 0; } } - return std::max(time_until_probe_ms, 0); } @@ -134,16 +162,12 @@ size_t BitrateProber::RecommendedMinProbeSize() const { void BitrateProber::ProbeSent(int64_t now_ms, size_t bytes) { RTC_DCHECK(probing_state_ == ProbingState::kActive); RTC_DCHECK_GT(bytes, 0); - + probe_size_last_sent_ = bytes; + time_last_probe_sent_ms_ = now_ms; if (!clusters_.empty()) { ProbeCluster* cluster = &clusters_.front(); - if (cluster->sent_probes == 0) { - RTC_DCHECK_EQ(cluster->time_started_ms, -1); - cluster->time_started_ms = now_ms; - } cluster->sent_bytes += static_cast(bytes); cluster->sent_probes += 1; - next_probe_time_ms_ = GetNextProbeTime(clusters_.front()); if (cluster->sent_bytes >= cluster->min_bytes && cluster->sent_probes >= cluster->min_probes) { clusters_.pop(); @@ -152,17 +176,4 @@ void BitrateProber::ProbeSent(int64_t now_ms, size_t bytes) { probing_state_ = ProbingState::kSuspended; } } - -int64_t BitrateProber::GetNextProbeTime(const ProbeCluster& cluster) { - RTC_CHECK_GT(cluster.bitrate_bps, 0); - RTC_CHECK_GE(cluster.time_started_ms, 0); - - // Compute the time delta from the cluster start to ensure probe bitrate stays - // close to the target bitrate. Result is in milliseconds. - int64_t delta_ms = (8000ll * cluster.sent_bytes + cluster.bitrate_bps / 2) / - cluster.bitrate_bps; - return cluster.time_started_ms + delta_ms; -} - - } // namespace webrtc diff --git a/webrtc/modules/pacing/bitrate_prober.h b/webrtc/modules/pacing/bitrate_prober.h index 225ee9e157..e1a0a84f10 100644 --- a/webrtc/modules/pacing/bitrate_prober.h +++ b/webrtc/modules/pacing/bitrate_prober.h @@ -72,36 +72,29 @@ class BitrateProber { }; // A probe cluster consists of a set of probes. Each probe in turn can be - // divided into a number of packets to accommodate the MTU on the network. + // divided into a number of packets to accomodate the MTU on the network. struct ProbeCluster { int min_probes = 0; + int sent_probes = 0; int min_bytes = 0; + int sent_bytes = 0; int bitrate_bps = 0; int id = -1; - - int sent_probes = 0; - int sent_bytes = 0; - int64_t time_started_ms = -1; }; // Resets the state of the prober and clears any cluster/timing data tracked. void ResetState(); - int64_t GetNextProbeTime(const ProbeCluster& cluster); - ProbingState probing_state_; - // Probe bitrate per packet. These are used to compute the delta relative to // the previous probe packet based on the size and time when that packet was // sent. std::queue clusters_; - - // Time the next probe should be sent when in kActive state. - int64_t next_probe_time_ms_; - + // A probe can include one or more packets. + size_t probe_size_last_sent_; + // The last time a probe was sent. + int64_t time_last_probe_sent_ms_; int next_cluster_id_; }; - } // namespace webrtc - #endif // WEBRTC_MODULES_PACING_BITRATE_PROBER_H_ diff --git a/webrtc/modules/pacing/bitrate_prober_unittest.cc b/webrtc/modules/pacing/bitrate_prober_unittest.cc index 9ada114625..8ac352de0e 100644 --- a/webrtc/modules/pacing/bitrate_prober_unittest.cc +++ b/webrtc/modules/pacing/bitrate_prober_unittest.cc @@ -21,53 +21,35 @@ TEST(BitrateProberTest, VerifyStatesAndTimeBetweenProbes) { int64_t now_ms = 0; EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms)); - const int kTestBitrate1 = 900000; - const int kTestBitrate2 = 1800000; - const int kClusterSize = 5; - const int kProbeSize = 1000; - const int kMinProbeDurationMs = 15; - - prober.CreateProbeCluster(kTestBitrate1); - prober.CreateProbeCluster(kTestBitrate2); + prober.CreateProbeCluster(900000); + prober.CreateProbeCluster(1800000); EXPECT_FALSE(prober.IsProbing()); - prober.OnIncomingPacket(kProbeSize); + prober.OnIncomingPacket(1000); EXPECT_TRUE(prober.IsProbing()); EXPECT_EQ(0, prober.CurrentClusterId()); // First packet should probe as soon as possible. EXPECT_EQ(0, prober.TimeUntilNextProbe(now_ms)); + prober.ProbeSent(now_ms, 1000); - for (int i = 0; i < kClusterSize; ++i) { - now_ms += prober.TimeUntilNextProbe(now_ms); + for (int i = 0; i < 4; ++i) { + EXPECT_EQ(8, prober.TimeUntilNextProbe(now_ms)); + now_ms += 4; + EXPECT_EQ(4, prober.TimeUntilNextProbe(now_ms)); + now_ms += 4; EXPECT_EQ(0, prober.TimeUntilNextProbe(now_ms)); EXPECT_EQ(0, prober.CurrentClusterId()); - prober.ProbeSent(now_ms, kProbeSize); + prober.ProbeSent(now_ms, 1000); } - - EXPECT_GE(now_ms, kMinProbeDurationMs); - // Verify that the actual bitrate is withing 10% of the target. - double bitrate = kProbeSize * (kClusterSize - 1) * 8 * 1000.0 / now_ms; - EXPECT_GT(bitrate, kTestBitrate1 * 0.9); - EXPECT_LT(bitrate, kTestBitrate1 * 1.1); - - now_ms += prober.TimeUntilNextProbe(now_ms); - int64_t probe2_started = now_ms; - - for (int i = 0; i < kClusterSize; ++i) { - now_ms += prober.TimeUntilNextProbe(now_ms); + for (int i = 0; i < 5; ++i) { + EXPECT_EQ(4, prober.TimeUntilNextProbe(now_ms)); + now_ms += 4; EXPECT_EQ(0, prober.TimeUntilNextProbe(now_ms)); EXPECT_EQ(1, prober.CurrentClusterId()); - prober.ProbeSent(now_ms, kProbeSize); + prober.ProbeSent(now_ms, 1000); } - // Verify that the actual bitrate is withing 10% of the target. - int duration = now_ms - probe2_started; - EXPECT_GE(duration, kMinProbeDurationMs); - bitrate = kProbeSize * (kClusterSize - 1) * 8 * 1000.0 / duration; - EXPECT_GT(bitrate, kTestBitrate2 * 0.9); - EXPECT_LT(bitrate, kTestBitrate2 * 1.1); - EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms)); EXPECT_FALSE(prober.IsProbing()); } diff --git a/webrtc/modules/pacing/paced_sender_unittest.cc b/webrtc/modules/pacing/paced_sender_unittest.cc index cde5fd1feb..fc80a98813 100644 --- a/webrtc/modules/pacing/paced_sender_unittest.cc +++ b/webrtc/modules/pacing/paced_sender_unittest.cc @@ -828,7 +828,6 @@ TEST_F(PacedSenderTest, ProbingWithInsertedPackets) { kFirstClusterBps, kBitrateProbingError); EXPECT_EQ(0, packet_sender.padding_sent()); - clock_.AdvanceTimeMilliseconds(send_bucket_->TimeUntilNextProcess()); start = clock_.TimeInMilliseconds(); while (packet_sender.packets_sent() < 10) { int time_until_process = send_bucket_->TimeUntilNextProcess(); @@ -837,9 +836,9 @@ TEST_F(PacedSenderTest, ProbingWithInsertedPackets) { } packets_sent = packet_sender.packets_sent() - packets_sent; // Validate second cluster bitrate. - EXPECT_NEAR((packets_sent - 1) * kPacketSize * 8000 / - (clock_.TimeInMilliseconds() - start), - kSecondClusterBps, kBitrateProbingError); + EXPECT_NEAR( + packets_sent * kPacketSize * 8000 / (clock_.TimeInMilliseconds() - start), + kSecondClusterBps, kBitrateProbingError); } TEST_F(PacedSenderTest, ProbingWithPaddingSupport) {