Make FlexFEC packets paceable through RTPSender.

Make FlexFEC packets paceable through RTPSender.

Prior to this change, FlexFEC packets that were paced would be lost in
the RTPSender, since they were not stored in a packet history. This CL
introduces such a packet history, as well as the needed wireup for
higher layers to be aware that the particular RTPSender is able to
send FlexFEC packets with a particular SSRC.

Updated RTPSender unit test to reflect the fact that paced packets
are now actually sent.

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

[brandtr, 2016-11-10 13:34:13]

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

[brandtr, 2016-11-10 13:34:14]

Final piece of the RTP puzzle!

All of this stuff can be removed whenever we move the FlexFEC generation to the
pacer.

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

https://codereview.webrtc.org/2491293002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:44: constexpr size_t
kFlexfecPacketsToStoreBeforePacing = 50;
Is this a reasonable number? We store up to 600 media packets in the main packet
history data structure:
https://cs.chromium.org/chromium/src/third_party/webrtc/video/video_send_stre...,
but the difference is that we will never retransmit a NACKed FlexFEC packet.

https://codereview.webrtc.org/2491293002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:908:
packet_history_.PutRtpPacket(std::move(packet), storage, true);
No need to store FlexFEC packets here, since at this time they have already been
sent to the transport.

[danilchap, 2016-11-10 16:15:13]

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

https://codereview.webrtc.org/2491293002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:908:
packet_history_.PutRtpPacket(std::move(packet), storage, true);
On 2016/11/10 13:34:14, brandtr wrote:
> No need to store FlexFEC packets here, since at this time they have already
been
> sent to the transport.

still need to check ssrc, right now you would store flexfex packets in regular
packet_history.

you do not need to store packets (both regular and flexfec) if storage ==
kDontRetransmit though.

