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_->RecommendedProbeSize();
+  }
   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)
+      if (is_probing && bytes_sent > recommended_probe_size) {

[stefan-webrtc, 2016/09/16 07:42:53]

This may look like a bug to the reader as it seems like we are sending packets
in the middle of a probe which are not part of the probe. It's by design because
we may have small audio packets that must be sent, but maybe we should say that
in a comment?

[Irfan, 2016/09/19 06:05:11]

Not sure I follow. The is_probing flag indicates whether we are in a probing
state or not. The check here is for ensuring we send enough probe packets before
we return.

Can you elaborate what comment what you would like to see here ?

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

I'm thinking of the case where we have paced out, e.g., an audio packet which is
smaller than recommended_probe_size. In that case we're not reporting it as a
ProbeSent, which looks odd if we don't explain with a comment.

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

We report once the aggregate bytes is above the recommended size. In the latest
PS, this should be more clear that we always report the byte size in the end
before returning from Process(). 

If there is an audio packet here, it will actually add up in this aggregate of
bytes_sent and be reported to be prober.

+        prober_->ProbeSent(clock_->TimeInMilliseconds(), bytes_sent);
         return;
+      }
     } else {
       // Send failed, put it back into the queue.
       packets_->CancelPop(packet);
       return;
     }
   }
 
   RTC_DCHECK(packets_->Empty());
   // TODO(holmer): Remove the paused_ check when issue 5307 has been fixed.
   if (paused_)
     return;
 
   // 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()
+    size_t padding_needed = is_probing ? (recommended_probe_size - bytes_sent)
                                        : padding_budget_->bytes_remaining();
 
     if (padding_needed > 0)
-      SendPadding(padding_needed, probe_cluster_id);
+      bytes_sent += SendPadding(padding_needed, probe_cluster_id);
   }
+  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