Probing: Add support for exponential startup probing

webrtc/modules/pacing/paced_sender.cc

Index: webrtc/modules/pacing/paced_sender.cc
diff --git a/webrtc/modules/pacing/paced_sender.cc b/webrtc/modules/pacing/paced_sender.cc
index a3cb69f2e14131a123d2924bdfe3e99577a10f54..86287280ba8eda35b56c63bd2abebde231e75dd1 100644
--- a/webrtc/modules/pacing/paced_sender.cc
+++ b/webrtc/modules/pacing/paced_sender.cc
@@ -32,6 +32,10 @@ const int64_t kMinPacketLimitMs = 5;
 // time.
 const int64_t kMaxIntervalTimeMs = 30;
 
+// Maximum waiting time from the time of sending first probe to getting
+// the measured results back.
+const int64_t kMaxTimeWaitingForProbingResultMs = 1000;
+
 }  // namespace
 
 // TODO(sprang): Move at least PacketQueue and MediaBudget out to separate
@@ -259,7 +263,8 @@ PacedSender::PacedSender(Clock* clock, PacketSender* packet_sender)
       pacing_bitrate_kbps_(0),
       time_last_update_us_(clock->TimeInMicroseconds()),
       packets_(new paced_sender::PacketQueue(clock)),
-      packet_counter_(0) {
+      packet_counter_(0),
+      pacing_state_(State::kInit) {
   UpdateBytesPerInterval(kMinPacketLimitMs);
 }
 
@@ -283,7 +288,7 @@ void PacedSender::SetProbingEnabled(bool enabled) {
   prober_->SetEnabled(enabled);
 }
 
-void PacedSender::SetEstimatedBitrate(uint32_t bitrate_bps) {
+void PacedSender::SetEstimatedBitrate(int bitrate_bps) {
   if (bitrate_bps == 0)
     LOG(LS_ERROR) << "PacedSender is not designed to handle 0 bitrate.";
   CriticalSectionScoped cs(critsect_.get());
@@ -318,7 +323,50 @@ void PacedSender::InsertPacket(RtpPacketSender::Priority priority,
         << "SetEstimatedBitrate must be called before InsertPacket.";
 
   int64_t now_ms = clock_->TimeInMilliseconds();
-  prober_->OnIncomingPacket(estimated_bitrate_bps_, bytes, now_ms);
+
+  if (bytes >= PacedSender::kMinProbePacketSize) {
+    switch (pacing_state_) {
+      case State::kInit: {
+        int multipliers[] = {3, 6};
+        if (!prober_->InitiateProbing(estimated_bitrate_bps_, multipliers, 2)) {
+          LOG(LS_ERROR) << "kInit: Failed to initiate probing";
+          pacing_state_ = State::kNormal;
+        } else {
+          LOG(LS_INFO) << "kInit: entering kWaitForProbingResult";
+          pacing_state_ = State::kWaitForProbingResult;
+          start_time_wait_probe_result_ms_ = clock_->TimeInMilliseconds();
+          // When probing at 1.8 Mbps, this represents a threshold of
+          // 1.2 Mbps to continue probing.
+          min_bitrate_to_probe_further_ = 4 * estimated_bitrate_bps_;

[stefan-webrtc, 2016/08/16 11:27:04]

What is different between being here and in kStartupProbing? Here we have a
factor of 4 while in startup we have a factor of 1.25?

[Irfan, 2016/08/16 18:12:47]

The multiplers are 3 and 6 on the very first time (if you recall, we kept this
to avoid slowing down ramp up). so we place a requirement of 4.

Afterwards, the multiplier is 2 and we keep a requirement of 1.25.

+        }
+      } break;
+      case State::kStartupProbing: {
+        int multipliers[] = {2};
+        if (!prober_->InitiateProbing(estimated_bitrate_bps_, multipliers, 1)) {
+          LOG(LS_ERROR) << "kStartupProbing: Failed to initiate probing";
+          pacing_state_ = State::kNormal;
+        } else {
+          LOG(LS_INFO) << "kStartupProbing: entering kWaitForProbingResult";
+          // A minimum of 25% gain to continue.
+          pacing_state_ = State::kWaitForProbingResult;
+          start_time_wait_probe_result_ms_ = clock_->TimeInMilliseconds();
+          min_bitrate_to_probe_further_ = 1.25 * estimated_bitrate_bps_;
+        }
+      } break;
+      case State::kWaitForProbingResult:
+        if ((clock_->TimeInMilliseconds() - start_time_wait_probe_result_ms_) >
+            kMaxTimeWaitingForProbingResultMs) {
+          pacing_state_ = State::kNormal;
+          LOG(LS_INFO) << "kWaitForProbingResult: timeout";
+        }
+        break;
+      case State::kNormal:
+        LOG(LS_INFO) << "Probing complete... kNormal";
+        break;
+      default:
+        RTC_NOTREACHED();
+    }
+  }

[stefan-webrtc, 2016/08/16 11:27:04]

Isn't this adding a lot of complexity to paced_sender.cc when it could as well
be in birtate_prober.cc?

[Irfan, 2016/08/16 18:12:47]

I will evaluate a way to simplify paced_sender and see if it works withm moving
to bitrate_prober.

 
   if (capture_time_ms < 0)
     capture_time_ms = now_ms;
@@ -330,7 +378,7 @@ void PacedSender::InsertPacket(RtpPacketSender::Priority priority,
 
 int64_t PacedSender::ExpectedQueueTimeMs() const {
   CriticalSectionScoped cs(critsect_.get());
-  RTC_DCHECK_GT(pacing_bitrate_kbps_, 0u);
+  RTC_DCHECK_GT(pacing_bitrate_kbps_, 0);
   return static_cast<int64_t>(packets_->SizeInBytes() * 8 /
                               pacing_bitrate_kbps_);
 }
@@ -368,6 +416,29 @@ int64_t PacedSender::TimeUntilNextProcess() {
   return std::max<int64_t>(kMinPacketLimitMs - elapsed_time_ms, 0);
 }
 
+PacedSender::State PacedSender::PacingState() {
+  CriticalSectionScoped(critsect_.get());
+  return pacing_state_;
+}
+
+void PacedSender::OnProbingBitrateMeasured(int bitrate_bps) {
+  CriticalSectionScoped cs(critsect_.get());
+  if (pacing_state_ == State::kWaitForProbingResult) {
+    LOG(LS_INFO) << "kWaitForProbingResult OnProbingBitrateMeasured "
+                 << bitrate_bps << " min_bitrate_to_probe_further_  "
+                 << min_bitrate_to_probe_further_
+                 << " bitrate_below_expected_count_ "
+                 << bitrate_below_expected_count_;
+    if (bitrate_bps > min_bitrate_to_probe_further_) {
+      pacing_state_ = State::kStartupProbing;
+    }
+    if (bitrate_bps > estimated_bitrate_bps_)
+      estimated_bitrate_bps_ = bitrate_bps;

[stefan-webrtc, 2016/08/16 11:27:04]

I'm not sure this is the right way to set this. It will also be set via
PacedSender::SetEstimatedBitrate(int bitrate_bps), and that estimate will
possibly be different as it takes lost packets into account. Can we have a
single way of setting the estimated bitrate to PacedSender?

[Irfan, 2016/08/16 18:12:47]

This is a good point. The reason its happening here now is to sync up on bitrate
the moment pacing state is updated.

I will see if we can get rid of duplicate setting of bitrate.

+  } else {
+    LOG(LS_WARNING) << "OnProbingBitrateMeasured while not waiting on results";

[stefan-webrtc, 2016/08/16 11:27:04]

Should we simply DCHECK on this?

[Irfan, 2016/08/16 18:12:47]

Done.

+  }
+}

[stefan-webrtc, 2016/08/16 11:27:04]

To keep the pacer simpler, I think it would be nice to move this into
bitrate_prober.cc. WDYT?

[Irfan, 2016/08/16 18:12:47]

Right, will evaluate simplifiying pacer

+
 void PacedSender::Process() {
   int64_t now_us = clock_->TimeInMicroseconds();
   CriticalSectionScoped cs(critsect_.get());
@@ -453,7 +524,13 @@ bool PacedSender::SendPacket(const paced_sender::Packet& packet,
   critsect_->Enter();
 
   if (success) {
-    prober_->PacketSent(clock_->TimeInMilliseconds(), packet.bytes);
+    // Is this a probe ?
+    if (probe_cluster_id != PacketInfo::kNotAProbe) {

[stefan-webrtc, 2016/08/16 11:27:04]

Why only notify about sent probe packets?

[Irfan, 2016/08/16 18:12:47]

I clarified this in an earlier reply. Let me know if I have missed something
here.

+      if (prober_->PacketSent(clock_->TimeInMilliseconds(), packet.bytes) ==
+          BitrateProber::State::kComplete) {
+        LOG(LS_INFO) << "Probe complete";
+      }
+    }
     // TODO(holmer): High priority packets should only be accounted for if we
     // are allocating bandwidth for audio.
     if (packet.priority != kHighPriority) {
@@ -473,7 +550,13 @@ void PacedSender::SendPadding(size_t padding_needed, int probe_cluster_id) {
   critsect_->Enter();
 
   if (bytes_sent > 0) {
-    prober_->PacketSent(clock_->TimeInMilliseconds(), bytes_sent);
+    // Is this a probe ?
+    if (probe_cluster_id != PacketInfo::kNotAProbe) {
+      if (prober_->PacketSent(clock_->TimeInMilliseconds(), bytes_sent) ==
+          BitrateProber::State::kComplete) {
+        LOG(LS_INFO) << "Probe complete";
+      }
+    }
     media_budget_->UseBudget(bytes_sent);
     padding_budget_->UseBudget(bytes_sent);
   }