Fix for video protection_bitrate in BWE with overhead.

webrtc/video/video_send_stream.cc

 /*
  *  Copyright (c) 2013 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/video/video_send_stream.h"
@@ skipping 258 matching lines @@
   } else if (pad_to_min_bitrate) {
     pad_up_to_bitrate_bps = streams[0].min_bitrate_bps;
   }
 
   pad_up_to_bitrate_bps =
       std::max(pad_up_to_bitrate_bps, min_transmit_bitrate_bps);
 
   return pad_up_to_bitrate_bps;
 }
 
+uint32_t CalculateOverheadRateBps(uint32_t bitrate_bps,

[minyue-webrtc, 2017/01/10 22:17:31]

not totally obvious to me that the bitrate_bps and packet_size_bytes can be
either
1. both are with overhead
or
2. both are without overhead

I think we may better change this function to

uint32_t CalculateOverheadRateBps(int packets_per_second, size_t
overhead_bytes_per_packet, uint32_t max_overhead_bps)

We may move "int packets_per_second = std::ceil(bitrate_bps / (8 *
packet_size_bytes));" outside the function, (Note that my other comment suggests
that the calculation may be wrong). You may move it to another utility function,
e.g., CalculatePacketRate()

[michaelt, 2017/01/11 08:55:15]

Done.

+                                  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));

[minyue-webrtc, 2017/01/10 22:17:31]

I don't ceil can do anything. it only makes sense when its argument is float.
you may want to do integer operations like.

(bitrate_bps + (8 * packet_size_bytes) - 1) / (8 * packet_size_bytes)

if you do it, add a comment that this is to do the ceil.

[michaelt, 2017/01/11 08:55:15]

Good catch. Thanks.

+  uint32_t overhead_bps =
+      static_cast<uint32_t>(8 * overhead_bytes_per_packet * packets_per_second);
+  return overhead_bps > max_overhead_bps ? max_overhead_bps : overhead_bps;

[minyue-webrtc, 2017/01/10 22:17:31]

I'd prefer std::min(xxx, xxx)

[michaelt, 2017/01/11 08:55:15]

Done.

+}
+
 }  // namespace
 
 namespace internal {
 
 // VideoSendStreamImpl implements internal::VideoSendStream.
 // It is created and destroyed on |worker_queue|. The intent is to decrease the
 // need for locking and to ensure methods are called in sequence.
 // Public methods except |DeliverRtcp| must be called on |worker_queue|.
 // DeliverRtcp is called on the libjingle worker thread or a network thread.
 // An encoder may deliver frames through the EncodedImageCallback on an
@@ skipping 890 matching lines @@
 }
 
 uint32_t VideoSendStreamImpl::OnBitrateUpdated(uint32_t bitrate_bps,
                                                uint8_t fraction_loss,
                                                int64_t rtt,
                                                int64_t probing_interval_ms) {
   RTC_DCHECK_RUN_ON(worker_queue_);
   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 encoder_overhead_rate_bps = CalculateOverheadRateBps(
+      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_);
+
+  // When the field trial "WebRTC-SendSideBwe-WithOverhead" is enabled
+  // protection_bitrate includes overhead.
+  uint32_t protection_bitrate =
+      bitrate_bps - (encoder_target_rate_bps_ + encoder_overhead_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_);
   return protection_bitrate;
 }
 
 void VideoSendStreamImpl::EnableEncodedFrameRecording(
     const std::vector<rtc::PlatformFile>& files,
@@ skipping 54 matching lines @@
   const uint16_t mtu = static_cast<uint16_t>(
       config_->rtp.max_packet_size + transport_overhead_bytes_per_packet);
   for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) {
     rtp_rtcp->SetTransportOverhead(transport_overhead_bytes_per_packet);
     rtp_rtcp->SetMaxTransferUnit(mtu);
   }
 }
 
 }  // namespace internal
 }  // namespace webrtc