Add packetization and coding/decoding of feedback message format.

Add packetization and coding/decoding of feedback message format.

BUG=webrtc:4312
R=stefan@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/a3b8769860bdb0a45dbff6d1e0092486fa59aaa4

[sprang_webrtc, 2015-06-11 10:08:36]

sprang@webrtc.org changed reviewers:
+ stefan@webrtc.org

[stefan-webrtc, 2015-06-16 08:29:14]

A first pass, nice job.

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc (right):

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:81: for (size_t i
= 0; i < 14; ++i) {
What is 14 here? I think it's worth naming it and explaining why it is 14.

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:125:
chunk->symbols_[index++] = DecodeSymbol((data[1] >> i) & 0x01);
Probably worth mentioning why this is done in two steps (5->0 and 7->0).
Basically, what are you reading in the first step and what in the second?

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:188:
FeedbackPacket::StatusSymbol symbols_[7];
Why 7? Name the constant and explain.

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:207: void
WriteTo(uint8_t* buffer) const override {
I find this method complicated enough to deserve a comment. :)

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:213: static
RunLengthChunk* ParseFrom(const uint8_t* buffer) {
Same with this.

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:230: if (delta <=
-0x8FFF) {
Shouldn't this be -0x7FFF?

I wonder if we could rewrite this using IsNewerSequenceNumber() or
LatestSequenceNumber() in module_common_types.h? I think it's worth a lot to
reuse this type of code.

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:243: int64_t
ref_timestamp_us) {
Would base_timestamp_us be better? Or is that not what it means?

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h (right):

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:23: class
FeedbackPacket : public RtcpPacket {
After reading most of the .cc file, I think it would be good to have a comment
somewhere, either here or in the .cc file, which roughly explains how the
encoding works. When do we AddSymbols, Encode and Emit things and what does
those functions mean? For instance, sometimes we Emit followed by Encode, and
sometimes we just do Emit.

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:32: // NOTE: These
methods requires strictly increasing sequence numbers.
Except for when they wrap, right?

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:59: class
PacketStatusChunk;
Does this have to be public?

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:71: //
TODO(sprang): Add support for dynamic max size for easier fragmentation?
I don't follow this. Could you explain?

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:74: static const
int kBaseScaleFactor = kDeltaScaleFactor * (1 << 8);
Could these be moved to the cc file?

[sprang_webrtc, 2015-06-22 12:20:11]

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc (right):

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:81: for (size_t i
= 0; i < 14; ++i) {
On 2015/06/16 08:29:13, stefan-webrtc (holmer) wrote:
> What is 14 here? I think it's worth naming it and explaining why it is 14.

Done.

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:125:
chunk->symbols_[index++] = DecodeSymbol((data[1] >> i) & 0x01);
On 2015/06/16 08:29:13, stefan-webrtc (holmer) wrote:
> Probably worth mentioning why this is done in two steps (5->0 and 7->0).
> Basically, what are you reading in the first step and what in the second?

Done.

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:188:
FeedbackPacket::StatusSymbol symbols_[7];
On 2015/06/16 08:29:13, stefan-webrtc (holmer) wrote:
> Why 7? Name the constant and explain.

Done.

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:207: void
WriteTo(uint8_t* buffer) const override {
On 2015/06/16 08:29:13, stefan-webrtc (holmer) wrote:
> I find this method complicated enough to deserve a comment. :)

Done.

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:213: static
RunLengthChunk* ParseFrom(const uint8_t* buffer) {
On 2015/06/16 08:29:13, stefan-webrtc (holmer) wrote:
> Same with this.

Should be ok with changes above?

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:230: if (delta <=
-0x8FFF) {
On 2015/06/16 08:29:13, stefan-webrtc (holmer) wrote:
> Shouldn't this be -0x7FFF?
> 
> I wonder if we could rewrite this using IsNewerSequenceNumber() or
> LatestSequenceNumber() in module_common_types.h? I think it's worth a lot to
> reuse this type of code.

Done.

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:243: int64_t
ref_timestamp_us) {
On 2015/06/16 08:29:13, stefan-webrtc (holmer) wrote:
> Would base_timestamp_us be better? Or is that not what it means?

Changed to use absolute ref time, as per discussion.

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h (right):

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:23: class
FeedbackPacket : public RtcpPacket {
On 2015/06/16 08:29:13, stefan-webrtc (holmer) wrote:
> After reading most of the .cc file, I think it would be good to have a comment
> somewhere, either here or in the .cc file, which roughly explains how the
> encoding works. When do we AddSymbols, Encode and Emit things and what does
> those functions mean? For instance, sometimes we Emit followed by Encode, and
> sometimes we just do Emit.

Added comments in .cc

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:32: // NOTE: These
methods requires strictly increasing sequence numbers.
On 2015/06/16 08:29:13, stefan-webrtc (holmer) wrote:
> Except for when they wrap, right?

Done.

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:59: class
PacketStatusChunk;
On 2015/06/16 08:29:14, stefan-webrtc (holmer) wrote:
> Does this have to be public?

Can't inherit from it otherwise. Moved it out of this class instead.

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:71: //
TODO(sprang): Add support for dynamic max size for easier fragmentation?
On 2015/06/16 08:29:13, stefan-webrtc (holmer) wrote:
> I don't follow this. Could you explain?

Adding a public method for setting max size lower than this constant may be
useful. For instance, if you know you have a certain number of bytes left in an
rtcp packet you can set the max to this number and then add as many symbols as
will fit, pushing the remaining to a subsequent packet. May be an uncommon
problem though.

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:74: static const
int kBaseScaleFactor = kDeltaScaleFactor * (1 << 8);
On 2015/06/16 08:29:14, stefan-webrtc (holmer) wrote:
> Could these be moved to the cc file?

Done.

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

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc (right):

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:213: static
RunLengthChunk* ParseFrom(const uint8_t* buffer) {
On 2015/06/22 12:20:10, språng wrote:
> On 2015/06/16 08:29:13, stefan-webrtc (holmer) wrote:
> > Same with this.
> 
> Should be ok with changes above?

Yes

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:252: // Include
precision loss in order to avoid drift.
Keep this comment, I found it really helpful.

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h (right):

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:71: //
TODO(sprang): Add support for dynamic max size for easier fragmentation?
On 2015/06/22 12:20:11, språng wrote:
> On 2015/06/16 08:29:13, stefan-webrtc (holmer) wrote:
> > I don't follow this. Could you explain?
> 
> Adding a public method for setting max size lower than this constant may be
> useful. For instance, if you know you have a certain number of bytes left in
an
> rtcp packet you can set the max to this number and then add as many symbols as
> will fit, pushing the remaining to a subsequent packet. May be an uncommon
> problem though.

Maybe, I guess it depends on bitrate?

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc (right):

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:397: return false;
Have we not enumerated all cases here?

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:418: bool
rle_candidate = symbol_vec_.size() == first_symbol_cardinality_ ||
This is still hard for me to understand. Can we explain it in a better way? What
exactly is first_symbol_cardinality_?

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:432: } else if
(first_symbol_cardinality_ > capacity) {
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. :)

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:436: return
Encode(symbol);
I assume there is no risk that this recursive call becomes a problem?

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:492:
symbol_vec_.clear();
DCHECK that symbol_vec_.size() == 1 above?

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:574:
const_cast<FeedbackPacket*>(this)->EmitRemaining();
Is this going to be done somewhere else instead?

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:609: if (length <
(packet_size_words * 4)) {
Remove ()

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:646: if
(payload_size - padding_bytes < kMinSizeBytes) {
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?

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:655:
packet->packet_sender_ssrc_ = ByteReader<uint32_t>::ReadBigEndian(&buffer[4]);
Maybe put a comment here with the header you are parsing so that it's easy to
verify?

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h (right):

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:36: // NOTE: This
methods requires increasing sequence numbers (excepting wraps).
s/requires/require

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:56: static const
int kDeltaScaleFactor = 250;  // Convert to multiples of 0.25ms
End with .

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:58: static const
uint8_t kPayloadType = 205;         // RTPFB, see RFC4585
End with .

[stefan-webrtc, 2015-07-14 12:15:10]

Otherwise I think this looks pretty good :)

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

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc (right):

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:213: static
RunLengthChunk* ParseFrom(const uint8_t* buffer) {
On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> On 2015/06/22 12:20:10, språng wrote:
> > On 2015/06/16 08:29:13, stefan-webrtc (holmer) wrote:
> > > Same with this.
> > 
> > Should be ok with changes above?
> 
> Yes

Acknowledged.

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h (right):

https://codereview.webrtc.org/1175263002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:71: //
TODO(sprang): Add support for dynamic max size for easier fragmentation?
On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> On 2015/06/22 12:20:11, språng wrote:
> > On 2015/06/16 08:29:13, stefan-webrtc (holmer) wrote:
> > > I don't follow this. Could you explain?
> > 
> > Adding a public method for setting max size lower than this constant may be
> > useful. For instance, if you know you have a certain number of bytes left in
> an
> > rtcp packet you can set the max to this number and then add as many symbols
as
> > will fit, pushing the remaining to a subsequent packet. May be an uncommon
> > problem though.
> 
> Maybe, I guess it depends on bitrate?

Yes. High bitrate with long feedback interval may cause this. Sounds like a bad
idea to use that, but still...

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc (right):

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:397: return false;
On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> Have we not enumerated all cases here?

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...

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:418: bool
rle_candidate = symbol_vec_.size() == first_symbol_cardinality_ ||
On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> This is still hard for me to understand. Can we explain it in a better way?
What
> exactly is first_symbol_cardinality_?

I've added more comments, have a look.

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:432: } else if
(first_symbol_cardinality_ > capacity) {
On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> 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. :)

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.

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:436: return
Encode(symbol);
On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> I assume there is no risk that this recursive call becomes a problem?

No. Added comment.

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:492:
symbol_vec_.clear();
On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> DCHECK that symbol_vec_.size() == 1 above?

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

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:574:
const_cast<FeedbackPacket*>(this)->EmitRemaining();
On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> Is this going to be done somewhere else instead?

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?

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:609: if (length <
(packet_size_words * 4)) {
On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> Remove ()

Done.

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:646: if
(payload_size - padding_bytes < kMinSizeBytes) {
On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> 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?

I'll rewrite it as 
  payload_size < kMinSizeBytes + padding_bytes

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:655:
packet->packet_sender_ssrc_ = ByteReader<uint32_t>::ReadBigEndian(&buffer[4]);
On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> Maybe put a comment here with the header you are parsing so that it's easy to
> verify?

Added the message format overview on top. OK?

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h (right):

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:36: // NOTE: This
methods requires increasing sequence numbers (excepting wraps).
On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> s/requires/require

Done.

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:56: static const
int kDeltaScaleFactor = 250;  // Convert to multiples of 0.25ms
On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> End with .

Done.

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:58: static const
uint8_t kPayloadType = 205;         // RTPFB, see RFC4585
On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> End with .

Done.

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

LG

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc (right):

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:418: bool
rle_candidate = symbol_vec_.size() == first_symbol_cardinality_ ||
On 2015/07/16 11:12:10, språng wrote:
> On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> > This is still hard for me to understand. Can we explain it in a better way?
> What
> > exactly is first_symbol_cardinality_?
> 
> I've added more comments, have a look.

A lot better

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:492:
symbol_vec_.clear();
On 2015/07/16 11:12:11, språng wrote:
> On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> > DCHECK that symbol_vec_.size() == 1 above?
> 
> No, it shouldn't be. Best I can do is DCHECK_GE(first_symbol_cardinality_,
> symbol_vec_.size())

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

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:574:
const_cast<FeedbackPacket*>(this)->EmitRemaining();
On 2015/07/16 11:12:11, språng wrote:
> On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> > Is this going to be done somewhere else instead?
> 
> 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?

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

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:646: if
(payload_size - padding_bytes < kMinSizeBytes) {
On 2015/07/16 11:12:11, språng wrote:
> On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> > 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?
> 
> I'll rewrite it as 
>   payload_size < kMinSizeBytes + padding_bytes

Acknowledged.

https://codereview.webrtc.org/1175263002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:655:
packet->packet_sender_ssrc_ = ByteReader<uint32_t>::ReadBigEndian(&buffer[4]);
On 2015/07/16 11:12:11, språng wrote:
> On 2015/07/14 12:14:54, stefan-webrtc (holmer) wrote:
> > Maybe put a comment here with the header you are parsing so that it's easy
to
> > verify?
> 
> Added the message format overview on top. OK?

Acknowledged.

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc (right):

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:30: static const
int kMaxSizeBytes = (1 << 16) * 4;
Why is it set to this? Seems like a large packet?

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:113: symbols_[i] =
FeedbackPacket::StatusSymbol::kNotReceived;
Hm, how do we handle the case if there are fewer symbols than 14? Do we mark
future packets as not received?

-- Nevermind, I see now that we can know how many we should ignore when parsing.

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:129: buffer[0] |=
EncodeSymbol(symbols_[0]) << 5;
Should we DCHECK that symbols_[i] < 2?

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:323: if (seq <=
last_seq_)
if (seq != base_seq_ && seq <= last_seq_)
  return false;

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:504: for (size_t i
= 1; i < symbol_vec_.size(); ++i) {
Comment: Update first symbol cardinality for the remaining symbols.

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h (right):

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:26: class
FeedbackPacket : public RtcpPacket {
I wonder if maybe we should rename this class to be a bit less generic?

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:36: // NOTE: This
method require increasing sequence numbers (excepting wraps).
method requires or methods require :)

[sprang_webrtc, 2015-07-28 08:28:54]

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc (right):

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:30: static const
int kMaxSizeBytes = (1 << 16) * 4;
On 2015/07/27 11:49:28, stefan-webrtc (holmer) wrote:
> Why is it set to this? Seems like a large packet?

Max length of an RTCP packet, as specified by the common RTCP header.
16bit length in four byte words, excluding the first header word.

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:113: symbols_[i] =
FeedbackPacket::StatusSymbol::kNotReceived;
On 2015/07/27 11:49:28, stefan-webrtc (holmer) wrote:
> Hm, how do we handle the case if there are fewer symbols than 14? Do we mark
> future packets as not received?
> 
> -- Nevermind, I see now that we can know how many we should ignore when
parsing.

Acknowledged.

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:129: buffer[0] |=
EncodeSymbol(symbols_[0]) << 5;
On 2015/07/27 11:49:28, stefan-webrtc (holmer) wrote:
> Should we DCHECK that symbols_[i] < 2?

That should be caught by EncodeSymbol, no?

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:323: if (seq <=
last_seq_)
On 2015/07/27 11:49:28, stefan-webrtc (holmer) wrote:
> if (seq != base_seq_ && seq <= last_seq_)
>   return false;

Done.

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:504: for (size_t i
= 1; i < symbol_vec_.size(); ++i) {
On 2015/07/27 11:49:28, stefan-webrtc (holmer) wrote:
> Comment: Update first symbol cardinality for the remaining symbols.

Done.

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h (right):

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:26: class
FeedbackPacket : public RtcpPacket {
On 2015/07/27 11:49:28, stefan-webrtc (holmer) wrote:
> I wonder if maybe we should rename this class to be a bit less generic?

Renamed to TransportFeedback

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.h:36: // NOTE: This
method require increasing sequence numbers (excepting wraps).
On 2015/07/27 11:49:28, stefan-webrtc (holmer) wrote:
> method requires or methods require :)

Bah, went too fast..

[stefan-webrtc, 2015-07-28 09:34:11]

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc (right):

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:30: static const
int kMaxSizeBytes = (1 << 16) * 4;
On 2015/07/28 08:28:54, språng wrote:
> On 2015/07/27 11:49:28, stefan-webrtc (holmer) wrote:
> > Why is it set to this? Seems like a large packet?
> 
> Max length of an RTCP packet, as specified by the common RTCP header.
> 16bit length in four byte words, excluding the first header word.

Ah, comment please. :)

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:129: buffer[0] |=
EncodeSymbol(symbols_[0]) << 5;
On 2015/07/28 08:28:54, språng wrote:
> On 2015/07/27 11:49:28, stefan-webrtc (holmer) wrote:
> > Should we DCHECK that symbols_[i] < 2?
> 
> That should be caught by EncodeSymbol, no?

Hm, maybe I misunderstood it then. Doesn't this OneBitVectorChunk encode one bit
per symbol? EncodeSymbol could potentially return 2 bits.

[sprang_webrtc, 2015-07-28 11:39:23]

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc (right):

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:30: static const
int kMaxSizeBytes = (1 << 16) * 4;
On 2015/07/28 09:34:10, stefan-webrtc (holmer) wrote:
> On 2015/07/28 08:28:54, språng wrote:
> > On 2015/07/27 11:49:28, stefan-webrtc (holmer) wrote:
> > > Why is it set to this? Seems like a large packet?
> > 
> > Max length of an RTCP packet, as specified by the common RTCP header.
> > 16bit length in four byte words, excluding the first header word.
> 
> Ah, comment please. :)

Done.

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_packet/feedback_packet.cc:129: buffer[0] |=
EncodeSymbol(symbols_[0]) << 5;
On 2015/07/28 09:34:10, stefan-webrtc (holmer) wrote:
> On 2015/07/28 08:28:54, språng wrote:
> > On 2015/07/27 11:49:28, stefan-webrtc (holmer) wrote:
> > > Should we DCHECK that symbols_[i] < 2?
> > 
> > That should be caught by EncodeSymbol, no?
> 
> Hm, maybe I misunderstood it then. Doesn't this OneBitVectorChunk encode one
bit
> per symbol? EncodeSymbol could potentially return 2 bits.

Or maybe you have thought this through more than me :)
Added new check.

[stefan-webrtc, 2015-07-28 12:59:08]

lgtm

[sprang_webrtc, 2015-07-29 08:47:09]

Committed patchset #14 (id:260001) manually as
a3b8769860bdb0a45dbff6d1e0092486fa59aaa4 (presubmit successful).

[ay, 2015-07-31 18:40:35]

On 2015/07/28 12:59:08, stefan-webrtc (holmer) wrote:
> lgtm

https://codereview.webrtc.org/1175263002/diff/180001/webrtc/modules/rtp_rtcp/...