Fix packetization logic to leave space for extensions in the last packet

webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc

 /*
  *  Copyright (c) 2014 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 <string>
 
 #include "webrtc/base/logging.h"
 #include "webrtc/modules/include/module_common_types.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_packet_to_send.h"
 
 namespace webrtc {
 
 static const size_t kGenericHeaderLength = 1;
 
 RtpPacketizerGeneric::RtpPacketizerGeneric(FrameType frame_type,
-                                           size_t max_payload_len)
+                                           size_t max_payload_len,
+                                           size_t last_packet_extension_len)
     : payload_data_(NULL),
       payload_size_(0),
       max_payload_len_(max_payload_len - kGenericHeaderLength),
-      frame_type_(frame_type) {
-}
+      last_packet_extension_len_(last_packet_extension_len),
+      frame_type_(frame_type),
+      num_packets_left_(0) {}
 

[danilchap, 2017/05/12 18:56:15]

initialize num_smaller_packets_ member too

[ilnik, 2017/05/15 09:38:33]

Done.

 RtpPacketizerGeneric::~RtpPacketizerGeneric() {
 }
 
-void RtpPacketizerGeneric::SetPayloadData(
+size_t RtpPacketizerGeneric::SetPayloadData(
     const uint8_t* payload_data,
     size_t payload_size,
     const RTPFragmentationHeader* fragmentation) {
   payload_data_ = payload_data;
   payload_size_ = payload_size;
 
-  // Fragment packets more evenly by splitting the payload up evenly.
-  size_t num_packets =
-      (payload_size_ + max_payload_len_ - 1) / max_payload_len_;
-  payload_length_ = (payload_size_ + num_packets - 1) / num_packets;
-  assert(payload_length_ <= max_payload_len_);
+  /* Fragment packets such that they are almost the same size, even accounting
+   * for larger header in the last packet due to extensions.
+   * Since we are given how much extra space is occupied by the longer header
+   * in the last packet, we can pretend that RTP headers are the same, but
+   * there's last_packet_extension_len_ extra payload, to be put at the end of
+   * the last packet.
+   */
+  size_t total_data = payload_size_ + last_packet_extension_len_;

[danilchap, 2017/05/12 18:56:15]

may be do not name variable 'data' when it represent size/length.

[ilnik, 2017/05/15 09:38:33]

I can't think of any better name.

[danilchap, 2017/05/15 11:03:17]

I think  total_size/total_length/total_len/total_bytes are better:
variable with '_data' sufix (e.g. payload_data) point to actual data.

Comment above suggest you may consider 
virtual_payload_size/virtual_payload_len name

[ilnik, 2017/05/15 12:11:38]

Done.

+
+  // Minimum needed number of packets to fit payload and extensions.
+  num_packets_left_ = (total_data + max_payload_len_ - 1) / max_payload_len_;
+  // Given number of packets we will use, calculate average size rounded up.
+  payload_len_per_packet_ =
+      (total_data + num_packets_left_ - 1) / num_packets_left_;
+  // If we can't divide everything perfectly evenly, we put 1 extra byte in some
+  // first packets: 14 bytes in 4 packets would be splitted as 4+4+3+3.

[danilchap, 2017/05/12 18:56:15]

did you consider 'small packets first, then larger packets'?

[ilnik, 2017/05/15 09:38:33]

It's just one byte of difference. Doesn't make much impact either way. But using
larger packets first is more clear, since we already have correct length
computed.

[danilchap, 2017/05/15 11:03:17]

function-wise it is sure the same.
It just looked to me that some arithmetic will be noticeably simpler with
smaller packets first.
e.g.:
payload_len_per_packet_ = total_size / num_packets_left_;  // smaller size.
num_larger_packets_ = total_size % num_packets_left_;  // no extra if.

But if that increase complexity in other parts making total complexity about the
same, then no point to switch it.

[ilnik, 2017/05/15 12:11:38]

Acknowledged.

