Generalize FEC unit tests and rename GenerateFec. (pt. 3)

Generalize FEC unit tests and rename GenerateFec.

- Rename GenerateFec -> EncodeFec in ForwardErrorCorrection. This naming
  is more consistent with DecodeFec.
- Add appropriate using directives, to reduce clutter in tests.
- Move ConstructMediaPackets to fec_test_helper.{h,cc}. This will help
  future tests of ULPFEC/FlexFEC header formatters.
- Generalize tests in rtp_fec_unittest.cc to typed tests. This will help
  testing ForwardErrorCorrection with both ULPFEC and FlexFEC.

This CL should not impact functionality or performance.

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

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

Description was changed from

==========
Generalize FEC unit tests.

- Rename GenerateFec -> EncodeFec in ForwardErrorCorrection. This naming
  is more consistent with DecodeFec.
- Add appropriate using directives, to reduce clutter in tests.
- Move ConstructMediaPackets to fec_test_helper.{h,cc}. This will help
  future tests of ULPFEC/FlexFEC header formatters.
- Generalize tests in rtp_fec_unittest.cc to typed tests. This will help
  testing ForwardErrorCorrection with both ULPFEC and FlexFEC.

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

to

==========
Generalize FEC unit tests.

- Rename GenerateFec -> EncodeFec in ForwardErrorCorrection. This naming
  is more consistent with DecodeFec.
- Add appropriate using directives, to reduce clutter in tests.
- Move ConstructMediaPackets to fec_test_helper.{h,cc}. This will help
  future tests of ULPFEC/FlexFEC header formatters.
- Generalize tests in rtp_fec_unittest.cc to typed tests. This will help
  testing ForwardErrorCorrection with both ULPFEC and FlexFEC.

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

[brandtr, 2016-08-23 08:12:43]

Description was changed from

==========
Generalize FEC unit tests.

- Rename GenerateFec -> EncodeFec in ForwardErrorCorrection. This naming
  is more consistent with DecodeFec.
- Add appropriate using directives, to reduce clutter in tests.
- Move ConstructMediaPackets to fec_test_helper.{h,cc}. This will help
  future tests of ULPFEC/FlexFEC header formatters.
- Generalize tests in rtp_fec_unittest.cc to typed tests. This will help
  testing ForwardErrorCorrection with both ULPFEC and FlexFEC.

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

to

==========
Generalize FEC unit tests and rename GenerateFec.

- Rename GenerateFec -> EncodeFec in ForwardErrorCorrection. This naming
  is more consistent with DecodeFec.
- Add appropriate using directives, to reduce clutter in tests.
- Move ConstructMediaPackets to fec_test_helper.{h,cc}. This will help
  future tests of ULPFEC/FlexFEC header formatters.
- Generalize tests in rtp_fec_unittest.cc to typed tests. This will help
  testing ForwardErrorCorrection with both ULPFEC and FlexFEC.

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

[brandtr, 2016-08-23 08:13:58]

Description was changed from

==========
Generalize FEC unit tests and rename GenerateFec.

- Rename GenerateFec -> EncodeFec in ForwardErrorCorrection. This naming
  is more consistent with DecodeFec.
- Add appropriate using directives, to reduce clutter in tests.
- Move ConstructMediaPackets to fec_test_helper.{h,cc}. This will help
  future tests of ULPFEC/FlexFEC header formatters.
- Generalize tests in rtp_fec_unittest.cc to typed tests. This will help
  testing ForwardErrorCorrection with both ULPFEC and FlexFEC.

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

to

==========
Generalize FEC unit tests and rename GenerateFec.

- Rename GenerateFec -> EncodeFec in ForwardErrorCorrection. This naming
  is more consistent with DecodeFec.
- Add appropriate using directives, to reduce clutter in tests.
- Move ConstructMediaPackets to fec_test_helper.{h,cc}. This will help
  future tests of ULPFEC/FlexFEC header formatters.
- Generalize tests in rtp_fec_unittest.cc to typed tests. This will help
  testing ForwardErrorCorrection with both ULPFEC and FlexFEC.

This CL should not impact functionality or performance.

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

[brandtr, 2016-08-23 08:18:45]

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

[brandtr, 2016-08-23 08:18:46]

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

[brandtr, 2016-08-23 09:45:03]

Moved using directives and unnamed namespaces into webrtc namespace.

