Generalize FEC header formatting. (pt. 4)

webrtc/modules/rtp_rtcp/source/fec_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/modules/rtp_rtcp/source/fec_test_helper.h"
+#include "webrtc/modules/rtp_rtcp/source/forward_error_correction.h"
+#include "webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.h"
+
+namespace webrtc {
+
+namespace {
+// Transport header size in bytes. Assume UDP/IPv4 as a reasonable minimum.
+constexpr size_t kTransportOverhead = 28;
+
+// ULPFEC header: largest packet mask.
+constexpr size_t kUlpfecMaxHeaderSize = 18;
+
+constexpr uint32_t kMediaSsrc = 1254983;
+}  // namespace
+
+class FecPacketizationTest : public ::testing::Test {
+ protected:
+  FecPacketizationTest(std::unique_ptr<FecHeaderReader> fec_header_reader,
+                       std::unique_ptr<FecHeaderWriter> fec_header_writer,
+                       size_t min_packet_size,
+                       size_t max_packet_size)
+      : fec_header_reader_(std::move(fec_header_reader)),
+        fec_header_writer_(std::move(fec_header_writer)),
+        random_(0xabcdef123456),
+        media_packet_generator_(min_packet_size,
+                                max_packet_size,
+                                kMediaSsrc,
+                                &random_),
+        packet_mask_() {}

[danilchap, 2016/08/24 16:41:27]

needed?

[brandtr, 2016/08/25 13:23:07]

Nope, gone.

+
+  // Set the corresponding bits of the ULPFEC packet mask.
+  void Bit15Clear();

[danilchap, 2016/08/24 16:41:26]

May be have just two functions instead:
void ClearBit(size_t index) {
  packet_mask_[index / 8] &= ~(1 << (7 - index % 8));
}
void SetBit(size_t index) {
  packet_mask_[index / 8] |= (1 << (7 - index % 8));
}
Or one:
void SetBit(size_t bit_index, bool set) {
  size_t byte_index = bit_index / 8;
  uint8_t bit = (1 << (7 - bit_index % 8));
  if (set)
    packet_mask_[byte_index] |= bit;
  else
    packet_mask_[byte_index] &= ~bit;
}
Two looks cleaner.

[brandtr, 2016/08/25 13:23:07]

Nice, thanks!

+  void Bit15Set();
+  void Bit46ClearBit47Clear();
+  void Bit46SetBit47Clear();
+  void Bit46ClearBit47Set();
+  void Bit46SetBit47Set();
+
+  // Randomly generate ULPFEC packet mask of specified size.
+  void GenerateUlpfecPacketMask(size_t packet_mask_size);

[danilchap, 2016/08/24 16:41:27]

Name of this function hints it is ulp specific. May be move it into
UlpfecPacketizationTest? Or rename to GenerateRandomPacketMask(size_t
packet_mask_size);

[brandtr, 2016/08/25 13:23:07]

It sounds like it is Ulpfec specific, but it's not. The parity erasure code
currently always uses Ulpfec packet masks, even though the FEC header formatting
might be done using Flexfec. So this function simply generates a packet mask
according to the 'Ulpfec format', which may then be used by either Ulpfec or
Flexfec. Therefore, it belongs in the general class.

Renamed to 'GenerateUlpfecFormattedPacketMask' to reduce confusion.

+
+  void ReadWriteAndVerifyHeaders(size_t ulpfec_packet_mask_size,

[danilchap, 2016/08/24 16:41:27]

Parameter name hints it is ulp specific.

[brandtr, 2016/08/25 13:23:07]

Same thing here. ulpfec_packet_mask_size corresponds to the size of the Ulpfec
packet mask, which is not the same as the 'Flexfecized' version of the packet
mask.

Renamed to 'ulpfec_formatted_packet_mask_size'.

+                                 size_t expected_fec_header_size,
+                                 size_t expected_packet_mask_size);
+
+  virtual void MaybeSetFecSsrcToMediaSsrc(
+      uint32_t media_ssrc,
+      ForwardErrorCorrection::ReceivedFecPacket* fec_packet) = 0;
+
+  virtual size_t ExpectedPacketMaskOffset() = 0;
+
+  std::unique_ptr<FecHeaderReader> fec_header_reader_;
+  std::unique_ptr<FecHeaderWriter> fec_header_writer_;
+
+  Random random_;
+  test::fec::MediaPacketGenerator media_packet_generator_;
+  ForwardErrorCorrection::PacketList* media_packets_;
+  uint8_t packet_mask_[14];  // Only filled with ULPFEC packet mask (6 bytes),

[danilchap, 2016/08/24 16:41:26]

may be add constant kMaxPacketMaskSize instead of 14 since that constants also
used in one of the tests.

[brandtr, 2016/08/25 13:23:07]

Done.

+                             // but needs to be 14 bytes in order to do
+                             // memcmp with parsed FlexFEC mask.
+};
+
+class UlpfecPacketizationTest : public FecPacketizationTest {
+ protected:
+  UlpfecPacketizationTest()
+      : FecPacketizationTest(
+            std::unique_ptr<FecHeaderReader>(new UlpfecHeaderReader()),
+            std::unique_ptr<FecHeaderWriter>(new UlpfecHeaderWriter()),
+            kRtpHeaderSize,
+            IP_PACKET_SIZE - kRtpHeaderSize - kTransportOverhead -
+                kUlpfecMaxHeaderSize) {}
+
+  void MaybeSetFecSsrcToMediaSsrc(
+      uint32_t media_ssrc,
+      ForwardErrorCorrection::ReceivedFecPacket* fec_packet) {

[danilchap, 2016/08/24 16:41:26]

override

[brandtr, 2016/08/25 13:23:07]

Hm, seems that the compiler does not warn about missing overrides when the
function is inlined?

+    fec_packet->rtp_ssrc = media_ssrc;
+  }
+
+  size_t ExpectedPacketMaskOffset() { return 12u; }

[danilchap, 2016/08/24 16:41:26]

add override
(otherwise it raise question why you need a function instead of a constant)

[brandtr, 2016/08/25 13:23:07]

Done.

+};
+
+// Verifies that the header size is correctly calculated.
+TEST_F(UlpfecPacketizationTest, HeaderSizeTest) {

[danilchap, 2016/08/24 16:41:27]

using 'Test' in Test name feels unnecessary (there is already word 'Test' in the
class name).
But test name could be more descriptive:
in addition to what it is about (HeaderSize), what is checked
(CalculatedCorrectly)
it also may hint setup, when it is calcuated:
TEST_F(UlpfecPacketizationTest, HeaderWriterCalculatesMinPacketSize)
or is it HeaderWriterCalcuatesShortMaskHeaderSize?

[danilchap, 2016/08/24 16:41:27]

put tests below helper functions.
It is also ok to 'inline' functions in the _unittest.cc files.
(They will appear in one .cc and thus in single .o anyway)

[brandtr, 2016/08/25 13:23:07]

Good point! Renamed.

[brandtr, 2016/08/25 13:23:07]

Moved helper functions.

I don't like the clutter of the inlined functions, so I keep the definitions
outside the class declaration.

+  constexpr size_t kShortMask = kUlpfecPacketMaskSizeLBitClear;
+  GenerateUlpfecPacketMask(kShortMask);
+  EXPECT_EQ(14u, fec_header_writer_->FecHeaderSize(

[danilchap, 2016/08/24 16:41:26]

why 14 is a correct value? May be name the constant.

[brandtr, 2016/08/25 13:23:07]

Done.

+                     fec_header_writer_->MinPacketMaskSize(packet_mask_,
+                                                           kShortMask)));
+

[danilchap, 2016/08/24 16:41:26]

Theese are two separate tests,
each with own setup, own run, own expectation check.
Split them.

[brandtr, 2016/08/25 13:23:07]

Done.

+  constexpr size_t kLongMask = kUlpfecPacketMaskSizeLBitSet;
+  GenerateUlpfecPacketMask(kLongMask);
+  EXPECT_EQ(
+      18u, fec_header_writer_->FecHeaderSize(

[danilchap, 2016/08/24 16:41:26]

may be kUlpfecMaxHeaderSize insteaf of 18?

[brandtr, 2016/08/25 13:23:07]

Done.

+               fec_header_writer_->MinPacketMaskSize(packet_mask_, kLongMask)));
+}
+
+// Verifies that the reading and writing operations are consistent.
+TEST_F(UlpfecPacketizationTest, ReaderWriterTest) {

[danilchap, 2016/08/24 16:41:27]

Same, omit 'Test' in test name, but put more description of the tested scenario:
'WriteAndReadWithShortMask', 'ReadMatchWriteWithShortMask'; 

[brandtr, 2016/08/25 13:23:07]

Done.

+  SCOPED_TRACE("");  // To get call site line number, if failing.
+
+  constexpr size_t kShortMask = kUlpfecPacketMaskSizeLBitClear;
+  GenerateUlpfecPacketMask(kShortMask);
+  ReadWriteAndVerifyHeaders(kShortMask, 14u, 2u);
+
+  constexpr size_t kLongMask = kUlpfecPacketMaskSizeLBitSet;
+  GenerateUlpfecPacketMask(kLongMask);
+  ReadWriteAndVerifyHeaders(kLongMask, 18u, 6u);
+}
+
+void FecPacketizationTest::Bit15Clear() {
+  packet_mask_[1] &= 0xfe;
+}
+
+void FecPacketizationTest::Bit15Set() {
+  packet_mask_[1] |= 0x01;
+}
+
+void FecPacketizationTest::Bit46ClearBit47Clear() {
+  packet_mask_[5] &= 0xfc;
+}
+
+void FecPacketizationTest::Bit46SetBit47Clear() {
+  Bit46ClearBit47Clear();
+  packet_mask_[5] ^= 0x02;
+}
+
+void FecPacketizationTest::Bit46ClearBit47Set() {
+  Bit46ClearBit47Clear();
+  packet_mask_[5] ^= 0x01;
+}
+
+void FecPacketizationTest::Bit46SetBit47Set() {
+  packet_mask_[5] |= 0x03;
+}
+
+void FecPacketizationTest::GenerateUlpfecPacketMask(size_t packet_mask_size) {
+  RTC_DCHECK_LE(packet_mask_size, 6u);
+  memset(packet_mask_, 0u, 14);
+  for (size_t i = 0; i < packet_mask_size; ++i) {
+    packet_mask_[i] = random_.Rand<uint8_t>();
+  }
+}
+
+void FecPacketizationTest::ReadWriteAndVerifyHeaders(
+    size_t ulpfec_packet_mask_size,
+    size_t expected_fec_header_size,
+    size_t expected_packet_mask_size) {
+  // Generate media.
+  constexpr int kNumMediaPackets = 1;
+  ForwardErrorCorrection::PacketList media_packets =
+      media_packet_generator_.ConstructMediaPackets(kNumMediaPackets);
+  uint16_t media_seq_num =
+      media_packet_generator_.GetFecSeqNum() - kNumMediaPackets;
+
+  // Set up FEC packet containers.
+  ForwardErrorCorrection::Packet* sent_fec_packet =
+      new ForwardErrorCorrection::Packet();  // deleted by scoped_refptr in

[danilchap, 2016/08/24 16:41:26]

Remove the comment and make the type scoped_refptr instead of the raw pointer.
Perfomance decrease would be negligible compare to readability increase.

[brandtr, 2016/08/25 13:23:07]

Good idea!! This isn't the way it's done in the current FEC code, but that
definitely makes it more readable.

Of course, these packet classes will be refactored later on...

+                                             // |received_fec_packet|
+  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>();
+  }
+  ForwardErrorCorrection::ReceivedFecPacket received_fec_packet;
+  received_fec_packet.pkt = sent_fec_packet;
+
+  // Write FEC header.
+  fec_header_writer_->FinalizeFecHeader(
+      media_packets, packet_mask_, ulpfec_packet_mask_size, sent_fec_packet);
+
+  // Read FEC header.
+  MaybeSetFecSsrcToMediaSsrc(kMediaSsrc, &received_fec_packet);
+  EXPECT_TRUE(fec_header_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,
+            received_fec_packet.protection_length);
+  auto stream_packet_mask_info =
+      received_fec_packet.packet_mask_infos[kMediaSsrc];
+  EXPECT_EQ(media_seq_num, stream_packet_mask_info.seq_num_base);
+  size_t offset = stream_packet_mask_info.offset;
+  EXPECT_EQ(ExpectedPacketMaskOffset(), 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],
+                      &received_fec_packet.pkt->data[expected_fec_header_size],
+                      sent_fec_packet->length - expected_fec_header_size));
+}
+
+}  // namespace webrtc