From 6e11efa6dced914e6f0927db3fcc92d875ec2cf5 Mon Sep 17 00:00:00 2001 From: Irfan Sheriff Date: Tue, 2 Aug 2016 12:57:37 -0700 Subject: [PATCH] Bitrate prober and paced sender improvements - Removes unnecessary casts to compute timedelta. - Renames ProbingState for clarity. This should help when we probe mid-call. - Enables probing by default to avoid checking on each incoming packet. - Removes duplicate probing state tracking in paced sender. These duplicate states were conflicting at times. - Removes passing through packets for bug 5307 which seems long fixed. - Cleanup handling of time_last_send_ms and avoid side effects of changing probing state at TimeUntilNextProbe(). - Clear cluster data when probing is restarted to avoid having old data after a reset. BUG=5859 R=stefan@webrtc.org Review URL: https://codereview.webrtc.org/2182603002 . Cr-Commit-Position: refs/heads/master@{#13612} --- webrtc/modules/pacing/bitrate_prober.cc | 94 ++++++++++--------- webrtc/modules/pacing/bitrate_prober.h | 21 ++++- .../modules/pacing/bitrate_prober_unittest.cc | 1 + webrtc/modules/pacing/paced_sender.cc | 6 +- webrtc/modules/pacing/paced_sender.h | 1 - 5 files changed, 72 insertions(+), 51 deletions(-) diff --git a/webrtc/modules/pacing/bitrate_prober.cc b/webrtc/modules/pacing/bitrate_prober.cc index 19eaff7a69..c5aa983266 100644 --- a/webrtc/modules/pacing/bitrate_prober.cc +++ b/webrtc/modules/pacing/bitrate_prober.cc @@ -14,6 +14,7 @@ #include #include #include +#include #include "webrtc/base/checks.h" #include "webrtc/base/logging.h" @@ -22,35 +23,40 @@ namespace webrtc { namespace { + +// Inactivity threshold above which probing is restarted. +constexpr int kInactivityThresholdMs = 5000; + int ComputeDeltaFromBitrate(size_t packet_size, uint32_t bitrate_bps) { assert(bitrate_bps > 0); // Compute the time delta needed to send packet_size bytes at bitrate_bps // bps. Result is in milliseconds. - return static_cast(1000ll * static_cast(packet_size) * 8ll / - bitrate_bps); + return static_cast((1000ll * packet_size * 8) / bitrate_bps); } } // namespace BitrateProber::BitrateProber() - : probing_state_(kDisabled), - packet_size_last_send_(0), - time_last_send_ms_(-1), - next_cluster_id_(0) {} + : probing_state_(ProbingState::kDisabled), + packet_size_last_sent_(0), + time_last_probe_sent_ms_(-1), + next_cluster_id_(0) { + SetEnabled(true); +} void BitrateProber::SetEnabled(bool enable) { if (enable) { - if (probing_state_ == kDisabled) { - probing_state_ = kAllowedToProbe; - LOG(LS_INFO) << "Initial bandwidth probing enabled"; + if (probing_state_ == ProbingState::kDisabled) { + probing_state_ = ProbingState::kInactive; + LOG(LS_INFO) << "Bandwidth probing enabled, set to inactive"; } } else { - probing_state_ = kDisabled; - LOG(LS_INFO) << "Initial bandwidth probing disabled"; + probing_state_ = ProbingState::kDisabled; + LOG(LS_INFO) << "Bandwidth probing disabled"; } } bool BitrateProber::IsProbing() const { - return probing_state_ == kProbing; + return probing_state_ == ProbingState::kActive; } void BitrateProber::OnIncomingPacket(uint32_t bitrate_bps, @@ -60,7 +66,7 @@ void BitrateProber::OnIncomingPacket(uint32_t bitrate_bps, // probing. if (packet_size < PacedSender::kMinProbePacketSize) return; - if (probing_state_ != kAllowedToProbe) + if (probing_state_ != ProbingState::kInactive) return; // Max number of packets used for probing. const int kMaxNumProbes = 2; @@ -81,36 +87,41 @@ void BitrateProber::OnIncomingPacket(uint32_t bitrate_bps, clusters_.push(cluster); } LOG(LS_INFO) << bitrate_log.str().c_str(); - // Set last send time to current time so TimeUntilNextProbe doesn't short - // circuit due to inactivity. - time_last_send_ms_ = now_ms; - probing_state_ = kProbing; + probing_state_ = ProbingState::kActive; +} + +void BitrateProber::ResetState() { + time_last_probe_sent_ms_ = -1; + packet_size_last_sent_ = 0; + clusters_ = std::queue(); + // If its enabled, reset to inactive. + if (probing_state_ != ProbingState::kDisabled) + probing_state_ = ProbingState::kInactive; } int BitrateProber::TimeUntilNextProbe(int64_t now_ms) { - if (probing_state_ != kDisabled && clusters_.empty()) { - probing_state_ = kWait; - } - - if (clusters_.empty() || time_last_send_ms_ == -1) { - // No probe started, probe finished, or too long since last probe packet. + // 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_; } - int64_t elapsed_time_ms = now_ms - time_last_send_ms_; - // If no packets have been sent for n milliseconds, temporarily deactivate to - // not keep spinning. - static const int kInactiveSendDeltaMs = 5000; - if (elapsed_time_ms > kInactiveSendDeltaMs) { - time_last_send_ms_ = -1; - probing_state_ = kAllowedToProbe; + // 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 (packet_size_last_send_ != 0 && probing_state_ == kProbing) { + if (packet_size_last_sent_ != 0 && probing_state_ == ProbingState::kActive) { int next_delta_ms = ComputeDeltaFromBitrate( - packet_size_last_send_, clusters_.front().probe_bitrate_bps); + packet_size_last_sent_, clusters_.front().probe_bitrate_bps); time_until_probe_ms = next_delta_ms - elapsed_time_ms; // There is no point in trying to probe with less than 1 ms between packets // as it essentially means trying to probe at infinite bandwidth. @@ -120,11 +131,8 @@ int BitrateProber::TimeUntilNextProbe(int64_t now_ms) { const int kMaxProbeDelayMs = 3; if (next_delta_ms < kMinProbeDeltaMs || time_until_probe_ms < -kMaxProbeDelayMs) { - // We currently disable probing after the first probe, as we only want - // to probe at the beginning of a connection. We should set this to - // kWait if we later want to probe periodically. - probing_state_ = kWait; - LOG(LS_INFO) << "Next delta too small, stop probing."; + probing_state_ = ProbingState::kSuspended; + LOG(LS_INFO) << "Delta too small or missed probing accurately, suspend"; time_until_probe_ms = 0; } } @@ -133,29 +141,29 @@ int BitrateProber::TimeUntilNextProbe(int64_t now_ms) { int BitrateProber::CurrentClusterId() const { RTC_DCHECK(!clusters_.empty()); - RTC_DCHECK_EQ(kProbing, probing_state_); + RTC_DCHECK(ProbingState::kActive == probing_state_); return clusters_.front().id; } size_t BitrateProber::RecommendedPacketSize() const { - return packet_size_last_send_; + return packet_size_last_sent_; } void BitrateProber::PacketSent(int64_t now_ms, size_t packet_size) { assert(packet_size > 0); if (packet_size < PacedSender::kMinProbePacketSize) return; - packet_size_last_send_ = packet_size; - time_last_send_ms_ = now_ms; - if (probing_state_ != kProbing) + packet_size_last_sent_ = packet_size; + if (probing_state_ != ProbingState::kActive) return; + time_last_probe_sent_ms_ = now_ms; if (!clusters_.empty()) { ProbeCluster* cluster = &clusters_.front(); ++cluster->sent_probe_packets; if (cluster->sent_probe_packets == cluster->max_probe_packets) clusters_.pop(); if (clusters_.empty()) - probing_state_ = kWait; + probing_state_ = ProbingState::kSuspended; } } } // namespace webrtc diff --git a/webrtc/modules/pacing/bitrate_prober.h b/webrtc/modules/pacing/bitrate_prober.h index e8967abde4..c2f9ad8c47 100644 --- a/webrtc/modules/pacing/bitrate_prober.h +++ b/webrtc/modules/pacing/bitrate_prober.h @@ -55,7 +55,18 @@ class BitrateProber { void PacketSent(int64_t now_ms, size_t packet_size); private: - enum ProbingState { kDisabled, kAllowedToProbe, kProbing, kWait }; + enum class ProbingState { + // Probing will not be triggered in this state at all times. + kDisabled, + // Probing is enabled and ready to trigger on the first packet arrival. + kInactive, + // Probe cluster is filled with the set of data rates to be probed and + // probes are being sent. + kActive, + // Probing is enabled, but currently suspended until an explicit trigger + // to start probing again. + kSuspended, + }; struct ProbeCluster { int max_probe_packets = 0; @@ -64,13 +75,17 @@ class BitrateProber { int id = -1; }; + // Resets the state of the prober and clears any cluster/timing data tracked. + void ResetState(); + 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_; - size_t packet_size_last_send_; - int64_t time_last_send_ms_; + size_t packet_size_last_sent_; + // The last time a probe was sent. + int64_t time_last_probe_sent_ms_; int next_cluster_id_; }; } // namespace webrtc diff --git a/webrtc/modules/pacing/bitrate_prober_unittest.cc b/webrtc/modules/pacing/bitrate_prober_unittest.cc index 9e38220e01..ce63781e68 100644 --- a/webrtc/modules/pacing/bitrate_prober_unittest.cc +++ b/webrtc/modules/pacing/bitrate_prober_unittest.cc @@ -65,6 +65,7 @@ TEST(BitrateProberTest, DoesntProbeWithoutRecentPackets) { prober.OnIncomingPacket(300000, 1000, now_ms); EXPECT_TRUE(prober.IsProbing()); EXPECT_EQ(0, prober.TimeUntilNextProbe(now_ms)); + prober.PacketSent(now_ms, 1000); // Let time pass, no large enough packets put into prober. now_ms += 6000; EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms)); diff --git a/webrtc/modules/pacing/paced_sender.cc b/webrtc/modules/pacing/paced_sender.cc index 33f2e23577..a3cb69f2e1 100644 --- a/webrtc/modules/pacing/paced_sender.cc +++ b/webrtc/modules/pacing/paced_sender.cc @@ -250,7 +250,6 @@ PacedSender::PacedSender(Clock* clock, PacketSender* packet_sender) packet_sender_(packet_sender), critsect_(CriticalSectionWrapper::CreateCriticalSection()), paused_(false), - probing_enabled_(true), media_budget_(new paced_sender::IntervalBudget(0)), padding_budget_(new paced_sender::IntervalBudget(0)), prober_(new BitrateProber()), @@ -280,7 +279,8 @@ void PacedSender::Resume() { void PacedSender::SetProbingEnabled(bool enabled) { RTC_CHECK_EQ(0u, packet_counter_); - probing_enabled_ = enabled; + CriticalSectionScoped cs(critsect_.get()); + prober_->SetEnabled(enabled); } void PacedSender::SetEstimatedBitrate(uint32_t bitrate_bps) { @@ -317,8 +317,6 @@ void PacedSender::InsertPacket(RtpPacketSender::Priority priority, RTC_DCHECK(estimated_bitrate_bps_ > 0) << "SetEstimatedBitrate must be called before InsertPacket."; - if (probing_enabled_ && !prober_->IsProbing()) - prober_->SetEnabled(true); int64_t now_ms = clock_->TimeInMilliseconds(); prober_->OnIncomingPacket(estimated_bitrate_bps_, bytes, now_ms); diff --git a/webrtc/modules/pacing/paced_sender.h b/webrtc/modules/pacing/paced_sender.h index 1d9f2de261..c904d4b6b7 100644 --- a/webrtc/modules/pacing/paced_sender.h +++ b/webrtc/modules/pacing/paced_sender.h @@ -143,7 +143,6 @@ class PacedSender : public Module, public RtpPacketSender { std::unique_ptr critsect_; bool paused_ GUARDED_BY(critsect_); - bool probing_enabled_; // This is the media budget, keeping track of how many bits of media // we can pace out during the current interval. std::unique_ptr media_budget_