Make OverheadObserver::OnOverheadChanged count RTP headers only

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 1170 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 total overhead (transport + rtp) from bitrate.
+    int rtp_overhead;
+    {
+      rtc::CritScope lock(&overhead_bytes_per_packet_crit_);
+      rtp_overhead = overhead_bytes_per_packet_;
+    }
+    RTC_CHECK_GE(rtp_overhead, 0);
+    RTC_DCHECK_LT(rtp_overhead, config_->rtp.max_packet_size);
+    if (static_cast<size_t>(rtp_overhead) >= config_->rtp.max_packet_size) {
+      bitrate_bps = 0;
+    } else {
+      bitrate_bps =
+          (uint64_t) bitrate_bps *

[stefan-webrtc, 2016/12/19 13:55:41]

static_cast please

[nisse-webrtc, 2016/12/19 14:41:04]

Done.

+          (config_->rtp.max_packet_size - rtp_overhead) /
+          (config_->rtp.max_packet_size + transport_overhead_bytes_per_packet_);
+    }
   }
 
   // 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_;
 
   encoder_target_rate_bps_ =
       std::min(encoder_max_bitrate_bps_, encoder_target_rate_bps_);
@@ skipping 53 matching lines @@
   overhead_bytes_per_packet_ = overhead_bytes_per_packet;
 }
 
 void VideoSendStreamImpl::SetTransportOverhead(
     int transport_overhead_bytes_per_packet) {
   transport_overhead_bytes_per_packet_ = transport_overhead_bytes_per_packet;
   RTC_DCHECK_LE(config_->rtp.max_packet_size,
                 0xFFFFu + transport_overhead_bytes_per_packet);
   const uint16_t mtu = static_cast<uint16_t>(
       config_->rtp.max_packet_size + transport_overhead_bytes_per_packet);
+  congestion_controller_->SetTransportOverhead(
+      transport_overhead_bytes_per_packet);
+
   for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) {
-    rtp_rtcp->SetTransportOverhead(transport_overhead_bytes_per_packet);
+    // TODO(nisse): Also call SetMaxPayloadLength?

[nisse-webrtc, 2016/12/19 12:37:23]

I don't quite get the notation here. The deleted SetTransportOverhead also
called SetMaxPayloadSize, as MaxPayloadLength() + (old transport overhead) -
(new transport overhead).

But why do we need both that and the MTU? It would make more sense to me to give
the RTP module tha maximum size of an RTP packet (i.e., including RTP headers
and any padding, but not transport headers).

Why does the code pass both MTU and MaxPayloadSize? The latter, is that
including RTP headers or not?

[stefan-webrtc, 2016/12/19 13:55:41]

This is probably legacy. We should clean it up the way we want it, and I agree
that the RTP code should know only about RTP header sizes if possible.

[michaelt, 2016/12/19 14:17:01]

Calling SetMaxTransferUnit here will be senseless. We can remove this code. But
we have to set mtu at the creation of the rtp_rtcp modules. After the Removal of
SetTransportOverhead in rtp_rtcp, "SetMaxTransferUnit" seams to be a misleading
name. How about "SetMaxRtpSize". And we may have to adapt/ simply the function a
little.

[nisse-webrtc, 2016/12/19 14:41:04]

I'll give it a try.

     rtp_rtcp->SetMaxTransferUnit(mtu);
   }
 }
 
 }  // namespace internal
 }  // namespace webrtc