[danilchap, 2016-08-23 13:08:50]

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right):

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:36: using Packet =
ForwardErrorCorrection::Packet;
use full name here too, i.e.
using Packet = ::webrtc::ForwardErrorCorrection::Packet;

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:37: using RawRtpPacket =
test::fec::RawRtpPacket;
test::fec is a namespace, so you can write
using ::webrtc::test::fec::RawRtpPacket;
("using test::fec::RawRtpPacket;" would work too, but full name is prefered)

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:101:
test::fec::FrameGenerator generator_;
may be add
using ::webrtc::test::fec::FrameGenerator;
to the top of the file instead of using test::fec:: here.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/fec_test_helper.cc (right):

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_test_helper.cc:113: uint32_t ssrc =
random_->Rand<uint32_t>();
ssrc probably shouldn't be regenerated every time one construct a new set of
packets. Probably better to pass or generate it in MediaPacketGenerator
constructor

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_test_helper.cc:162:
ConstructMediaPacketsSeqNum(num_media_packets, random_->Rand<int>());
Rand<uint16_t>()

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/fec_test_helper.h (right):

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_test_helper.h:22: // needed to not clash with
another webrtc::FrameGenerator.
Uppercase 1st letter: 'Needed'.
Though probably better to remove this comment from code.
Or fix it by renaming FrameGenerator to something fec specific.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right):

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:108:
RTC_DCHECK_LE(static_cast<size_t>(num_important_packets), num_media_packets);
may be instead change num_important_packets type to size_t?
It doesn's seem to propagate too much (just into 
ProducerFec::num_important_packets_ and ProducerFec::SetFecParams)

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc (right):

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc:52:
test::fec::FrameGenerator generator_;
likely better to add
using ::webrtc::test::fec::FrameGenerator;
into using section.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right):

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:34: kRtpHeaderSize,  //
minimum packet size
Start comments with uppcase, end with '.'

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:36: fec_.MaxPacketOverhead(),
 // maximum packet size
same here

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:81: typedef
::testing::Types<ForwardErrorCorrection> FecTypes;
prefer using over typedef:
using FecTypes = ::testing::Types<ForwardErrorCorrection>
(or move
using ::testing::Types; to the top and write
using FecTypes = Types<ForwardErrorCorrection>; here.
Advantage of using over typedef - you can do it for templates)

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:188:
this->media_packet_generator_.ConstructMediaPacketsSeqNum(3, 65535);
ConstructMediaPackets and GetMediaPackets seems always go together.
May be make ConstructMediaPackets return the list. (list is a cheap class to
move)
and make media_packets a list itself, not pointer to it.

this 3 lines will turn into one then:
this->media_packets_ = this->media_packets_generator_.ConstructPackets(3,
65535);

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:933: void
RtpFecTest<ForwardErrorCorrectionType>::DeepCopyEveryNthPacket(
does it need to be templated or member of the RtpFecTest?
May be put this helper into unnamed namespace.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:934: const
ForwardErrorCorrection::PacketList& src,
if you expect PacketList would be different for different FECs, use 
typename ForwardErrorCorrectionType::PacketList here instead

[brandtr, 2016-08-23 13:43:39]

Patchset #4 (id:60001) has been deleted

[brandtr, 2016-08-23 14:03:16]

Patchset #4 (id:80001) has been deleted

[brandtr, 2016-08-23 14:05:07]

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right):

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:36: using Packet =
ForwardErrorCorrection::Packet;
On 2016/08/23 13:08:49, danilchap wrote:
> use full name here too, i.e.
> using Packet = ::webrtc::ForwardErrorCorrection::Packet;

Done.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:37: using RawRtpPacket =
test::fec::RawRtpPacket;
On 2016/08/23 13:08:49, danilchap wrote:
> test::fec is a namespace, so you can write
> using ::webrtc::test::fec::RawRtpPacket;
> ("using test::fec::RawRtpPacket;" would work too, but full name is prefered)

Done.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:101:
test::fec::FrameGenerator generator_;
On 2016/08/23 13:08:49, danilchap wrote:
> may be add
> using ::webrtc::test::fec::FrameGenerator;
> to the top of the file instead of using test::fec:: here.

Done.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/fec_test_helper.cc (right):

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_test_helper.cc:113: uint32_t ssrc =
random_->Rand<uint32_t>();
On 2016/08/23 13:08:50, danilchap wrote:
> ssrc probably shouldn't be regenerated every time one construct a new set of
> packets. Probably better to pass or generate it in MediaPacketGenerator
> constructor

Done.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_test_helper.cc:162:
ConstructMediaPacketsSeqNum(num_media_packets, random_->Rand<int>());
On 2016/08/23 13:08:50, danilchap wrote:
> Rand<uint16_t>()

