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

webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.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 120 matching lines @@
   virtual void SetUp() {
     expected_.InitRTPVideoHeaderVP9();
   }
 
   RtpPacketToSend packet_;
   std::unique_ptr<uint8_t[]> payload_;
   size_t payload_size_;
   size_t payload_pos_;
   RTPVideoHeaderVP9 expected_;
   std::unique_ptr<RtpPacketizerVp9> packetizer_;
+  size_t num_packets_;
 
   void Init(size_t payload_size, size_t packet_size) {
     payload_.reset(new uint8_t[payload_size]);
     memset(payload_.get(), 7, payload_size);
     payload_size_ = payload_size;
     payload_pos_ = 0;
-    packetizer_.reset(new RtpPacketizerVp9(expected_, packet_size));
-    packetizer_->SetPayloadData(payload_.get(), payload_size_, NULL);
+    packetizer_.reset(new RtpPacketizerVp9(expected_, packet_size,
+                                           /*last_packet_reduction_len=*/0));
+    num_packets_ =
+        packetizer_->SetPayloadData(payload_.get(), payload_size_, NULL);

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

since you are touching this line anyway, replace NULL with nullptr

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

Done.

   }
 
   void CheckPayload(const uint8_t* packet,
                     size_t start_pos,
                     size_t end_pos,
                     bool last) {
     for (size_t i = start_pos; i < end_pos; ++i) {
       EXPECT_EQ(packet[i], payload_[payload_pos_++]);
     }
     EXPECT_EQ(last, payload_pos_ == payload_size_);
   }
 
   void CreateParseAndCheckPackets(const size_t* expected_hdr_sizes,
                                   const size_t* expected_sizes,
                                   size_t expected_num_packets) {
     ASSERT_TRUE(packetizer_.get() != NULL);
-    bool last = false;
     if (expected_num_packets == 0) {
-      EXPECT_FALSE(packetizer_->NextPacket(&packet_, &last));
+      EXPECT_FALSE(packetizer_->NextPacket(&packet_));
       return;
     }
+    EXPECT_EQ(expected_num_packets, num_packets_);
     for (size_t i = 0; i < expected_num_packets; ++i) {
-      EXPECT_TRUE(packetizer_->NextPacket(&packet_, &last));
+      EXPECT_TRUE(packetizer_->NextPacket(&packet_));
       auto rtp_payload = packet_.payload();
       EXPECT_EQ(expected_sizes[i], rtp_payload.size());
       RTPVideoHeaderVP9 hdr = expected_;
       hdr.beginning_of_frame = (i == 0);
-      hdr.end_of_frame = last;
+      hdr.end_of_frame = (i + 1) == expected_num_packets;
       ParseAndCheckPacket(rtp_payload.data(), hdr, expected_hdr_sizes[i],
                           rtp_payload.size());
       CheckPayload(rtp_payload.data(), expected_hdr_sizes[i],
-                   rtp_payload.size(), last);
+                   rtp_payload.size(), (i + 1) == expected_num_packets);
     }
