Add FlexFEC header formatters. (pt. 5)

Add FlexFEC header formatters.

Add classes that can read and finalize FlexFEC headers.

BUG=webrtc:5654
Committed: https://crrev.com/0496de298e38f8689ecefef27c31a3433bf3d073
Cr-Commit-Position: refs/heads/master@{#14469}

[brandtr, 2016-08-23 07:55:38]

Description was changed from

==========
Add FlexFEC header formatters.

This CL builds on the four prior ones. It adds FlexFEC header formatting
capabilities, together with unit tests.

BUG=webrtc:5654
==========

to

==========
Add FlexFEC header formatters.

This CL builds on the four prior ones. It adds FlexFEC header formatting
capabilities, together with unit tests.

BUG=webrtc:5654
==========

[brandtr, 2016-08-23 08:19:32]

brandtr@webrtc.org changed reviewers:
+ danilchap@webrtc.org, stefan@webrtc.org

[brandtr, 2016-08-23 08:19:33]

Split up previous large CL in five semi-indenpendent ones.

[brandtr, 2016-08-25 12:38:42]

Rebase + fixes.

[brandtr, 2016-08-25 13:15:23]

Fixed missing constant.

[brandtr, 2016-08-25 13:20:53]

Rebase + fixes.

[brandtr, 2016-08-26 11:21:08]

Rebase + changes.

[brandtr, 2016-08-30 07:05:28]

Rebase + fiexes.

[brandtr, 2016-08-30 07:05:31]

Patchset #12 (id:210001) has been deleted

[brandtr, 2016-08-30 07:07:21]

Rebase + fiexes.

[brandtr, 2016-08-30 07:08:37]

Patchset #12 (id:230001) has been deleted

[brandtr, 2016-08-30 08:34:03]

Rebase on top of 'Reorder member functions in RtpFecTest.'

[brandtr, 2016-08-30 09:19:36]

Rebase + add unit test.

[brandtr, 2016-08-30 09:27:24]

Rename ssrc_base to ssrc.

[brandtr, 2016-08-30 13:32:17]

Update based on feedback comments in pt. 4.

[brandtr, 2016-08-30 14:25:04]

Patchset #17 (id:350001) has been deleted

[brandtr, 2016-08-30 14:28:26]

Rebase

[brandtr, 2016-09-01 11:56:38]

Rebase + updates.

[brandtr, 2016-09-01 13:55:38]

Patchset #18 (id:390001) has been deleted

[brandtr, 2016-09-01 13:55:41]

Rebase + updates.

[brandtr, 2016-09-02 07:24:53]

Rebase.

[brandtr, 2016-09-02 08:27:39]

Rebase.

[brandtr, 2016-09-02 11:08:01]

Changes.

[brandtr, 2016-09-02 11:52:38]

Changes.

[danilchap, 2016-09-06 09:40:54]

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:37: constexpr
size_t kBaseHeaderSize = 12;
that doesn't include first SSRC and base Seq Num? why?

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:41: constexpr
size_t kAuxHeaderSize0KBitsSet = 6 + kFlexfecPacketMaskSize0KBitsSet;
not sure how this constants helps.
It seems it would be nicer with kBaseHeaderSize = 18 and without this 4
constants.

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:72: bool f_bit =
(fec_packet->pkt->data[0] & 0x80) != 0u;
will (win/android) compiler complain if you write "!= 0"?

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:96: bool k_bit0 =
false, k_bit1 = false, k_bit2 = false;
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.

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:101:
ByteWriter<uint16_t>::WriteBigEndian(&fec_packet->pkt->data[18], mask_part0);
Can Writing to the data you are reading from
in the ReadSomething function be avoided?

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:102: if (!k_bit0)
{
did you check if base/bitbuffer can make your code cleaner?

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:116: uint64_t one
= 1;  // Ensures enough bits available for the bit shifts.
may be use 1ULL instead of this constant?

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:167: bool
ulpfec_l_bit = (packet_mask_size == kUlpfecPacketMaskSizeLBitSet);
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";
}

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:170: bool bit46 =
(packet_mask[5] & 0x01) != 0u;
can you operator on bit46 and bit47 togehter?
if (packet_mask[5] & 0x03) {
  // Comment explaining why it doesn't fit.
  return k..2KBitsSet;
}

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h (right):

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h:19: // FlexFEC
header, minimum 24 bytes.
I can see only 20 non optional bytes. Where are the rest 4?

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h:23: // |F|R| P|X| 
CC   |M| PT recovery |         length recovery      |
May be add a two char column where you write offset for each row:
//  0 |F|R|...
//  4 |   TS recovery...
//  8 | ssrcCount | ...
// 12 | ... SSRC_1
Since you are using those offset constants directly, in the Reader.

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right):

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:988: // Simulate FlexFEC
packet received on different SSRC.
Why do you need to simulate?
Does it mean EncodeFec can't use distinct ssrcs for media and fec?

[brandtr, 2016-09-06 14:36:08]

Rebase.

[brandtr, 2016-09-07 08:16:37]

Patchset #24 (id:530001) has been deleted

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

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:37: constexpr
size_t kBaseHeaderSize = 12;
On 2016/09/06 09:40:53, danilchap wrote:
> that doesn't include first SSRC and base Seq Num? why?

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.

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:41: constexpr
size_t kAuxHeaderSize0KBitsSet = 6 + kFlexfecPacketMaskSize0KBitsSet;
On 2016/09/06 09:40:53, danilchap wrote:
> not sure how this constants helps.
> It seems it would be nicer with kBaseHeaderSize = 18 and without this 4
> constants.

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.

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:72: bool f_bit =
(fec_packet->pkt->data[0] & 0x80) != 0u;
On 2016/09/06 09:40:53, danilchap wrote:
> will (win/android) compiler complain if you write "!= 0"?

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

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:96: bool k_bit0 =
false, k_bit1 = false, k_bit2 = false;
On 2016/09/06 09:40:53, danilchap wrote:
> 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.

Done.

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:101:
ByteWriter<uint16_t>::WriteBigEndian(&fec_packet->pkt->data[18], mask_part0);
On 2016/09/06 09:40:53, danilchap wrote:
> Can Writing to the data you are reading from
> in the ReadSomething function be avoided?

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

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:102: if (!k_bit0)
{
On 2016/09/06 09:40:53, danilchap wrote:
> did you check if base/bitbuffer can make your code cleaner?

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

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:116: uint64_t one
= 1;  // Ensures enough bits available for the bit shifts.
On 2016/09/06 09:40:53, danilchap wrote:
> may be use 1ULL instead of this constant?

Done.

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:167: bool
ulpfec_l_bit = (packet_mask_size == kUlpfecPacketMaskSizeLBitSet);
On 2016/09/06 09:40:53, danilchap wrote:
> 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";
> }