Done.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/fec_test_helper.h (right):

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_test_helper.h:22: // needed to not clash with
another webrtc::FrameGenerator.
On 2016/08/23 13:08:50, danilchap wrote:
> Uppercase 1st letter: 'Needed'.
> Though probably better to remove this comment from code.
> Or fix it by renaming FrameGenerator to something fec specific.

See
https://codereview.webrtc.org/2276473002/diff/1/webrtc/modules/rtp_rtcp/sourc....

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right):

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:108:
RTC_DCHECK_LE(static_cast<size_t>(num_important_packets), num_media_packets);
On 2016/08/23 13:08:50, danilchap wrote:
> may be instead change num_important_packets type to size_t?
> It doesn's seem to propagate too much (just into 
> ProducerFec::num_important_packets_ and ProducerFec::SetFecParams)

I would like to, but it also propagates into internal::GeneratePacketMasks.

In the internal:: functions, all parameters are currently int, even though they
probably should be size_t. I'm afraid of "search and replace" changing it
though, since it may affect the choice of the packet masks. It is on my to do
list to look into though.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc (right):

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc:52:
test::fec::FrameGenerator generator_;
On 2016/08/23 13:08:50, danilchap wrote:
> likely better to add
> using ::webrtc::test::fec::FrameGenerator;
> into using section.

Done.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right):

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:34: kRtpHeaderSize,  //
minimum packet size
On 2016/08/23 13:08:50, danilchap wrote:
> Start comments with uppcase, end with '.'

Done.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:36: fec_.MaxPacketOverhead(),
 // maximum packet size
On 2016/08/23 13:08:50, danilchap wrote:
> same here

Done.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:81: typedef
::testing::Types<ForwardErrorCorrection> FecTypes;
On 2016/08/23 13:08:50, danilchap wrote:
> prefer using over typedef:
> using FecTypes = ::testing::Types<ForwardErrorCorrection>
> (or move
> using ::testing::Types; to the top and write
> using FecTypes = Types<ForwardErrorCorrection>; here.
> Advantage of using over typedef - you can do it for templates)

Done.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:188:
this->media_packet_generator_.ConstructMediaPacketsSeqNum(3, 65535);
On 2016/08/23 13:08:50, danilchap wrote:
> ConstructMediaPackets and GetMediaPackets seems always go together.
> May be make ConstructMediaPackets return the list. (list is a cheap class to
> move)
> and make media_packets a list itself, not pointer to it.
> 
> this 3 lines will turn into one then:
> this->media_packets_ = this->media_packets_generator_.ConstructPackets(3,
> 65535);

Done.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:933: void
RtpFecTest<ForwardErrorCorrectionType>::DeepCopyEveryNthPacket(
On 2016/08/23 13:08:50, danilchap wrote:
> does it need to be templated or member of the RtpFecTest?
> May be put this helper into unnamed namespace.

Done.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:934: const
ForwardErrorCorrection::PacketList& src,
On 2016/08/23 13:08:50, danilchap wrote:
> if you expect PacketList would be different for different FECs, use 
> typename ForwardErrorCorrectionType::PacketList here instead

Nope, the PacketList will always be the same type.

I only template the RtpFecTest to be able to run it over both ULPFEC and
FlexFEC. ForwardErrorCorrection will not be templated.

[danilchap, 2016-08-23 14:51:19]

lgtm % nit

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right):

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:37: using RawRtpPacket =
test::fec::RawRtpPacket;
On 2016/08/23 14:05:06, brandtr wrote:
> On 2016/08/23 13:08:49, danilchap wrote:
> > test::fec is a namespace, so you can write
> > using ::webrtc::test::fec::RawRtpPacket;
> > ("using test::fec::RawRtpPacket;" would work too, but full name is prefered)
> 
> Done.

Not exactly, I meant 
using ::webrtc::test::fec::RawRtpPacket;
not
using RawRtpPacket = ::webrtc::test::fec::RawRtpPacket;

(can't do that with Packet because ForwardErrorCorrection is a class, but fec is
a namespace)
Same next line and reorder in alphabetical order.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/fec_test_helper.h (right):

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_test_helper.h:22: // needed to not clash with
another webrtc::FrameGenerator.
On 2016/08/23 14:05:07, brandtr wrote:
> On 2016/08/23 13:08:50, danilchap wrote:
> > Uppercase 1st letter: 'Needed'.
> > Though probably better to remove this comment from code.
> > Or fix it by renaming FrameGenerator to something fec specific.
> 
> See
>
https://codereview.webrtc.org/2276473002/diff/1/webrtc/modules/rtp_rtcp/sourc....

