Delete member VideoReceiveStream::Config::Rtp::ulpfec.

Delete member VideoReceiveStream::Config::Rtp::ulpfec.

Replaced with scalars ulpfec_payload_type and red_payload_type.

In particular, ulpfec.red_rtx_payload_type, which duplicated info in
rtx_associated_payload_types, is deleted. This is a followup to cl
https://codereview.webrtc.org/3012963002.

BUG=webrtc:7135
Review-Url: https://codereview.webrtc.org/3019453002
Cr-Commit-Position: refs/heads/master@{#19965}
Committed: https://webrtc.googlesource.com/src/+/3b3622fafc6d1fd910074f7bab78c8427d80649e

[nisse-webrtc, 2017-09-21 07:10:24]

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

[brandtr, 2017-09-21 08:11:33]

Good change, lgtm.

https://codereview.webrtc.org/3019453002/diff/20001/media/engine/webrtcvideoe...
File media/engine/webrtcvideoengine.cc (right):

https://codereview.webrtc.org/3019453002/diff/20001/media/engine/webrtcvideoe...
media/engine/webrtcvideoengine.cc:2267:
recv_codecs.front().ulpfec.ulpfec_payload_type;
Maybe nicer to keep a const ref to |recv_codecs.front()|, as you use it four
times?

https://codereview.webrtc.org/3019453002/diff/20001/video/end_to_end_tests.cc
File video/end_to_end_tests.cc (left):

https://codereview.webrtc.org/3019453002/diff/20001/video/end_to_end_tests.cc...
video/end_to_end_tests.cc:856: send_config->rtp.ulpfec.red_rtx_payload_type =
RED should not be used in this test, so you can remove this line too?

https://codereview.webrtc.org/3019453002/diff/20001/video/end_to_end_tests.cc...
video/end_to_end_tests.cc:2703: bool use_red,
Would you mind renaming |use_red| to |use_fec| ? Because that's how the flag is
actually used.

https://codereview.webrtc.org/3019453002/diff/20001/video/end_to_end_tests.cc...
video/end_to_end_tests.cc:2756: void ModifyVideoConfigs(
It doesn't look like this test sets ups RED/RTX correctly, when both are used.
Can you add the correct configs?

[nisse-webrtc, 2017-09-21 08:37:46]

https://codereview.webrtc.org/3019453002/diff/20001/media/engine/webrtcvideoe...
File media/engine/webrtcvideoengine.cc (right):

https://codereview.webrtc.org/3019453002/diff/20001/media/engine/webrtcvideoe...
media/engine/webrtcvideoengine.cc:2267:
recv_codecs.front().ulpfec.ulpfec_payload_type;
On 2017/09/21 08:11:33, brandtr wrote:
> Maybe nicer to keep a const ref to |recv_codecs.front()|, as you use it four
> times?

Done.

https://codereview.webrtc.org/3019453002/diff/20001/media/engine/webrtcvideoe...
media/engine/webrtcvideoengine.cc:2271: HasNack(recv_codecs.begin()->codec) ?
kNackHistoryMs : 0;
And here for some reason |begin()->| is used instead of |front().|, but that
should be the same, right?

https://codereview.webrtc.org/3019453002/diff/20001/video/end_to_end_tests.cc
File video/end_to_end_tests.cc (left):

https://codereview.webrtc.org/3019453002/diff/20001/video/end_to_end_tests.cc...
video/end_to_end_tests.cc:856: send_config->rtp.ulpfec.red_rtx_payload_type =
On 2017/09/21 08:11:33, brandtr wrote:
> RED should not be used in this test, so you can remove this line too?

Done.

https://codereview.webrtc.org/3019453002/diff/20001/video/end_to_end_tests.cc...
video/end_to_end_tests.cc:2703: bool use_red,
On 2017/09/21 08:11:33, brandtr wrote:
> Would you mind renaming |use_red| to |use_fec| ? Because that's how the flag
is
> actually used.

Just in this method and its local classes, or globally in this file?

https://codereview.webrtc.org/3019453002/diff/20001/video/end_to_end_tests.cc...
video/end_to_end_tests.cc:2756: void ModifyVideoConfigs(
On 2017/09/21 08:11:33, brandtr wrote:
> It doesn't look like this test sets ups RED/RTX correctly, when both are used.
> Can you add the correct configs?

I've given it a try, please have a look.

[brandtr, 2017-09-21 08:45:23]

Thanks, this lgtm.

https://codereview.webrtc.org/3019453002/diff/20001/media/engine/webrtcvideoe...
File media/engine/webrtcvideoengine.cc (right):

https://codereview.webrtc.org/3019453002/diff/20001/media/engine/webrtcvideoe...
media/engine/webrtcvideoengine.cc:2271: HasNack(recv_codecs.begin()->codec) ?
kNackHistoryMs : 0;
On 2017/09/21 08:37:46, nisse-webrtc wrote:
> And here for some reason |begin()->| is used instead of |front().|, but that
> should be the same, right?

I believe so. This is probably an artefact of different people using different
styles, without looking at the adjacent code.

https://codereview.webrtc.org/3019453002/diff/20001/video/end_to_end_tests.cc
File video/end_to_end_tests.cc (left):

https://codereview.webrtc.org/3019453002/diff/20001/video/end_to_end_tests.cc...
video/end_to_end_tests.cc:2703: bool use_red,
On 2017/09/21 08:37:46, nisse-webrtc wrote:
> On 2017/09/21 08:11:33, brandtr wrote:
> > Would you mind renaming |use_red| to |use_fec| ? Because that's how the flag
> is
> > actually used.
> 
> Just in this method and its local classes, or globally in this file?

I think they are equivalent: I have already tried to remove all uses of "RED"
when what is actually meant is "RED+ULPFEC" in this file. But apparently I
missed this particular test.

[brandtr, 2017-09-21 08:49:40]

https://codereview.webrtc.org/3019453002/diff/40001/media/engine/webrtcvideoe...
File media/engine/webrtcvideoengine.cc (right):

https://codereview.webrtc.org/3019453002/diff/40001/media/engine/webrtcvideoe...
media/engine/webrtcvideoengine.cc:2265: 
Maybe we should add a RTC_DCHECK(!recv_codecs.empty()) here, WDYT? I quickly
looked through the call sites and I couldn't see a guarantee that recv_codecs is
not empty there.

[nisse-webrtc, 2017-09-21 08:57:28]

nisse@webrtc.org changed reviewers:
+ magjed@webrtc.org, stefan@webrtc.org

[nisse-webrtc, 2017-09-21 08:57:29]

https://codereview.webrtc.org/3019453002/diff/20001/video/end_to_end_tests.cc
File video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/3019453002/diff/20001/video/end_to_end_tests.cc...
video/end_to_end_tests.cc:1205: int GetPayloadType(bool use_rtx, bool use_red) {
I'm doing a search-and-replace then, changing to use_fec here too.

https://codereview.webrtc.org/3019453002/diff/40001/media/engine/webrtcvideoe...
File media/engine/webrtcvideoengine.cc (right):

https://codereview.webrtc.org/3019453002/diff/40001/media/engine/webrtcvideoe...
media/engine/webrtcvideoengine.cc:2265: 
On 2017/09/21 08:49:40, brandtr wrote:
> Maybe we should add a RTC_DCHECK(!recv_codecs.empty()) here, WDYT? I quickly
> looked through the call sites and I couldn't see a guarantee that recv_codecs
is
> not empty there.

Added a DCHECK at the top of the method.

I don't know what this is intended to do; if it in some way supports multiple
codecs, but configures RTP based only on the first of them.

[nisse-webrtc, 2017-09-21 08:58:10]

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

[commit-bot: I haz the power, 2017-09-21 08:58:20]

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, 2017-09-21 09:30:51]

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

[commit-bot: I haz the power, 2017-09-21 09:30:53]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-09-25 10:48:15]

Ping? OWNER's approval needed.

[stefan-webrtc, 2017-09-25 12:05:22]

lgtm

[magjed_webrtc, 2017-09-26 09:05:03]

media/engine lgtm

[nisse-webrtc, 2017-09-26 09:13:58]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-09-26 09:13:59]

The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3019453002/#ps80001 (title:
"Added DCHECK.")

