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

webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc

Index: webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..55265a5f7a5572202d612caf03282a55deb3104d
--- /dev/null
+++ b/webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc
@@ -0,0 +1,145 @@
+/*
+ *  Copyright (c) 2017 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 <algorithm>
+#include <limits>
+#include <memory>
+#include <vector>
+
+#include "webrtc/base/array_view.h"
+#include "webrtc/modules/include/module_common_types.h"
+#include "webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h"
+#include "webrtc/modules/rtp_rtcp/source/byte_io.h"
+#include "webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.h"
+#include "webrtc/modules/rtp_rtcp/source/rtp_packet_to_send.h"
+#include "webrtc/test/gmock.h"
+#include "webrtc/test/gtest.h"
+
+namespace webrtc {
+namespace {
+
+using ::testing::ElementsAreArray;
+using ::testing::Le;
+using ::testing::SizeIs;
+using ::testing::Each;
+
+const size_t kMaxPayloadSize = 1200;
+
+uint8_t kTestPayload[kMaxPayloadSize];
+
+std::vector<size_t> NextPacketFillPayloadSizes(
+    RtpPacketizerGeneric* packetizer) {
+  RtpPacketToSend packet(nullptr);
+  std::vector<size_t> result;
+  while (packetizer->NextPacket(&packet)) {
+    result.push_back(packet.payload_size());
+  }
+  return result;
+}
+
+size_t GetEffectivePacketsSizeDifference(std::vector<size_t> payload_sizes,

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

may be
const std::vector<size_t>& or
rtc::ArrayView<const size_t>
(i.e. do not pass std::vector<size_t> by value unless you save it.
rtc::ArrayView though should be pass by value and can often be used instead of
const vector& parameter.
Though in this case, passing const vector& is perfectly fine too - it is an
internal function with very limited use)

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

It should have been const reference. Totally a typo. Thanks for catching that.

+                                         size_t last_packet_reduction_len) {
+  // Account for larger last packet header.
+  size_t min = payload_sizes.back() + last_packet_reduction_len;
+  size_t max = min;
+  for (size_t i = 0; i + 1 < payload_sizes.size(); ++i) {

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

may be use c++ for loop:
for (size_t payload_size : payload_sizes)
or std::minmax_element:
auto minmax = std::minmax_element(payload_sizes.begin(), payload_sized.end());
return *minmax.second - *minmax.first;
[though these approaches would require incrementing last packet payload size
before passing to this function]

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

Done.

+    min = std::min(payload_sizes[i], min);
+    max = std::max(payload_sizes[i], max);
+  }
+  return max - min;
+}
+
+}  // namespace
+
+TEST(RtpPacketizerVideoGeneric, RespectsMaxPayloadSize1) {
+  const size_t max_payload_length = 5;
+  const size_t last_packet_reduction_len = 2;
+  const size_t payload_size = 12;
+  const size_t min_num_packets = 4;  // Computed by hand.
+  RtpPacketizerGeneric packetizer(kVideoFrameKey, max_payload_length,
+                                  last_packet_reduction_len);
+  size_t num_packets =
+      packetizer.SetPayloadData(kTestPayload, payload_size, nullptr);
+  auto payload_sizes = NextPacketFillPayloadSizes(&packetizer);
+
+  EXPECT_THAT(payload_sizes, SizeIs(num_packets));

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

this check probably do not belong to this test, but should be in a
'GenerateMinimumNumberOfPackets' tests

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

Done.

+  EXPECT_EQ(num_packets, min_num_packets);
+  EXPECT_THAT(payload_sizes, Each(Le(max_payload_length)));
+  EXPECT_LE(payload_sizes.back(),
+            max_payload_length - last_packet_reduction_len);
+  size_t sizes_difference = GetEffectivePacketsSizeDifference(
+      payload_sizes, last_packet_reduction_len);
+  EXPECT_LE(sizes_difference, 1u);

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

this check probably better to move to 'GeneratePacketsOfAlmostEqualSize'

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

Done.

+}
+
+TEST(RtpPacketizerVideoGeneric, RespectsMaxPayloadSize2) {

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

instead of 1/2, can you name tests describing what scenario is tested?
This is 'AllPacketsCanBeEqualSize' scenario?

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

Done.

+  const size_t max_payload_length = 6;
+  const size_t last_packet_reduction_len = 2;
+  const size_t payload_size = 13;
+  const size_t min_num_packets = 3;  // Computed by hand.
+  RtpPacketizerGeneric packetizer(kVideoFrameKey, max_payload_length,
+                                  last_packet_reduction_len);
+  size_t num_packets =
+      packetizer.SetPayloadData(kTestPayload, payload_size, nullptr);
+  auto payload_sizes = NextPacketFillPayloadSizes(&packetizer);
+
+  EXPECT_THAT(payload_sizes, SizeIs(num_packets));
+  EXPECT_EQ(num_packets, min_num_packets);
+  EXPECT_THAT(payload_sizes, Each(Le(max_payload_length)));
+  EXPECT_LE(payload_sizes.back(),
+            max_payload_length - last_packet_reduction_len);
+  size_t sizes_difference = GetEffectivePacketsSizeDifference(
+      payload_sizes, last_packet_reduction_len);
+  EXPECT_LE(sizes_difference, 1u);
+}
+
+TEST(RtpPacketizerVideoGeneric, RespectsLastPacketReductionLength1) {

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

It is good that you check it in a separate test,
but then remove explicit check for last packet in previous tests, and remove
others checks from this test: Make it only test what it advertise it checks:
that is easier to read.

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

Done.

+  const size_t max_payload_length = 6;
+  const size_t last_packet_reduction_len = 2;
+  const size_t payload_size = 13;
+  const size_t min_num_packets = 3;  // Computed by hand.
+  RtpPacketizerGeneric packetizer(kVideoFrameKey, max_payload_length,
+                                  last_packet_reduction_len);
+  size_t num_packets =
+      packetizer.SetPayloadData(kTestPayload, payload_size, nullptr);
+  auto payload_sizes = NextPacketFillPayloadSizes(&packetizer);
+
+  EXPECT_THAT(payload_sizes, SizeIs(num_packets));
+  EXPECT_EQ(num_packets, min_num_packets);
+  EXPECT_THAT(payload_sizes, Each(Le(max_payload_length)));
+  EXPECT_LE(payload_sizes.back(),
+            max_payload_length - last_packet_reduction_len);
+  size_t sizes_difference = GetEffectivePacketsSizeDifference(
+      payload_sizes, last_packet_reduction_len);
+  EXPECT_LE(sizes_difference, 1u);
+}
+
+TEST(RtpPacketizerVideoGeneric, RespectsLastPacketReductionLength2) {
+  const size_t max_payload_length = 6;
+  const size_t last_packet_reduction_len = 3;
+  const size_t payload_size = 13;
+  const size_t min_num_packets = 4;  // Computed by hand.
+  RtpPacketizerGeneric packetizer(kVideoFrameKey, max_payload_length,
+                                  last_packet_reduction_len);
+  size_t num_packets =
+      packetizer.SetPayloadData(kTestPayload, payload_size, nullptr);
+  auto payload_sizes = NextPacketFillPayloadSizes(&packetizer);
+
+  EXPECT_THAT(payload_sizes, SizeIs(num_packets));
+  EXPECT_EQ(num_packets, min_num_packets);
+  EXPECT_THAT(payload_sizes, Each(Le(max_payload_length)));
+  EXPECT_LE(payload_sizes.back(),
+            max_payload_length - last_packet_reduction_len);
+  size_t sizes_difference = GetEffectivePacketsSizeDifference(
+      payload_sizes, last_packet_reduction_len);
+  EXPECT_LE(sizes_difference, 1u);
+}
+
+}  // namespace webrtc