diff --git a/webrtc/modules/pacing/bitrate_prober.cc b/webrtc/modules/pacing/bitrate_prober.cc index 51c87fb23..5475ef3f1 100644 --- a/webrtc/modules/pacing/bitrate_prober.cc +++ b/webrtc/modules/pacing/bitrate_prober.cc @@ -81,7 +81,7 @@ int BitrateProber::TimeUntilNextProbe(int64_t now_ms) { } if (probe_bitrates_.empty()) { // No probe started, or waiting for next probe. - return std::numeric_limits::max(); + return -1; } int64_t elapsed_time_ms = now_ms - time_last_send_ms_; // We will send the first probe packet immediately if no packet has been diff --git a/webrtc/modules/pacing/bitrate_prober_unittest.cc b/webrtc/modules/pacing/bitrate_prober_unittest.cc index fac6a7213..c966f5cfa 100644 --- a/webrtc/modules/pacing/bitrate_prober_unittest.cc +++ b/webrtc/modules/pacing/bitrate_prober_unittest.cc @@ -19,7 +19,7 @@ TEST(BitrateProberTest, VerifyStatesAndTimeBetweenProbes) { BitrateProber prober; EXPECT_FALSE(prober.IsProbing()); int64_t now_ms = 0; - EXPECT_EQ(std::numeric_limits::max(), prober.TimeUntilNextProbe(now_ms)); + EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms)); prober.SetEnabled(true); EXPECT_FALSE(prober.IsProbing()); @@ -45,7 +45,7 @@ TEST(BitrateProberTest, VerifyStatesAndTimeBetweenProbes) { prober.PacketSent(now_ms, 1000); } - EXPECT_EQ(std::numeric_limits::max(), prober.TimeUntilNextProbe(now_ms)); + EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms)); EXPECT_FALSE(prober.IsProbing()); } } // namespace webrtc diff --git a/webrtc/modules/pacing/include/paced_sender.h b/webrtc/modules/pacing/include/paced_sender.h index ab3ce7fd7..76d8b6038 100644 --- a/webrtc/modules/pacing/include/paced_sender.h +++ b/webrtc/modules/pacing/include/paced_sender.h @@ -87,6 +87,11 @@ class PacedSender : public Module { // Resume sending packets. void Resume(); + // Enable bitrate probing. Enabled by default, mostly here to simplify + // testing. Must be called before any packets are being sent to have an + // effect. + void SetProbingEnabled(bool enabled); + // Set target bitrates for the pacer. // We will pace out bursts of packets at a bitrate of |max_bitrate_kbps|. // |bitrate_kbps| is our estimate of what we are allowed to send on average. @@ -121,9 +126,6 @@ class PacedSender : public Module { // Process any pending packets in the queue(s). virtual int32_t Process() OVERRIDE; - protected: - virtual bool ProbingExperimentIsEnabled() const; - private: // Updates the number of bytes that can be sent for the next time interval. void UpdateBytesPerInterval(int64_t delta_time_in_ms) @@ -139,6 +141,7 @@ class PacedSender : public Module { scoped_ptr critsect_; bool enabled_ GUARDED_BY(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. scoped_ptr media_budget_ GUARDED_BY(critsect_); @@ -154,7 +157,7 @@ class PacedSender : public Module { int64_t time_last_update_us_ GUARDED_BY(critsect_); scoped_ptr packets_ GUARDED_BY(critsect_); - uint64_t packet_counter_ GUARDED_BY(critsect_); + uint64_t packet_counter_; }; } // namespace webrtc #endif // WEBRTC_MODULES_PACING_INCLUDE_PACED_SENDER_H_ diff --git a/webrtc/modules/pacing/paced_sender.cc b/webrtc/modules/pacing/paced_sender.cc index 4b9607439..6186f96c3 100644 --- a/webrtc/modules/pacing/paced_sender.cc +++ b/webrtc/modules/pacing/paced_sender.cc @@ -90,9 +90,9 @@ class PacketQueue { virtual ~PacketQueue() {} void Push(const Packet& packet) { - if (!AddToDupeSet(packet)) + if (!AddToDupeSet(packet)) { return; - + } // Store packet in list, use pointers in priority queue for cheaper moves. // Packets have a handle to its own iterator in the list, for easy removal // when popping from queue. @@ -215,6 +215,7 @@ PacedSender::PacedSender(Clock* clock, critsect_(CriticalSectionWrapper::CreateCriticalSection()), enabled_(true), paused_(false), + probing_enabled_(true), media_budget_(new paced_sender::IntervalBudget(max_bitrate_kbps)), padding_budget_(new paced_sender::IntervalBudget(min_bitrate_kbps)), prober_(new BitrateProber()), @@ -237,6 +238,11 @@ void PacedSender::Resume() { paused_ = false; } +void PacedSender::SetProbingEnabled(bool enabled) { + assert(packet_counter_ == 0); + probing_enabled_ = enabled; +} + void PacedSender::SetStatus(bool enable) { CriticalSectionScoped cs(critsect_.get()); enabled_ = enable; @@ -264,8 +270,7 @@ bool PacedSender::SendPacket(Priority priority, uint32_t ssrc, if (!enabled_) { return true; // We can send now. } - // Enable probing if the probing experiment is enabled. - if (!prober_->IsProbing() && ProbingExperimentIsEnabled()) { + if (probing_enabled_ && !prober_->IsProbing()) { prober_->SetEnabled(true); } prober_->MaybeInitializeProbe(bitrate_bps_); @@ -305,7 +310,10 @@ int64_t PacedSender::QueueInMs() const { int64_t PacedSender::TimeUntilNextProcess() { CriticalSectionScoped cs(critsect_.get()); if (prober_->IsProbing()) { - return prober_->TimeUntilNextProbe(clock_->TimeInMilliseconds()); + int64_t ret = prober_->TimeUntilNextProbe(clock_->TimeInMilliseconds()); + if (ret >= 0) { + return ret; + } } int64_t elapsed_time_us = clock_->TimeInMicroseconds() - time_last_update_us_; int64_t elapsed_time_ms = (elapsed_time_us + 500) / 1000; @@ -325,10 +333,10 @@ int32_t PacedSender::Process() { int64_t delta_time_ms = std::min(kMaxIntervalTimeMs, elapsed_time_ms); UpdateBytesPerInterval(delta_time_ms); } - while (!packets_->Empty()) { - if (media_budget_->bytes_remaining() <= 0 && !prober_->IsProbing()) + if (media_budget_->bytes_remaining() <= 0 && !prober_->IsProbing()) { return 0; + } // Since we need to release the lock in order to send, we first pop the // element from the priority queue but keep it in storage, so that we can @@ -337,8 +345,9 @@ int32_t PacedSender::Process() { if (SendPacket(packet)) { // Send succeeded, remove it from the queue. packets_->FinalizePop(packet); - if (prober_->IsProbing()) + if (prober_->IsProbing()) { return 0; + } } else { // Send failed, put it back into the queue. packets_->CancelPop(packet); @@ -386,9 +395,4 @@ void PacedSender::UpdateBytesPerInterval(int64_t delta_time_ms) { media_budget_->IncreaseBudget(delta_time_ms); padding_budget_->IncreaseBudget(delta_time_ms); } - -bool PacedSender::ProbingExperimentIsEnabled() const { - return webrtc::field_trial::FindFullName("WebRTC-BitrateProbing") == - "Enabled"; -} } // namespace webrtc diff --git a/webrtc/modules/pacing/paced_sender_unittest.cc b/webrtc/modules/pacing/paced_sender_unittest.cc index 4303b6474..29b856c67 100644 --- a/webrtc/modules/pacing/paced_sender_unittest.cc +++ b/webrtc/modules/pacing/paced_sender_unittest.cc @@ -108,6 +108,10 @@ class PacedSenderTest : public ::testing::Test { kTargetBitrate, kPaceMultiplier * kTargetBitrate, 0)); + // Default to bitrate probing disabled for testing purposes. Probing tests + // have to enable probing, either by creating a new PacedSender instance or + // by calling SetProbingEnabled(true). + send_bucket_->SetProbingEnabled(false); } void SendAndExpectPacket(PacedSender::Priority priority, @@ -418,6 +422,7 @@ TEST_F(PacedSenderTest, VerifyAverageBitrateVaryingMediaPayload) { PacedSenderPadding callback; send_bucket_.reset(new PacedSender( &clock_, &callback, kTargetBitrate, kPaceMultiplier * kTargetBitrate, 0)); + send_bucket_->SetProbingEnabled(false); send_bucket_->UpdateBitrate( kTargetBitrate, kPaceMultiplier * kTargetBitrate, kTargetBitrate); int64_t start_time = clock_.TimeInMilliseconds(); @@ -697,22 +702,6 @@ TEST_F(PacedSenderTest, QueueTimeGrowsOverTime) { EXPECT_EQ(0, send_bucket_->QueueInMs()); } -class ProbingPacedSender : public PacedSender { - public: - ProbingPacedSender(Clock* clock, - Callback* callback, - int bitrate_kbps, - int max_bitrate_kbps, - int min_bitrate_kbps) - : PacedSender(clock, - callback, - bitrate_kbps, - max_bitrate_kbps, - min_bitrate_kbps) {} - - virtual bool ProbingExperimentIsEnabled() const OVERRIDE { return true; } -}; - TEST_F(PacedSenderTest, ProbingWithInitialFrame) { const int kNumPackets = 11; const int kNumDeltas = kNumPackets - 1; @@ -725,12 +714,15 @@ TEST_F(PacedSenderTest, ProbingWithInitialFrame) { std::list expected_deltas_list(expected_deltas, expected_deltas + kNumPackets - 1); PacedSenderProbing callback(expected_deltas_list, &clock_); + // Probing implicitly enabled by creating a new PacedSender which defaults to + // probing on. send_bucket_.reset( - new ProbingPacedSender(&clock_, - &callback, - kInitialBitrateKbps, - kPaceMultiplier * kInitialBitrateKbps, - 0)); + new PacedSender(&clock_, + &callback, + kInitialBitrateKbps, + kPaceMultiplier * kInitialBitrateKbps, + 0)); + for (int i = 0; i < kNumPackets; ++i) { EXPECT_FALSE(send_bucket_->SendPacket(PacedSender::kNormalPriority, ssrc, diff --git a/webrtc/modules/remote_bitrate_estimator/test/packet_sender.h b/webrtc/modules/remote_bitrate_estimator/test/packet_sender.h index d5ea3e8b4..227be19e1 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/packet_sender.h +++ b/webrtc/modules/remote_bitrate_estimator/test/packet_sender.h @@ -85,27 +85,11 @@ class PacedVideoSender : public PacketSender, public PacedSender::Callback { int64_t rtt) OVERRIDE; private: - class ProbingPacedSender : public PacedSender { - public: - ProbingPacedSender(Clock* clock, - Callback* callback, - int bitrate_kbps, - int max_bitrate_kbps, - int min_bitrate_kbps) - : PacedSender(clock, - callback, - bitrate_kbps, - max_bitrate_kbps, - min_bitrate_kbps) {} - - virtual bool ProbingExperimentIsEnabled() const OVERRIDE { return true; } - }; - int64_t TimeUntilNextProcess(const std::list& modules); void CallProcess(const std::list& modules); void QueuePackets(Packets* batch, int64_t end_of_batch_time_us); - ProbingPacedSender pacer_; + PacedSender pacer_; Packets queue_; Packets pacer_queue_;