https://codereview.webrtc.org/2491293002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2491293002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:55: if (ssrc == media_ssrc ||
(flexfec_ssrc && ssrc == flexfec_ssrc)) {
no need to check if optional is not nullopt before comparing it with value.

https://codereview.webrtc.org/2491293002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2491293002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:708: } else {
may be check here ssrc is the media ssrc
then that check can be removed from the RtpRtcpImpl

https://codereview.webrtc.org/2491293002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1119: return
rtc::Optional<uint32_t>();  // No value.
not sure if comment is helpful.

https://codereview.webrtc.org/2491293002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2491293002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:498: uint32_t ssrc = 0;
may be rather than complicating test with extracting ssrc and seq_num, set them:
const uint32_t kSsrc = 12354;
const uint16_t kSeqNum = 4321;
rtp_sender_->SetSSRC(kSsrc);
rtp_sender_->SetSequenceNumber(kSeqNum);

[brandtr, 2016-11-11 08:44:46]

Patchset #3 (id:40001) has been deleted

[brandtr, 2016-11-11 08:57:07]

Patchset #3 (id:60001) has been deleted

[brandtr, 2016-11-11 10:28:16]

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

https://codereview.webrtc.org/2491293002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:908:
packet_history_.PutRtpPacket(std::move(packet), storage, true);
On 2016/11/10 16:15:13, danilchap wrote:
> On 2016/11/10 13:34:14, brandtr wrote:
> > No need to store FlexFEC packets here, since at this time they have already
> been
> > sent to the transport.
> 
> still need to check ssrc, right now you would store flexfex packets in regular
> packet_history.
> 
> you do not need to store packets (both regular and flexfec) if storage ==
> kDontRetransmit though.

Good point, you are right.

Right now, I have not built support for retransmitting FlexFEC packets. I think
there _is_ support for this for ULPFEC, but we just never activate it.

https://codereview.webrtc.org/2491293002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2491293002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:51: if (!rtp_module->SendingMedia())
Centralizing this check to the RTP module.

https://codereview.webrtc.org/2491293002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:55: if (ssrc == media_ssrc ||
(flexfec_ssrc && ssrc == flexfec_ssrc)) {
On 2016/11/10 16:15:13, danilchap wrote:
> no need to check if optional is not nullopt before comparing it with value.

Ah, ok. I didn't understand the declarations in base/optional.h properly.

https://codereview.webrtc.org/2491293002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2491293002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:708: } else {
On 2016/11/10 16:15:13, danilchap wrote:
> may be check here ssrc is the media ssrc
> then that check can be removed from the RtpRtcpImpl

Done.

https://codereview.webrtc.org/2491293002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1119: return
rtc::Optional<uint32_t>();  // No value.
On 2016/11/10 16:15:13, danilchap wrote:
> not sure if comment is helpful.

Removed.

https://codereview.webrtc.org/2491293002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2491293002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:498: uint32_t ssrc = 0;
On 2016/11/10 16:15:13, danilchap wrote:
> may be rather than complicating test with extracting ssrc and seq_num, set
them:
> const uint32_t kSsrc = 12354;
> const uint16_t kSeqNum = 4321;
> rtp_sender_->SetSSRC(kSsrc);
> rtp_sender_->SetSequenceNumber(kSeqNum);
> 
> 

Done. Also made some EXPECT_CALLs a bit more specific.

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:51: if (ssrc == rtp_module->SSRC() ||
ssrc == rtp_module->FlexfecSsrc()) {
Moved this check to RTP module itself.

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:407: if (!SendingMedia()) {
Moved SSRC check to RTPSender.

[danilchap, 2016-11-11 15:26:06]

lgtm

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

https://codereview.webrtc.org/2491293002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:44: constexpr size_t
kFlexfecPacketsToStoreBeforePacing = 50;
On 2016/11/10 13:34:14, brandtr wrote:
> Is this a reasonable number? We store up to 600 media packets in the main
packet
> history data structure:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/video/video_send_stre...,
> but the difference is that we will never retransmit a NACKed FlexFEC packet.

this is initial size of the packet history, buffer will grow if needed.
max capacity (9000 packets) is unchangeable.
i.e the actual value is not that important.

may be add word 'Min' to variable name (similar to the constant you link to)

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:407: if (!SendingMedia()) {
On 2016/11/11 10:28:16, brandtr wrote:
> Moved SSRC check to RTPSender.

yep, it nicer that way.
may be move !SendingMedia check too.

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:711: // Incorrect SSRC. Allow
sending to continue.
because of if (!packet) return true; this block can be omitted,
but if you want to keep it, better RTC_NOTREACHABLE since checks this is right
module should be done outside.

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:911: if (ssrc == flexfec_ssrc)
comment hint that instead you should check:
if (storage != kDontRetransmit) {
  RTC_DCHECK_EQ(ssrc, Ssrc());
  packet_histry_.Put();
}

return sent;

[stefan-webrtc, 2016-11-13 13:38:18]

lgtm

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right):

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/flexfec_sender.h:41: uint32_t Ssrc() const {
return ssrc_; }
Perhaps ssrc() to make it clear that it's a simple getter?

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h (right):

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h:19: #include
"webrtc/base/optional.h"
Sort

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:716: // Packet cannot be found.
Should we log a warning?

[brandtr, 2016-11-14 08:41:17]

Patchset #5 (id:120001) has been deleted

[brandtr, 2016-11-14 09:48:09]

Thanks for feedback. See answers, and question to danilchap.

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

https://codereview.webrtc.org/2491293002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:44: constexpr size_t
kFlexfecPacketsToStoreBeforePacing = 50;
On 2016/11/11 15:26:06, danilchap wrote:
> On 2016/11/10 13:34:14, brandtr wrote:
> > Is this a reasonable number? We store up to 600 media packets in the main
> packet
> > history data structure:
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/video/video_send_stre...,
> > but the difference is that we will never retransmit a NACKed FlexFEC packet.
> 
> this is initial size of the packet history, buffer will grow if needed.
> max capacity (9000 packets) is unchangeable.
> i.e the actual value is not that important.
> 
> may be add word 'Min' to variable name (similar to the constant you link to)

Done.

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right):

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/flexfec_sender.h:41: uint32_t Ssrc() const {
return ssrc_; }
On 2016/11/13 13:38:18, stefan-webrtc (holmer) wrote:
> Perhaps ssrc() to make it clear that it's a simple getter?

Done.

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:407: if (!SendingMedia()) {
On 2016/11/11 15:26:06, danilchap wrote:
> On 2016/11/11 10:28:16, brandtr wrote:
> > Moved SSRC check to RTPSender.
> 
> yep, it nicer that way.
> may be move !SendingMedia check too.

Done.

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h (right):

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h:19: #include
"webrtc/base/optional.h"
On 2016/11/13 13:38:18, stefan-webrtc (holmer) wrote:
> Sort

Done.

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:711: // Incorrect SSRC. Allow
sending to continue.
On 2016/11/11 15:26:06, danilchap wrote:
> because of if (!packet) return true; this block can be omitted,
> but if you want to keep it, better RTC_NOTREACHABLE since checks this is right
> module should be done outside.

Right. Removing block.

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:716: // Packet cannot be found.
On 2016/11/13 13:38:18, stefan-webrtc (holmer) wrote:
> Should we log a warning?

That's already done within the packet history itself:
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour...

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:911: if (ssrc == flexfec_ssrc)
On 2016/11/11 15:26:06, danilchap wrote:
> comment hint that instead you should check:
> if (storage != kDontRetransmit) {
>   RTC_DCHECK_EQ(ssrc, Ssrc());
>   packet_histry_.Put();
> }
> 
> return sent;

This would change how we store the packets in the packet history though. Right
now, we insert the packets even if storage == kDontRetransmit. But then we never
return them from the packet history...
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour...
I'm not familiar enough with that data structure to know if this is reasonable
or not.

Just wanted to confirm that this is an intentional idea in your suggestion?

[danilchap, 2016-11-14 11:20:33]

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2491293002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:911: if (ssrc == flexfec_ssrc)
On 2016/11/14 09:48:09, brandtr wrote:
> On 2016/11/11 15:26:06, danilchap wrote:
> > comment hint that instead you should check:
> > if (storage != kDontRetransmit) {
> >   RTC_DCHECK_EQ(ssrc, Ssrc());
> >   packet_histry_.Put();
> > }
> > 
> > return sent;
> 
> This would change how we store the packets in the packet history though. Right
> now, we insert the packets even if storage == kDontRetransmit. But then we
never
> return them from the packet history...
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour...
> I'm not familiar enough with that data structure to know if this is reasonable
> or not.
> 
> Just wanted to confirm that this is an intentional idea in your suggestion?

I think it is reasonable (no point to store something you would never retrieve),
but on second though - doesn't belong to this CL, so just DCHECK(storage ==
kNoRetransmit) to be safer.

On third though - it is not even needed, if sender is configured in no pacer, no
nack mode, then history should be disabled altogether.

(Current behavior is not nice left over from my refactoring - before we stored
the packet before checking if pacer is enabled or not. I've split these two
cases, but didn't notice that in second case no need to store the packet all the
time).

[brandtr, 2016-11-14 11:34:23]

Rebase.

[brandtr, 2016-11-14 12:18:17]

Patchset #7 (id:180001) has been deleted

[brandtr, 2016-11-14 12:34:44]

Patchset #7 (id:200001) has been deleted

[brandtr, 2016-11-14 12:36:20]

https://codereview.webrtc.org/2491293002/diff/220001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2491293002/diff/220001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:911: if (storage ==
kAllowRetransmission) {
I thought it was nice to check the complement (i.e. == kAllowRetransmission, not
!= kDontRetransmit) here.

[brandtr, 2016-11-14 12:36:28]

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

[commit-bot: I haz the power, 2016-11-14 12:36: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-14 13:04:44]

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

[commit-bot: I haz the power, 2016-11-14 13:04:46]

Dry run: This issue passed the CQ dry run.

[brandtr, 2016-11-14 13:13:10]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2016-11-14 13:13:11]

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/2491293002/#ps220001 (title:
"Feedback response 3.")

[commit-bot: I haz the power, 2016-11-14 13:13:16]

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-14 13:14:53]

Committed patchset #7 (id:220001)

[commit-bot: I haz the power, 2016-11-14 13:14:56]

Description was changed from

==========
Make FlexFEC packets paceable through RTPSender.

Prior to this change, FlexFEC packets that were paced would be lost in
the RTPSender, since they were not stored in a packet history. This CL
introduces such a packet history, as well as the needed wireup for
higher layers to be aware that the particular RTPSender is able to
send FlexFEC packets with a particular SSRC.

Updated RTPSender unit test to reflect the fact that paced packets
are now actually sent.

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

to

==========
Make FlexFEC packets paceable through RTPSender.

Prior to this change, FlexFEC packets that were paced would be lost in
the RTPSender, since they were not stored in a packet history. This CL
introduces such a packet history, as well as the needed wireup for
higher layers to be aware that the particular RTPSender is able to
send FlexFEC packets with a particular SSRC.

Updated RTPSender unit test to reflect the fact that paced packets
are now actually sent.

BUG=webrtc:5654

Committed: https://crrev.com/9dfff29bc4c8422a7e18827e325fb98fab80405b
Cr-Commit-Position: refs/heads/master@{#15066}
==========

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

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