Done.

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:170: bool bit46 =
(packet_mask[5] & 0x01) != 0u;
On 2016/09/06 09:40:53, danilchap wrote:
> can you operator on bit46 and bit47 togehter?
> if (packet_mask[5] & 0x03) {
>   // Comment explaining why it doesn't fit.
>   return k..2KBitsSet;
> }

Done.

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h (right):

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h:19: // FlexFEC
header, minimum 24 bytes.
On 2016/09/06 09:40:53, danilchap wrote:
> I can see only 20 non optional bytes. Where are the rest 4?

24 was incorrect, 20 is correct.

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h:23: // |F|R| P|X| 
CC   |M| PT recovery |         length recovery      |
On 2016/09/06 09:40:53, danilchap wrote:
> May be add a two char column where you write offset for each row:
> //  0 |F|R|...
> //  4 |   TS recovery...
> //  8 | ssrcCount | ...
> // 12 | ... SSRC_1
> Since you are using those offset constants directly, in the Reader.

Done.

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right):

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:988: // Simulate FlexFEC
packet received on different SSRC.
On 2016/09/06 09:40:53, danilchap wrote:
> Why do you need to simulate?
> Does it mean EncodeFec can't use distinct ssrcs for media and fec?

That's right. EncodeFec is oblivious to if the FEC packets are encapsulated in
RED (i.e., sent on the media SSRC) or if they are sent as FlexFEC packets (i.e.,
sent on their own SSRC).

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

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/490001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:72: bool f_bit =
(fec_packet->pkt->data[0] & 0x80) != 0u;
On 2016/09/07 08:21:14, brandtr wrote:
> On 2016/09/06 09:40:53, danilchap wrote:
> > will (win/android) compiler complain if you write "!= 0"?
> 
> 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".

