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

webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc

Index: webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc b/webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc
index 2d847c17a608ba5f64dae5f938b3ca4b7689693f..a1ed0e2d7f2287d9fad9e93ad6e563bb0dd64cc6 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc
@@ -20,59 +20,75 @@ 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_(0) {}
 
 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;
 
+  size_t total_data = payload_size_ + last_packet_extension_len_;

[danilchap, 2017/05/12 13:56:10]

what this value is? doesn't look like total size of the bytes Packetizer will
write to the payload fields.

[ilnik, 2017/05/12 14:46:08]

It's payload + extra bytes in the last packet. We count them together, so the
logic below will make all the packets of almost the same size. If we have 5
payload bytes and 1 byte of extensions in the last packet, and max_payload_len
is 5, we can fit everything in 2 packets, and we will get (5+1)/2=3 bytes of
payload or extensions in each packet. Thus first packet will have 3 bytes of
payload, and second packet only 2. If we make calculations using only payload,
we could fit that into 1 packet and then packets would have different sizes.

Added comments about this. I though it would be obvious, but it's not.

+
   // 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_);
+  num_packets_ = (total_data + max_payload_len_ - 1) / max_payload_len_;
+  payload_per_packet_ = (total_data + num_packets_ - 1) / num_packets_;
+  smaller_packets_ = num_packets_ - total_data % num_packets_;

[danilchap, 2017/05/12 13:56:10]

Can you explain why all these arithmetic steps needed?

[ilnik, 2017/05/12 14:46:08]

Done.

+  if (smaller_packets_ == num_packets_)
+    smaller_packets_ = 0;
+  assert(payload_per_packet_ <= max_payload_len_);

[danilchap, 2017/05/12 13:56:10]

since you are touching this line, change assert into RTC_DCHECK_LE

[ilnik, 2017/05/12 14:46:08]

Done.

 
   generic_header_ = RtpFormatVideoGeneric::kFirstPacketBit;
+  if (frame_type_ == kVideoFrameKey) {
+    generic_header_ |= RtpFormatVideoGeneric::kKeyFrameBit;
+  }
+  return num_packets_;
 }
 
-bool RtpPacketizerGeneric::NextPacket(RtpPacketToSend* packet,
-                                      bool* last_packet) {
+bool RtpPacketizerGeneric::NextPacket(RtpPacketToSend* packet) {
   RTC_DCHECK(packet);
-  RTC_DCHECK(last_packet);
-  if (payload_size_ < payload_length_) {
-    payload_length_ = payload_size_;
+  // last smaller_packets_ packets are 1 byte smaller than previous packets.

[danilchap, 2017/05/12 13:56:10]

Start comment Uppercase:
Last smaller...

[ilnik, 2017/05/12 14:46:08]

Done.

+  if (num_packets_ == smaller_packets_)
+    payload_per_packet_--;
+  // whole payload fit into this packet

[danilchap, 2017/05/12 13:56:10]

which line this comment explain?

[ilnik, 2017/05/12 14:46:08]

if() line below. Moved the comment.

+  size_t current_payload = payload_per_packet_;

[danilchap, 2017/05/12 13:56:10]

doesn't look like variable contain payload
may be 
next_packet_payload_size

[ilnik, 2017/05/12 14:46:08]

Done.

+  if (payload_size_ <= current_payload) {
+    current_payload = payload_size_;
+    // But this is the pre-last packet. Leave at least 1 payload byte for the

[danilchap, 2017/05/12 13:56:10]

probably better to remove 'But' and put this comment inside if (num_packets_ ==
2) clause

[ilnik, 2017/05/12 14:46:08]

Done.

+    // last packet. Should happen only if extensions take at least half of
+    // available capacity.
+    if (num_packets_ == 2) {
+      RTC_DCHECK(last_packet_extension_len_ >= max_payload_len_ / 2);

[danilchap, 2017/05/12 13:56:10]

why do you check it? Do you rely on this assumption?
RTC_DCHECK_GE if you do.

[ilnik, 2017/05/12 14:46:08]

This is sanity checking what it works as expected. This is really not best
situation then we have to move 1 byte to the next packet - it messes size of two
last packets. From my calculations it happens only in extreme cases where
extensions take half of all capacity.

It's not needed now, removed it.

+      current_payload -= 1;

[danilchap, 2017/05/12 13:56:10]

--maybe

[ilnik, 2017/05/12 14:46:08]

Done.

+    }
   }
-
-  payload_size_ -= payload_length_;
-  RTC_DCHECK_LE(payload_length_, max_payload_len_);
+  RTC_DCHECK_LE(current_payload, max_payload_len_);
 
   uint8_t* out_ptr =
-      packet->AllocatePayload(kGenericHeaderLength + payload_length_);
+      packet->AllocatePayload(kGenericHeaderLength + current_payload);
   // Put generic header in packet

[danilchap, 2017/05/12 13:56:10]

end comments with a .

[ilnik, 2017/05/12 14:46:07]

Done.

-  if (frame_type_ == kVideoFrameKey) {
-    generic_header_ |= RtpFormatVideoGeneric::kKeyFrameBit;
-  }
   out_ptr[0] = generic_header_;
   // 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_;
+  memcpy(out_ptr + kGenericHeaderLength, payload_data_, current_payload);
+  payload_data_ += current_payload;
+  payload_size_ -= current_payload;
+  num_packets_--;
 

[danilchap, 2017/05/12 13:56:10]

may be
RTC_DCHECK_EQ(num_packets_ == 0, payload_size_ == 0);

[ilnik, 2017/05/12 14:46:08]

Done.

-  *last_packet = payload_size_ <= 0;
-  packet->SetMarker(*last_packet);
+  packet->SetMarker(payload_size_ == 0);
   return true;
 }