Wire up FlexfecSender in RTPSender and add unit tests.

Wire up FlexfecSender in RTPSender and add unit tests.

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

[brandtr, 2016-11-08 09:42:27]

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

[brandtr, 2016-11-08 09:42:27]

This one is related too!

[danilchap, 2016-11-08 14:52:50]

https://codereview.webrtc.org/2484143002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2484143002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:933: // TODO(brandtr):
Update this code when the FlexFEC packets are no longer
does it mean flexfec packets will not be sent with pacer present?
May fix that before wiring flexfex to RtpRtcpModule.

https://codereview.webrtc.org/2484143002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1468:
TEST_F(RtpSenderVideoTest, SendFlexfecPackets) {
this test is likely not needed: the use case already covered by new tests above.

[brandtr, 2016-11-09 09:25:47]

Description was changed from

==========
Wire up FlexfecSender in RTPSender and RTP module.

This CL also adds unit tests on the RTPSender/RTPSenderVideo
level.

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

to

==========
Wire up FlexfecSender in RTPSender and add unit tests.

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

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

Patchset #2 (id:20001) has been deleted

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

Rebase.

[brandtr, 2016-11-09 09:54:23]

Patchset #4 (id:80001) has been deleted

[brandtr, 2016-11-09 09:59:34]

Patchset #3 (id:60001) has been deleted

[brandtr, 2016-11-09 10:17:08]

Patchset #3 (id:100001) has been deleted

[brandtr, 2016-11-09 10:40:05]

https://codereview.webrtc.org/2484143002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2484143002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:933: // TODO(brandtr):
Update this code when the FlexFEC packets are no longer
On 2016/11/08 14:52:49, danilchap wrote:
> does it mean flexfec packets will not be sent with pacer present?
> May fix that before wiring flexfex to RtpRtcpModule.

Yes :( My next CL will address this issue, using the hacky way that we talked
about ~a week ago.

Removing the wireup with RTP module.

https://codereview.webrtc.org/2484143002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1468:
TEST_F(RtpSenderVideoTest, SendFlexfecPackets) {
On 2016/11/08 14:52:49, danilchap wrote:
> this test is likely not needed: the use case already covered by new tests
above.

Removed!

[danilchap, 2016-11-09 11:41:44]

https://codereview.webrtc.org/2484143002/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2484143002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:935: constexpr int
kNumPacketsExpected = 1;
rather than test for incorrect behavior, make tests less strict (but leave TODO
to make checks stricter):
EXPECT_CALL(...).Times(AtLeast(1));
...
EXPECT_LE(1, transport_.packets_sent());

https://codereview.webrtc.org/2484143002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:974: }
check that one of those packets is flexfec and another is media (i.e. check
ssrcs and payloadtypes).
If you know media should always go before fec, it is easy.
if order doesn't matter, then still possible, but matcher might be complicated.

[brandtr, 2016-11-09 15:16:30]

https://codereview.webrtc.org/2484143002/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2484143002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:935: constexpr int
kNumPacketsExpected = 1;
On 2016/11/09 11:41:44, danilchap wrote:
> rather than test for incorrect behavior, make tests less strict (but leave
TODO
> to make checks stricter):
> EXPECT_CALL(...).Times(AtLeast(1));
> ...
> EXPECT_LE(1, transport_.packets_sent());

Good idea. Done.

https://codereview.webrtc.org/2484143002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:974: }
On 2016/11/09 11:41:44, danilchap wrote:
> check that one of those packets is flexfec and another is media (i.e. check
> ssrcs and payloadtypes).
> If you know media should always go before fec, it is easy.
> if order doesn't matter, then still possible, but matcher might be
complicated.

Yes, media will always come out before FEC. Adding.

[danilchap, 2016-11-09 16:27:42]

lgtm
polishing tests is a different topic

https://codereview.webrtc.org/2484143002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2484143002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:932:
.WillOnce(testing::SaveArg<2>(&media_seq_num));
shouldn't media_seq_num == kSeqNum? becuase of line
rtp_sender_->SetSequenceNumber(kSeqNum)
you can adjust EXPECT_CALL then (kSeqNum instead of 2nd _)

https://codereview.webrtc.org/2484143002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:980: EXPECT_EQ(2,
transport_.packets_sent());
EXPECT_EQ -> ASSERT_EQ (to avoid crash if test will fail sending less packets)

[brandtr, 2016-11-10 08:34:41]

https://codereview.webrtc.org/2484143002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2484143002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:932:
.WillOnce(testing::SaveArg<2>(&media_seq_num));
On 2016/11/09 16:27:42, danilchap wrote:
> shouldn't media_seq_num == kSeqNum? becuase of line
> rtp_sender_->SetSequenceNumber(kSeqNum)
> you can adjust EXPECT_CALL then (kSeqNum instead of 2nd _)

Yep, that is the case. But in the followup CL, where I fix the pacer problem,
this way looks more symmetric :)

I will reconsider when I upload that CL.

https://codereview.webrtc.org/2484143002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:980: EXPECT_EQ(2,
transport_.packets_sent());
On 2016/11/09 16:27:42, danilchap wrote:
> EXPECT_EQ -> ASSERT_EQ (to avoid crash if test will fail sending less packets)

Done.

[brandtr, 2016-11-10 09:04:18]

Rebase.

[brandtr, 2016-11-10 09:07:56]

Patchset #6 (id:180001) has been deleted

[brandtr, 2016-11-10 09:30:15]

Rebase.

[brandtr, 2016-11-10 12:02:09]

Rebase.

[brandtr, 2016-11-10 12:02:26]

The CQ bit was checked by brandtr@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 12:02:38]

Dry run: 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-11-10 12:55:27]

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

[commit-bot: I haz the power, 2016-11-10 12:55:28]

Dry run: This issue passed the CQ dry run.

[brandtr, 2016-11-10 13:03:08]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2016-11-10 13:03:09]

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/2484143002/#ps220001 (title:
"Rebase.")

[commit-bot: I haz the power, 2016-11-10 13:03:13]

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-11-10 13:04:50]

Description was changed from

==========
Wire up FlexfecSender in RTPSender and add unit tests.

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

to

==========
Wire up FlexfecSender in RTPSender and add unit tests.

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

[commit-bot: I haz the power, 2016-11-10 13:04:51]

Committed patchset #7 (id:220001)

[commit-bot: I haz the power, 2016-11-10 13:04:58]

Description was changed from

==========
Wire up FlexfecSender in RTPSender and add unit tests.

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

to

==========
Wire up FlexfecSender in RTPSender and add unit tests.

BUG=webrtc:5654

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

[commit-bot: I haz the power, 2016-11-10 13:04:59]

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