Add FlexFEC header formatters. (pt. 5)

webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.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 "webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h"
+
+#include <string.h>
+
+#include <utility>
+
+#include "webrtc/base/checks.h"
+#include "webrtc/modules/rtp_rtcp/source/byte_io.h"
+#include "webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.h"
+
+namespace webrtc {
+
+namespace {
+
+// Maximum number of media packets that can be protected in one batch.
+constexpr size_t kMaxMediaPackets = 48;  // Since we are reusing ULPFEC masks.
+
+// Maximum number of FEC packets stored inside ForwardErrorCorrection.
+constexpr size_t kMaxFecPackets = kMaxMediaPackets;
+
+// Size (in bytes) of packet masks, given number of K bits set.
+constexpr size_t kFlexfecPacketMaskSize0KBitsSet = 2;
+constexpr size_t kFlexfecPacketMaskSize1KBitsSet = 6;
+constexpr size_t kFlexfecPacketMaskSize2KBitsSet = 14;
+
+// Size (in bytes) of part of header which is not packet mask specific.
+constexpr size_t kBaseHeaderSize = 12;

[danilchap, 2016/09/06 09:40:53]

that doesn't include first SSRC and base Seq Num? why?

[brandtr, 2016/09/07 08:21:14]

Because in the multistream case, there will be several "rows" of SSRC and seq
num base. This constant shows the size of the header which does not have any
stream-specific information.

+
+// Size (in bytes) of part of header which is packet mask specific,
+// given number of K-bits set.
+constexpr size_t kAuxHeaderSize0KBitsSet = 6 + kFlexfecPacketMaskSize0KBitsSet;

[danilchap, 2016/09/06 09:40:53]

not sure how this constants helps.
It seems it would be nicer with kBaseHeaderSize = 18 and without this 4
constants.

[brandtr, 2016/09/07 08:21:14]

I've simplified it a bit. I now distinguish the base header (information that IS
NOT stream specific), the aux header (information that IS stream specific), and
the packet mask size.

+constexpr size_t kAuxHeaderSize1KBitsSet = 6 + kFlexfecPacketMaskSize1KBitsSet;
+constexpr size_t kAuxHeaderSize2KBitsSet = 6 + kFlexfecPacketMaskSize2KBitsSet;
+
+constexpr size_t kPacketMaskOffset = kBaseHeaderSize + 6;
+
+// Here we count the K-bits as belonging to the packet mask.
+// This can be used in conjunction with FlexfecHeaderWriter::MinPacketMaskSize,
+// which calculates a bound on the needed packet mask size including K-bits,
+// given a packet mask without K-bits.
+size_t FlexfecHeaderSize(size_t packet_mask_size) {
+  RTC_DCHECK_LE(packet_mask_size, kFlexfecPacketMaskSize2KBitsSet);
+  if (packet_mask_size <= kFlexfecPacketMaskSize0KBitsSet) {
+    return kBaseHeaderSize + kAuxHeaderSize0KBitsSet;
+  } else if (packet_mask_size <= kFlexfecPacketMaskSize1KBitsSet) {
+    return kBaseHeaderSize + kAuxHeaderSize1KBitsSet;
+  }
+  return kBaseHeaderSize + kAuxHeaderSize2KBitsSet;
+}
+
+}  // namespace
+
+FlexfecHeaderReader::FlexfecHeaderReader()
+    : FecHeaderReader(kMaxMediaPackets, kMaxFecPackets) {}
+
+FlexfecHeaderReader::~FlexfecHeaderReader() = default;
+
+// TODO(brandtr): Update this function when we support flexible masks,
+// retransmissions, and/or several protected SSRCs.
+bool FlexfecHeaderReader::ReadFecHeader(
+    ForwardErrorCorrection::ReceivedFecPacket* fec_packet) const {
+  bool f_bit = (fec_packet->pkt->data[0] & 0x80) != 0u;

[danilchap, 2016/09/06 09:40:53]

will (win/android) compiler complain if you write "!= 0"?

[brandtr, 2016/09/07 08:21:14]

Not sure, trying without the 'u'.

If I remember correctly, the Win compiler complained if I didn't explicitly
convert to a boolean using "!= 0".

[danilchap, 2016/09/07 12:41:17]

!= 0 helps to see explicit conversion from bit to bool.
If you do not like it, you might write
uint8_t f_bit = fec_packet->data[0] & 0x80;
if (f_bit) {....

+  if (f_bit) {
+    return false;
+  }
+  bool r_bit = (fec_packet->pkt->data[0] & 0x40) != 0u;
+  if (r_bit) {
+    return false;
+  }
+  uint8_t ssrc_count =
+      ByteReader<uint8_t>::ReadBigEndian(&fec_packet->pkt->data[8]);
+  if (ssrc_count != 1u) {
+    return false;
+  }
+  uint32_t protected_ssrc =
+      ByteReader<uint32_t>::ReadBigEndian(&fec_packet->pkt->data[12]);
+  uint16_t seq_num_base =
+      ByteReader<uint16_t>::ReadBigEndian(&fec_packet->pkt->data[16]);
+
+  // Read packet mask and pack it tight by removing the interleaved K bits.
+  // This destroys the FlexFEC standards compliance of the packet masks,
+  // but makes it compatible with the ULPFEC masks.
+  //
+  // We treat the mask parts as unsigned integers with host order endianness
+  // in order to simplify the bit shifting between bytes.
+  bool k_bit0 = false, k_bit1 = false, k_bit2 = false;

[danilchap, 2016/09/06 09:40:53]

prefer 3 lines.
Also it might be good idea to declare on first assign.
It seems it would work in this case as long as you move calculation
packet_mask_size into main switches.

[brandtr, 2016/09/07 08:21:13]

Done.

+  uint16_t mask_part0 =
+      ByteReader<uint16_t>::ReadBigEndian(&fec_packet->pkt->data[18]);
+  k_bit0 = (mask_part0 & (1 << 15)) != 0u;  // Read K-bit 0.
+  mask_part0 <<= 1;                         // Shift away K-bit 0.
+  ByteWriter<uint16_t>::WriteBigEndian(&fec_packet->pkt->data[18], mask_part0);

[danilchap, 2016/09/06 09:40:53]

Can Writing to the data you are reading from
in the ReadSomething function be avoided?

[brandtr, 2016/09/07 08:21:14]

It will be in the future, when I do the refactoring of the Packet classes. Then
this function will be a typical parsing function.

+  if (!k_bit0) {

[danilchap, 2016/09/06 09:40:53]

did you check if base/bitbuffer can make your code cleaner?

[brandtr, 2016/09/07 08:21:13]

I considered it, but I found the explicit masking to be clearer.

+    uint32_t mask_part1 =
+        ByteReader<uint32_t>::ReadBigEndian(&fec_packet->pkt->data[20]);
+    k_bit1 = (mask_part1 & (1 << 31)) != 0u;      // Read K-bit 1.
+    bool bit15 = (mask_part1 & (1 << 30)) != 0u;  // Read bit 15.
+    if (bit15) {
+      fec_packet->pkt->data[19] |= 0x01;  // Set bit 15.
+    }
+    mask_part1 <<= 2;  // Shift away K-bit 1 and bit 15.
+    ByteWriter<uint32_t>::WriteBigEndian(&fec_packet->pkt->data[20],
+                                         mask_part1);
+    if (!k_bit1) {
+      uint64_t mask_part2 =
+          ByteReader<uint64_t>::ReadBigEndian(&fec_packet->pkt->data[24]);
+      uint64_t one = 1;  // Ensures enough bits available for the bit shifts.

[danilchap, 2016/09/06 09:40:53]

may be use 1ULL instead of this constant?

[brandtr, 2016/09/07 08:21:14]

Done.

+      k_bit2 = (mask_part2 & (one << 63)) != 0u;  // Read K-bit 2.
+      if (!k_bit2) {
+        // Malformed header.
+        return false;
+      }
+      bool bit46 = (mask_part2 & (one << 62)) != 0u;  // Read bit 46.
+      bool bit47 = (mask_part2 & (one << 61)) != 0u;  // Read bit 47.
+      if (bit46) {
+        fec_packet->pkt->data[23] |= 0x01;  // Set bit 46.
+      }
+      if (bit47) {
+        fec_packet->pkt->data[23] |= 0x02;  // Set bit 47.
+      }
+      mask_part2 <<= 3;  // Shift away K-bit 2, bit 46, and bit 47.
+      ByteWriter<uint64_t>::WriteBigEndian(&fec_packet->pkt->data[24],
+                                           mask_part2);
+    }
+  }
+
+  // Store "ULPFECized" packet mask info.
+  size_t packet_mask_size;
+  if (k_bit0) {
+    packet_mask_size = kFlexfecPacketMaskSize0KBitsSet;
+  } else if (k_bit1) {
+    packet_mask_size = kFlexfecPacketMaskSize1KBitsSet;
+  } else {
+    packet_mask_size = kFlexfecPacketMaskSize2KBitsSet;
+  }
+  fec_packet->fec_header_size = FlexfecHeaderSize(packet_mask_size);
+  fec_packet->protected_ssrc = protected_ssrc;
+  fec_packet->seq_num_base = seq_num_base;
+  fec_packet->packet_mask_offset = kPacketMaskOffset;
+  fec_packet->packet_mask_size = packet_mask_size;
+
+  // In FlexFEC, all media packets are protected in their entirety.
+  fec_packet->protection_length =
+      fec_packet->pkt->length - fec_packet->fec_header_size;
+
+  return true;
+}
+
+FlexfecHeaderWriter::FlexfecHeaderWriter()
+    : FecHeaderWriter(kMaxMediaPackets,
+                      kMaxFecPackets,
+                      kBaseHeaderSize + kAuxHeaderSize2KBitsSet) {}
+
+FlexfecHeaderWriter::~FlexfecHeaderWriter() = default;
+
+size_t FlexfecHeaderWriter::MinPacketMaskSize(const uint8_t* packet_mask,
+                                              size_t packet_mask_size) const {
+  bool ulpfec_l_bit = (packet_mask_size == kUlpfecPacketMaskSizeLBitSet);

[danilchap, 2016/09/06 09:40:53]

does code in this file rely on packet_mask_size beeing one of two sizes?
Even if you does, it may be cleaner to DCHECK and calculate PacketMask
separately:
if (packet_mask_size < 2 && (packet_mask[1] % 0x01) != 0)
  return 0KBits;
} else if (packet_mask_size < && )
  return 1KBits;
else if (...)
else {
  NOT_REACHABLE() << "Mask " << packet_mask_size << " too big";
}

[brandtr, 2016/09/07 08:21:14]

Done.

+  if (ulpfec_l_bit) {
+    // The packet mask is 48 bits long.
+    bool bit46 = (packet_mask[5] & 0x01) != 0u;

[danilchap, 2016/09/06 09:40:53]

can you operator on bit46 and bit47 togehter?
if (packet_mask[5] & 0x03) {
  // Comment explaining why it doesn't fit.
  return k..2KBitsSet;
}

[brandtr, 2016/09/07 08:21:13]

Done.

+    bool bit47 = (packet_mask[5] & 0x02) != 0u;
+    if (bit46 || bit47) {
+      return kFlexfecPacketMaskSize2KBitsSet;
+    }
+    return kFlexfecPacketMaskSize1KBitsSet;
+  } else {
+    RTC_DCHECK_EQ(packet_mask_size, kUlpfecPacketMaskSizeLBitClear);
+    // The packet mask is 16 bits long.
+    bool bit15 = (packet_mask[1] & 0x01) != 0u;
+    if (bit15) {
+      return kFlexfecPacketMaskSize1KBitsSet;
+    }
+    return kFlexfecPacketMaskSize0KBitsSet;
+  }
+}
+
+size_t FlexfecHeaderWriter::FecHeaderSize(size_t packet_mask_size) const {
+  return FlexfecHeaderSize(packet_mask_size);
+}
+
+// This function adapts the precomputed ULPFEC packet masks to the
+// FlexFEC header standard. Note that the header size is computed by
+// FecHeaderSize(), so in this function we can be sure that we are
+// writing in space that is intended for the header.
+//
+// TODO(brandtr): Update this function when we support offset-based masks,
+// retransmissions, and protecting multiple SSRCs.
+void FlexfecHeaderWriter::FinalizeFecHeader(
+    uint32_t ssrc,
+    uint16_t seq_num_base,
+    const uint8_t* packet_mask,
+    size_t packet_mask_size,
+    ForwardErrorCorrection::Packet* fec_packet) const {
+  fec_packet->data[0] &= 0x7f;  // Clear F bit.
+  fec_packet->data[0] &= 0xbf;  // Clear R bit.
+  // Write SSRC count, SSRC base, sequence number base.
+  ByteWriter<uint8_t>::WriteBigEndian(&fec_packet->data[8], 1u);
+  ByteWriter<uint32_t>::WriteBigEndian(&fec_packet->data[12], ssrc);
+  ByteWriter<uint16_t>::WriteBigEndian(&fec_packet->data[16], seq_num_base);
+  // Adapt ULPFEC packet mask to FlexFEC header.
+  //
+  // We treat the mask parts as unsigned integers with host order endianness
+  // in order to simplify the bit shifting between bytes.
+  bool ulpfec_l_bit = (packet_mask_size == kUlpfecPacketMaskSizeLBitSet);
+  if (ulpfec_l_bit) {
+    // The packet mask is 48 bits long.
+    uint16_t tmp_mask_part0 =
+        ByteReader<uint16_t>::ReadBigEndian(&packet_mask[0]);
+    uint32_t tmp_mask_part1 =
+        ByteReader<uint32_t>::ReadBigEndian(&packet_mask[2]);
+
+    tmp_mask_part0 >>= 1;  // Shift, thus clearing K-bit 0.
+    ByteWriter<uint16_t>::WriteBigEndian(&fec_packet->data[18], tmp_mask_part0);
+    tmp_mask_part1 >>= 2;  // Shift, thus clearing K-bit 1 and bit 15.
+    ByteWriter<uint32_t>::WriteBigEndian(&fec_packet->data[20], tmp_mask_part1);
+    bool bit15 = (packet_mask[1] & 0x01) != 0u;
+    if (bit15) {
+      fec_packet->data[20] |= 0x40;  // Set bit 15.
+    }
+    bool bit46 = (packet_mask[5] & 0x01) != 0u;
+    bool bit47 = (packet_mask[5] & 0x02) != 0u;
+    if (!bit46 && !bit47) {
+      fec_packet->data[20] |= 0x80;  // Set K-bit 1.
+    } else {
+      memset(&fec_packet->data[24], 0u, 8);  // Clear all trailing bits.
+      fec_packet->data[24] |= 0x80;          // Set K-bit 2.
+      if (bit46) {
+        fec_packet->data[24] |= 0x40;  // Set bit 46.
+      }
+      if (bit47) {
+        fec_packet->data[24] |= 0x20;  // Set bit 47.
+      }
+    }
+  } else {
+    RTC_DCHECK_EQ(packet_mask_size, kUlpfecPacketMaskSizeLBitClear);
+    // The packet mask is 16 bits long.
+    uint16_t tmp_mask_part0 =
+        ByteReader<uint16_t>::ReadBigEndian(&packet_mask[0]);
+
+    tmp_mask_part0 >>= 1;  // Shift, thus clearing K-bit 0.
+    ByteWriter<uint16_t>::WriteBigEndian(&fec_packet->data[18], tmp_mask_part0);
+    bool bit15 = (packet_mask[1] & 0x01) != 0u;
+    if (!bit15) {
+      fec_packet->data[18] |= 0x80;  // Set K-bit 0.
+    } else {
+      memset(&fec_packet->data[20], 0u, 4);  // Clear all trailing bits.
+      fec_packet->data[20] |= 0x80;          // Set K-bit 1.
+      fec_packet->data[20] |= 0x40;          // Set bit 15.
+    }
+  }
+}
+
+}  // namespace webrtc