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

webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc

 /*
  *  Copyright (c) 2015 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 12 matching lines @@
 #define RETURN_FALSE_ON_ERROR(x) \
   if (!(x)) {                    \
     return false;                \
   }
 
 namespace webrtc {
 namespace {
 // Length of VP9 payload descriptors' fixed part.
 const size_t kFixedPayloadDescriptorBytes = 1;
 
-// Packet fragmentation mode. If true, packets are split into (almost) equal
-// sizes. Otherwise, as many bytes as possible are fit into one packet.
-const bool kBalancedMode = true;
-
 const uint32_t kReservedBitValue0 = 0;
 
 uint8_t TemporalIdxField(const RTPVideoHeaderVP9& hdr, uint8_t def) {
   return (hdr.temporal_idx == kNoTemporalIdx) ? def : hdr.temporal_idx;
 }
 
 uint8_t SpatialIdxField(const RTPVideoHeaderVP9& hdr, uint8_t def) {
   return (hdr.spatial_idx == kNoSpatialIdx) ? def : hdr.spatial_idx;
 }
 
@@ skipping 408 matching lines @@
     vp9->gof.num_ref_pics[i] = r;
 
     for (uint8_t p = 0; p < vp9->gof.num_ref_pics[i]; ++p) {
       uint8_t p_diff;
       RETURN_FALSE_ON_ERROR(parser->ReadUInt8(&p_diff));
       vp9->gof.pid_diff[i][p] = p_diff;
     }
   }
   return true;
 }
-
-// Gets the size of next payload chunk to send. Returns 0 on error.
-size_t CalcNextSize(size_t max_length, size_t rem_bytes) {
-  if (max_length == 0 || rem_bytes == 0) {
-    return 0;
-  }
-  if (kBalancedMode) {
-    size_t num_frags = std::ceil(static_cast<double>(rem_bytes) / max_length);
-    return static_cast<size_t>(
-        static_cast<double>(rem_bytes) / num_frags + 0.5);
-  }
-  return max_length >= rem_bytes ? rem_bytes : max_length;
-}
 }  // namespace
 
-
 RtpPacketizerVp9::RtpPacketizerVp9(const RTPVideoHeaderVP9& hdr,
-                                   size_t max_payload_length)
+                                   size_t max_payload_length,
+                                   size_t last_packet_reduction_len)
     : hdr_(hdr),
       max_payload_length_(max_payload_length),
       payload_(nullptr),
-      payload_size_(0) {
-}
+      payload_size_(0),
+      last_packet_reduction_len_(last_packet_reduction_len) {}
 
 RtpPacketizerVp9::~RtpPacketizerVp9() {
 }
 
 ProtectionType RtpPacketizerVp9::GetProtectionType() {
   bool protect =
       hdr_.temporal_idx == 0 || hdr_.temporal_idx == kNoTemporalIdx;
   return protect ? kProtectedPacket : kUnprotectedPacket;
 }
 
 StorageType RtpPacketizerVp9::GetStorageType(uint32_t retransmission_settings) {
   StorageType storage = kAllowRetransmission;
   if (hdr_.temporal_idx == 0 &&
       !(retransmission_settings & kRetransmitBaseLayer)) {
     storage = kDontRetransmit;
   } else if (hdr_.temporal_idx != kNoTemporalIdx && hdr_.temporal_idx > 0 &&
              !(retransmission_settings & kRetransmitHigherLayers)) {
     storage = kDontRetransmit;
   }
   return storage;
 }
 
 std::string RtpPacketizerVp9::ToString() {
   return "RtpPacketizerVp9";
 }
 
-void RtpPacketizerVp9::SetPayloadData(
+size_t RtpPacketizerVp9::SetPayloadData(
     const uint8_t* payload,
     size_t payload_size,
     const RTPFragmentationHeader* fragmentation) {
   payload_ = payload;
   payload_size_ = payload_size;
   GeneratePackets();
+  return packets_.size();
 }
 
+// Splits payload in minimal number of roughly equal in size packets.
 void RtpPacketizerVp9::GeneratePackets() {
   if (max_payload_length_ < PayloadDescriptorLength(hdr_) + 1) {
-    LOG(LS_ERROR) << "Payload header and one payload byte won't fit.";
+    LOG(LS_ERROR) << "Payload header and one payload byte won't fit in packet.";

[danilchap, 2017/05/16 10:25:11]

may be 'in the first packet'
(since only 1st packet includes SS data)

[ilnik, 2017/05/16 12:24:08]

Done.

     return;
   }
+  if (max_payload_length_ < PayloadDescriptorLengthMinusSsData(hdr_) + 1 +
+                                last_packet_reduction_len_) {
+    LOG(LS_ERROR) << "Payload header and one payload byte won't fit in the last"
+                     " packet.";
+    return;
+  }
+
+  // Instead of making last packet smaller, we pretend that we must write

[danilchap, 2017/05/16 10:25:11]

may be also mention 1st packet that has extra SS header since you add both
last_packet_extra_len and first_packet_extra_len to the total_bytes

[ilnik, 2017/05/16 12:24:08]

Done.

+  // additional data into it. We account for this  virtual payload while

[danilchap, 2017/05/16 10:25:11]

single space  between 'this' and 'virtual'

[ilnik, 2017/05/16 12:24:08]

Done.

+  // calculating packets number and sizes.
+
+  // Payload, virtual payload and SS hdr data in the first packet together.
+  size_t total_bytes =
+      payload_size_ + last_packet_reduction_len_ + SsDataLength(hdr_);

[danilchap, 2017/05/16 10:25:11]

nit: may be reorder terms in the order closer to how they appear on the wire:
SSdataLength() + payload_size_ + last_packet_reduction_len

[ilnik, 2017/05/16 12:24:08]

Done.

+  // Now all packets will have the same lenght of vp9 headers.
+  size_t capacity =
+      max_payload_length_ - PayloadDescriptorLengthMinusSsData(hdr_);
+  // Integer divisions rounding up.
+  size_t num_packets = (total_bytes + capacity - 1) / capacity;
+  size_t per_packet_data = (total_bytes + num_packets - 1) / num_packets;
+  // Several last packets are 1 byte smaller than the rest.
+  // i.e. if 14 bytes were splitted between 4 packets, it would be 4+4+3+3.
+  size_t smaller_packets = num_packets - total_bytes % num_packets;
+  // If all packets are the same size, we assume 0 packets are smaller.
+  if (smaller_packets == num_packets)
+    smaller_packets = 0;
   size_t bytes_processed = 0;
+  size_t packets_left = num_packets;
   while (bytes_processed < payload_size_) {
+    if (packets_left == smaller_packets)
+      per_packet_data--;
+    size_t packet_bytes = per_packet_data;
+    // First packet also has SS hdr data.
+    if (bytes_processed == 0)
+      packet_bytes -= SsDataLength(hdr_);

[danilchap, 2017/05/16 10:25:11]

can packet_bytes become negative?

[ilnik, 2017/05/16 12:24:08]

Actually, in some extreme cases, it can. The fix for that is done now.
It could happen only if SS data is at least half of the packet, i.e. ~700 bytes.

[danilchap, 2017/05/17 10:31:19]

Can you also add a test for this scenario.
(btw, packet_bytes is unsigned, so 3 - 5 = 4294967294 on 32bit system)

[ilnik, 2017/05/17 12:30:55]

Yep, my mistake about unsignedness. Thanks for catching that. Also added a test
for that.

     size_t rem_bytes = payload_size_ - bytes_processed;
-    size_t rem_payload_len = max_payload_length_ -
-         (bytes_processed ? PayloadDescriptorLengthMinusSsData(hdr_)
-                          : PayloadDescriptorLength(hdr_));
-
-    size_t packet_bytes = CalcNextSize(rem_payload_len, rem_bytes);
-    if (packet_bytes == 0) {
-      LOG(LS_ERROR) << "Failed to generate VP9 packets.";
-      while (!packets_.empty())
-        packets_.pop();
-      return;
+    if (packet_bytes >= rem_bytes) {
+      // All remaining payload fits into this packet.
+      packet_bytes = rem_bytes;
+      // If this is the pre-last packet, leave at least 1 byte of payload for
+      // the last packet.
+      if (packets_left == 2)
+        packet_bytes--;

[danilchap, 2017/05/16 10:25:11]

can packet_bytes be zero before decrement?

[ilnik, 2017/05/16 12:24:08]

Now it can't due to the fix above.

There's still a special case then the whole frame is just 1 byte and it doesn't
fit in one packet due to extensions length. Here we will make the first packet
with 0 length payload and second packet with 1 byte of payload. But we can't do
anything here.

Note, in that case previous implementation would always fail to generate
packets. I've added that assertion at the beginning now.

     }
     QueuePacket(bytes_processed, packet_bytes, bytes_processed == 0,
                 rem_bytes == packet_bytes, &packets_);
+    packets_left--;
     bytes_processed += packet_bytes;
+    // Last packet should be smaller
+    RTC_DCHECK(packets_left > 0 ||
+               capacity >= packet_bytes + last_packet_reduction_len_);
   }
   assert(bytes_processed == payload_size_);
 }
 
-bool RtpPacketizerVp9::NextPacket(RtpPacketToSend* packet, bool* last_packet) {
+bool RtpPacketizerVp9::NextPacket(RtpPacketToSend* packet) {
   RTC_DCHECK(packet);
-  RTC_DCHECK(last_packet);
   if (packets_.empty()) {
     return false;
   }
   PacketInfo packet_info = packets_.front();
   packets_.pop();
 
-  if (!WriteHeaderAndPayload(packet_info, packet)) {
+  if (!WriteHeaderAndPayload(packet_info, packet, packets_.empty())) {
     return false;
   }
-  *last_packet = packets_.empty();
   packet->SetMarker(packets_.empty() &&
                     (hdr_.spatial_idx == kNoSpatialIdx ||
                      hdr_.spatial_idx == hdr_.num_spatial_layers - 1));
   return true;
 }
 
 // VP9 format:
 //
 // Payload descriptor for F = 1 (flexible mode)
 //       0 1 2 3 4 5 6 7
@@ skipping 23 matching lines @@
 //      +-+-+-+-+-+-+-+-+
 // L:   |  T  |U|  S  |D| (CONDITIONALLY RECOMMENDED)
 //      +-+-+-+-+-+-+-+-+
 //      |   TL0PICIDX   | (CONDITIONALLY REQUIRED)
 //      +-+-+-+-+-+-+-+-+
 // V:   | SS            |
 //      | ..            |
 //      +-+-+-+-+-+-+-+-+
 
 bool RtpPacketizerVp9::WriteHeaderAndPayload(const PacketInfo& packet_info,
-                                             RtpPacketToSend* packet) const {
-  uint8_t* buffer = packet->AllocatePayload(max_payload_length_);
+                                             RtpPacketToSend* packet,
+                                             bool last) const {
+  uint8_t* buffer = packet->AllocatePayload(
+      last ? max_payload_length_ - last_packet_reduction_len_
+           : max_payload_length_);
   RTC_DCHECK(buffer);
   size_t header_length;
   if (!WriteHeader(packet_info, buffer, &header_length))
     return false;
 
   // Copy payload data.
   memcpy(&buffer[header_length],
          &payload_[packet_info.payload_start_pos], packet_info.size);
 
   packet->SetPayloadSize(header_length + packet_info.size);
@@ skipping 120 matching lines @@
   if (parsed_payload->payload_length == 0) {
     LOG(LS_ERROR) << "Failed parsing VP9 payload data.";
     return false;
   }
   parsed_payload->payload =
       payload + payload_length - parsed_payload->payload_length;
 
   return true;
 }
 }  // namespace webrtc