[commit-bot: I haz the power, 2017-09-26 09:14:07]

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, 2017-09-26 09:49:22]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1506417238923370,
"parent_rev": "daea5bf2deb843567a792f22ea2047a037e09d78", "commit_rev":
"3b3622fafc6d1fd910074f7bab78c8427d80649e"}

[commit-bot: I haz the power, 2017-09-26 09:49:27]

Description was changed from

==========
Delete member VideoReceiveStream::Config::Rtp::ulpfec.

Replaced with scalars ulpfec_payload_type and red_payload_type.

In particular, ulpfec.red_rtx_payload_type, which duplicated info in
rtx_associated_payload_types, is deleted. This is a followup to cl
https://codereview.webrtc.org/3012963002.

BUG=webrtc:7135
==========

to

==========
Delete member VideoReceiveStream::Config::Rtp::ulpfec.

Replaced with scalars ulpfec_payload_type and red_payload_type.

In particular, ulpfec.red_rtx_payload_type, which duplicated info in
rtx_associated_payload_types, is deleted. This is a followup to cl
https://codereview.webrtc.org/3012963002.

BUG=webrtc:7135

Review-Url: https://codereview.webrtc.org/3019453002
Cr-Commit-Position: refs/heads/master@{#19965}
Committed:
https://webrtc.googlesource.com/src/+/3b3622fafc6d1fd910074f7bab78c8427d80649e
==========

[commit-bot: I haz the power, 2017-09-26 09:49:28]

Committed patchset #5 (id:80001) as
https://webrtc.googlesource.com/src/+/3b3622fafc6d1fd910074f7bab78c8427d80649e