Generalize FEC header formatting. (pt. 4)

webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc

+/*
+ *  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 {
+
+using Packet = ::webrtc::ForwardErrorCorrection::Packet;
+using PacketList = ::webrtc::ForwardErrorCorrection::PacketList;

[danilchap, 2016/09/02 09:54:55]

using?

[brandtr, 2016/09/02 11:09:14]

Nope. The compiler should complain about unused using directives :)

+using ReceivedFecPacket = ::webrtc::ForwardErrorCorrection::ReceivedFecPacket;
+
+constexpr uint32_t kMediaSsrc = 1254983;
+constexpr uint16_t kMediaStartSeqNum = 825;
+constexpr size_t kMediaPacketLength = 1234;
+
+constexpr size_t kUlpfecHeaderSizeLBitClear = 14;
+constexpr size_t kUlpfecHeaderSizeLBitSet = 18;
+constexpr size_t kUlpfecPacketMaskOffset = 12;
+
+std::unique_ptr<uint8_t[]> GeneratePacketMask(size_t packet_mask_size,
+                                              uint64_t seed) {
+  Random random(seed);
+  std::unique_ptr<uint8_t[]> packet_mask(new uint8_t[kUlpfecMaxPacketMaskSize]);

[danilchap, 2016/09/02 09:54:55]

Does it have to be MaxPacketMasksSize? may be packet_mask_size is enough.

[brandtr, 2016/09/02 11:09:14]

Here, that would be fine. But in the Flexfec unit tests (here:
https://codereview.webrtc.org/2269903002/diff/450001/webrtc/modules/rtp_rtcp/...)
I need to make the distinction. This is because of the memcmp in VerifyHeader is
comparing effective packet mask sizes larger than the precomputed (i.e. Ulpfec)
packet masks, and I don't want to memcmp the possibly trailing garbage.

What do you think, symmetry (i.e. keep it as is) or asymmetry (make a small
change here)? I think I'm biased towards symmetry, but that might be a "easy to
write" thing rather than an "easy to read" thing.

[danilchap, 2016/09/02 11:34:58]

I would vote for asymmetry in this case: Since it is different for ulpfec and
flexfec, make it look different.
Do not ask reader to guess or look into different file to understand why
capacity and size are different., specially that max size constant has a word
'Ulpfec' in it.

[brandtr, 2016/09/02 11:55:29]

Done.

+  memset(packet_mask.get(), 0, kUlpfecMaxPacketMaskSize);
+  for (size_t i = 0; i < packet_mask_size; ++i) {
+    packet_mask[i] = random.Rand<uint8_t>();
+  }
+  return packet_mask;
+}
+
+std::unique_ptr<Packet> WriteHeader(const uint8_t* packet_mask,
+                                    size_t packet_mask_size) {
+  UlpfecHeaderWriter writer;
+  std::unique_ptr<Packet> written_packet(new Packet());
+  written_packet->length = kMediaPacketLength;
+  for (size_t i = 0; i < written_packet->length; ++i) {
+    written_packet->data[i] = i;  // Actual content doesn't matter.
+  }
+  writer.FinalizeFecHeader(kMediaStartSeqNum, packet_mask, packet_mask_size,
+                           written_packet.get());
+  return written_packet;
+}
+
+std::unique_ptr<ReceivedFecPacket> ReadHeader(const Packet& written_packet) {
+  UlpfecHeaderReader reader;
+  std::unique_ptr<ReceivedFecPacket> read_packet(new ReceivedFecPacket());
+  read_packet->ssrc = kMediaSsrc;
+  read_packet->protected_ssrc = kMediaSsrc;

[danilchap, 2016/09/02 09:54:55]

shouldn't ReadFecHeader set this?

[brandtr, 2016/09/02 11:09:14]

Ehm, yeah. It should definitely not be here.

+  read_packet->pkt = rtc::scoped_refptr<Packet>(new Packet());
+  memcpy(read_packet->pkt->data, written_packet.data, written_packet.length);
+  read_packet->pkt->length = written_packet.length;
+  EXPECT_TRUE(reader.ReadFecHeader(read_packet.get()));
+  return read_packet;
+}
+
+void VerifyHeaders(size_t expected_fec_header_size,
+                   const uint8_t* expected_packet_mask,
+                   size_t expected_packet_mask_size,
+                   const Packet& written_packet,
+                   const ReceivedFecPacket& read_packet) {
+  EXPECT_EQ(kMediaSsrc, read_packet.ssrc);
+  EXPECT_EQ(expected_fec_header_size, read_packet.fec_header_size);
+  EXPECT_EQ(kMediaSsrc, read_packet.protected_ssrc);
+  EXPECT_EQ(kMediaStartSeqNum, read_packet.seq_num_base);
+  EXPECT_EQ(kUlpfecPacketMaskOffset, read_packet.packet_mask_offset);
+  ASSERT_EQ(expected_packet_mask_size, read_packet.packet_mask_size);
+  EXPECT_EQ(written_packet.length - expected_fec_header_size,
+            read_packet.protection_length);
+  EXPECT_EQ(0, memcmp(expected_packet_mask,
+                      &read_packet.pkt->data[read_packet.packet_mask_offset],
+                      read_packet.packet_mask_size));
+  // Verify that the call to ReadFecHeader did not tamper with the payload.
+  EXPECT_EQ(0, memcmp(&written_packet.data[expected_fec_header_size],
+                      &read_packet.pkt->data[expected_fec_header_size],
+                      written_packet.length - expected_fec_header_size));
+}
+
+}  // namespace
+
+TEST(UlpfecHeaderReaderWriterTest, CalculateSmallHeaderSize) {
+  UlpfecHeaderWriter writer;
+  auto packet_mask = GeneratePacketMask(kUlpfecPacketMaskSizeLBitClear, 0xabcd);
+  EXPECT_EQ(kUlpfecHeaderSizeLBitClear,
+            writer.FecHeaderSize(writer.MinPacketMaskSize(

[danilchap, 2016/09/02 09:54:55]

possible to test these two methods separately?
Or caller should always use them together?

[brandtr, 2016/09/02 11:09:14]

Currently, the only public caller uses these methods together. I'll split it
into two expectations though.

+                packet_mask.get(), kUlpfecPacketMaskSizeLBitClear)));
+}
+
+TEST(UlpfecHeaderReaderWriterTest, CalculateLargeHeaderSize) {
+  UlpfecHeaderWriter writer;
+  auto packet_mask = GeneratePacketMask(kUlpfecPacketMaskSizeLBitSet, 0xabcd);
+  EXPECT_EQ(kUlpfecHeaderSizeLBitSet,
+            writer.FecHeaderSize(writer.MinPacketMaskSize(
+                packet_mask.get(), kUlpfecPacketMaskSizeLBitSet)));
+}
+
+TEST(UlpfecHeaderReaderWriterTest, WriteAndReadSmallHeader) {
+  auto packet_mask = GeneratePacketMask(kUlpfecPacketMaskSizeLBitClear, 0xabcd);

[danilchap, 2016/09/02 09:54:55]

may be add size_t packet_mask_size = kUlpfecPacketMaskSizeLBitClear and use that
local variable inside the test.
Would be make more clear why packet_mask and kUlpfecPacketMaskSizeLBitClear go
together.

[brandtr, 2016/09/02 11:09:14]

Done. I made it constexpr and named it accordingly however, let me know what you
think.

[danilchap, 2016/09/02 11:34:58]

I think using non-constexpr variable packet_mask_size would be cleaner because
that would better align with packet_mask variable.
It is not really a constant because it take different values in different tests.
If you want to stress it will not change during the test, make it const:
const size_t packet_mask_size = kUlpfecPacketMaskSizeLBitClear;

[brandtr, 2016/09/02 11:55:29]

Done.

+  auto written_packet =
+      WriteHeader(packet_mask.get(), kUlpfecPacketMaskSizeLBitClear);
+  auto read_packet = ReadHeader(*written_packet);
+  VerifyHeaders(kUlpfecHeaderSizeLBitClear, packet_mask.get(),
+                kUlpfecPacketMaskSizeLBitClear, *written_packet, *read_packet);
+}
+
+TEST(UlpfecHeaderReaderWriterTest, WriteAndReadLargeHeader) {
+  auto packet_mask = GeneratePacketMask(kUlpfecPacketMaskSizeLBitSet, 0xabcd);
+  auto written_packet =
+      WriteHeader(packet_mask.get(), kUlpfecPacketMaskSizeLBitSet);
+  auto read_packet = ReadHeader(*written_packet);
+  VerifyHeaders(kUlpfecHeaderSizeLBitSet, packet_mask.get(),
+                kUlpfecPacketMaskSizeLBitSet, *written_packet, *read_packet);
+}
+
+}  // namespace webrtc