Generalize FEC header formatting. (pt. 4)

webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc

Index: webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc
diff --git a/webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc b/webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1d0fbdc0f046fcd6f27edc249f9ad33f47abbfa0
--- /dev/null
+++ b/webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc
@@ -0,0 +1,164 @@
+/*
+ *  Copyright (c) 2016 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.h>
+
+#include <memory>
+#include <utility>
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "webrtc/base/basictypes.h"
+#include "webrtc/base/checks.h"
+#include "webrtc/base/random.h"
+#include "webrtc/base/scoped_ref_ptr.h"
+#include "webrtc/modules/rtp_rtcp/source/fec_test_helper.h"
+#include "webrtc/modules/rtp_rtcp/source/forward_error_correction.h"
+#include "webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.h"
+#include "webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.h"
+
+namespace webrtc {
+
+namespace {
+constexpr uint32_t kMediaSsrc = 1254983;
+constexpr uint16_t kMediaStartSeqNum = 825;
+
+constexpr size_t kTransportOverhead = 28;
+
+constexpr size_t kUlpfecHeaderSizeLBitClear = 14;
+constexpr size_t kUlpfecHeaderSizeLBitSet = 18;
+constexpr size_t kUlpfecMaxHeaderSize = kUlpfecHeaderSizeLBitSet;
+constexpr size_t kUlpfecPacketMaskOffset = 12;
+}  // namespace
+
+using Packet = ::webrtc::ForwardErrorCorrection::Packet;
+using PacketList = ::webrtc::ForwardErrorCorrection::PacketList;
+using ReceivedFecPacket = ::webrtc::ForwardErrorCorrection::ReceivedFecPacket;
+
+class UlpfecHeaderReaderWriterTest : public ::testing::Test {
+ protected:
+  UlpfecHeaderReaderWriterTest()
+      : random_(0xabcdef123456),
+        media_packet_generator_(kRtpHeaderSize,
+                                IP_PACKET_SIZE - kRtpHeaderSize -
+                                    kTransportOverhead -

[danilchap, 2016/08/29 12:26:03]

swap -kTranposrtOverhead and -kRtpHeaderSize
to match how this bytes are used in the packet:
1st transport headers, then rtp header, then ulpfec header.

might name this constant as well (kMaxRtpPayloadSize?) by calculating it in the
unnamed namespace.

[brandtr, 2016/08/30 07:12:42]

Done.

+                                    kUlpfecMaxHeaderSize,
+                                kMediaSsrc,
+                                &random_) {}
+
+  // Set the corresponding bits of the packet mask.
+  void ClearBit(size_t index);
+  void SetBit(size_t index);
+
+  // Randomly generate packet mask of specified size.
+  void GeneratePacketMask(size_t packet_mask_size);
+
+  void WriteReadAndVerifyHeaders(size_t original_packet_mask_size,

[danilchap, 2016/08/29 12:26:03]

possible to split this function into WriteRead and Verify?
to have clearer border-line betwen setup/run and expectation phases of a test.
as well as clarify what is passed between this phases.

[brandtr, 2016/08/30 07:12:42]

Done.

+                                 size_t expected_fec_header_size,
+                                 size_t expected_packet_mask_size);
+
+  UlpfecHeaderReader reader_;
+  UlpfecHeaderWriter writer_;
+
+  Random random_;
+  test::fec::MediaPacketGenerator media_packet_generator_;
+  PacketList* media_packets_;
+  uint8_t packet_mask_[kUlpfecMaxPacketMaskSize];
+};
+
+void UlpfecHeaderReaderWriterTest::ClearBit(size_t index) {
+  packet_mask_[index / 8] &= ~(1 << (7 - index % 8));
+}
+
+void UlpfecHeaderReaderWriterTest::SetBit(size_t index) {
+  packet_mask_[index / 8] |= (1 << (7 - index % 8));
+}
+
+void UlpfecHeaderReaderWriterTest::GeneratePacketMask(size_t packet_mask_size) {
+  RTC_DCHECK_LE(packet_mask_size, kUlpfecMaxPacketMaskSize);
+  memset(packet_mask_, 0u, sizeof(packet_mask_));
+  for (size_t i = 0; i < packet_mask_size; ++i) {
+    packet_mask_[i] = random_.Rand<uint8_t>();
+  }
+}
+
+void UlpfecHeaderReaderWriterTest::WriteReadAndVerifyHeaders(
+    size_t original_packet_mask_size,
+    size_t expected_fec_header_size,
+    size_t expected_packet_mask_size) {
+  // Generate media.
+  constexpr size_t kNumMediaPackets = 1;
+  PacketList media_packets =
+      media_packet_generator_.ConstructMediaPacketsSeqNum(kNumMediaPackets,
+                                                          kMediaStartSeqNum);
+
+  // Set up FEC packet containers.
+  ReceivedFecPacket received_fec_packet;

[danilchap, 2016/08/29 12:26:03]

there is no sending or receiving in this function. Only writing and reading.
may be call variable just 'fec_packet'

[brandtr, 2016/08/30 07:12:42]

Renamed.

+  received_fec_packet.rtp_ssrc = kMediaSsrc;
+  received_fec_packet.pkt = rtc::scoped_refptr<Packet>(new Packet());
+  Packet* sent_fec_packet = received_fec_packet.pkt.get();

[danilchap, 2016/08/29 12:26:03]

is it raw_fec_packet? or unparsed_fec_packet

[brandtr, 2016/08/30 07:12:42]

Renamed.

+  sent_fec_packet->length = media_packets.front()->length;
+  for (size_t i = 0; i < sent_fec_packet->length; ++i) {
+    sent_fec_packet->data[i] = random_.Rand<uint8_t>();
+  }
+
+  // Write FEC header.
+  writer_.FinalizeFecHeader(media_packets, packet_mask_,
+                            original_packet_mask_size, sent_fec_packet);
+
+  // Read FEC header.
+  EXPECT_TRUE(reader_.ReadFecHeader(&received_fec_packet));
+
+  // Verify that written and read FEC headers are consistent.
+  EXPECT_EQ(expected_fec_header_size, received_fec_packet.fec_header_size);
+  EXPECT_EQ(received_fec_packet.pkt->length - expected_fec_header_size,

[danilchap, 2016/08/29 12:26:03]

which variable is tested this line?
received_fec_packet.protection_length.
may be use sent_fec_packet instead of received_fec_packet.pkt to stress this
variable is known, not under test.

[brandtr, 2016/08/30 07:12:42]

Makes sense! protection_length is the variable tested here.

+            received_fec_packet.protection_length);
+  auto stream_packet_mask_info =
+      received_fec_packet.packet_mask_infos[kMediaSsrc];
+  EXPECT_EQ(kMediaStartSeqNum, stream_packet_mask_info.seq_num_base);
+  size_t offset = stream_packet_mask_info.offset;

[danilchap, 2016/08/29 12:26:03]

may be name local variables mask_offset/mask_size

[brandtr, 2016/08/30 07:12:42]

Done.

+  EXPECT_EQ(kUlpfecPacketMaskOffset, offset);
+  size_t size = stream_packet_mask_info.size;
+  ASSERT_EQ(expected_packet_mask_size, size);
+  EXPECT_EQ(0,
+            memcmp(packet_mask_, &received_fec_packet.pkt->data[offset], size));
+  EXPECT_EQ(0, memcmp(&sent_fec_packet->data[expected_fec_header_size],

[danilchap, 2016/08/29 12:26:03]

since sent_fec_packet = received_fec_packet.pkt (same refcpy) this test doesn't
look like test anything.
Can you clarify part I miss.

[brandtr, 2016/08/30 07:12:42]

Pretty glaring mistake here! I've changed so I'm making a copy.

+                      &received_fec_packet.pkt->data[expected_fec_header_size],
+                      sent_fec_packet->length - expected_fec_header_size));
+}
+
+TEST_F(UlpfecHeaderReaderWriterTest, CalculateSmallHeaderSize) {
+  GeneratePacketMask(kUlpfecPacketMaskSizeLBitClear);

[danilchap, 2016/08/29 12:26:03]

Does randomness of packet mask itself affects FecHeaderSize function in any way?
Does it for Flexfec?

[brandtr, 2016/08/30 07:12:42]

For Ulpfec, the contents of the packet mask does not affect the call to
FecHeaderSize.
For Flexfec, the contents do affect the call. This is controlled in that unit
test however, and not left up to chance.

+  EXPECT_EQ(kUlpfecHeaderSizeLBitClear,
+            writer_.FecHeaderSize(writer_.MinPacketMaskSize(
+                packet_mask_, kUlpfecPacketMaskSizeLBitClear)));
+}
+
+TEST_F(UlpfecHeaderReaderWriterTest, CalculateLargeHeaderSize) {
+  GeneratePacketMask(kUlpfecPacketMaskSizeLBitSet);
+  EXPECT_EQ(kUlpfecHeaderSizeLBitSet,
+            writer_.FecHeaderSize(writer_.MinPacketMaskSize(
+                packet_mask_, kUlpfecPacketMaskSizeLBitSet)));
+}
+
+TEST_F(UlpfecHeaderReaderWriterTest, WriteAndReadSmallHeader) {
+  GeneratePacketMask(kUlpfecPacketMaskSizeLBitClear);
+  WriteReadAndVerifyHeaders(kUlpfecPacketMaskSizeLBitClear,
+                            kUlpfecHeaderSizeLBitClear,
+                            kUlpfecPacketMaskSizeLBitClear);
+}
+
+TEST_F(UlpfecHeaderReaderWriterTest, WriteAndReadLargeHeader) {
+  GeneratePacketMask(kUlpfecPacketMaskSizeLBitSet);
+  WriteReadAndVerifyHeaders(kUlpfecPacketMaskSizeLBitSet,
+                            kUlpfecHeaderSizeLBitSet,
+                            kUlpfecPacketMaskSizeLBitSet);
+}
+
+}  // namespace webrtc