Ok, Uppercase 1st letter then for now.

https://codereview.webrtc.org/2267393002/diff/100001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc (right):

https://codereview.webrtc.org/2267393002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc:29: using
::webrtc::test::fec::FrameGenerator;
switch with previous line for alphabetical order

[brandtr, 2016-08-23 15:08:01]

Thanks for quick feedback!

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right):

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:37: using RawRtpPacket =
test::fec::RawRtpPacket;
On 2016/08/23 14:51:18, danilchap wrote:
> On 2016/08/23 14:05:06, brandtr wrote:
> > On 2016/08/23 13:08:49, danilchap wrote:
> > > test::fec is a namespace, so you can write
> > > using ::webrtc::test::fec::RawRtpPacket;
> > > ("using test::fec::RawRtpPacket;" would work too, but full name is
prefered)
> > 
> > Done.
> 
> Not exactly, I meant 
> using ::webrtc::test::fec::RawRtpPacket;
> not
> using RawRtpPacket = ::webrtc::test::fec::RawRtpPacket;
> 
> (can't do that with Packet because ForwardErrorCorrection is a class, but fec
is
> a namespace)
> Same next line and reorder in alphabetical order.

Got it, thanks.

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/fec_test_helper.h (right):

https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_test_helper.h:22: // needed to not clash with
another webrtc::FrameGenerator.
On 2016/08/23 14:51:18, danilchap wrote:
> On 2016/08/23 14:05:07, brandtr wrote:
> > On 2016/08/23 13:08:50, danilchap wrote:
> > > Uppercase 1st letter: 'Needed'.
> > > Though probably better to remove this comment from code.
> > > Or fix it by renaming FrameGenerator to something fec specific.
> > 
> > See
> >
>
https://codereview.webrtc.org/2276473002/diff/1/webrtc/modules/rtp_rtcp/sourc....
> 
> Ok, Uppercase 1st letter then for now.

Done.

https://codereview.webrtc.org/2267393002/diff/100001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc (right):

https://codereview.webrtc.org/2267393002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc:29: using
::webrtc::test::fec::FrameGenerator;
On 2016/08/23 14:51:19, danilchap wrote:
> switch with previous line for alphabetical order

Done.

[brandtr, 2016-09-02 08:25:41]

Rebase.

[brandtr, 2016-09-06 14:35:20]

Rebase.

[stefan-webrtc, 2016-09-13 07:39:47]

lgtm

https://codereview.webrtc.org/2267393002/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/fec_test_helper.cc (right):

https://codereview.webrtc.org/2267393002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/fec_test_helper.cc:109:
MediaPacketGenerator::ConstructMediaPacketsSeqNum(int num_media_packets,
I would probably have ommitted the SeqNum part of this method since it confuses
me more than adds to readability.

https://codereview.webrtc.org/2267393002/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/fec_test_helper.h (right):

https://codereview.webrtc.org/2267393002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/fec_test_helper.h:71:
ForwardErrorCorrection::PacketList ConstructMediaPacketsSeqNum(
Does it have to be public?

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

https://codereview.webrtc.org/2267393002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:97: using FecTypes =
Types<ForwardErrorCorrection>;
I would probably have made these changes with the CL that adds the second type
to avoid introducing complexity before it's needed. No need to change though. :)

[stefan-webrtc, 2016-09-13 07:39:50]

lgtm

[stefan-webrtc, 2016-09-13 07:40:06]

Sorry, clicked the wrong button. not lgtm for now :)

[brandtr, 2016-09-13 09:27:14]

https://codereview.webrtc.org/2267393002/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/fec_test_helper.cc (right):

https://codereview.webrtc.org/2267393002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/fec_test_helper.cc:109:
MediaPacketGenerator::ConstructMediaPacketsSeqNum(int num_media_packets,
On 2016/09/13 07:39:47, stefan-webrtc (holmer) wrote:
> I would probably have ommitted the SeqNum part of this method since it
confuses
> me more than adds to readability.

Done.

https://codereview.webrtc.org/2267393002/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/fec_test_helper.h (right):

https://codereview.webrtc.org/2267393002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/fec_test_helper.h:71:
ForwardErrorCorrection::PacketList ConstructMediaPacketsSeqNum(
On 2016/09/13 07:39:47, stefan-webrtc (holmer) wrote:
> Does it have to be public?

Yep, see e.g.
https://codereview.webrtc.org/2267393002/diff/180001/webrtc/modules/rtp_rtcp/...

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

https://codereview.webrtc.org/2267393002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:97: using FecTypes =
Types<ForwardErrorCorrection>;
On 2016/09/13 07:39:47, stefan-webrtc (holmer) wrote:
> I would probably have made these changes with the CL that adds the second type
> to avoid introducing complexity before it's needed. No need to change though.
:)

Yes, that's a good point. This is an artifact from the fact I split up one CL
into five, and the resulting granularity is not perfect.

[brandtr, 2016-09-20 09:16:51]

ping holmer.

[brandtr, 2016-09-20 09:23:50]

Rebase after gyp deprecation.

[stefan-webrtc, 2016-09-20 09:39:59]

lgtm

[brandtr, 2016-09-20 10:50:52]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2016-09-20 10:50:52]

The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2267393002/#ps220001 (title:
"Rebase after gyp deprecation.")