-    EXPECT_TRUE(last);
   }
 };
 
 TEST_F(RtpPacketizerVp9Test, TestEqualSizedMode_OnePacket) {
   const size_t kFrameSize = 25;
   const size_t kPacketSize = 26;
   Init(kFrameSize, kPacketSize);
 
   // One packet:
   // I:0, P:0, L:0, F:0, B:1, E:1, V:0  (1hdr + 25 payload)
@@ skipping 234 matching lines @@
   // T:1, U:1, R:3 | P_DIFF[1][0]:5 | P_DIFF[1][1]:6 | P_DIFF[1][2]:7
   // T:2, U:0, R:2 | P_DIFF[2][0]:8 | P_DIFF[2][0]:9
   const size_t kExpectedHdrSizes[] = {19};
   const size_t kExpectedSizes[] = {40};
   const size_t kExpectedNum = GTEST_ARRAY_SIZE_(kExpectedSizes);
   CreateParseAndCheckPackets(kExpectedHdrSizes, kExpectedSizes, kExpectedNum);
 }
 
 TEST_F(RtpPacketizerVp9Test, TestOnlyHighestSpatialLayerSetMarker) {
   const size_t kFrameSize = 10;
-  const size_t kPacketSize = 9;  // 2 packet per frame.
+  const size_t kPacketSize = 8;
+  const size_t kExtensionsLen = 0;

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

prefer kLastPacketReudctionLen name
to avoid false impression it is size of all header extensions of the last
packet.

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

Done.

+  // Calculated by hand. Headers are 2 bytes. One packet would be 2+10 bytes

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

may be mention which headers (since there are so many different headers when
composing an rtp packet):
Without optional fields VP9 payload descriptor is 2 bytes.

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

Done.

+  // which is more than packet size. But two packets are enough, since they have
+  // capacity of 2*(8-2)=12 bytes.

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

May be:
One packet can contain |kPacketSize| - |kVp9MinDiscriptorSize| = 6 bytes of the
frame payload, thus to fit 10 bytes two packets are required.

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

Done.

+  const size_t kMinNumberOfPackets = 2;
   const uint8_t kFrame[kFrameSize] = {7};
   const RTPFragmentationHeader* kNoFragmentation = nullptr;
 
   RTPVideoHeaderVP9 vp9_header;
   vp9_header.InitRTPVideoHeaderVP9();
   vp9_header.flexible_mode = true;
   vp9_header.num_spatial_layers = 3;
 
   RtpPacketToSend packet(kNoExtensions);
-  bool last;
 
   vp9_header.spatial_idx = 0;
-  RtpPacketizerVp9 packetizer0(vp9_header, kPacketSize);
-  packetizer0.SetPayloadData(kFrame, sizeof(kFrame), kNoFragmentation);
-  ASSERT_TRUE(packetizer0.NextPacket(&packet, &last));
-  EXPECT_FALSE(last);
+  RtpPacketizerVp9 packetizer0(vp9_header, kPacketSize, kExtensionsLen);
+  EXPECT_EQ(
+      packetizer0.SetPayloadData(kFrame, sizeof(kFrame), kNoFragmentation),
+      kMinNumberOfPackets);

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

may be rather than hijacking test designed to check something else, add a new
one, with name matching expectations.

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

Done.

+  ASSERT_TRUE(packetizer0.NextPacket(&packet));
   EXPECT_FALSE(packet.Marker());
-  ASSERT_TRUE(packetizer0.NextPacket(&packet, &last));
-  EXPECT_TRUE(last);
+  ASSERT_TRUE(packetizer0.NextPacket(&packet));
   EXPECT_FALSE(packet.Marker());
 
   vp9_header.spatial_idx = 1;
-  RtpPacketizerVp9 packetizer1(vp9_header, kPacketSize);
-  packetizer1.SetPayloadData(kFrame, sizeof(kFrame), kNoFragmentation);
-  ASSERT_TRUE(packetizer1.NextPacket(&packet, &last));
-  EXPECT_FALSE(last);
+  RtpPacketizerVp9 packetizer1(vp9_header, kPacketSize, kExtensionsLen);
+  EXPECT_EQ(
+      packetizer1.SetPayloadData(kFrame, sizeof(kFrame), kNoFragmentation),
+      kMinNumberOfPackets);
+  ASSERT_TRUE(packetizer1.NextPacket(&packet));
   EXPECT_FALSE(packet.Marker());
-  ASSERT_TRUE(packetizer1.NextPacket(&packet, &last));
-  EXPECT_TRUE(last);
+  ASSERT_TRUE(packetizer1.NextPacket(&packet));
   EXPECT_FALSE(packet.Marker());
 
   vp9_header.spatial_idx = 2;
-  RtpPacketizerVp9 packetizer2(vp9_header, kPacketSize);
-  packetizer2.SetPayloadData(kFrame, sizeof(kFrame), kNoFragmentation);
-  ASSERT_TRUE(packetizer2.NextPacket(&packet, &last));
-  EXPECT_FALSE(last);
+  RtpPacketizerVp9 packetizer2(vp9_header, kPacketSize, kExtensionsLen);
+  EXPECT_EQ(
+      packetizer2.SetPayloadData(kFrame, sizeof(kFrame), kNoFragmentation),
+      kMinNumberOfPackets);
+  ASSERT_TRUE(packetizer2.NextPacket(&packet));
   EXPECT_FALSE(packet.Marker());
-  ASSERT_TRUE(packetizer2.NextPacket(&packet, &last));
-  EXPECT_TRUE(last);
+  ASSERT_TRUE(packetizer2.NextPacket(&packet));
   EXPECT_TRUE(packet.Marker());
 }
 
+TEST_F(RtpPacketizerVp9Test, TestRespectsLastPacketReduction) {
+  const size_t kFrameSize = 10;
+  const size_t kPacketSize = 8;
+  const size_t kExtensionsLen = 5;
+  // Calculated by hand. Headers are 2 bytes. Like in the test above, 1 packet
+  // is not enough. 2 packets would have capacity of (8-2)+(8-2-5)=7 because the
+  // last packet has reserved space for extensions. But three packets are
+  // enough, since they have capacity of 3*(8-2)-5=13 bytes.
+  const size_t kMinNumberOfPackets = 3;
+  const uint8_t kFrame[kFrameSize] = {7};
+  const RTPFragmentationHeader* kNoFragmentation = nullptr;
+
+  RTPVideoHeaderVP9 vp9_header;
+  vp9_header.InitRTPVideoHeaderVP9();
+  vp9_header.flexible_mode = true;
+  vp9_header.num_spatial_layers = 3;

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

is number of spatial layers (or presence of this field) relevant to this test?

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

No, it's from copy-paste. Removed.

+
+  RtpPacketToSend packet(kNoExtensions);
+
+  vp9_header.spatial_idx = 0;
+  RtpPacketizerVp9 packetizer0(vp9_header, kPacketSize, kExtensionsLen);
+  EXPECT_EQ(
+      packetizer0.SetPayloadData(kFrame, sizeof(kFrame), kNoFragmentation),
+      kMinNumberOfPackets);
+  ASSERT_TRUE(packetizer0.NextPacket(&packet));
+  EXPECT_FALSE(packet.Marker());
+  ASSERT_TRUE(packetizer0.NextPacket(&packet));
+  EXPECT_FALSE(packet.Marker());
+}
+
 TEST_F(RtpPacketizerVp9Test, TestBaseLayerProtectionAndStorageType) {
   const size_t kFrameSize = 10;
   const size_t kPacketSize = 12;
 
   // I:0, P:0, L:1, F:1, B:1, E:1, V:0 (2hdr + 10 payload)
   // L:   T:0, U:0, S:0, D:0
   expected_.flexible_mode = true;
   expected_.temporal_idx = 0;
   Init(kFrameSize, kPacketSize);
   EXPECT_EQ(kProtectedPacket, packetizer_->GetProtectionType());
@@ skipping 240 matching lines @@
 }
 
 TEST_F(RtpDepacketizerVp9Test, ParseFailsForTooShortBufferToFitPayload) {
   const uint8_t kHeaderLength = 1;
   uint8_t packet[kHeaderLength] = {0};
   RtpDepacketizer::ParsedPayload parsed;
   EXPECT_FALSE(depacketizer_->Parse(&parsed, packet, sizeof(packet)));
 }
 
 }  // namespace webrtc