Added ProbeBitrate(bitrate_bps, num_probes) to BitrateProber.

webrtc/modules/pacing/bitrate_prober.cc

 /*
  *  Copyright (c) 2014 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #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"
 #include "webrtc/modules/pacing/paced_sender.h"
 
 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);
+  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);
 }
 }  // namespace
 
 BitrateProber::BitrateProber()
     : probing_state_(ProbingState::kDisabled),
       packet_size_last_sent_(0),
       time_last_probe_sent_ms_(-1),
@@ skipping 10 matching lines @@
   } else {
     probing_state_ = ProbingState::kDisabled;
     LOG(LS_INFO) << "Bandwidth probing disabled";
   }
 }
 
 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_++;
+  if (probing_state_ == ProbingState::kInactive &&
+      packet_size >= PacedSender::kMinProbePacketSize) {
+    probing_state_ = ProbingState::kActive;
+  }
+}
 
-    bitrate_log << "(" << cluster.probe_bitrate_bps << ":"
-                << cluster.max_probe_packets << ") ";
-
-    clusters_.push(cluster);
-  }
-  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;
 }
 
 int BitrateProber::TimeUntilNextProbe(int64_t now_ms) {
   // 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.
@@ skipping 47 matching lines @@
   if (packet_size < PacedSender::kMinProbePacketSize)
     return;
   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();
+      clusters_.pop_front();
     if (clusters_.empty())
       probing_state_ = ProbingState::kSuspended;
   }
 }
 }  // namespace webrtc