Always send RED headers if configured.

Always send RED headers if configured.

This shouldn't be needed, but because the receiver assumes RTX packets
contain RED if configured to receive them (due to an incompatibility
issue), we also have to make sure we send them for now.

BUG=webrtc:5675
Committed: https://crrev.com/8f4c77fea10010b5879a01bf6fcd485318594a85
Cr-Commit-Position: refs/heads/master@{#13024}

[pbos-webrtc, 2016-06-02 14:51:29]

pbos@webrtc.org changed reviewers:
+ pbos@webrtc.org

[pbos-webrtc, 2016-06-02 14:51:29]

lgtm with tests updated

[pbos-webrtc, 2016-06-02 14:53:23]

Description was changed from

==========
Always send red headers if configured.

This shouldn't be needed, but because the receiver assumes rtx packets
contain red if configured to receive them (due to an incompatibility
issue), we also have to make sure we send them for now.

BUG=webrtc:5675
==========

to

==========
Always send RED headers if configured.

This shouldn't be needed, but because the receiver assumes rtx packets
contain red if configured to receive them (due to an incompatibility
issue), we also have to make sure we send them for now.

BUG=webrtc:5675
==========

[pbos-webrtc, 2016-06-02 14:53:37]

Description was changed from

==========
Always send RED headers if configured.

This shouldn't be needed, but because the receiver assumes rtx packets
contain red if configured to receive them (due to an incompatibility
issue), we also have to make sure we send them for now.

BUG=webrtc:5675
==========

to

==========
Always send RED headers if configured.

This shouldn't be needed, but because the receiver assumes RTX packets
contain RED if configured to receive them (due to an incompatibility
issue), we also have to make sure we send them for now.

BUG=webrtc:5675
==========

[stefan-webrtc, 2016-06-02 14:56:04]

Add test.

[stefan-webrtc, 2016-06-02 15:37:02]

PTAL

[stefan-webrtc, 2016-06-02 15:37:07]

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

[commit-bot: I haz the power, 2016-06-02 15:37:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2033763002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2033763002/40001

[pbos-webrtc, 2016-06-02 15:52:52]

lgtm, fingers crossed on sufficient test coverage

[commit-bot: I haz the power, 2016-06-02 16:37:33]

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

[commit-bot: I haz the power, 2016-06-02 16:37:34]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2016-06-03 07:14:57]

Verified with apprtc demo, landing.

[stefan-webrtc, 2016-06-03 07:15:02]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2016-06-03 07:15:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2033763002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2033763002/40001

[commit-bot: I haz the power, 2016-06-03 07:16:51]

Description was changed from

==========
Always send RED headers if configured.

This shouldn't be needed, but because the receiver assumes RTX packets
contain RED if configured to receive them (due to an incompatibility
issue), we also have to make sure we send them for now.

BUG=webrtc:5675
==========

to

==========
Always send RED headers if configured.

This shouldn't be needed, but because the receiver assumes RTX packets
contain RED if configured to receive them (due to an incompatibility
issue), we also have to make sure we send them for now.

BUG=webrtc:5675
==========

[commit-bot: I haz the power, 2016-06-03 07:16:52]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-03 07:17:03]

Description was changed from

==========
Always send RED headers if configured.

This shouldn't be needed, but because the receiver assumes RTX packets
contain RED if configured to receive them (due to an incompatibility
issue), we also have to make sure we send them for now.

BUG=webrtc:5675
==========

to

==========
Always send RED headers if configured.

This shouldn't be needed, but because the receiver assumes RTX packets
contain RED if configured to receive them (due to an incompatibility
issue), we also have to make sure we send them for now.

BUG=webrtc:5675

Committed: https://crrev.com/8f4c77fea10010b5879a01bf6fcd485318594a85
Cr-Commit-Position: refs/heads/master@{#13024}
==========

[commit-bot: I haz the power, 2016-06-03 07:17:04]

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

[mflodman, 2016-06-03 07:28:15]

mflodman@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[mflodman, 2016-06-03 07:28:17]

After the fact comments, LGTM with those addressed as a follow up.

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

https://codereview.webrtc.org/2033763002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:173: fec_enabled_ = enable;
I think we should DCHECK the pl types != 0 here, or is that done elsewhere?
Since 0 actually is a valid number I think that might slip though higher layers.

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

https://codereview.webrtc.org/2033763002/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:684:
RTC_DCHECK_GE(config_.rtp.fec.red_payload_type, 0);
Or change this to not allow 0.

https://codereview.webrtc.org/2033763002/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:694:
RTC_DCHECK_GE(config_.rtp.fec.ulpfec_payload_type, 0);
And here.

[stefan-webrtc, 2016-06-03 07:50:19]

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

https://codereview.webrtc.org/2033763002/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:684:
RTC_DCHECK_GE(config_.rtp.fec.red_payload_type, 0);
On 2016/06/03 07:28:16, mflodman wrote:
> Or change this to not allow 0.

Will do! https://codereview.webrtc.org/2036083002