Wire up FlexfecSender in RTP module and VideoSendStream.

Wire up FlexfecSender in RTP module and VideoSendStream.

FlexfecSender is owned and configured by VideoSendStream.

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

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

Description was changed from

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

VideoSendStream owns the FlexfecSender, and a pointer of it is propagated
to the RTP module.

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

to

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

FlexfecSender is owned and configured by VideoSendStream.

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

[brandtr, 2016-11-14 14:44:40]

Patchset #1 (id:1) has been deleted

[brandtr, 2016-11-14 15:01:56]

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

[brandtr, 2016-11-14 15:01:57]

Please have a look :)

https://codereview.webrtc.org/2501503003/diff/20001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2501503003/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:1014: // See http://crbug.com/webrtc/6650 for
more information.
holmer, can you verify that this thinking is correct?

[stefan-webrtc, 2016-11-14 15:28:40]

lg

https://codereview.webrtc.org/2501503003/diff/20001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2501503003/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:90: if
(config.rtp.flexfec.flexfec_payload_type == -1) {
Depending on answer to the other CL, this might be < 0

https://codereview.webrtc.org/2501503003/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:107: LOG(LS_WARNING) << "Both FlexFEC and
simulcast are enabled. This "
What would it have meant if it was supported? How is it different to the if
statement below covering multiple media streams?

https://codereview.webrtc.org/2501503003/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:1014: // See http://crbug.com/webrtc/6650 for
more information.
On 2016/11/14 15:01:57, brandtr wrote:
> holmer, can you verify that this thinking is correct?

I believe it is. :)

https://codereview.webrtc.org/2501503003/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:1016: LOG(LS_WARNING) << "Both FlexFEC and RED
are configured. Disabling RED.";
Should we warn, or should we simply have an info log? I can imagine that this
may be common if we don't filter it out in, e.g., webrtcvideoengine2.cc.

https://codereview.webrtc.org/2501503003/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:1021: << "Both FlexFEC and ULPFEC are
configured. Disabling ULPFEC.";
Same here.

[brandtr, 2016-11-15 09:19:08]

https://codereview.webrtc.org/2501503003/diff/20001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2501503003/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:90: if
(config.rtp.flexfec.flexfec_payload_type == -1) {
On 2016/11/14 15:28:39, stefan-webrtc (holmer) wrote:
> Depending on answer to the other CL, this might be < 0

Changed.

https://codereview.webrtc.org/2501503003/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:107: LOG(LS_WARNING) << "Both FlexFEC and
simulcast are enabled. This "
On 2016/11/14 15:28:39, stefan-webrtc (holmer) wrote:
> What would it have meant if it was supported? How is it different to the if
> statement below covering multiple media streams?

This is an unfortunate side-effect of the way that we have wired up the
FlexfecSender with RTPSenderVideo. There is only a single FlexfecSender, owned
by VideoSendStream, whose address is propagated down to the RTPSenderVideo. If
there are multiple RTPSenderVideo's, e.g. in the simulcast scenario, this leads
to a problem, since we can only protect a single media stream but the single
FlexfecSender may be fed RTP packets from multiple RTPSenderVideos.

My idea was to ensure that we don't enable FlexFEC for simulcast in the first
place, and then DCHECK in the FlexfecSender that we only receive media packets
from one media ssrc. Another approach would to allow for simulcast here, and
change the DCHECK to a runtime check.

The if statement below checks that the FlexfecSender is not configured to
protect multiple media streams, which is different from the problem described
above that has to do with the wire up.

https://codereview.webrtc.org/2501503003/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:1014: // See http://crbug.com/webrtc/6650 for
more information.
On 2016/11/14 15:28:40, stefan-webrtc (holmer) wrote:
> On 2016/11/14 15:01:57, brandtr wrote:
> > holmer, can you verify that this thinking is correct?
> 
> I believe it is. :)

Acknowledged.

https://codereview.webrtc.org/2501503003/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:1016: LOG(LS_WARNING) << "Both FlexFEC and RED
are configured. Disabling RED.";
On 2016/11/14 15:28:40, stefan-webrtc (holmer) wrote:
> Should we warn, or should we simply have an info log? I can imagine that this
> may be common if we don't filter it out in, e.g., webrtcvideoengine2.cc.

Yes, going with LS_INFO is more coherent with how the Video{Send,Receive}Stream
configs are logged. Changing to that.

https://codereview.webrtc.org/2501503003/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:1021: << "Both FlexFEC and ULPFEC are
configured. Disabling ULPFEC.";
On 2016/11/14 15:28:40, stefan-webrtc (holmer) wrote:
> Same here.

Done.

[stefan-webrtc, 2016-11-15 09:50:01]

lgtm

[terelius, 2016-11-15 09:58:09]

lgtm with a couple of questions

https://codereview.webrtc.org/2501503003/diff/40001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2501503003/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:107: LOG(LS_WARNING) << "Both FlexFEC and
simulcast are enabled. This "
Should this be LS_ERROR?

https://codereview.webrtc.org/2501503003/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:114: LOG(LS_WARNING)
Should this be LS_ERROR?

[brandtr, 2016-11-15 10:28:20]

https://codereview.webrtc.org/2501503003/diff/40001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2501503003/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:107: LOG(LS_WARNING) << "Both FlexFEC and
simulcast are enabled. This "
On 2016/11/15 09:58:09, terelius wrote:
> Should this be LS_ERROR?

Changed all to be WARNINGs, as per offline discussion.

[brandtr, 2016-11-15 10:28:36]

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

[commit-bot: I haz the power, 2016-11-15 10:28:37]

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-15 10:52:01]

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

[commit-bot: I haz the power, 2016-11-15 10:52:02]

Dry run: This issue passed the CQ dry run.

[brandtr, 2016-11-15 11:33:19]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2016-11-15 11:33:19]

The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2501503003/#ps60001 (title:
"LS_ERROR -> LS_WARNING.")

[commit-bot: I haz the power, 2016-11-15 11:33:37]

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-15 13:25:42]

Description was changed from

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

FlexfecSender is owned and configured by VideoSendStream.

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

to

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

FlexfecSender is owned and configured by VideoSendStream.

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

[commit-bot: I haz the power, 2016-11-15 13:25:43]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-11-15 13:25:51]

Description was changed from

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

FlexfecSender is owned and configured by VideoSendStream.

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

to

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

FlexfecSender is owned and configured by VideoSendStream.

BUG=webrtc:5654

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

[commit-bot: I haz the power, 2016-11-15 13:25:52]

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