!= 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) {....

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:89: if
(ssrc_count != 1U) {
I think integer promotion work very well for constants outside templates, i.e.
no need for suffix here.

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:105: bool k_bit0
= (mask_part0 & (1 << 15)) != 0;  // Read K-bit 0.
0x80 for f_bit and (1 << 15) here.
Choose the one you like more and use same, for consistency.

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:114: bool k_bit1
= (mask_part1 & (1 << 31)) != 0;  // Read K-bit 1.
may be k_bit1 = (fec_packet->pkt->data[20] & 0x80) != 0; ?

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:242:
memset(&fec_packet->data[24], 0U, 8);  // Clear all trailing bits.
memset take 2nd parameter as (signed) int actually.

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h (right):

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h:28: //   
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
May be annotate this line differently:
+=+=+=+=+
to stress border between base header and stream specific header.

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h:38: //   
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
And make this line same

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h:39: // 32 |       
             ... next in SSRC_i ...                    |
Because of variable length of previous header, it might be other value than 32,
so might be better to remove 32 (that number you do not use directly while
reading)

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h:40: //   
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
May be remove this line to note 'next in SSRC_i' take more than 4 bytes.
For same reason replace '|' with ':' in previous line

[brandtr, 2016-09-12 08:36:33]

Patchset #26 (id:590001) has been deleted

[brandtr, 2016-09-12 09:36:35]

Patchset #28 (id:650001) has been deleted

[brandtr, 2016-09-12 11:31:21]

Comments adressed.

Also added some bounds checking in the header reading.

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:89: if
(ssrc_count != 1U) {
On 2016/09/07 12:41:17, danilchap wrote:
> I think integer promotion work very well for constants outside templates, i.e.
> no need for suffix here.

Done.

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:105: bool k_bit0
= (mask_part0 & (1 << 15)) != 0;  // Read K-bit 0.
On 2016/09/07 12:41:17, danilchap wrote:
> 0x80 for f_bit and (1 << 15) here.
> Choose the one you like more and use same, for consistency.

Updated to use variable & 0xYZ for consistency.

I had to reorder some things, since I'm not able to index into the mask_partX
variables. (This is since I do bitshifts on them, see e.g. line 121 in the new
code.)

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:114: bool k_bit1
= (mask_part1 & (1 << 31)) != 0;  // Read K-bit 1.
On 2016/09/07 12:41:17, danilchap wrote:
> may be k_bit1 = (fec_packet->pkt->data[20] & 0x80) != 0; ?

With the reordering, I now always read from the packet buffer like this. Before
I would copy to the temp variables, and read from there.

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:242:
memset(&fec_packet->data[24], 0U, 8);  // Clear all trailing bits.
On 2016/09/07 12:41:17, danilchap wrote:
> memset take 2nd parameter as (signed) int actually.

Fixed.

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h (right):

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h:28: //   
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
On 2016/09/07 12:41:17, danilchap wrote:
> May be annotate this line differently:
> +=+=+=+=+
> to stress border between base header and stream specific header.

Done.

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h:38: //   
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
On 2016/09/07 12:41:17, danilchap wrote:
> And make this line same

Done.

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h:39: // 32 |       
             ... next in SSRC_i ...                    |
On 2016/09/07 12:41:17, danilchap wrote:
> Because of variable length of previous header, it might be other value than
32,
> so might be better to remove 32 (that number you do not use directly while
> reading)

Done.

https://codereview.webrtc.org/2269903002/diff/550001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h:40: //   
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
On 2016/09/07 12:41:17, danilchap wrote:
> May be remove this line to note 'next in SSRC_i' take more than 4 bytes.
> For same reason replace '|' with ':' in previous line

Good idea, done.

[danilchap, 2016-09-12 13:30:30]

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:84: // Not enough
space for the packet mask.
may be turn this comment into log.

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:116: bool k_bit0
= (fec_packet->pkt->data[18] & 0x80) != 0;  // Read K-bit 0.
may be
uint8_t* const packet_mask = &fec_packet->pkt->data[kPacketMaskOffset];
and then use that variable instead of referencing fec_packet each time.

(after todo above would be resolved, this variable will split into two:
const uint8_t* const fec_packet_mask = ...
uint8_t* const dst_packet_mask = ...)

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:154:
fec_packet->pkt->data[23] |= 0x01;  // Set bit 46 in adapted mask.
shouldn't bit46 be more significant than bit47?
i.e. |= 0x02 here
Might be simpler to work with bit46&bit47 together:
uint8_t tail_bits = (packet_mask[6] >> 5) & 0x03;
packet_mask[5] |= tail_bits;

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:211: return
kFlexfecPacketMaskSize2KBitsSet;  // Should never reach here.
move inside else just below RTC_NOTREACHED, comment wouldn't be needed then.

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
(right):

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:267:
TEST(FlexfecHeaderReaderWriterTest, ReadPacketWithoutStreamSpecificHeader) {
It is recommended to add the expectation to the test name too:
ReadPacketWithoutStreamSpecificHeaderShouldFail

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:291:
std::unique_ptr<ReceivedFecPacket> read_packet0(new ReceivedFecPacket());
do you need pointer here?
ReceivedFecPacket read_packet0;
...
EXPECT_FALSE(reader.ReadFecHeader(&read_packet0));

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:293:
read_packet0->pkt = rtc::scoped_refptr<Packet>(new Packet());
may be make WriteHeader return rtc::scoped_refptr instead of unique_ptr so that
initialization can be slight simpler here:
read_packet0.pkt.reset(std::move(written_packet));
read_packet0.pkt->length = 18;

[brandtr, 2016-09-13 11:39:24]

Rebase.

[brandtr, 2016-09-13 12:04:04]

Rebase.

[brandtr, 2016-09-13 12:04:44]

Patchset #29 (id:690001) has been deleted

[brandtr, 2016-09-14 07:00:50]

Patchset #30 (id:730001) has been deleted

[brandtr, 2016-09-14 11:37:45]

Patchset #32 (id:790001) has been deleted

[brandtr, 2016-09-14 11:45:55]

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:84: // Not enough
space for the packet mask.
On 2016/09/12 13:30:29, danilchap wrote:
> may be turn this comment into log.

Done.

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:116: bool k_bit0
= (fec_packet->pkt->data[18] & 0x80) != 0;  // Read K-bit 0.
On 2016/09/12 13:30:29, danilchap wrote:
> may be
> uint8_t* const packet_mask = &fec_packet->pkt->data[kPacketMaskOffset];
> and then use that variable instead of referencing fec_packet each time.
> 
> (after todo above would be resolved, this variable will split into two:
> const uint8_t* const fec_packet_mask = ...
> uint8_t* const dst_packet_mask = ...)

Done.

Cannot make it a '_const_ uint8_t* const' however, since the ReadFecHeader
function currently uses in-band storage for the ULPFECized packet mask. This
will change when I refactor the packet classes.

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:154:
fec_packet->pkt->data[23] |= 0x01;  // Set bit 46 in adapted mask.
On 2016/09/12 13:30:29, danilchap wrote:
> shouldn't bit46 be more significant than bit47?
> i.e. |= 0x02 here
> Might be simpler to work with bit46&bit47 together:
> uint8_t tail_bits = (packet_mask[6] >> 5) & 0x03;
> packet_mask[5] |= tail_bits;

Good idea, done!

Yes, this was a bug in both the reader and the writer... Will add custom tests
as proposed by holmer in the Ulpfec CL.

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:211: return
kFlexfecPacketMaskSize2KBitsSet;  // Should never reach here.
On 2016/09/12 13:30:29, danilchap wrote:
> move inside else just below RTC_NOTREACHED, comment wouldn't be needed then.

Ok. I placed the return outside the if clause due to the "Don't use else after
return." Chromium rule, but I guess that I violated that by having the
RTC_NOTREACHED() inside the else in the first place.

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
(right):

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:267:
TEST(FlexfecHeaderReaderWriterTest, ReadPacketWithoutStreamSpecificHeader) {
On 2016/09/12 13:30:29, danilchap wrote:
> It is recommended to add the expectation to the test name too:
> ReadPacketWithoutStreamSpecificHeaderShouldFail

Done.

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:291:
std::unique_ptr<ReceivedFecPacket> read_packet0(new ReceivedFecPacket());
On 2016/09/12 13:30:29, danilchap wrote:
> do you need pointer here?
> ReceivedFecPacket read_packet0;
> ...
> EXPECT_FALSE(reader.ReadFecHeader(&read_packet0));

Nope, changed.

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:293:
read_packet0->pkt = rtc::scoped_refptr<Packet>(new Packet());
On 2016/09/12 13:30:29, danilchap wrote:
> may be make WriteHeader return rtc::scoped_refptr instead of unique_ptr so
that
> initialization can be slight simpler here:
> read_packet0.pkt.reset(std::move(written_packet));
> read_packet0.pkt->length = 18;

Done.

scoped_refptr does not have a reset(..) method, so using operator= instead. I
guess std::move is not strictly necessary then, but still using it.

https://codereview.webrtc.org/2269903002/diff/750001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/750001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:34: constexpr
size_t kFlexfecPacketMaskSize1KBitsSet = 6;
These names were not strictly correct, so I changed them here and in the unit
tests.

https://codereview.webrtc.org/2269903002/diff/750001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
(right):

https://codereview.webrtc.org/2269903002/diff/750001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:41:
constexpr size_t kFlexfecHeaderSize2KBitsSet = 32;
See comment in implementation file.

[danilchap, 2016-09-14 13:12:10]

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
(right):

https://codereview.webrtc.org/2269903002/diff/670001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:293:
read_packet0->pkt = rtc::scoped_refptr<Packet>(new Packet());
On 2016/09/14 11:45:54, brandtr wrote:
> On 2016/09/12 13:30:29, danilchap wrote:
> > may be make WriteHeader return rtc::scoped_refptr instead of unique_ptr so
> that
> > initialization can be slight simpler here:
> > read_packet0.pkt.reset(std::move(written_packet));
> > read_packet0.pkt->length = 18;
> 
> Done.
> 
> scoped_refptr does not have a reset(..) method, so using operator= instead. I
> guess std::move is not strictly necessary then, but still using it.

Look like it can accept raw pointers in the operator=, i.e.
read_packet0->pkt = new Packet();
should work


while move is not needed, it would cause move constructor instead of copy
constructor and save unneccesary AddRef/Release.
So yes, still a good idea, though not that important for tests.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:33: constexpr
size_t kFlexfecPacketMaskSizeKBit0Set = 2;
Yes, this name(s) explain constant(s) better.
Do you think last 'Set' is helpful or constant can be shorten a bit?
May be reorder words to make 'Size' last word, e.g.
kFlexfexPacketMaskKBit0Size = 2;
or 
kFlexfecPacketMaskSizes = {2, 6, 14};

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:119: bool k_bit0
= (packet_mask[0] & 0x80) != 0;  // Read K-bit 0.
comment probably not needed because duplicates variable name.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
(right):

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:116:
TEST(FlexfecHeaderReaderTest, CorrectlyReadsHeaderWithKBit0Set) {
'Correctly' is implicit for tests (at least should be), so can be dropped.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:118:
0x00, 0x00, 0xab, 0xcd,  // F, R bits clear, "random" length recovery
remove 'random', it is misleading

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:119:
0x01, 0x23, 0x45, 0x67,  // "random" TS recovery
what is TS?

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:120:
0x01, 0x00, 0x00, 0x00,  // SSRCCount = 1. Reserved bits are zero.
may be 
constexpr size_t kSsrcCount = 1;
constexpr uint8_t kReserved = 0;
kSsrcCount, kReserved, kReserved, kReserved;

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:122:
0xaa, 0xbb, 0x88, 0x81,  // "random" SN base, K-bit 0 set, "random" mask
Having 3 comments describing different bytes doesn't help much unless you
remember flexfec header format by heart.
consider
constexpr uint8_t kKBit = 0x80;
0xaa, 0xbb,  // Sequence Number base.
             kKBit | 0x08, 0x81,   // K-bit0 | packet mask.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:123:
0x00, 0x00, 0x00, 0x00   // payload
try to start comments with Uppercase and finish with a dot.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:131:
EXPECT_TRUE(reader.ReadFecHeader(&read_packet));
add an empty line above to split preparation and execution.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:135:
static_cast<uint32_t>((0x11 << 24) + (0x22 << 16) + (0x33 << 8) + 0x44),
use 0x11223344
or better define a constant above kSsrc = 0x11223344;

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:142:
EXPECT_EQ(packet_length - 20, read_packet.protection_length);
what is 20?

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:144:
const uint8_t ulpfec_packet_mask[] = {0x11, 0x02};
name it as a constant, not a variable and move to preparation part.
May be add a constants part with comments that manually created kPacket matches
constants.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:302:
static_cast<uint32_t>((packet[12] << 24) + (packet[13] << 16) +
may be 
EXPECT_EQ(kMediaSsrc, ByteReader<uint32_t>::ReadBigEndian(packet + 12));

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:308: 0,
memcmp(flexfec_packet_mask, packet + 18, sizeof(flexfec_packet_mask)));
might be better to split into 2 expectations:
EXPECT_EQ(kFlexfecPacketMask[0], packet[18]);
EXPECT_EQ(kFlexfecPacketMask[1], packet[19]);
this way if it failes, you can also see the values of expected and actual bytes,
but with memcmp all you would see is that they are different.
(but avoid using for loop - with it will be hard to see which byte is different)
Or, consider using gmock matchers to compare arrays:
EXPECT_THAT(::testing::make_tuple(packet + 18, 2), ElementsAre(0x88, 0x81)); //
Or ElementsAreArray(kFlexfexPacketMask);

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:323:
EXPECT_EQ(0x00, packet[0] & 0xc0);  // F and R bits clear.
may be test seperatly:
constexpr uint8_t kFbit = 1 << 7;
constexpr uint8_t kRbit = 1 << 6;
EXPECT_FALSE(packet[0] & kFbit);
EXPECT_FALSE(packet[0] & kRbit);

[brandtr, 2016-09-20 09:15:31]

Patchset #33 (id:830001) has been deleted

[brandtr, 2016-09-20 09:31:48]

Rebase after gyp deprecation.

[brandtr, 2016-09-20 11:02:17]

Patchset #35 (id:890001) has been deleted

[brandtr, 2016-09-20 11:07:51]

Rebase.

[brandtr, 2016-09-20 11:13:34]

Addressed comments from danilchap.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:33: constexpr
size_t kFlexfecPacketMaskSizeKBit0Set = 2;
On 2016/09/14 13:12:09, danilchap wrote:
> Yes, this name(s) explain constant(s) better.
> Do you think last 'Set' is helpful or constant can be shorten a bit?
> May be reorder words to make 'Size' last word, e.g.
> kFlexfexPacketMaskKBit0Size = 2;
> or 
> kFlexfecPacketMaskSizes = {2, 6, 14};

Done.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:119: bool k_bit0
= (packet_mask[0] & 0x80) != 0;  // Read K-bit 0.
On 2016/09/14 13:12:09, danilchap wrote:
> comment probably not needed because duplicates variable name.

Done.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
(right):

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:116:
TEST(FlexfecHeaderReaderTest, CorrectlyReadsHeaderWithKBit0Set) {
On 2016/09/14 13:12:09, danilchap wrote:
> 'Correctly' is implicit for tests (at least should be), so can be dropped.

Changed.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:118:
0x00, 0x00, 0xab, 0xcd,  // F, R bits clear, "random" length recovery
On 2016/09/14 13:12:09, danilchap wrote:
> remove 'random', it is misleading

Done.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:119:
0x01, 0x23, 0x45, 0x67,  // "random" TS recovery
On 2016/09/14 13:12:09, danilchap wrote:
> what is TS?

TimeStamp. This follows the nomenclature in the Internet Draft.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:120:
0x01, 0x00, 0x00, 0x00,  // SSRCCount = 1. Reserved bits are zero.
On 2016/09/14 13:12:09, danilchap wrote:
> may be 
> constexpr size_t kSsrcCount = 1;
> constexpr uint8_t kReserved = 0;
> kSsrcCount, kReserved, kReserved, kReserved;

Done.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:122:
0xaa, 0xbb, 0x88, 0x81,  // "random" SN base, K-bit 0 set, "random" mask
On 2016/09/14 13:12:09, danilchap wrote:
> Having 3 comments describing different bytes doesn't help much unless you
> remember flexfec header format by heart.
> consider
> constexpr uint8_t kKBit = 0x80;
> 0xaa, 0xbb,  // Sequence Number base.
>              kKBit | 0x08, 0x81,   // K-bit0 | packet mask.

Changed, now I align the different packet masks with each other.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:123:
0x00, 0x00, 0x00, 0x00   // payload
On 2016/09/14 13:12:09, danilchap wrote:
> try to start comments with Uppercase and finish with a dot.

Done.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:131:
EXPECT_TRUE(reader.ReadFecHeader(&read_packet));
On 2016/09/14 13:12:09, danilchap wrote:
> add an empty line above to split preparation and execution.

Done.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:135:
static_cast<uint32_t>((0x11 << 24) + (0x22 << 16) + (0x33 << 8) + 0x44),
On 2016/09/14 13:12:09, danilchap wrote:
> use 0x11223344
> or better define a constant above kSsrc = 0x11223344;

Done.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:142:
EXPECT_EQ(packet_length - 20, read_packet.protection_length);
On 2016/09/14 13:12:09, danilchap wrote:
> what is 20?

It's the FEC header size. This has been clarified, by reusing existing
variables.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:144:
const uint8_t ulpfec_packet_mask[] = {0x11, 0x02};
On 2016/09/14 13:12:09, danilchap wrote:
> name it as a constant, not a variable and move to preparation part.
> May be add a constants part with comments that manually created kPacket
matches
> constants.

Done.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:302:
static_cast<uint32_t>((packet[12] << 24) + (packet[13] << 16) +
On 2016/09/14 13:12:09, danilchap wrote:
> may be 
> EXPECT_EQ(kMediaSsrc, ByteReader<uint32_t>::ReadBigEndian(packet + 12));

Done.

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:308: 0,
memcmp(flexfec_packet_mask, packet + 18, sizeof(flexfec_packet_mask)));
On 2016/09/14 13:12:09, danilchap wrote:
> might be better to split into 2 expectations:
> EXPECT_EQ(kFlexfecPacketMask[0], packet[18]);
> EXPECT_EQ(kFlexfecPacketMask[1], packet[19]);
> this way if it failes, you can also see the values of expected and actual
bytes,
> but with memcmp all you would see is that they are different.
> (but avoid using for loop - with it will be hard to see which byte is
different)
> Or, consider using gmock matchers to compare arrays:
> EXPECT_THAT(::testing::make_tuple(packet + 18, 2), ElementsAre(0x88, 0x81));
//
> Or ElementsAreArray(kFlexfexPacketMask);

Done.

Thanks for writing out the gmock code for me! I find that that library has a
steep learning curve for the syntax...

https://codereview.webrtc.org/2269903002/diff/810001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:323:
EXPECT_EQ(0x00, packet[0] & 0xc0);  // F and R bits clear.
On 2016/09/14 13:12:09, danilchap wrote:
> may be test seperatly:
> constexpr uint8_t kFbit = 1 << 7;
> constexpr uint8_t kRbit = 1 << 6;
> EXPECT_FALSE(packet[0] & kFbit);
> EXPECT_FALSE(packet[0] & kRbit);

Done.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
(right):

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:102:
const uint8_t* expected_packet_mask,
Function added to verify the headers read by the Reader.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:123:
size_t expected_packet_mask_size,
Function added to verify the headers finalized by the Writer.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:138:
void VerifyWrittenAndReadHeaders(size_t expected_fec_header_size,
This is the old "VerifyHeaders" function.

[danilchap, 2016-09-20 15:59:14]

looks good overall,
next batch of nits

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:83: return false;
may be log as well.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:221:
ByteWriter<uint32_t>::WriteBigEndian(&fec_packet->data[12], ssrc);
may be clear bytes [9,11] too. Or do you assume there was memset(0) on  full
fec_packet?

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
(right):

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:46:
constexpr uint8_t kFbit = 0x00;
may be
kNoFbit = 0 << 7;
kNoRbit = 0 << 6;

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:116:
EXPECT_THAT(
EXPECT_THAT use reverse to EXPECT_EQ meaning for parameters:
1st parameter is value under test,
2nd parameter - matcher/expected value.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:197:
constexpr uint8_t kBit0 = 0x00;
kKBit0 ?

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:202:
constexpr uint8_t kFlxfcPktMsk[] =   {kBit0 | 0x48, 0x81,
can remove two spaces after =

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:217:
read_packet.pkt = rtc::scoped_refptr<Packet>(new Packet());
doesn't
read_packet.pkt = new Packet();
work?
or you want to be explicit about using smart pointer here?

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:428:
TEST(FlexfecHeaderWriterTest, ExpandsLongUlpfecPacketMaskWithBits46And47Clear) {
WithBit46Set

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right):

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:165: const uint32_t
ssrc = ParseSsrc(media_packets.front().get()->data);
".get()" can be removed: unique_pointers have operator->

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:319: uint32_t ssrc,
that one is media_ssrc or fec_ssrc?

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right):

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:998:
recovered_packets_.clear();
is cleanup a part of the test?

[brandtr, 2016-09-21 08:55:07]

Patchset #37 (id:950001) has been deleted

[brandtr, 2016-09-21 09:08:06]

Patchset #37 (id:970001) has been deleted

[brandtr, 2016-09-21 09:15:11]

Patchset #37 (id:990001) has been deleted

[brandtr, 2016-09-21 09:23:24]

Batch of nits addressed :)

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:83: return false;
On 2016/09/20 15:59:14, danilchap wrote:
> may be log as well.

So actually an F bit (or R bit) which is set is not a malformed FEC packet, it's
just that we currently do not support those packets. If I return true here, then
an unparsed packet will be used in the recovery, which is not good. But by
returning false, I somehow imply that there is something wrong with this FEC
packet, which there isn't.

I could change to returning an int, with 0 for success, -1 for error and -2 for
unsupported packets. Or maybe an enum. WDYT?

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:87: return false;
Ditto.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:92: return false;
Ditto.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:221:
ByteWriter<uint32_t>::WriteBigEndian(&fec_packet->data[12], ssrc);
On 2016/09/20 15:59:14, danilchap wrote:
> may be clear bytes [9,11] too. Or do you assume there was memset(0) on  full
> fec_packet?

Currently we memset the entire packet to zero initially, but I might as well
write zeroes here too. Better to avoid hidden assumptions like that, and you
could argue that settings those bits to zero is part of finalizing the FEC
header.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
(right):

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:46:
constexpr uint8_t kFbit = 0x00;
On 2016/09/20 15:59:14, danilchap wrote:
> may be
> kNoFbit = 0 << 7;
> kNoRbit = 0 << 6;

Good idea, done.

Also removed kMbit, because that is actually the M _recovery_ bit in the FEC
header, not the M bit in the RTP header.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:116:
EXPECT_THAT(
On 2016/09/20 15:59:14, danilchap wrote:
> EXPECT_THAT use reverse to EXPECT_EQ meaning for parameters:
> 1st parameter is value under test,
> 2nd parameter - matcher/expected value.

Oops, thanks!

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:197:
constexpr uint8_t kBit0 = 0x00;
On 2016/09/20 15:59:14, danilchap wrote:
> kKBit0 ?

Thanks.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:202:
constexpr uint8_t kFlxfcPktMsk[] =   {kBit0 | 0x48, 0x81,
On 2016/09/20 15:59:14, danilchap wrote:
> can remove two spaces after =

Done.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:217:
read_packet.pkt = rtc::scoped_refptr<Packet>(new Packet());
On 2016/09/20 15:59:14, danilchap wrote:
> doesn't
> read_packet.pkt = new Packet();
> work?
> or you want to be explicit about using smart pointer here?

Yes, my intention is to be explicit here. Otherwise you have to look into the
class definition to see that |pkt| is an rtc::scoped_refptr, and then realize
that operator=(Packet*) increments the refcount of Packet pointed to by the raw
pointer. I like the explicit way better.

If I understand things correctly, the scoped_refptr that I create here is an
rvalue, so the move assignment operator will be used.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:428:
TEST(FlexfecHeaderWriterTest, ExpandsLongUlpfecPacketMaskWithBits46And47Clear) {
On 2016/09/20 15:59:14, danilchap wrote:
> WithBit46Set

Done.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right):

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:165: const uint32_t
ssrc = ParseSsrc(media_packets.front().get()->data);
On 2016/09/20 15:59:14, danilchap wrote:
> ".get()" can be removed: unique_pointers have operator->

Yep, this was sloppy.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:319: uint32_t ssrc,
On 2016/09/20 15:59:14, danilchap wrote:
> that one is media_ssrc or fec_ssrc?

Clarified.

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/BUILD.gn...
webrtc/modules/BUILD.gn:248:
"audio_coding/audio_network_adaptor/smoothing_filter_unittest.cc",
Rebase artifact.

[brandtr, 2016-09-21 10:50:16]

Rebase.

[brandtr, 2016-09-21 10:59:21]

Missed one comment.

[brandtr, 2016-09-21 11:00:43]

Missed one comment in previous reply.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right):

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:998:
recovered_packets_.clear();
On 2016/09/20 15:59:14, danilchap wrote:
> is cleanup a part of the test?

Nope, this was an artifact from the times when we used raw pointers and manual
memory management. Removed.

[danilchap, 2016-09-21 11:17:56]

lgtm
there are always nits though, but still looks good

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:83: return false;
On 2016/09/21 09:23:23, brandtr wrote:
> On 2016/09/20 15:59:14, danilchap wrote:
> > may be log as well.
> 
> So actually an F bit (or R bit) which is set is not a malformed FEC packet,
it's
> just that we currently do not support those packets. If I return true here,
then
> an unparsed packet will be used in the recovery, which is not good. But by
> returning false, I somehow imply that there is something wrong with this FEC
> packet, which there isn't.
> 
> I could change to returning an int, with 0 for success, -1 for error and -2
for
> unsupported packets. Or maybe an enum. WDYT?

returning false means ReadFecHeader failed to read the fec header. (function is
not called 'Valid')
'Not supported' is one of the reason why read may fail.

Do you plan as caller treat 'fail because invalid' and 'fail because
unsupported' different? Do not complicate return type until you would.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:221:
ByteWriter<uint32_t>::WriteBigEndian(&fec_packet->data[12], ssrc);
On 2016/09/21 09:23:23, brandtr wrote:
> On 2016/09/20 15:59:14, danilchap wrote:
> > may be clear bytes [9,11] too. Or do you assume there was memset(0) on  full
> > fec_packet?
> 
> Currently we memset the entire packet to zero initially, but I might as well
> write zeroes here too. Better to avoid hidden assumptions like that, and you
> could argue that settings those bits to zero is part of finalizing the FEC
> header.

if you like this line to look more like neighbors, you can write 
ByteWriter<uint32_t, 3>::WriteBigEndian(&fec_packet->data, 0, kReserved);
or DCHECK those bytes are already 0:
RTC_DCHECK_EQ(0u, ByteReader<uint32_t, 3>::ReadBigEndian(&fec_packet->data[9]);

Choose the one you like most. (memset is nice too)

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
(right):

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:217:
read_packet.pkt = rtc::scoped_refptr<Packet>(new Packet());
On 2016/09/21 09:23:23, brandtr wrote:
> On 2016/09/20 15:59:14, danilchap wrote:
> > doesn't
> > read_packet.pkt = new Packet();
> > work?
> > or you want to be explicit about using smart pointer here?
> 
> Yes, my intention is to be explicit here. Otherwise you have to look into the
> class definition to see that |pkt| is an rtc::scoped_refptr, and then realize
> that operator=(Packet*) increments the refcount of Packet pointed to by the
raw
> pointer. I like the explicit way better.
> 
> If I understand things correctly, the scoped_refptr that I create here is an
> rvalue, so the move assignment operator will be used.

Yes, from perfomance point of view it is same. Even if an extra AddRef/Release
would be involved, it doesn't really matter for the test code.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:428:
TEST(FlexfecHeaderWriterTest, ExpandsLongUlpfecPacketMaskWithBits46And47Clear) {
On 2016/09/21 09:23:24, brandtr wrote:
> On 2016/09/20 15:59:14, danilchap wrote:
> > WithBit46Set
> 
> Done.

It still reads as if Bit46 is cleared too.
Since it shouldn't matter if Bit47 Set or Cleared for correctness of this test,
might be better not to mention Bit47 in the name.
Still good idea to Clear bit47 to avoid this test pass for the wrong reason.

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
(right):

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:158:
::testing::make_tuple(written_packet.data + expected_fec_header_size,
may be swap here too (read_packet is tested, written_packet is expected value)

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:162:
written_packet.length - expected_fec_header_size));
may be read_packet.pkt->length instead of written_packet.length?

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:176:
kNoFBit | kNoRBit, kPtRecov,       kLengthRecov[0],    kLengthRecov[1],
may do not shortent since you have spaces for fuller name kPtRecovery

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:471:
WriteAndReadSmallUlpfecPacketHeaderWithMaskKBit15Clear) {
KBit15? wasn't K prefix just for the 1st bits in each of the 3 packet mask
blocks

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:485:
WriteAndReadSmallUlpfecPacketHeaderWithMaskKBit15Set) {
same here

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right):

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:167:
ParseSequenceNumber(media_packets.front().get()->data);
can remove .get() here too

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc:102: uint32_t
media_ssrc,
there is construct
uint32_t /* media_ssrc */
to stress variable is unused. Not widely used in webrtc code, but recommended by
base google style
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...

[brandtr, 2016-09-21 11:43:28]

Thanks for super quick response time. Nits addressed :)

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
(right):

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:158:
::testing::make_tuple(written_packet.data + expected_fec_header_size,
On 2016/09/21 11:17:56, danilchap wrote:
> may be swap here too (read_packet is tested, written_packet is expected value)

Good idea, done.

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:162:
written_packet.length - expected_fec_header_size));
On 2016/09/21 11:17:56, danilchap wrote:
> may be read_packet.pkt->length instead of written_packet.length?

Switched to read |written_packet|, so not changing the length here.

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:176:
kNoFBit | kNoRBit, kPtRecov,       kLengthRecov[0],    kLengthRecov[1],
On 2016/09/21 11:17:56, danilchap wrote:
> may do not shortent since you have spaces for fuller name kPtRecovery

Good point, done.

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:471:
WriteAndReadSmallUlpfecPacketHeaderWithMaskKBit15Clear) {
On 2016/09/21 11:17:55, danilchap wrote:
> KBit15? wasn't K prefix just for the 1st bits in each of the 3 packet mask
> blocks

This was a search-and-replace error :(

...WithMas_kBit_15...

Thanks for noticing!

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:485:
WriteAndReadSmallUlpfecPacketHeaderWithMaskKBit15Set) {
On 2016/09/21 11:17:56, danilchap wrote:
> same here

Done.

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right):

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:167:
ParseSequenceNumber(media_packets.front().get()->data);
On 2016/09/21 11:17:56, danilchap wrote:
> can remove .get() here too

Done.

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/1010001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc:102: uint32_t
media_ssrc,
On 2016/09/21 11:17:56, danilchap wrote:
> there is construct
> uint32_t /* media_ssrc */
> to stress variable is unused. Not widely used in webrtc code, but recommended
by
> base google style
>
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...

Done.

[brandtr, 2016-09-21 11:44:06]

Patchset #41 (id:1090001) has been deleted

[brandtr, 2016-09-21 12:51:29]

Patchset #41 (id:1110001) has been deleted

[brandtr, 2016-09-21 12:53:11]

Missed some comments from older patchset.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:83: return false;
On 2016/09/21 11:17:55, danilchap wrote:
> On 2016/09/21 09:23:23, brandtr wrote:
> > On 2016/09/20 15:59:14, danilchap wrote:
> > > may be log as well.
> > 
> > So actually an F bit (or R bit) which is set is not a malformed FEC packet,
> it's
> > just that we currently do not support those packets. If I return true here,
> then
> > an unparsed packet will be used in the recovery, which is not good. But by
> > returning false, I somehow imply that there is something wrong with this FEC
> > packet, which there isn't.
> > 
> > I could change to returning an int, with 0 for success, -1 for error and -2
> for
> > unsupported packets. Or maybe an enum. WDYT?
> 
> returning false means ReadFecHeader failed to read the fec header. (function
is
> not called 'Valid')
> 'Not supported' is one of the reason why read may fail.
> 
> Do you plan as caller treat 'fail because invalid' and 'fail because
> unsupported' different? Do not complicate return type until you would.

Will leave as is. I'm not planning on handling the two cases differently, I'm
rather planning to add support for the currently unsupported cases instead.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:221:
ByteWriter<uint32_t>::WriteBigEndian(&fec_packet->data[12], ssrc);
On 2016/09/21 11:17:55, danilchap wrote:
> On 2016/09/21 09:23:23, brandtr wrote:
> > On 2016/09/20 15:59:14, danilchap wrote:
> > > may be clear bytes [9,11] too. Or do you assume there was memset(0) on 
full
> > > fec_packet?
> > 
> > Currently we memset the entire packet to zero initially, but I might as well
> > write zeroes here too. Better to avoid hidden assumptions like that, and you
> > could argue that settings those bits to zero is part of finalizing the FEC
> > header.
> 
> if you like this line to look more like neighbors, you can write 
> ByteWriter<uint32_t, 3>::WriteBigEndian(&fec_packet->data, 0, kReserved);
> or DCHECK those bytes are already 0:
> RTC_DCHECK_EQ(0u, ByteReader<uint32_t,
3>::ReadBigEndian(&fec_packet->data[9]);
> 
> Choose the one you like most. (memset is nice too)

Cool, didn't know about the <TYPE, NUMBER_OF_BYTES> version.

I initially wrote this using two ByteWriters, one uint8_t and one uint16_t. That
didn't look very nice though, so I changed to memset. Agree that using a single
ByteWriter here looks cleaner though.

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
(right):

https://codereview.webrtc.org/2269903002/diff/930001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc:428:
TEST(FlexfecHeaderWriterTest, ExpandsLongUlpfecPacketMaskWithBits46And47Clear) {
On 2016/09/21 11:17:55, danilchap wrote:
> On 2016/09/21 09:23:24, brandtr wrote:
> > On 2016/09/20 15:59:14, danilchap wrote:
> > > WithBit46Set
> > 
> > Done.
> 
> It still reads as if Bit46 is cleared too.
> Since it shouldn't matter if Bit47 Set or Cleared for correctness of this
test,
> might be better not to mention Bit47 in the name.
> Still good idea to Clear bit47 to avoid this test pass for the wrong reason.

This is poor naming. I've updated the names to try to make it clearer that I try
all four combinations of bits 46 and 47 set and clear.

[brandtr, 2016-09-21 12:55:18]

Description was changed from

==========
Add FlexFEC header formatters.

This CL builds on the four prior ones. It adds FlexFEC header formatting
capabilities, together with unit tests.

BUG=webrtc:5654
==========

to

==========
Add FlexFEC header formatters.

Add classes that can read and finalize FlexFEC headers.

BUG=webrtc:5654
==========

[brandtr, 2016-09-21 13:44:22]

Will wait for feedback from holmer@ as well, before landing.

[brandtr, 2016-09-27 08:20:00]

Rebase.

[stefan-webrtc, 2016-09-28 07:53:30]

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:114: // but makes
it compatible with the ULPFEC masks.
It's not entirely clear to me how severe this compliance issue is?

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:144: if (k_bit1)
{
It's a bit hard to follow what the k_bit1 being set means. Can we have a comment
on that so the reader doesn't have to refer to the spec?

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:148: LOG(LS_INFO)
<< "Discarding truncated FlexFEC packet.";
Is this perhaps worth a warning?

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:152: if (k_bit2)
{
I'd like a comment here too

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:155: LOG(LS_INFO)
<< "Discarding FlexFEC packet with malformed header.";
Same here, perhaps warn?

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:158: // Copy bits
46 and 47.
Are you copying them from byte 6 to byte 5 of the packet mask? An explanation of
why would be good.

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:163: // Shift
away K-bit 2, bit 46, and bit 47 from adapted mask.
Same here I guess. An option would be to have a general comment describing what
this code does at line 144 or something.

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:190: if
(packet_mask_size == kUlpfecPacketMaskSizeLBitClear &&
I don't follow why we have ulpfec constants here.

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:191:
((packet_mask[1] & 0x01) == 0)) {
You can remove () around the equality check, here and below.

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:255: if (bit15) {
Remove {}

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:265: if (bit46) {
Remove {}, and below.

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right):

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:158:
std::unique_ptr<ForwardErrorCorrection::RecoveredPacket>;
nit: Not sure I see the value of using here. You only have these types ones
below?

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:989: it++;
++it

[brandtr, 2016-09-28 08:59:04]

Patchset #43 (id:1170001) has been deleted

[brandtr, 2016-09-28 09:10:25]

Patchset #43 (id:1190001) has been deleted

[brandtr, 2016-09-28 09:11:32]

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:114: // but makes
it compatible with the ULPFEC masks.
On 2016/09/28 07:53:29, stefan-webrtc (holmer) wrote:
> It's not entirely clear to me how severe this compliance issue is?

It's not a problem, since this parsed packet will only be operated on by the
code in ForwardErrorCorrection.

Storing the packed packet masks (i.e., without the interleaved K-bits) inside
the packet buffer is a bit of a hack, which will be removed when the FEC packet
classes have been refactored.

I have updated the comment to try to reduce any confusion.

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:144: if (k_bit1)
{
On 2016/09/28 07:53:29, stefan-webrtc (holmer) wrote:
> It's a bit hard to follow what the k_bit1 being set means. Can we have a
comment
> on that so the reader doesn't have to refer to the spec?

Done.

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:148: LOG(LS_INFO)
<< "Discarding truncated FlexFEC packet.";
On 2016/09/28 07:53:29, stefan-webrtc (holmer) wrote:
> Is this perhaps worth a warning?

Done.

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:152: if (k_bit2)
{
On 2016/09/28 07:53:29, stefan-webrtc (holmer) wrote:
> I'd like a comment here too

Done.

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:155: LOG(LS_INFO)
<< "Discarding FlexFEC packet with malformed header.";
On 2016/09/28 07:53:29, stefan-webrtc (holmer) wrote:
> Same here, perhaps warn?

Done.

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:158: // Copy bits
46 and 47.
On 2016/09/28 07:53:29, stefan-webrtc (holmer) wrote:
> Are you copying them from byte 6 to byte 5 of the packet mask? An explanation
of
> why would be good.

Done.

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:163: // Shift
away K-bit 2, bit 46, and bit 47 from adapted mask.
On 2016/09/28 07:53:29, stefan-webrtc (holmer) wrote:
> Same here I guess. An option would be to have a general comment describing
what
> this code does at line 144 or something.

Done.

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:190: if
(packet_mask_size == kUlpfecPacketMaskSizeLBitClear &&
On 2016/09/28 07:53:29, stefan-webrtc (holmer) wrote:
> I don't follow why we have ulpfec constants here.

For the time being, I call our pregenerated packet masks (which live in
forward_error_correction_internal.h) as "ULPFEC packet masks", since they were
designed with ULPFEC in mind. Whenever I get around to cleaning up
forward_error_correction_internal.{h,cc}, I could rename things to make it
clearer that the packet masks actually are agnostic to the FEC header formatting
used.

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:191:
((packet_mask[1] & 0x01) == 0)) {
On 2016/09/28 07:53:29, stefan-webrtc (holmer) wrote:
> You can remove () around the equality check, here and below.

Done.

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:255: if (bit15) {
On 2016/09/28 07:53:29, stefan-webrtc (holmer) wrote:
> Remove {}

Done.

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:265: if (bit46) {
On 2016/09/28 07:53:29, stefan-webrtc (holmer) wrote:
> Remove {}, and below.

Done.

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right):

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:158:
std::unique_ptr<ForwardErrorCorrection::RecoveredPacket>;
On 2016/09/28 07:53:29, stefan-webrtc (holmer) wrote:
> nit: Not sure I see the value of using here. You only have these types ones
> below?

Yep, no huge value. Removed.

https://codereview.webrtc.org/2269903002/diff/1130001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:989: it++;
On 2016/09/28 07:53:29, stefan-webrtc (holmer) wrote:
> ++it

Done.

[brandtr, 2016-09-30 06:57:24]

ping holmer :)

[stefan-webrtc, 2016-09-30 08:22:43]

lgtm, but I think we may want clarification about kbit2 before we start using
this.

https://codereview.webrtc.org/2269903002/diff/1210001/webrtc/modules/rtp_rtcp...
File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc (right):

https://codereview.webrtc.org/2269903002/diff/1210001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc:171: return
false;
Seems like a waste that k_bit2 has no meaning. We should look into that.

[brandtr, 2016-09-30 08:28:31]

I will reach out to the draft authors for clarification re. K-bit 2.

[brandtr, 2016-09-30 11:23:05]

Patchset #44 (id:1230001) has been deleted

[brandtr, 2016-09-30 11:57:55]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2016-09-30 11:57:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
danilchap@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2269903002/#ps1250001
(title: "Change gmock/gtest paths.")

[commit-bot: I haz the power, 2016-09-30 11:58:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-30 15:04:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 15:04:11]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[brandtr, 2016-10-03 05:59:06]

The CQ bit was checked by brandtr@webrtc.org

[commit-bot: I haz the power, 2016-10-03 05:59:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-03 07:43:27]

Description was changed from

==========
Add FlexFEC header formatters.

Add classes that can read and finalize FlexFEC headers.

BUG=webrtc:5654
==========

to

==========
Add FlexFEC header formatters.

Add classes that can read and finalize FlexFEC headers.

BUG=webrtc:5654
==========

[commit-bot: I haz the power, 2016-10-03 07:43:28]

Committed patchset #44 (id:1250001)

[commit-bot: I haz the power, 2016-10-03 07:43:37]

Description was changed from

==========
Add FlexFEC header formatters.

Add classes that can read and finalize FlexFEC headers.

BUG=webrtc:5654
==========

to

==========
Add FlexFEC header formatters.

Add classes that can read and finalize FlexFEC headers.

BUG=webrtc:5654

Committed: https://crrev.com/0496de298e38f8689ecefef27c31a3433bf3d073
Cr-Commit-Position: refs/heads/master@{#14469}
==========

[commit-bot: I haz the power, 2016-10-03 07:43:38]

Patchset 44 (id:??) landed as
https://crrev.com/0496de298e38f8689ecefef27c31a3433bf3d073
Cr-Commit-Position: refs/heads/master@{#14469}