Add packetization and coding/decoding of feedback message format.

webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc

Index: webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e22bfa8324b17552f68cf1e24891a0c1d2aae8db
--- /dev/null
+++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc
@@ -0,0 +1,747 @@
+/*
+ *  Copyright (c) 2015 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/rtcp_packet/feedback_packet.h"
+
+#include "webrtc/base/checks.h"
+#include "webrtc/base/logging.h"
+#include "webrtc/modules/rtp_rtcp/source/byte_io.h"
+
+namespace webrtc {
+namespace rtcp {
+
+// Header size:
+// * 12 bytes Common Packet Format for RTCP Feedback Messages
+// * 8 bytes FeedbackPacket header
+static const int kHeaderSizeBytes = 12 + 8;
+static const int kChunkSizeBytes = 2;
+static const int kOneBitVectorCapacity = 14;
+static const int kTwoBitVectorCapacity = 7;
+static const int kRunLengthCapacity = 0x1FFF;
+// TODO(sprang): Add support for dynamic max size for easier fragmentation,
+// eg. set it to what's left in the buffer or IP_PACKET_SIZE.
+static const int kMaxSizeBytes = (1 << 16) * 4;
+static const int kMinSizeBytes = kHeaderSizeBytes + kChunkSizeBytes;
+static const int kBaseScaleFactor =
+    FeedbackPacket::kDeltaScaleFactor * (1 << 8);
+
+class PacketStatusChunk {
+ public:
+  virtual ~PacketStatusChunk() {}
+  virtual uint16_t NumSymbols() const = 0;
+  virtual void AppendSymbolsTo(
+      std::vector<FeedbackPacket::StatusSymbol>* vec) const = 0;
+  virtual void WriteTo(uint8_t* buffer) const = 0;
+};
+
+uint8_t EncodeSymbol(FeedbackPacket::StatusSymbol symbol) {
+  switch (symbol) {
+    case FeedbackPacket::StatusSymbol::kNotReceived:
+      return 0;
+    case FeedbackPacket::StatusSymbol::kReceivedSmallDelta:
+      return 1;
+    case FeedbackPacket::StatusSymbol::kReceivedLargeDelta:
+      return 2;
+    default:
+      abort();
+  }
+}
+
+FeedbackPacket::StatusSymbol DecodeSymbol(uint8_t value) {
+  switch (value) {
+    case 0:
+      return FeedbackPacket::StatusSymbol::kNotReceived;
+    case 1:
+      return FeedbackPacket::StatusSymbol::kReceivedSmallDelta;
+    case 2:
+      return FeedbackPacket::StatusSymbol::kReceivedLargeDelta;
+    default:
+      RTC_NOTREACHED();
+      return FeedbackPacket::StatusSymbol::kNotReceived;
+  }
+}
+
+FeedbackPacket::FeedbackPacket()
+    : packet_sender_ssrc_(0),
+      media_source_ssrc_(0),
+      base_seq_(-1),
+      base_time_(-1),
+      feedback_seq_(0),
+      last_seq_(-1),
+      last_timestamp_(-1),
+      first_symbol_cardinality_(0),
+      vec_needs_two_bit_symbols_(false),
+      size_bytes_(kHeaderSizeBytes) {
+}
+
+FeedbackPacket::~FeedbackPacket() {
+  for (PacketStatusChunk* chunk : status_chunks_)
+    delete chunk;
+}
+
+//  One Bit Status Vector Chunk
+//
+//  0                   1
+//  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5
+//  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+//  |T|S|       symbol list         |
+//  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+//
+//  T = 1
+//  S = 0
+//  symbol list = 14 entries where 0 = not received, 1 = received
+
+class OneBitVectorChunk : public PacketStatusChunk {
+ public:
+  static const int kCapacity = 14;
+
+  explicit OneBitVectorChunk(
+      std::deque<FeedbackPacket::StatusSymbol>* symbols) {
+    size_t input_size = symbols->size();
+    for (size_t i = 0; i < kCapacity; ++i) {
+      if (i < input_size) {
+        symbols_[i] = symbols->front();
+        symbols->pop_front();
+      } else {
+        symbols_[i] = FeedbackPacket::StatusSymbol::kNotReceived;
+      }
+    }
+  }
+
+  virtual ~OneBitVectorChunk() {}
+
+  uint16_t NumSymbols() const override { return kCapacity; }
+
+  void AppendSymbolsTo(
+      std::vector<FeedbackPacket::StatusSymbol>* vec) const override {
+    vec->insert(vec->end(), &symbols_[0], &symbols_[kCapacity]);
+  }
+
+  void WriteTo(uint8_t* buffer) const override {
+    buffer[0] = 0x80;
+    buffer[0] |= EncodeSymbol(symbols_[0]) << 5;
+    buffer[0] |= EncodeSymbol(symbols_[1]) << 4;
+    buffer[0] |= EncodeSymbol(symbols_[2]) << 3;
+    buffer[0] |= EncodeSymbol(symbols_[3]) << 2;
+    buffer[0] |= EncodeSymbol(symbols_[4]) << 1;
+    buffer[0] |= EncodeSymbol(symbols_[5]);
+    buffer[1] = EncodeSymbol(symbols_[6]) << 7;
+    buffer[1] |= EncodeSymbol(symbols_[7]) << 6;
+    buffer[1] |= EncodeSymbol(symbols_[8]) << 5;
+    buffer[1] |= EncodeSymbol(symbols_[9]) << 4;
+    buffer[1] |= EncodeSymbol(symbols_[10]) << 3;
+    buffer[1] |= EncodeSymbol(symbols_[11]) << 2;
+    buffer[1] |= EncodeSymbol(symbols_[12]) << 1;
+    buffer[1] |= EncodeSymbol(symbols_[13]);
+  }
+
+  static OneBitVectorChunk* ParseFrom(const uint8_t* data) {
+    OneBitVectorChunk* chunk = new OneBitVectorChunk();
+
+    size_t index = 0;
+    for (int i = 5; i >= 0; --i)  // Last 5 bits from first byte.
+      chunk->symbols_[index++] = DecodeSymbol((data[0] >> i) & 0x01);
+    for (int i = 7; i >= 0; --i)  // 8 bits from the last byte.
+      chunk->symbols_[index++] = DecodeSymbol((data[1] >> i) & 0x01);
+
+    return chunk;
+  }
+
+ private:
+  OneBitVectorChunk() {}
+
+  FeedbackPacket::StatusSymbol symbols_[kCapacity];
+};
+
+//  Two Bit Status Vector Chunk
+//
+//  0                   1
+//  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5
+//  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+//  |T|S|       symbol list         |
+//  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+//
+//  T = 1
+//  S = 1
+//  symbol list = 7 entries of two bits each, see (Encode|Decode)Symbol
+
+class TwoBitVectorChunk : public PacketStatusChunk {
+ public:
+  static const int kCapacity = 7;
+
+  explicit TwoBitVectorChunk(
+      std::deque<FeedbackPacket::StatusSymbol>* symbols) {
+    size_t input_size = symbols->size();
+    for (size_t i = 0; i < kCapacity; ++i) {
+      if (i < input_size) {
+        symbols_[i] = symbols->front();
+        symbols->pop_front();
+      } else {
+        symbols_[i] = FeedbackPacket::StatusSymbol::kNotReceived;
+      }
+    }
+  }
+
+  virtual ~TwoBitVectorChunk() {}
+
+  uint16_t NumSymbols() const override { return kCapacity; }
+
+  void AppendSymbolsTo(
+      std::vector<FeedbackPacket::StatusSymbol>* vec) const override {
+    vec->insert(vec->end(), &symbols_[0], &symbols_[kCapacity]);
+  }
+
+  void WriteTo(uint8_t* buffer) const override {
+    buffer[0] = 0xC0;
+    buffer[0] |= EncodeSymbol(symbols_[0]) << 4;
+    buffer[0] |= EncodeSymbol(symbols_[1]) << 2;
+    buffer[0] |= EncodeSymbol(symbols_[2]);
+    buffer[1] = EncodeSymbol(symbols_[3]) << 6;
+    buffer[1] |= EncodeSymbol(symbols_[4]) << 4;
+    buffer[1] |= EncodeSymbol(symbols_[5]) << 2;
+    buffer[1] |= EncodeSymbol(symbols_[6]);
+  }
+
+  static TwoBitVectorChunk* ParseFrom(const uint8_t* buffer) {
+    TwoBitVectorChunk* chunk = new TwoBitVectorChunk();
+
+    chunk->symbols_[0] = DecodeSymbol((buffer[0] >> 4) & 0x03);
+    chunk->symbols_[1] = DecodeSymbol((buffer[0] >> 2) & 0x03);
+    chunk->symbols_[2] = DecodeSymbol(buffer[0] & 0x03);
+    chunk->symbols_[3] = DecodeSymbol((buffer[1] >> 6) & 0x03);
+    chunk->symbols_[4] = DecodeSymbol((buffer[1] >> 4) & 0x03);
+    chunk->symbols_[5] = DecodeSymbol((buffer[1] >> 2) & 0x03);
+    chunk->symbols_[6] = DecodeSymbol(buffer[1] & 0x03);
+
+    return chunk;
+  }
+
+ private:
+  TwoBitVectorChunk() {}
+
+  FeedbackPacket::StatusSymbol symbols_[kCapacity];
+};
+
+//  Two Bit Status Vector Chunk
+//
+//  0                   1
+//  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5
+//  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+//  |T| S |       Run Length        |
+//  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+//
+//  T = 0
+//  S = symbol, see (Encode|Decode)Symbol
+//  Run Length = Unsigned integer denoting the run length of the symbol
+
+class RunLengthChunk : public PacketStatusChunk {
+ public:
+  RunLengthChunk(FeedbackPacket::StatusSymbol symbol, size_t size)
+      : symbol_(symbol), size_(size) {
+    DCHECK_LE(size, 0x1FFFu);
+  }
+
+  virtual ~RunLengthChunk() {}
+
+  uint16_t NumSymbols() const override { return size_; }
+
+  void AppendSymbolsTo(
+      std::vector<FeedbackPacket::StatusSymbol>* vec) const override {
+    vec->insert(vec->end(), size_, symbol_);
+  }
+
+  void WriteTo(uint8_t* buffer) const override {
+    buffer[0] = EncodeSymbol(symbol_) << 5;  // Write S (T = 0 implicitly)
+    buffer[0] |= (size_ >> 8) & 0x1F;  // 5 most significant bits of run length.
+    buffer[1] = size_ & 0xFF;  // 8 least significant bits of run length.
+  }
+
+  static RunLengthChunk* ParseFrom(const uint8_t* buffer) {
+    DCHECK_EQ(0, buffer[0] & 0x80);
+    FeedbackPacket::StatusSymbol symbol = DecodeSymbol((buffer[0] >> 5) & 0x03);
+    uint16_t count = (static_cast<uint16_t>(buffer[0] & 0x1F) << 8) | buffer[1];
+
+    return new RunLengthChunk(symbol, count);
+  }
+
+ private:
+  const FeedbackPacket::StatusSymbol symbol_;
+  const size_t size_;
+};
+
+// Unwrap to a larger type, for easier handling of wraps.
+int64_t FeedbackPacket::Unwrap(uint16_t sequence_number) {
+  if (last_seq_ == -1)
+    return sequence_number;
+
+  int64_t delta = sequence_number - last_seq_;
+  if (IsNewerSequenceNumber(sequence_number,
+                            static_cast<uint16_t>(last_seq_))) {
+    if (delta < 0)
+      delta += (1 << 16);
+  } else if (delta > 0) {
+    delta -= (1 << 16);
+  }
+
+  return last_seq_ + delta;
+}
+
+void FeedbackPacket::WithPacketSenderSsrc(uint32_t ssrc) {
+  packet_sender_ssrc_ = ssrc;
+}
+
+void FeedbackPacket::WithMediaSourceSsrc(uint32_t ssrc) {
+  media_source_ssrc_ = ssrc;
+}
+
+void FeedbackPacket::WithBase(uint16_t base_sequence,
+                              int64_t ref_timestamp_us) {
+  DCHECK_EQ(-1, base_seq_);
+  DCHECK_NE(-1, ref_timestamp_us);
+  base_seq_ = base_sequence;
+  last_seq_ = base_sequence;
+  base_time_ = ref_timestamp_us / kBaseScaleFactor;
+  last_timestamp_ = base_time_ * kBaseScaleFactor;
+}
+
+void FeedbackPacket::WithFeedbackSequenceNumber(uint8_t feedback_sequence) {
+  feedback_seq_ = feedback_sequence;
+}
+
+bool FeedbackPacket::WithReceivedPacket(uint16_t sequence_number,
+                                        int64_t timestamp) {
+  DCHECK_NE(-1, base_seq_);
+  int64_t seq = Unwrap(sequence_number);
+  if (seq != base_seq_) {
+    if (seq <= last_seq_)
+      return false;
+  }
+
+  // Convert to ticks and round.
+  int64_t delta_full = timestamp - last_timestamp_;
+  delta_full +=
+      delta_full < 0 ? -(kDeltaScaleFactor / 2) : kDeltaScaleFactor / 2;
+  delta_full /= kDeltaScaleFactor;
+
+  int16_t delta = static_cast<int16_t>(delta_full);
+  // If larger than 16bit signed, we can't represent it - need new fb packet.
+  if (delta != delta_full) {
+    LOG(LS_WARNING) << "Delta value too large ( >= 2^16 ticks )";
+    return false;
+  }
+
+  StatusSymbol symbol;
+  if (delta >= 0 && delta <= 0xFF) {
+    symbol = StatusSymbol::kReceivedSmallDelta;
+  } else {
+    symbol = StatusSymbol::kReceivedLargeDelta;
+  }
+
+  if (!AddSymbol(symbol, seq))
+    return false;
+
+  receive_deltas_.push_back(delta);
+  last_timestamp_ += delta * kDeltaScaleFactor;
+  return true;
+}
+
+// Add a symbol for a received packet, with the given sequence number. This
+// method will add any "packet not received" symbols needed before this one.
+bool FeedbackPacket::AddSymbol(StatusSymbol symbol, int64_t seq) {
+  while (last_seq_ < seq - 1) {
+    if (!Encode(StatusSymbol::kNotReceived))
+      return false;
+    ++last_seq_;
+  }
+
+  if (!Encode(symbol))
+    return false;
+
+  last_seq_ = seq;
+  return true;
+}
+
+// Append a symbol to the internal symbol vector. If the new state cannot be
+// represented using a single status chunk, a chunk will first be emitted and
+// the associated symbols removed from the internal symbol vector.
+bool FeedbackPacket::Encode(StatusSymbol symbol) {
+  if (last_seq_ - base_seq_ + 1 > 0xFFFF) {
+    LOG(LS_WARNING) << "Packet status count too large ( >= 2^16 )";
+    return false;
+  }
+
+  bool is_two_bit;
+  int delta_size;
+  switch (symbol) {
+    case StatusSymbol::kReceivedSmallDelta:
+      delta_size = 1;
+      is_two_bit = false;
+      break;
+    case StatusSymbol::kReceivedLargeDelta:
+      delta_size = 2;
+      is_two_bit = true;
+      break;
+    case StatusSymbol::kNotReceived:
+      is_two_bit = false;
+      delta_size = 0;
+      break;
+    default:
+      RTC_NOTREACHED();
+      return false;

[stefan-webrtc, 2015/07/14 12:14:54]

Have we not enumerated all cases here?

[sprang_webrtc, 2015/07/16 11:12:11]

Yes, I just want to make sure we somehow capture an error here if a new value is
introduced without a matching case.
Ideally we wouldn't have a default and make the compiler catch missing case, but
it seems that is not supported on all our platforms...

+  }
+
+  if (symbol_vec_.empty()) {
+    if (size_bytes_ + delta_size + kChunkSizeBytes > kMaxSizeBytes)
+      return false;
+
+    symbol_vec_.push_back(symbol);
+    vec_needs_two_bit_symbols_ = is_two_bit;
+    first_symbol_cardinality_ = 1;
+    size_bytes_ += delta_size + kChunkSizeBytes;
+    return true;
+  }
+  if (size_bytes_ + delta_size > kMaxSizeBytes)
+    return false;
+
+  // Capacity, in number of symbols, that a vector chunk could hold.
+  size_t capacity = vec_needs_two_bit_symbols_ ? kTwoBitVectorCapacity
+                                               : kOneBitVectorCapacity;
+
+  // If symbol_vec only contains one kind of symbol, it may be RLE-coded.
+  bool rle_candidate = symbol_vec_.size() == first_symbol_cardinality_ ||

[stefan-webrtc, 2015/07/14 12:14:54]

This is still hard for me to understand. Can we explain it in a better way? What
exactly is first_symbol_cardinality_?

[sprang_webrtc, 2015/07/16 11:12:10]

I've added more comments, have a look.

[stefan-webrtc, 2015/07/27 11:49:28]

A lot better

+                       first_symbol_cardinality_ > capacity;
+  if (rle_candidate) {
+    if (symbol_vec_.back() == symbol) {
+      ++first_symbol_cardinality_;
+      if (first_symbol_cardinality_ <= capacity) {
+        // Only add to symbol_vec if we are not certain this will be RLE-coded.
+        symbol_vec_.push_back(symbol);
+      } else if (first_symbol_cardinality_ == kRunLengthCapacity) {
+        // Max length for an RLE-chunk reached.
+        EmitRunLengthChunk();
+      }
+      size_bytes_ += delta_size;
+      return true;
+    } else if (first_symbol_cardinality_ > capacity) {

[stefan-webrtc, 2015/07/14 12:14:54]

Do we always end up in this else if if the expression on line 419 is true? Are
there other situations when we should end up here? Otherwise maybe we can
restructure this a bit... I don't really follow the logic here, so it's hard to
make good suggestion. :)

[sprang_webrtc, 2015/07/16 11:12:11]

Just breaking this out into a separate method doesn't improve much though, imo.
We still need to pass in symbol, capacity and delta_size, and then need logic to
handle the fall-through case, where (symbol) is not the same as the other
symbols
in symbol_vec and size is not enough RLE-encode current state. I'll add an
explicit
block for this case and comment.

+      // Symbols in symbol_vec can only be RLE-encoded but input symbol is of
+      // a different kind. Emit the RLE-chunk and re-add input.
+      EmitRunLengthChunk();
+      return Encode(symbol);

[stefan-webrtc, 2015/07/14 12:14:54]

I assume there is no risk that this recursive call becomes a problem?

[sprang_webrtc, 2015/07/16 11:12:11]

No. Added comment.

+    }
+  }
+
+  // If this code point is reached, symbols in symbol_vec cannot be RLE-encoded.
+
+  if (is_two_bit && !vec_needs_two_bit_symbols_) {
+    // If the symbols in symbol_vec can be encoded using a one-bit chunk but
+    // the input symbol cannot, first check if we can simply change target type.
+    vec_needs_two_bit_symbols_ = true;
+    if (symbol_vec_.size() >= kTwoBitVectorCapacity) {
+      // symbol_vec contains more symbols than we can encode in a single
+      // two-bit chunk. Emit a new vector append to the remains, if any.
+      if (size_bytes_ + delta_size + kChunkSizeBytes > kMaxSizeBytes)
+        return false;
+      EmitVectorChunk();
+      return Encode(symbol);
+    }
+    // symbol_vec symbols fit within a single two-bit vector chunk.
+    capacity = kTwoBitVectorCapacity;
+  }
+
+  symbol_vec_.push_back(symbol);
+  if (symbol_vec_.size() == capacity)
+    EmitVectorChunk();
+
+  size_bytes_ += delta_size;
+  return true;
+}
+
+// Upon packet completion, emit any remaining symbols in symbol_vec that have
+// not yet been emitted in a status chunk.
+void FeedbackPacket::EmitRemaining() {
+  if (symbol_vec_.empty())
+    return;
+
+  size_t capacity = vec_needs_two_bit_symbols_ ? kTwoBitVectorCapacity
+                                               : kOneBitVectorCapacity;
+  if (first_symbol_cardinality_ > capacity) {
+    EmitRunLengthChunk();
+  } else {
+    EmitVectorChunk();
+  }
+}
+
+void FeedbackPacket::EmitVectorChunk() {
+  if (vec_needs_two_bit_symbols_) {
+    status_chunks_.push_back(new TwoBitVectorChunk(&symbol_vec_));
+  } else {
+    status_chunks_.push_back(new OneBitVectorChunk(&symbol_vec_));
+  }
+}
+
+void FeedbackPacket::EmitRunLengthChunk() {
+  status_chunks_.push_back(
+      new RunLengthChunk(symbol_vec_.front(), first_symbol_cardinality_));
+  symbol_vec_.clear();

[stefan-webrtc, 2015/07/14 12:14:54]

DCHECK that symbol_vec_.size() == 1 above?

[sprang_webrtc, 2015/07/16 11:12:11]

No, it shouldn't be. Best I can do is DCHECK_GE(first_symbol_cardinality_,
symbol_vec_.size())

[stefan-webrtc, 2015/07/27 11:49:28]

Yes, I understand this better now, agree. :)

+}
+
+size_t FeedbackPacket::BlockLength() const {
+  return size_bytes_;
+}
+
+uint16_t FeedbackPacket::GetBaseSequence() const {
+  return base_seq_;
+}
+
+int32_t FeedbackPacket::GetBaseTime() const {
+  return static_cast<int32_t>(base_time_ & 0x00FFFFFF);
+}
+
+int64_t FeedbackPacket::GetBaseTimeUs() const {
+  return GetBaseTime() * kBaseScaleFactor;
+}
+
+std::vector<FeedbackPacket::StatusSymbol> FeedbackPacket::GetStatusVector()
+    const {
+  std::vector<FeedbackPacket::StatusSymbol> symbols;
+  for (PacketStatusChunk* chunk : status_chunks_)
+    chunk->AppendSymbolsTo(&symbols);
+  int64_t status_count = last_seq_ - base_seq_ + 1;
+  // If packet ends with a vector chunk, it may contain extraneous "packet not
+  // received"-symbols at the end. Crop any such symbols.
+  symbols.erase(symbols.begin() + status_count, symbols.end());
+  return symbols;
+}
+
+std::vector<int16_t> FeedbackPacket::GetReceiveDeltas() const {
+  return receive_deltas_;
+}
+
+std::vector<int64_t> FeedbackPacket::GetReceiveDeltasUs() const {
+  if (receive_deltas_.empty())
+    return std::vector<int64_t>();
+
+  std::vector<int64_t> us_deltas;
+  for (int16_t delta : receive_deltas_)
+    us_deltas.push_back(static_cast<int64_t>(delta) * kDeltaScaleFactor);
+
+  return us_deltas;
+}
+
+// Serialize packet.
+bool FeedbackPacket::Create(uint8_t* packet,
+                            size_t* position,
+                            size_t max_length,
+                            PacketReadyCallback* callback) const {
+  if (base_seq_ == -1)
+    return false;
+
+  while (*position + size_bytes_ > max_length) {
+    if (!OnBufferFull(packet, position, callback))
+      return false;
+  }
+
+  CreateHeader(kFeedbackMessageType, kPayloadType, HeaderLength(), packet,
+               position);
+  ByteWriter<uint32_t>::WriteBigEndian(&packet[*position], packet_sender_ssrc_);
+  *position += 4;
+  ByteWriter<uint32_t>::WriteBigEndian(&packet[*position], media_source_ssrc_);
+  *position += 4;
+
+  DCHECK_LE(base_seq_, 0xFFFF);
+  ByteWriter<uint16_t>::WriteBigEndian(&packet[*position], base_seq_);
+  *position += 2;
+
+  int64_t status_count = last_seq_ - base_seq_ + 1;
+  DCHECK_LE(status_count, 0xFFFF);
+  ByteWriter<uint16_t>::WriteBigEndian(&packet[*position], status_count);
+  *position += 2;
+
+  ByteWriter<int32_t, 3>::WriteBigEndian(&packet[*position],
+                                         static_cast<int16_t>(base_time_));
+  *position += 3;
+
+  packet[(*position)++] = feedback_seq_;
+
+  // TODO(sprang): Get rid of this cast.
+  const_cast<FeedbackPacket*>(this)->EmitRemaining();

[stefan-webrtc, 2015/07/14 12:14:54]

Is this going to be done somewhere else instead?

[sprang_webrtc, 2015/07/16 11:12:11]

Can you elaborate on that statement?

The alternative as I seet it is to require an extra method call in between
adding the values to the packet and calling Build. Like

FeedbackPacket packet;
packet.WithBase(...);
packet.WithReceivedPacket(...);

packet.Finish();  <--

packet.Build();


WDYT?

[stefan-webrtc, 2015/07/27 11:49:28]

I think I was mostly wondering about the TODO. Wasn't clear to me how the cast
would be removed.

+  for (PacketStatusChunk* chunk : status_chunks_) {
+    chunk->WriteTo(&packet[*position]);
+    *position += 2;
+  }
+
+  for (int16_t delta : receive_deltas_) {
+    if (delta >= 0 && delta <= 0xFF) {
+      packet[(*position)++] = delta;
+    } else {
+      ByteWriter<int16_t>::WriteBigEndian(&packet[*position], delta);
+      *position += 2;
+    }
+  }
+
+  while ((*position % 4) != 0)
+    packet[(*position)++] = 0;
+
+  return true;
+}
+
+// De-serialize packet.
+rtc::scoped_ptr<FeedbackPacket> FeedbackPacket::ParseFrom(const uint8_t* buffer,
+                                                          size_t length) {
+  rtc::scoped_ptr<FeedbackPacket> packet(new FeedbackPacket());
+
+  if (length < kMinSizeBytes) {
+    LOG(LS_WARNING) << "Buffer too small (" << length
+                    << " bytes) to fit a "
+                       "FeedbackPacket. Minimum size = " << kMinSizeBytes;
+    return nullptr;
+  }
+
+  size_t packet_size_words =
+      ByteReader<uint16_t>::ReadBigEndian(&buffer[2]) + 1;
+  if (length < (packet_size_words * 4)) {

[stefan-webrtc, 2015/07/14 12:14:54]

Remove ()

[sprang_webrtc, 2015/07/16 11:12:11]

Done.

+    LOG(LS_WARNING) << "Buffer too small (" << length
+                    << " bytes) to fit a FeedbackPacket of "
+                    << packet_size_words << " 32bit words.";
+    return nullptr;
+  }
+
+  // TODO(sprang): Break this out and generalize when implementing parsing of
+  // other RtcpPacket subclasses.
+
+  const uint8_t kRtcpVersion = 2;
+  uint8_t version = buffer[0] >> 6;
+  if (version != kRtcpVersion) {
+    LOG(LS_WARNING) << "Invalid RTCP header: Version must be " << kRtcpVersion
+                    << " but was " << version;
+    return nullptr;
+  }
+
+  bool has_padding = (buffer[0] & 0x20) != 0;
+
+  uint8_t format = buffer[0] & 0x1F;
+  if (format != kFeedbackMessageType) {
+    LOG(LS_WARNING) << "Invalid RTCP header: FMT must be "
+                    << kFeedbackMessageType << " but was " << format;
+    return nullptr;
+  }
+
+  uint8_t payload_type = buffer[1];
+  if (payload_type != kPayloadType) {
+    LOG(LS_WARNING) << "Invalid RTCP header: PT must be " << kPayloadType
+                    << " but was " << payload_type;
+    return nullptr;
+  }
+
+  size_t payload_size = packet_size_words * 4;
+  if (has_padding) {
+    uint8_t padding_bytes = buffer[payload_size - 1];
+    if (payload_size - padding_bytes < kMinSizeBytes) {

[stefan-webrtc, 2015/07/14 12:14:54]

I think you may have to verify that this subtraction doesn't underflow
payload_size? Or is it safe because this is promoted to an integer?

[sprang_webrtc, 2015/07/16 11:12:11]

I'll rewrite it as 
  payload_size < kMinSizeBytes + padding_bytes

[stefan-webrtc, 2015/07/27 11:49:28]

Acknowledged.

+      LOG(LS_WARNING) << "Invalid RTCP header: Too many padding bytes ("
+                      << padding_bytes << ") for a packet size of "
+                      << payload_size << "bytes.";
+      return nullptr;
+    }
+    payload_size -= padding_bytes;
+  }
+
+  packet->packet_sender_ssrc_ = ByteReader<uint32_t>::ReadBigEndian(&buffer[4]);

[stefan-webrtc, 2015/07/14 12:14:54]

Maybe put a comment here with the header you are parsing so that it's easy to
verify?

[sprang_webrtc, 2015/07/16 11:12:11]

Added the message format overview on top. OK?

[stefan-webrtc, 2015/07/27 11:49:28]

Acknowledged.

+  packet->media_source_ssrc_ = ByteReader<uint32_t>::ReadBigEndian(&buffer[8]);
+  packet->base_seq_ = ByteReader<uint16_t>::ReadBigEndian(&buffer[12]);
+  uint16_t num_packets = ByteReader<uint16_t>::ReadBigEndian(&buffer[14]);
+  packet->base_time_ = ByteReader<int32_t, 3>::ReadBigEndian(&buffer[16]);
+  packet->feedback_seq_ = buffer[19];
+  size_t index = 20;
+
+  if (num_packets == 0) {
+    LOG(LS_WARNING) << "Empty feedback messages not allowed.";
+    return nullptr;
+  }
+  packet->last_seq_ = packet->base_seq_ + num_packets - 1;
+
+  size_t packets_read = 0;
+  while (packets_read < num_packets) {
+    if (index + 2 > payload_size) {
+      LOG(LS_WARNING) << "Buffer overflow while parsing packet.";
+      return nullptr;
+    }
+
+    PacketStatusChunk* chunk =
+        ParseChunk(&buffer[index], num_packets - packets_read);
+    if (chunk == nullptr)
+      return nullptr;
+
+    index += 2;
+    packet->status_chunks_.push_back(chunk);
+    packets_read += chunk->NumSymbols();
+  }
+
+  std::vector<StatusSymbol> symbols = packet->GetStatusVector();
+
+  DCHECK_EQ(num_packets, symbols.size());
+
+  for (StatusSymbol symbol : symbols) {
+    switch (symbol) {
+      case StatusSymbol::kReceivedSmallDelta:
+        if (index + 1 > payload_size) {
+          LOG(LS_WARNING) << "Buffer overflow while parsing packet.";
+          return nullptr;
+        }
+        packet->receive_deltas_.push_back(buffer[index]);
+        ++index;
+        break;
+      case StatusSymbol::kReceivedLargeDelta:
+        if (index + 2 > payload_size) {
+          LOG(LS_WARNING) << "Buffer overflow while parsing packet.";
+          return nullptr;
+        }
+        packet->receive_deltas_.push_back(
+            ByteReader<int16_t>::ReadBigEndian(&buffer[index]));
+        index += 2;
+        break;
+      default:
+        continue;
+    }
+  }
+
+  DCHECK_GE(index, payload_size - 3);
+  DCHECK_LE(index, payload_size);
+
+  return packet;
+}
+
+PacketStatusChunk* FeedbackPacket::ParseChunk(const uint8_t* buffer,
+                                              size_t max_size) {
+  if (buffer[0] & 0x80) {
+    // First bit set => vector chunk.
+    std::deque<StatusSymbol> symbols;
+    if (buffer[0] & 0x40) {
+      // Second bit set => two bits per symbol vector.
+      return TwoBitVectorChunk::ParseFrom(buffer);
+    }
+
+    // Second bit not set => one bit per symbol vector.
+    return OneBitVectorChunk::ParseFrom(buffer);
+  }
+
+  // First bit not set => RLE chunk.
+  RunLengthChunk* rle_chunk = RunLengthChunk::ParseFrom(buffer);
+  if (rle_chunk->NumSymbols() > max_size) {
+    LOG(LS_WARNING) << "Header/body mismatch. "
+                       "RLE block of size " << rle_chunk->NumSymbols()
+                    << " but only " << max_size << " left to read.";
+    delete rle_chunk;
+    return nullptr;
+  }
+  return rle_chunk;
+}
+
+}  // namespace rtcp
+}  // namespace webrtc