[commit-bot: I haz the power, 2016-09-20 10:51:00]

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-20 12:51:35]

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

[commit-bot: I haz the power, 2016-09-20 12:51:36]

Exceeded global retry quota

[brandtr, 2016-09-20 13:41:18]

The CQ bit was checked by brandtr@webrtc.org

[commit-bot: I haz the power, 2016-09-20 13:41:35]

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-20 13:59:44]

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

[commit-bot: I haz the power, 2016-09-20 13:59:46]

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

[brandtr, 2016-09-21 06:07:18]

The CQ bit was checked by brandtr@webrtc.org

[commit-bot: I haz the power, 2016-09-21 06:07:24]

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-21 06:16:31]

Description was changed from

==========
Generalize FEC unit tests and rename GenerateFec.

- Rename GenerateFec -> EncodeFec in ForwardErrorCorrection. This naming
  is more consistent with DecodeFec.
- Add appropriate using directives, to reduce clutter in tests.
- Move ConstructMediaPackets to fec_test_helper.{h,cc}. This will help
  future tests of ULPFEC/FlexFEC header formatters.
- Generalize tests in rtp_fec_unittest.cc to typed tests. This will help
  testing ForwardErrorCorrection with both ULPFEC and FlexFEC.

This CL should not impact functionality or performance.

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

to

==========
Generalize FEC unit tests and rename GenerateFec.

- Rename GenerateFec -> EncodeFec in ForwardErrorCorrection. This naming
  is more consistent with DecodeFec.
- Add appropriate using directives, to reduce clutter in tests.
- Move ConstructMediaPackets to fec_test_helper.{h,cc}. This will help
  future tests of ULPFEC/FlexFEC header formatters.
- Generalize tests in rtp_fec_unittest.cc to typed tests. This will help
  testing ForwardErrorCorrection with both ULPFEC and FlexFEC.

This CL should not impact functionality or performance.

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

[commit-bot: I haz the power, 2016-09-21 06:16:32]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2016-09-21 06:16:41]

Description was changed from

==========
Generalize FEC unit tests and rename GenerateFec.

- Rename GenerateFec -> EncodeFec in ForwardErrorCorrection. This naming
  is more consistent with DecodeFec.
- Add appropriate using directives, to reduce clutter in tests.
- Move ConstructMediaPackets to fec_test_helper.{h,cc}. This will help
  future tests of ULPFEC/FlexFEC header formatters.
- Generalize tests in rtp_fec_unittest.cc to typed tests. This will help
  testing ForwardErrorCorrection with both ULPFEC and FlexFEC.

This CL should not impact functionality or performance.

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

to

==========
Generalize FEC unit tests and rename GenerateFec.

- Rename GenerateFec -> EncodeFec in ForwardErrorCorrection. This naming
  is more consistent with DecodeFec.
- Add appropriate using directives, to reduce clutter in tests.
- Move ConstructMediaPackets to fec_test_helper.{h,cc}. This will help
  future tests of ULPFEC/FlexFEC header formatters.
- Generalize tests in rtp_fec_unittest.cc to typed tests. This will help
  testing ForwardErrorCorrection with both ULPFEC and FlexFEC.

This CL should not impact functionality or performance.

BUG=webrtc:5654

Committed: https://crrev.com/ece4aba64ed873c54ecbcb7ba92bf6ed2fcb9d98
Cr-Commit-Position: refs/heads/master@{#14314}
==========

[commit-bot: I haz the power, 2016-09-21 06:16:43]

Patchset 10 (id:??) landed as
https://crrev.com/ece4aba64ed873c54ecbcb7ba92bf6ed2fcb9d98
Cr-Commit-Position: refs/heads/master@{#14314}