Fix for video protection_bitrate in BWE with overhead.

webrtc/video/video_send_stream.cc

Index: webrtc/video/video_send_stream.cc
diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc
index 9a3b51e278a0c4f69608e9ea5966f9b82e7f76d2..39711b1f35284e54b7dc018e2ad15618577a114e 100644
--- a/webrtc/video/video_send_stream.cc
+++ b/webrtc/video/video_send_stream.cc
@@ -276,6 +276,19 @@ int CalculateMaxPadBitrateBps(std::vector<VideoStream> streams,
   return pad_up_to_bitrate_bps;
 }
 
+uint32_t CalculateOverheadRateBps(uint32_t bitrate_bps,
+                                  size_t packet_size_bytes,
+                                  size_t overhead_bytes_per_packet,
+                                  uint32_t max_overhead_bps) {
+  if (webrtc::field_trial::FindFullName("WebRTC-SendSideBwe-WithOverhead") !=
+      "Enabled")
+    return 0;
+  int packets_per_second = std::ceil(bitrate_bps / (8 * packet_size_bytes));
+  uint32_t overhead_bps =
+      static_cast<uint32_t>(8 * overhead_bytes_per_packet * packets_per_second);
+  return overhead_bps > max_overhead_bps ? 0 : overhead_bps;

[stefan-webrtc, 2016/12/20 15:25:12]

Why do we have to return 0 if the overhead is greater than some max overhead?
Seems like returning max_overhead_bps would make more sense in that case.

[michaelt, 2016/12/22 10:32:59]

True, that would make more sense :)

+}
+
 }  // namespace
 
 namespace internal {
@@ -1186,28 +1199,33 @@ uint32_t VideoSendStreamImpl::OnBitrateUpdated(uint32_t bitrate_bps,
   RTC_DCHECK(payload_router_.IsActive())
       << "VideoSendStream::Start has not been called.";
 
-  if (webrtc::field_trial::FindFullName("WebRTC-SendSideBwe-WithOverhead") ==
-      "Enabled") {
-    // Substract overhead from bitrate.
-    rtc::CritScope lock(&overhead_bytes_per_packet_crit_);
-    int packets_per_second =
-        std::ceil(bitrate_bps / (8 * (config_->rtp.max_packet_size +
-                                      transport_overhead_bytes_per_packet_)));
-    uint32_t overhead_bps = static_cast<uint32_t>(
-        8 * overhead_bytes_per_packet_ * packets_per_second);
-    bitrate_bps = overhead_bps > bitrate_bps ? 0u : bitrate_bps - overhead_bps;
-  }
+  // Substract overhead from bitrate.
+  rtc::CritScope lock(&overhead_bytes_per_packet_crit_);
+  uint32_t payload_bitrate_bps =
+      bitrate_bps - CalculateOverheadRateBps(
+                        bitrate_bps, config_->rtp.max_packet_size +
+                                         transport_overhead_bytes_per_packet_,
+                        overhead_bytes_per_packet_, bitrate_bps);
 
   // Get the encoder target rate. It is the estimated network rate -
   // protection overhead.
   encoder_target_rate_bps_ = protection_bitrate_calculator_.SetTargetRates(
-      bitrate_bps, stats_proxy_->GetSendFrameRate(), fraction_loss, rtt);
-  uint32_t protection_bitrate = bitrate_bps - encoder_target_rate_bps_;
+      payload_bitrate_bps, stats_proxy_->GetSendFrameRate(), fraction_loss,
+      rtt);
+  uint32_t protection_bitrate =
+      bitrate_bps -
+      (encoder_target_rate_bps_ +
+       CalculateOverheadRateBps(

[stefan-webrtc, 2016/12/20 15:25:12]

Put this on its own line and assign to temp so that we this becomes more
readable. Give it a good name, because currently I'm not sure what it is.

[michaelt, 2016/12/22 10:32:59]

Done.

+           encoder_target_rate_bps_,
+           config_->rtp.max_packet_size + transport_overhead_bytes_per_packet_ -
+               overhead_bytes_per_packet_,
+           overhead_bytes_per_packet_, bitrate_bps - encoder_target_rate_bps_));
 
   encoder_target_rate_bps_ =
       std::min(encoder_max_bitrate_bps_, encoder_target_rate_bps_);
   vie_encoder_->OnBitrateUpdated(encoder_target_rate_bps_, fraction_loss, rtt);
   stats_proxy_->OnSetEncoderTargetRate(encoder_target_rate_bps_);
+  // Add overhead to the protection_bitrate

[stefan-webrtc, 2016/12/20 15:25:12]

Is this a todo?

[michaelt, 2016/12/22 10:32:59]

Not removed the comment.

[michaelt, 2016/12/22 10:54:16]

Should be:
No, I removed the comment. 
:)

   return protection_bitrate;
 }