Reland of Enable audio streams to send padding. (patchset #4 id:60001 of https://codereview.webrtc.…

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 18 matching lines @@
 #include "webrtc/modules/rtp_rtcp/source/rtp_sender_audio.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_sender_video.h"
 #include "webrtc/modules/rtp_rtcp/source/time_util.h"
 #include "webrtc/system_wrappers/include/field_trial.h"
 
 namespace webrtc {
 
 namespace {
 // Max in the RFC 3550 is 255 bytes, we limit it to be modulus 32 for SRTP.
 constexpr size_t kMaxPaddingLength = 224;
+constexpr size_t kMinAudioPaddingLength = 50;
 constexpr int kSendSideDelayWindowMs = 1000;
 constexpr size_t kRtpHeaderLength = 12;
 constexpr uint16_t kMaxInitRtpSeqNumber = 32767;  // 2^15 -1.
 constexpr uint32_t kTimestampTicksPerMs = 90;
 constexpr int kBitrateStatisticsWindowMs = 1000;
 
 constexpr size_t kMinFlexfecPacketsToStoreForPacing = 50;
 
 const char* FrameTypeToString(FrameType frame_type) {
   switch (frame_type) {
@@ skipping 159 matching lines @@
     case kRtpExtensionTransportSequenceNumber:
       return rtp_header_extension_map_.Register(type, id);
     case kRtpExtensionNone:
     case kRtpExtensionNumberOfExtensions:
       LOG(LS_ERROR) << "Invalid RTP extension type for registration";
       return -1;
   }
   return -1;
 }
 
-bool RTPSender::IsRtpHeaderExtensionRegistered(RTPExtensionType type) {
+bool RTPSender::IsRtpHeaderExtensionRegistered(RTPExtensionType type) const {
   rtc::CritScope lock(&send_critsect_);
   return rtp_header_extension_map_.IsRegistered(type);
 }
 
 int32_t RTPSender::DeregisterRtpHeaderExtension(RTPExtensionType type) {
   rtc::CritScope lock(&send_critsect_);
   return rtp_header_extension_map_.Deregister(type);
 }
 
 int32_t RTPSender::RegisterPayload(
@@ skipping 245 matching lines @@
       break;
     size_t payload_size = packet->payload_size();
     if (!PrepareAndSendPacket(std::move(packet), true, false, probe_cluster_id))
       break;
     bytes_left -= payload_size;
   }
   return bytes_to_send - bytes_left;
 }
 
 size_t RTPSender::SendPadData(size_t bytes, int probe_cluster_id) {
-  // 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.
-  size_t padding_bytes_in_packet =
-      std::min(MaxPayloadSize(), kMaxPaddingLength);
+  size_t padding_bytes_in_packet;
+  if (audio_configured_) {
+    // Allow smaller padding packets for audio.
+    padding_bytes_in_packet =
+        std::max(std::min(bytes, MaxPayloadSize()), kMinAudioPaddingLength);

[terelius, 2017/02/03 14:26:14]

Note this order of operation allows sending padding packets that are  larger
than MaxPayloadSize(). It this the intent? Or do we assume that
kMinAudioPaddingLength < MaxPayloadSize()?

[stefan-webrtc, 2017/02/03 14:39:25]

Thanks, that's not the intent.

+    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);
+  }
   size_t bytes_sent = 0;
   while (bytes_sent < bytes) {
     int64_t now_ms = clock_->TimeInMilliseconds();
     uint32_t ssrc;
     uint32_t timestamp;
     int64_t capture_time_ms;
     uint16_t sequence_number;
     int payload_type;
     bool over_rtx;
     {
       rtc::CritScope lock(&send_critsect_);
       if (!sending_media_)
         break;
       timestamp = last_rtp_timestamp_;
       capture_time_ms = capture_time_ms_;
       if (rtx_ == kRtxOff) {
+        if (payload_type_ == -1)
+          break;
         // Without RTX we can't send padding in the middle of frames.
-        if (!last_packet_marker_bit_)
+        // For audio marker bits doesn't mark the end of a frame and frames

[terelius, 2017/02/03 14:26:14]

What does the marker bit mean for audio? Though unlikely, what happens if we add
padding in the middle of an audio frame? Can it corrupt the decoder state?

[stefan-webrtc, 2017/02/03 14:39:25]

It has no clear meaning, but one possible interpretation is "the beginning of a
talk spurt"
http://www.cs.columbia.edu/~hgs/rtp/faq.html#marker

It should not affect the decoder state. I don't even think padding packets are
passed down to the decoder. They don't contain anything, so they won't add bits
to the bitstream.

It's a bit worse for video because we have to keep track of sequence numbers in
a more complicated way, and we also need to know when a frame starts and ends.
In theory we could remove this check also for video, but it would require us to
nack some padding packets.

[terelius, 2017/02/03 14:42:19]

Acknowledged.

+        // are usually a single packet, so for now we don't apply this rule
+        // for audio.
+        if (!audio_configured_ && !last_packet_marker_bit_) {
           break;
+        }
         ssrc = ssrc_;
         sequence_number = sequence_number_;
         ++sequence_number_;
         payload_type = payload_type_;
         over_rtx = false;
       } else {
         // Without abs-send-time or transport sequence number a media packet
         // must be sent before padding so that the timestamps used for
         // estimation are correct.
         if (!media_has_been_sent_ &&
@@ skipping 271 matching lines @@
 
   // RED+ULPFEC.
   int pt_red;
   int pt_fec;
   video_->GetUlpfecConfig(&pt_red, &pt_fec);
   return static_cast<int>(packet.PayloadType()) == pt_red &&
          static_cast<int>(packet.payload()[0]) == pt_fec;
 }
 
 size_t RTPSender::TimeToSendPadding(size_t bytes, int probe_cluster_id) {
-  if (audio_configured_ || bytes == 0)
+  if (bytes == 0)
     return 0;
   size_t bytes_sent = TrySendRedundantPayloads(bytes, probe_cluster_id);
   if (bytes_sent < bytes)
     bytes_sent += SendPadData(bytes - bytes_sent, probe_cluster_id);
   return bytes_sent;
 }
 
 bool RTPSender::SendToNetwork(std::unique_ptr<RtpPacketToSend> packet,
                               StorageType storage,
                               RtpPacketSender::Priority priority) {
@@ skipping 468 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