Bitrate prober now keep track of probing cluster id.

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 fbd9b8174193aa55b9e9d371bc87b476d196caf1..7baf716392cc46eaaf84145289b494c49e958913 100644
--- a/webrtc/modules/pacing/bitrate_prober.cc
+++ b/webrtc/modules/pacing/bitrate_prober.cc
@@ -33,7 +33,8 @@ int ComputeDeltaFromBitrate(size_t packet_size, int bitrate_bps) {
 BitrateProber::BitrateProber()
     : probing_state_(kDisabled),
       packet_size_last_send_(0),
-      time_last_send_ms_(-1) {
+      time_last_send_ms_(-1),
+      cluster_id_(0) {
 }
 
 void BitrateProber::SetEnabled(bool enable) {
@@ -61,24 +62,24 @@ void BitrateProber::OnIncomingPacket(int bitrate_bps,
     return;
   if (probing_state_ != kAllowedToProbe)
     return;
-  probe_bitrates_.clear();
   // Max number of packets used for probing.
   const int kMaxNumProbes = 2;
   const int kPacketsPerProbe = 5;
   const float kProbeBitrateMultipliers[kMaxNumProbes] = {3, 6};
-  int bitrates_bps[kMaxNumProbes];
   std::stringstream bitrate_log;
   bitrate_log << "Start probing for bandwidth, bitrates:";
   for (int i = 0; i < kMaxNumProbes; ++i) {
-    bitrates_bps[i] = kProbeBitrateMultipliers[i] * bitrate_bps;
-    bitrate_log << " " << bitrates_bps[i];
-    // We need one extra to get 5 deltas for the first probe.
-    if (i == 0)
-      probe_bitrates_.push_back(bitrates_bps[i]);
-    for (int j = 0; j < kPacketsPerProbe; ++j)
-      probe_bitrates_.push_back(bitrates_bps[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);

[stefan-webrtc, 2016/05/04 09:42:35]

I don't like adding a bool. Either (i==0 ? 1 : 0) or maybe even:
if (i == 0)
  ++cluster.max_probe_packets;

[philipel, 2016/05/04 10:36:32]

Done.

+    cluster.probe_bitrate = kProbeBitrateMultipliers[i] * bitrate_bps;
+    cluster.id = cluster_id_++;
+
+    bitrate_log << " " << cluster.probe_bitrate;
+    bitrate_log << ", num packets: " << cluster.max_probe_packets;
+
+    clusters_.push(cluster);
   }
-  bitrate_log << ", num packets: " << probe_bitrates_.size();
   LOG(LS_INFO) << bitrate_log.str().c_str();
   // Set last send time to current time so TimeUntilNextProbe doesn't short
   // circuit due to inactivity.
@@ -87,10 +88,11 @@ void BitrateProber::OnIncomingPacket(int bitrate_bps,
 }
 
 int BitrateProber::TimeUntilNextProbe(int64_t now_ms) {
-  if (probing_state_ != kDisabled && probe_bitrates_.empty()) {
+  if (probing_state_ != kDisabled && clusters_.empty()) {
     probing_state_ = kWait;
   }
-  if (probe_bitrates_.empty() || time_last_send_ms_ == -1) {
+
+  if (clusters_.empty() || time_last_send_ms_ == -1) {
     // No probe started, probe finished, or too long since last probe packet.
     return -1;
   }
@@ -108,7 +110,7 @@ int BitrateProber::TimeUntilNextProbe(int64_t now_ms) {
   int time_until_probe_ms = 0;
   if (packet_size_last_send_ != 0 && probing_state_ == kProbing) {
     int next_delta_ms = ComputeDeltaFromBitrate(packet_size_last_send_,
-                                                probe_bitrates_.front());
+                                         clusters_.front().probe_bitrate);

[stefan-webrtc, 2016/05/04 09:42:35]

git cl format

[philipel, 2016/05/04 10:36:32]

Done.

     time_until_probe_ms = next_delta_ms - elapsed_time_ms;
     // There is no point in trying to probe with less than 1 ms between packets
     // as it essentially means trying to probe at infinite bandwidth.
@@ -129,6 +131,12 @@ int BitrateProber::TimeUntilNextProbe(int64_t now_ms) {
   return std::max(time_until_probe_ms, 0);
 }
 
+int BitrateProber::CurrentClusterId() const {

[stefan-webrtc, 2016/05/04 09:42:35]

It's not clear to me how this method is going to be used since it returns -1
when all probes have been sent. Is it only for testing? In that case I prefer
writing the tests differently.

[philipel, 2016/05/04 10:36:32]

This method is suppose to be used to get the id of the cluster that the packet
that is about to be sent belongs to. Returning -1 means this packet does not
belong to a cluster, but maybe it should be DCHECKED instead since you should
never try to get the cluster id of a packet that has not been used for probing.

[stefan-webrtc, 2016/05/06 11:22:44]

Yes, add a DCHECK instead.

+  if (probing_state_ == kProbing && !clusters_.empty())
+    return clusters_.front().id;
+  return -1;
+}
+
 size_t BitrateProber::RecommendedPacketSize() const {
   return packet_size_last_send_;
 }
@@ -141,7 +149,11 @@ void BitrateProber::PacketSent(int64_t now_ms, size_t packet_size) {
   time_last_send_ms_ = now_ms;
   if (probing_state_ != kProbing)
     return;
-  if (!probe_bitrates_.empty())
-    probe_bitrates_.pop_front();
+  if (!clusters_.empty()) {
+    ProbeCluster& cluster = clusters_.front();

[stefan-webrtc, 2016/05/04 09:42:35]

Only const references allowed

[philipel, 2016/05/04 10:36:32]

Changed to |ProbeCluster*|.

+    ++cluster.sent_probe_packets;
+    if (cluster.sent_probe_packets == cluster.max_probe_packets)
+      clusters_.pop();
+  }
 }
 }  // namespace webrtc