Remove overhead from video bitrate.

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 51c9dad13160a4cbbc454e6576db27fb4274b3aa..da35f753a8ed1b1c0c50282c3a88ed83bacaee10 100644
--- a/webrtc/video/video_send_stream.cc
+++ b/webrtc/video/video_send_stream.cc
@@ -49,6 +49,7 @@ std::vector<RtpRtcp*> CreateRtpRtcpModules(
     SendDelayStats* send_delay_stats,
     RtcEventLog* event_log,
     RateLimiter* retransmission_rate_limiter,
+    OverheadObserver* overhead_observer,
     size_t num_modules) {
   RTC_DCHECK_GT(num_modules, 0u);
   RtpRtcp::Configuration configuration;
@@ -72,7 +73,7 @@ std::vector<RtpRtcp*> CreateRtpRtcpModules(
   configuration.send_packet_observer = send_delay_stats;
   configuration.event_log = event_log;
   configuration.retransmission_rate_limiter = retransmission_rate_limiter;
-
+  configuration.overhead_observer = overhead_observer;
   std::vector<RtpRtcp*> modules;
   for (size_t i = 0; i < num_modules; ++i) {
     RtpRtcp* rtp_rtcp = RtpRtcp::CreateRtpRtcp(configuration);
@@ -284,6 +285,7 @@ namespace internal {
 // An encoder may deliver frames through the EncodedImageCallback on an
 // arbitrary thread.
 class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver,
+                            public webrtc::OverheadObserver,
                             public webrtc::VCMProtectionCallback,
                             public ViEEncoder::EncoderSink {
  public:
@@ -337,6 +339,9 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver,
                         uint32_t* sent_nack_rate_bps,
                         uint32_t* sent_fec_rate_bps) override;
 
+  // Implements OverheadObserver.
+  void OnOverheadChanged(size_t overhead_bytes_per_packet) override;
+
   void OnEncoderConfigurationChanged(std::vector<VideoStream> streams,
                                      int min_transmit_bitrate_bps) override;
 
@@ -398,6 +403,9 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver,
   // |weak_ptr_factory_| must be declared last to make sure all WeakPtr's are
   // invalidated before any other members are destroyed.
   rtc::WeakPtrFactory<VideoSendStreamImpl> weak_ptr_factory_;
+
+  float payload_fraction_;
+  size_t transport_overhead_per_packet_;
 };
 
 // TODO(tommi): See if there's a more elegant way to create a task that creates
@@ -734,10 +742,13 @@ VideoSendStreamImpl::VideoSendStreamImpl(
           send_delay_stats,
           event_log,
           congestion_controller_->GetRetransmissionRateLimiter(),
+          // TODO(michaelt): Use here "this" when BWE with overhead implemented.
+          nullptr,
           config_->rtp.ssrcs.size())),
       payload_router_(rtp_rtcp_modules_,
                       config_->encoder_settings.payload_type),
-      weak_ptr_factory_(this) {
+      weak_ptr_factory_(this),
+      payload_fraction_(1) {
   RTC_DCHECK_RUN_ON(worker_queue_);
   LOG(LS_INFO) << "VideoSendStreamInternal: " << config_->ToString();
   weak_ptr_ = weak_ptr_factory_.GetWeakPtr();
@@ -774,15 +785,11 @@ VideoSendStreamImpl::VideoSendStreamImpl(
 
   // TODO(pbos): Should we set CNAME on all RTP modules?
   rtp_rtcp_modules_.front()->SetCNAME(config_->rtp.c_name.c_str());
-  // 28 to match packet overhead in ModuleRtpRtcpImpl.
-  static const size_t kRtpPacketSizeOverhead = 28;
-  RTC_DCHECK_LE(config_->rtp.max_packet_size, 0xFFFFu + kRtpPacketSizeOverhead);

[stefan-webrtc, 2016/11/22 17:17:01]

Is there still a reasonable check we should make on the max_packet_size?

[michaelt, 2016/11/23 09:19:13]

We could check for lower than IP_MTU(1500)

-  const uint16_t mtu = static_cast<uint16_t>(config_->rtp.max_packet_size +
-                                             kRtpPacketSizeOverhead);
+
   for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) {
     rtp_rtcp->RegisterRtcpStatisticsCallback(stats_proxy_);
     rtp_rtcp->RegisterSendChannelRtpStatisticsCallback(stats_proxy_);
-    rtp_rtcp->SetMaxTransferUnit(mtu);
+    rtp_rtcp->SetMaxTransferUnit(config_->rtp.max_packet_size);
     rtp_rtcp->RegisterVideoSendPayload(
         config_->encoder_settings.payload_type,
         config_->encoder_settings.payload_name.c_str());
@@ -1152,6 +1159,10 @@ uint32_t VideoSendStreamImpl::OnBitrateUpdated(uint32_t bitrate_bps,
   RTC_DCHECK_RUN_ON(worker_queue_);
   RTC_DCHECK(payload_router_.active())
       << "VideoSendStream::Start has not been called.";
+
+  // Substract overhead from bitrate.
+  bitrate_bps *= payload_fraction_;

[minyue-webrtc, 2016/11/23 10:48:05]

I think we need to create a global flag to make clear transition from
no-overhead era to overhead era.

I think of "BANDWITDH_INCLUDE_OVERHEAD"

[stefan-webrtc, 2016/11/23 11:08:11]

I don't think that's enough. We have to only subtract overhead if we're using
send-side BWE. Call and CongestionController knows about this at least.

Once we have added a check like that I think we can enable this either under a
field trial, so that it's only used when audio frame length can change. This is
the more cautious approach. The other option is to enable it by default as it
doesn't really depend on varying audio frame lengths. This however requires that
all code related to overhead changes is landed, ideally in one CL.

You could land this behind a flag as suggested if you want to land it in
separate CLs, but then I'd suggest using a field trial anyway so that it's
easier to test it.

[michaelt, 2016/11/23 12:00:24]

Would prefer a field trial as well. I even think we may should test this without
without changing frame length. So that we would be able the to detect a bugs in
the "BWE with overhead" impl. 

+
   // Get the encoder target rate. It is the estimated network rate -
   // protection overhead.
   encoder_target_rate_bps_ = protection_bitrate_calculator_.SetTargetRates(
@@ -1211,10 +1222,24 @@ int VideoSendStreamImpl::ProtectionRequest(
   return 0;
 }
 
+void VideoSendStreamImpl::OnOverheadChanged(size_t overhead_bytes_per_packet) {
+  RTC_DCHECK_RUN_ON(worker_queue_);
+  payload_fraction_ =
+      1.0 - (static_cast<float>(overhead_bytes_per_packet) /
+             (config_->rtp.max_packet_size + transport_overhead_per_packet_));

[stefan-webrtc, 2016/11/22 17:17:01]

This is an approximation assuming only max sized packets, right? I think it
would be good to improve it a bit since the overhead is a bit larger for low
bitrates (say 50 kbps)... Maybe do something like this in OnBitrateUpdated():
framerate = stats_proxy_->GetSendFrameRate();
bits_per_frame = bitrate_bps / framerate
packets_per_second = ceil(bits_per_frame / (8 * config_->rtp.max_packet_size)) *
framerate
overhead_per_second = 8 * overhead_bytes_per_packet * packets_per_second

I probably made some mistake, so please double check :)

[michaelt, 2016/11/23 09:19:13]

Yes I assumed only max packets.
I think we can simplify you calculation a little.
Framerate seams obsolete to me.
I would do:
packets_per_second = ceil(bitrate_bps / (8 * (config_->rtp.max_packet_size +
transport_overhead_per_packet_)));
overhead_per_second = 8 * overhead_bytes_per_packet * packets_per_second;

The ceil should make the difference to my old approach.

[stefan-webrtc, 2016/11/23 10:01:07]

Yes, you are probably right, the ceil is what's important.

+}
+
 void VideoSendStreamImpl::SetTransportOverhead(
     int transport_overhead_per_packet) {
-  for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_)
+  transport_overhead_per_packet_ = transport_overhead_per_packet;
+  RTC_DCHECK_LE(config_->rtp.max_packet_size,
+                0xFFFFu + transport_overhead_per_packet);
+  const uint16_t mtu = static_cast<uint16_t>(config_->rtp.max_packet_size +
+                                             transport_overhead_per_packet);
+  for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) {
     rtp_rtcp->SetTransportOverhead(transport_overhead_per_packet);
+    rtp_rtcp->SetMaxTransferUnit(mtu);
+  }
 }
 
 }  // namespace internal