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 {
+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;
+
+constexpr size_t kMaxRtpPayloadSize =
+    IP_PACKET_SIZE - kTransportOverhead - kRtpHeaderSize - kUlpfecMaxHeaderSize;
+}  // 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,
+                                kMaxRtpPayloadSize,
+                                kMediaSsrc,
+                                &random_) {}
+
+  // Set the corresponding bits of the packet mask.
+  void ClearBit(size_t index);

[danilchap, 2016/08/30 12:29:01]

Will these two helpers be used for ulpfec?

[brandtr, 2016/08/30 14:14:24]

Nope, removed!

+  void SetBit(size_t index);
+
+  // Randomly generate packet mask of specified size.
+  void GeneratePacketMask(size_t packet_mask_size);
+
+  std::pair<std::unique_ptr<Packet>, std::unique_ptr<ReceivedFecPacket>>
+  WriteAndReadHeaders(size_t original_packet_mask_size);
+
+  void VerifyHeaders(std::pair<std::unique_ptr<Packet>,

[danilchap, 2016/08/30 12:29:01]

to follow EXPECT macros style might be better to put expected values first,
values under test - last.

[brandtr, 2016/08/30 14:14:24]

Done.

+                               std::unique_ptr<ReceivedFecPacket>> packets,
+                     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_;

[danilchap, 2016/08/30 12:29:01]

looks unused

[brandtr, 2016/08/30 14:14:24]

Removed.

+  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>();
+  }
+}
+
+std::pair<std::unique_ptr<Packet>, std::unique_ptr<ReceivedFecPacket>>
+UlpfecHeaderReaderWriterTest::WriteAndReadHeaders(
+    size_t original_packet_mask_size) {
+  // Generate media.
+  constexpr size_t kNumMediaPackets = 1;
+  PacketList media_packets =
+      media_packet_generator_.ConstructMediaPacketsSeqNum(kNumMediaPackets,
+                                                          kMediaStartSeqNum);
+
+  // Write FEC header.
+  std::unique_ptr<Packet> packet_to_write(new Packet());
+  packet_to_write->length = media_packets.front()->length;

[danilchap, 2016/08/30 12:29:01]

looks like length and seq_no is all your need from media_packets.
May be do not generate media_packets, but add kMediaPacketLength instead.

[brandtr, 2016/08/30 14:14:24]

Done.

+  for (size_t i = 0; i < packet_to_write->length; ++i) {
+    packet_to_write->data[i] = random_.Rand<uint8_t>();
+  }
+  const uint16_t seq_num_base = ForwardErrorCorrection::ParseSequenceNumber(

[danilchap, 2016/08/30 12:29:01]

is it kMediaStartSeqNum?

[brandtr, 2016/08/30 14:14:24]

Yep, changed.

+      media_packets.front().get()->data);
+  writer_.FinalizeFecHeader(seq_num_base, packet_mask_,

[danilchap, 2016/08/30 12:29:01]

in another comment you wrote that XorHeaders should be called before calling
FinalizeFecHeader, but in the tests you do not.
Is it easy to adjust tests so that they can serve as a documentation how to use
writer_?

[brandtr, 2016/08/30 14:14:24]

It's not super-easy, since XorHeaders are in ForwardErrorCorrection which is one
layer about these FEC formatting classes. I could move the XorHeaders to the FEC
header formatters, but I should probably combine that move with moving the whole
packet mask logic too then. (This is on my maybe-list.)

(The FEC code had a lot of these order-dependent things when I started looking
at it. It's better now.)

+                            original_packet_mask_size, packet_to_write.get());
+
+  // Read FEC header.
+  std::unique_ptr<ReceivedFecPacket> packet_to_read(new ReceivedFecPacket());
+  packet_to_read->ssrc = kMediaSsrc;
+  packet_to_read->protected_ssrc = kMediaSsrc;
+  packet_to_read->pkt = rtc::scoped_refptr<Packet>(new Packet());
+  memcpy(packet_to_read->pkt->data, packet_to_write->data,
+         packet_to_write->length);
+  packet_to_read->pkt->length = packet_to_write->length;
+  EXPECT_TRUE(reader_.ReadFecHeader(packet_to_read.get()));
+
+  return std::make_pair(std::move(packet_to_write), std::move(packet_to_read));
+}
+
+void UlpfecHeaderReaderWriterTest::VerifyHeaders(
+    std::pair<std::unique_ptr<Packet>, std::unique_ptr<ReceivedFecPacket>>

[danilchap, 2016/08/30 12:29:01]

probably cleaner to take two parameters instead of one pair

[brandtr, 2016/08/30 14:14:24]

Done.

+        packets,
+    size_t expected_fec_header_size,
+    size_t expected_packet_mask_size) {
+  Packet* packet_to_write = packets.first.get();
+  ReceivedFecPacket* packet_to_read = packets.second.get();
+
+  // Verify that written and read FEC headers are consistent.
+  EXPECT_EQ(expected_fec_header_size, packet_to_read->fec_header_size);
+  EXPECT_EQ(packet_to_write->length - expected_fec_header_size,
+            packet_to_read->protection_length);
+  ReceivedFecPacket::StreamPacketMaskInfo stream_packet_mask_info =
+      packet_to_read->packet_mask_infos[kMediaSsrc];
+  EXPECT_EQ(kMediaStartSeqNum, stream_packet_mask_info.seq_num_base);
+  size_t packet_mask_offset = stream_packet_mask_info.offset;
+  EXPECT_EQ(kUlpfecPacketMaskOffset, packet_mask_offset);
+  size_t size = stream_packet_mask_info.size;
+  ASSERT_EQ(expected_packet_mask_size, size);
+  EXPECT_EQ(0, memcmp(packet_mask_,
+                      &packet_to_read->pkt->data[packet_mask_offset], size));
+  EXPECT_EQ(0, memcmp(&packet_to_write->data[expected_fec_header_size],

[danilchap, 2016/08/30 12:29:01]

What functionality of HeaderReaderWriter is tested by this expectation? 

As an almost last step of WriteAndReadHeaders
you do a memcpy from packet_to_write to packet_to_read.
here you check that memory is equal.

[brandtr, 2016/08/30 14:14:24]

This memcmp is to check that the ReadFecHeader function wouldn't tamper with the
payload. (For FlexFEC, the ReadFecHeader function 'normalizes' the packet mask
to Ulpfec format, so it actually does a bit more than just reading.)

+                      &packet_to_read->pkt->data[expected_fec_header_size],
+                      packet_to_write->length - expected_fec_header_size));
+}
+
+TEST_F(UlpfecHeaderReaderWriterTest, CalculateSmallHeaderSize) {
+  GeneratePacketMask(kUlpfecPacketMaskSizeLBitClear);
+  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);
+  VerifyHeaders(WriteAndReadHeaders(kUlpfecPacketMaskSizeLBitClear),

[danilchap, 2016/08/30 12:29:01]

for clearer separation of run  and verify phases it might be better to avoid
chaining, and, may be, split a bit more:

auto packet_to_write = WriteHeaders(original_packet_mask_size)
auto packet_to_read = ReadHeaders(packet_to_write->data,
packet_to_write->length);

VerifyFecHeadersAreSame(kExtraExpectationValues, *packet_to_write,
*packet_to_read);

[brandtr, 2016/08/30 14:14:24]

Done.

+                kUlpfecHeaderSizeLBitClear, kUlpfecPacketMaskSizeLBitClear);
+}
+
+TEST_F(UlpfecHeaderReaderWriterTest, WriteAndReadLargeHeader) {
+  GeneratePacketMask(kUlpfecPacketMaskSizeLBitSet);
+  VerifyHeaders(WriteAndReadHeaders(kUlpfecPacketMaskSizeLBitSet),
+                kUlpfecHeaderSizeLBitSet, kUlpfecPacketMaskSizeLBitSet);
+}
+
+}  // namespace webrtc