+  // There are exactly total_data % num_packets larger packets.
+  num_smaller_packets_ = num_packets_left_ - total_data % num_packets_left_;
+  // If all packets are the same size, we already have correct per packet
+  // length.
+  if (num_smaller_packets_ == num_packets_left_)
+    num_smaller_packets_ = 0;
+  RTC_DCHECK_LE(payload_len_per_packet_, max_payload_len_);
 
   generic_header_ = RtpFormatVideoGeneric::kFirstPacketBit;
-}
-
-bool RtpPacketizerGeneric::NextPacket(RtpPacketToSend* packet,
-                                      bool* last_packet) {
-  RTC_DCHECK(packet);
-  RTC_DCHECK(last_packet);
-  if (payload_size_ < payload_length_) {
-    payload_length_ = payload_size_;
-  }
-
-  payload_size_ -= payload_length_;
-  RTC_DCHECK_LE(payload_length_, max_payload_len_);
-
-  uint8_t* out_ptr =
-      packet->AllocatePayload(kGenericHeaderLength + payload_length_);
-  // Put generic header in packet
   if (frame_type_ == kVideoFrameKey) {
     generic_header_ |= RtpFormatVideoGeneric::kKeyFrameBit;
   }
+  return num_packets_left_;
+}
+
+bool RtpPacketizerGeneric::NextPacket(RtpPacketToSend* packet) {
+  RTC_DCHECK(packet);
+  if (num_packets_left_ == 0)
+    return false;
+  // Last smaller_packets_ packets are 1 byte smaller than previous packets.
+  // Reduce per packet payload once needed.
+  if (num_packets_left_ == num_smaller_packets_)
+    payload_len_per_packet_--;
+  size_t next_packet_payload_len = payload_len_per_packet_;
+  if (payload_size_ <= next_packet_payload_len) {
+    // Whole payload fits into this packet
+    next_packet_payload_len = payload_size_;
+    if (num_packets_left_ == 2) {
+      // This is the pre-last packet. Leave at least 1 payload byte for the
+      // last packet.
+      next_packet_payload_len--;
+      RTC_DCHECK_GT(next_packet_payload_len, 0);
+    }
+  }
+  RTC_DCHECK_LE(next_packet_payload_len, max_payload_len_);
+
+  uint8_t* out_ptr =
+      packet->AllocatePayload(kGenericHeaderLength + next_packet_payload_len);
+  // Put generic header in packet.
   out_ptr[0] = generic_header_;
-  // Remove first-packet bit, following packets are intermediate
+  // Remove first-packet bit, following packets are intermediate.
   generic_header_ &= ~RtpFormatVideoGeneric::kFirstPacketBit;
 
-  // Put payload in packet
-  memcpy(out_ptr + kGenericHeaderLength, payload_data_, payload_length_);
-  payload_data_ += payload_length_;
+  // Put payload in packet.
+  memcpy(out_ptr + kGenericHeaderLength, payload_data_,
+         next_packet_payload_len);
+  payload_data_ += next_packet_payload_len;
+  payload_size_ -= next_packet_payload_len;
+  num_packets_left_--;
+  // Packets left to produce and data left to split should end at the same time.
+  RTC_DCHECK_EQ(num_packets_left_ == 0, payload_size_ == 0);
 
-  *last_packet = payload_size_ <= 0;
-  packet->SetMarker(*last_packet);
+  packet->SetMarker(payload_size_ == 0);
+
   return true;
 }
 
 ProtectionType RtpPacketizerGeneric::GetProtectionType() {
   return kProtectedPacket;
 }
 
 StorageType RtpPacketizerGeneric::GetStorageType(
     uint32_t retransmission_settings) {
   return kAllowRetransmission;
@@ skipping 23 matching lines @@
       (generic_header & RtpFormatVideoGeneric::kFirstPacketBit) != 0;
   parsed_payload->type.Video.codec = kRtpVideoGeneric;
   parsed_payload->type.Video.width = 0;
   parsed_payload->type.Video.height = 0;
 
   parsed_payload->payload = payload_data;
   parsed_payload->payload_length = payload_data_length;
   return true;
 }
 }  // namespace webrtc