Added ProbeBitrate(bitrate_bps, num_probes) to BitrateProber.

webrtc/modules/pacing/bitrate_prober.cc

Index: webrtc/modules/pacing/bitrate_prober.cc
diff --git a/webrtc/modules/pacing/bitrate_prober.cc b/webrtc/modules/pacing/bitrate_prober.cc
index c5aa983266c4c923b7189675a68fadfbff5e508d..fb396efa422ed05e74ea0c6ebe77aef0e7489485 100644
--- a/webrtc/modules/pacing/bitrate_prober.cc
+++ b/webrtc/modules/pacing/bitrate_prober.cc
@@ -10,11 +10,7 @@
 
 #include "webrtc/modules/pacing/bitrate_prober.h"
 
-#include <assert.h>
 #include <algorithm>
-#include <limits>
-#include <sstream>
-#include <utility>
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
@@ -28,7 +24,7 @@ namespace {
 constexpr int kInactivityThresholdMs = 5000;
 
 int ComputeDeltaFromBitrate(size_t packet_size, uint32_t bitrate_bps) {
-  assert(bitrate_bps > 0);
+  RTC_CHECK_GT(bitrate_bps, 0u);
   // Compute the time delta needed to send packet_size bytes at bitrate_bps
   // bps. Result is in milliseconds.
   return static_cast<int>((1000ll * packet_size * 8) / bitrate_bps);
@@ -59,41 +55,37 @@ bool BitrateProber::IsProbing() const {
   return probing_state_ == ProbingState::kActive;
 }
 
-void BitrateProber::OnIncomingPacket(uint32_t bitrate_bps,
-                                     size_t packet_size,
-                                     int64_t now_ms) {
+void BitrateProber::OnIncomingPacket(size_t packet_size) {

[philipel, 2016/08/12 12:53:44]

Instead of creating clusters and trigger probing (setting state to active) we
now use ProbeBitrate/2 to first setup clusters and then this function to trigger
when we have large enough packets.

   // Don't initialize probing unless we have something large enough to start
   // probing.
-  if (packet_size < PacedSender::kMinProbePacketSize)
-    return;
-  if (probing_state_ != ProbingState::kInactive)
-    return;
-  // Max number of packets used for probing.
-  const int kMaxNumProbes = 2;
-  const int kPacketsPerProbe = 5;
-  const float kProbeBitrateMultipliers[kMaxNumProbes] = {3, 6};
-  std::stringstream bitrate_log;
-  bitrate_log << "Start probing for bandwidth, (bitrate:packets): ";
-  for (int i = 0; i < kMaxNumProbes; ++i) {
-    ProbeCluster cluster;
-    // We need one extra to get 5 deltas for the first probe, therefore (i == 0)
-    cluster.max_probe_packets = kPacketsPerProbe + (i == 0 ? 1 : 0);
-    cluster.probe_bitrate_bps = kProbeBitrateMultipliers[i] * bitrate_bps;
-    cluster.id = next_cluster_id_++;
-
-    bitrate_log << "(" << cluster.probe_bitrate_bps << ":"
-                << cluster.max_probe_packets << ") ";
-
-    clusters_.push(cluster);
+  if (probing_state_ == ProbingState::kInactive &&
+      packet_size >= PacedSender::kMinProbePacketSize) {
+    probing_state_ = ProbingState::kActive;
   }
-  LOG(LS_INFO) << bitrate_log.str().c_str();
-  probing_state_ = ProbingState::kActive;
+}
+
+void BitrateProber::ProbeBitrate(uint32_t bitrate_bps, int num_packets) {
+  ProbeCluster cluster;
+  cluster.max_probe_packets = num_packets;
+  cluster.probe_bitrate_bps = bitrate_bps;
+  cluster.id = next_cluster_id_++;
+  clusters_.push_back(cluster);
+  LOG(LS_INFO) << "Probe cluster (bitrate:packets): ("
+               << cluster.probe_bitrate_bps << ":" << cluster.max_probe_packets
+               << ") ";
+  if (probing_state_ != ProbingState::kActive)

[danilchap, 2016/08/12 15:13:34]

wouldn't that activate probing even if it is in ProbingState::kDisable state?
The comment in header file state probing wouldn't be activate from Disable
state, only from Suspended.
Can you ensure comments are behavior match.

[Irfan, 2016/08/12 15:26:06]

There should probably be an RTC_DCHECK() that ensures this function does not get
called in disabled state.

With exponential probing, where we track the state when probing needs to be
initiated at paced_sender, I do refactor this to have control over when probing
starts.

+    probing_state_ = ProbingState::kInactive;
 }
 
 void BitrateProber::ResetState() {
   time_last_probe_sent_ms_ = -1;
   packet_size_last_sent_ = 0;
-  clusters_ = std::queue<ProbeCluster>();
+
+  // Reset all queued probing clusters.

[stefan-webrtc, 2016/08/12 13:19:58]

What does resetting mean? It seems odd to me that we keep the same number of
clusters, but set new ids. Why not simply removing them and have the user create
new probes if he wants?

[philipel, 2016/08/12 14:08:06]

Now use ProbeAtBitrate instead to recreate the probing clusters.

+  for (auto& cluster : clusters_) {
+    cluster.id = next_cluster_id_++;
+    cluster.sent_probe_packets = 0;
+  }

[philipel, 2016/08/12 12:53:44]

In the unittest BitrateProberTest.DoesntProbeWithoutRecentPackets we test that
we retry the probing if there is a too large gap in between sent packets.

[stefan-webrtc, 2016/08/12 13:19:58]

If this is the answer to my question above, then maybe we at least need to
comment on it.

[philipel, 2016/08/12 14:08:06]

Yes, the current behavior is to retry probing if we reset.

   // If its enabled, reset to inactive.
   if (probing_state_ != ProbingState::kDisabled)
     probing_state_ = ProbingState::kInactive;
@@ -161,7 +153,7 @@ void BitrateProber::PacketSent(int64_t now_ms, size_t packet_size) {
     ProbeCluster* cluster = &clusters_.front();
     ++cluster->sent_probe_packets;
     if (cluster->sent_probe_packets == cluster->max_probe_packets)
-      clusters_.pop();
+      clusters_.pop_front();
     if (clusters_.empty())
       probing_state_ = ProbingState::kSuspended;
   }