BitrateProber: Support higher probing bitrates

webrtc/modules/pacing/paced_sender.cc

 /*
  *  Copyright (c) 2012 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.
  */
 
@@ skipping 384 matching lines @@
         target_bitrate_kbps = min_bitrate_needed_kbps;
     }
 
     media_budget_->set_target_rate_kbps(target_bitrate_kbps);
 
     int64_t delta_time_ms = std::min(kMaxIntervalTimeMs, elapsed_time_ms);
     UpdateBytesPerInterval(delta_time_ms);
   }
 
   bool is_probing = prober_->IsProbing();
-  int probe_cluster_id = is_probing ? prober_->CurrentClusterId()
-                                    : PacketInfo::kNotAProbe;
+  int probe_cluster_id = PacketInfo::kNotAProbe;
+  size_t bytes_sent = 0;
+  size_t recommended_probe_size = 0;
+  if (is_probing) {
+    probe_cluster_id = prober_->CurrentClusterId();
+    recommended_probe_size = prober_->RecommendedMinProbeSize();
+  }
   while (!packets_->Empty()) {
     // 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
     // reinsert it if send fails.
     const paced_sender::Packet& packet = packets_->BeginPop();
 
     if (SendPacket(packet, probe_cluster_id)) {
       // Send succeeded, remove it from the queue.
+      bytes_sent += packet.bytes;
       packets_->FinalizePop(packet);
-      if (is_probing)
-        return;
+      if (is_probing && bytes_sent > recommended_probe_size)
+        break;

[stefan-webrtc, 2016/09/19 07:24:56]

Is this because you want to be able to fill up with padding or retransmits
packets if this probe wasn't large enough?

[Irfan, 2016/09/19 18:23:30]

I restructured this to ensure there is a single call to ProbeSent() towards the
end. 

Basically, we had 3 different places where we would return and avoiding those
helps so we do not duplicate (or forget) reporting bytes sent. This also helps
further with a follow up CL where we report to AlrDetector.

     } else {
       // Send failed, put it back into the queue.
       packets_->CancelPop(packet);
-      return;
+      break;
     }
   }
 
-  RTC_DCHECK(packets_->Empty());
   // TODO(holmer): Remove the paused_ check when issue 5307 has been fixed.
-  if (paused_)
-    return;
+  if (packets_->Empty() && !paused_) {
+    // We can not send padding unless a normal packet has first been sent. If we
+    // do, timestamps get messed up.
+    if (packet_counter_ > 0) {
+      int padding_needed = is_probing ? (recommended_probe_size - bytes_sent)
+                                      : padding_budget_->bytes_remaining();
 
-  // We can not send padding unless a normal packet has first been sent. If we
-  // do, timestamps get messed up.
-  if (packet_counter_ > 0) {
-    size_t padding_needed = is_probing ? prober_->RecommendedPacketSize()
-                                       : padding_budget_->bytes_remaining();
-
-    if (padding_needed > 0)
-      SendPadding(padding_needed, probe_cluster_id);
+      if (padding_needed > 0)
+        bytes_sent += SendPadding(padding_needed, probe_cluster_id);

[stefan-webrtc, 2016/09/19 07:24:56]

I'm starting to think that we may want a higher level test which verifies that
when we do this type of probing (multiple packets per probe) we should ensure
that retransmits are used so we don't end up sending a large number of padding
packets. Maybe in video_send_stream_tests.cc?

[Irfan, 2016/09/19 18:23:30]

Yes, I did want to add a test once the CL looks good to you guys.

+    }
   }
+  if (is_probing)
+    prober_->ProbeSent(clock_->TimeInMilliseconds(), bytes_sent);
 }
 
 bool PacedSender::SendPacket(const paced_sender::Packet& packet,
                              int probe_cluster_id) {
   // TODO(holmer): Because of this bug issue 5307 we have to send audio
   // packets even when the pacer is paused. Here we assume audio packets are
   // always high priority and that they are the only high priority packets.
   if (packet.priority != kHighPriority) {
     if (paused_)
       return false;
     if (media_budget_->bytes_remaining() == 0 &&
         probe_cluster_id == PacketInfo::kNotAProbe) {
       return false;
     }
   }
   critsect_->Leave();
   const bool success = packet_sender_->TimeToSendPacket(
       packet.ssrc, packet.sequence_number, packet.capture_time_ms,
       packet.retransmission, probe_cluster_id);
   critsect_->Enter();
 
   if (success) {
-    prober_->PacketSent(clock_->TimeInMilliseconds(), packet.bytes);
     // TODO(holmer): High priority packets should only be accounted for if we
     // are allocating bandwidth for audio.
     if (packet.priority != kHighPriority) {
       // Update media bytes sent.
       media_budget_->UseBudget(packet.bytes);
       padding_budget_->UseBudget(packet.bytes);
     }
   }
 
   return success;
 }
 
-void PacedSender::SendPadding(size_t padding_needed, int probe_cluster_id) {
+size_t PacedSender::SendPadding(size_t padding_needed, int probe_cluster_id) {
   critsect_->Leave();
   size_t bytes_sent =
       packet_sender_->TimeToSendPadding(padding_needed, probe_cluster_id);
   critsect_->Enter();
 
   if (bytes_sent > 0) {
-    prober_->PacketSent(clock_->TimeInMilliseconds(), bytes_sent);
     media_budget_->UseBudget(bytes_sent);
     padding_budget_->UseBudget(bytes_sent);
   }
+  return bytes_sent;
 }
 
 void PacedSender::UpdateBytesPerInterval(int64_t delta_time_ms) {
   media_budget_->IncreaseBudget(delta_time_ms);
   padding_budget_->IncreaseBudget(delta_time_ms);
 }
 }  // namespace webrtc