Enable bitrate probing by default in PacedSender.

BUG=crbug:425925
R=mflodman@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/33359004

Cr-Commit-Position: refs/heads/master@{#8379}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8379 4adac7df-926f-26a2-2b94-8c16560cd09d

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<int>::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

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<int>::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<int>::max(), prober.TimeUntilNextProbe(now_ms));
+  EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms));
   EXPECT_FALSE(prober.IsProbing());
 }
 }  // namespace webrtc

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<CriticalSectionWrapper> 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<paced_sender::IntervalBudget> media_budget_ GUARDED_BY(critsect_);
@@ -154,7 +157,7 @@ class PacedSender : public Module {
   int64_t time_last_update_us_ GUARDED_BY(critsect_);
 
   scoped_ptr<paced_sender::PacketQueue> 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_

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 (probing_enabled_ && !prober_->IsProbing()) {
-  if (!prober_->IsProbing() && ProbingExperimentIsEnabled()) {
     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

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<int> 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_,
+      new PacedSender(&clock_,
                       &callback,
                       kInitialBitrateKbps,
                       kPaceMultiplier * kInitialBitrateKbps,
                       0));
+
   for (int i = 0; i < kNumPackets; ++i) {
     EXPECT_FALSE(send_bucket_->SendPacket(PacedSender::kNormalPriority,
                                           ssrc,

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<Module*>& modules);
   void CallProcess(const std::list<Module*>& modules);
   void QueuePackets(Packets* batch, int64_t end_of_batch_time_us);
 
-  ProbingPacedSender pacer_;
+  PacedSender pacer_;
   Packets queue_;
   Packets pacer_queue_;