Correcting the amount of padding when send side bwe includes RTP overhead.

webrtc/modules/rtp_rtcp/source/rtp_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 444 matching lines @@
   int bytes_left = static_cast<int>(bytes_to_send);
   while (bytes_left > 0) {
     std::unique_ptr<RtpPacketToSend> packet =
         packet_history_.GetBestFittingPacket(bytes_left);
     if (!packet)
       break;
     size_t payload_size = packet->payload_size();
     if (!PrepareAndSendPacket(std::move(packet), true, false, pacing_info))
       break;
     bytes_left -= payload_size;
+    if (send_side_bwe_with_overhead_) {
+      // When WebRTC-SendSideBwe-WithOverhead is enabled, the padding budget
+      // includes overhead.
+      bytes_left -= packet->headers_size();

[danilchap, 2017/03/24 08:47:20]

packet was moved, so you should not use it.
save the correct size before sending:
size_t bytes_used = send_side_bwe_with_overhead_ ? packet->size() :
packet->payload_size();
if (!PrepareAndSend(std::move(packet))
  break;
bytes_left -= bytes_used;

[minyue-webrtc, 2017/03/24 09:08:54]

Thanks! of course

+    }
   }
   return bytes_to_send - bytes_left;
 }
 
 size_t RTPSender::SendPadData(size_t bytes,
                               const PacedPacketInfo& pacing_info) {
   size_t padding_bytes_in_packet;
   if (audio_configured_) {
     // Allow smaller padding packets for audio.
     padding_bytes_in_packet =

[michaelt, 2017/03/27 14:25:01]

Shouldn't we calculate padding_bytes_in_packet inside the loop ?

         std::min(std::max(bytes, kMinAudioPaddingLength), MaxPayloadSize());
     if (padding_bytes_in_packet > kMaxPaddingLength)
       padding_bytes_in_packet = kMaxPaddingLength;
   } else {
     // Always send full padding packets. This is accounted for by the
     // RtpPacketSender, which will make sure we don't send too much padding even
     // if a single packet is larger than requested.
     // We do this to avoid frequently sending small packets on higher bitrates.
     padding_bytes_in_packet = std::min(MaxPayloadSize(), kMaxPaddingLength);
   }
@@ skipping 74 matching lines @@
     padding_packet.SetSsrc(ssrc);
 
     if (capture_time_ms > 0) {
       padding_packet.SetExtension<TransmissionOffset>(
           (now_ms - capture_time_ms) * kTimestampTicksPerMs);
     }
     padding_packet.SetExtension<AbsoluteSendTime>(now_ms);
     PacketOptions options;
     bool has_transport_seq_num =
         UpdateTransportSequenceNumber(&padding_packet, &options.packet_id);
     padding_packet.SetPadding(padding_bytes_in_packet, &random_);

[philipel, 2017/03/27 10:55:38]

Is this really correct? When you call SendPadData with the experiment on you
expect to send |bytes| number of bytes, including the header. So I think you
need to subtract the size of the header from |padding_bytes_in_packet| to not
send |bytes| + <header size> number of bytes.

[danilchap, 2017/03/27 11:20:23]

I think this is correct: 
here you set number of padding bytes per packet, which is normally maximum
padding size.
you want to send total size 'bytes', thus bytes_sent need to be updated
correctly to terminate "while (bytes_sent < bytes)" loop earlier.

[philipel, 2017/03/27 11:38:49]

I agree that we want to terminate the loop as early as possible by maximizing
the size of the padding packets. But in the case of sending say 100 bytes? Then
we would create a packet of 100 bytes padding + <header size>, which exceeds the
number of bytes we want to send.

I think this might be important in the case of audio BWE since the bitrates we
work with are so low (which is why the overhead of headers are important to fix
in the first place I guess?).

[danilchap, 2017/03/27 12:23:00]

sorry, I forgout about audio case where we want smaller packets.
Yes, it is incorrect for audio then.
padding_bytes_in_packet should be calculated with header size in mind, likely
here instead of top of this function.

[michaelt, 2017/03/27 14:25:01]

Acknowledged.

 
     if (has_transport_seq_num) {
       AddPacketToTransportFeedback(options.packet_id, padding_packet,
                                    pacing_info);
     }
 
     if (!SendPacketToNetwork(padding_packet, options, pacing_info))
       break;
 
     bytes_sent += padding_bytes_in_packet;
+    if (send_side_bwe_with_overhead_) {
+      // When WebRTC-SendSideBwe-WithOverhead is enabled, the padding budget
+      // includes overhead.
+      bytes_sent += padding_packet.headers_size();

[danilchap, 2017/03/24 08:47:20]

may be if else is better:
if (with_overhead_) {
  bytes_sent += padding_packet.size();
} else {
  bytes_sent += padding_bytes_in_packet or padding_packet.padding_size();
}

[minyue-webrtc, 2017/03/24 09:08:54]

I like that too.

+    }
     UpdateRtpStats(padding_packet, over_rtx, false);
   }
 
   return bytes_sent;
 }
 
 void RTPSender::SetStorePacketsStatus(bool enable, uint16_t number_to_store) {
   packet_history_.SetStorePacketsStatus(enable, number_to_store);
 }
 
@@ skipping 680 matching lines @@
     if (rtp_overhead_bytes_per_packet_ == packet.headers_size()) {
       return;
     }
     rtp_overhead_bytes_per_packet_ = packet.headers_size();
     overhead_bytes_per_packet = rtp_overhead_bytes_per_packet_;
   }
   overhead_observer_->OnOverheadChanged(overhead_bytes_per_packet);
 }
 
 }  // namespace webrtc