Rename {,Set}GenericFECStatus to {,Set}UlpfecConfig.

Rename {,Set}GenericFECStatus to {,Set}UlpfecConfig.

At the same time, change to using int's instead of uint8_t's for the payload type.
This allows us to signal disabled FEC or RED using the sentinel value -1, which
is commonplace in other parts of the code.

These APIs will be deprecated when ULPFEC is deprecated.

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

[brandtr, 2016-10-24 15:06:53]

Please have a look :)

danilchap: rtp_rtcp/
holmer: video/

Renaming clarifies what the methods are used for. (Similar methods will not be
needed for FlexFEC, since we there set payload types in the ctors. A payload
type != -1 means that FlexFEC is enabled.) When ULPFEC is deprecated, we can
remove these methods.

Changing from uint8_t -> int is according to old discussion with danilchap. This
change also makes VideoSendStream::ConfigureProtection() work nicer with its
config, removing the need to convert a payload type of -1 to a payload type of
0.

[brandtr, 2016-10-25 06:42:14]

Description was changed from

==========
Rename {,Set}GenericFECStatus to {,Set}UlpfecConfig.

At the same time, change to using int's instead of uint8_t's for the payload
type.
This allows us to signal disabled FEC or RED using the sentinel value -1, which
is commonplace in other parts of the code.

These APIs will be deprecated when ULPFEC is deprecated.

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

to

==========
Rename {,Set}GenericFECStatus to {,Set}UlpfecConfig.

At the same time, change to using int's instead of uint8_t's for the payload
type.
This allows us to signal disabled FEC or RED using the sentinel value -1, which
is commonplace in other parts of the code.

These APIs will be deprecated when ULPFEC is deprecated.

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

[brandtr, 2016-10-25 06:42:14]

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

[brandtr, 2016-10-25 06:42:57]

Somehow Rietveld didn't add the reviewers properly when I sent out the original
email. Please see issue for original message.

[danilchap, 2016-10-26 12:59:14]

https://codereview.webrtc.org/2448463003/diff/1/webrtc/modules/rtp_rtcp/inclu...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right):

https://codereview.webrtc.org/2448463003/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:456: // Turn on/off generic FEC.
update comments too.

https://codereview.webrtc.org/2448463003/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:457: virtual void
SetUlpfecConfig(bool ulpfec_enabled,
since function already called UlpfecConfig, 'ulpfec_' prefix in this variable
name feels a bit redundant.
May be remove it (call just 'enable')
(this comment doesn't apply to payload_type since there are two payload type, so
prefix is helpful to distinguish them)

https://codereview.webrtc.org/2448463003/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:462: virtual void UlpfecConfig(bool*
ulpfec_enabled,
It seems this function is not in use any more. (e.g. no use of it updated in
this CL)
May be just delete it.

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

https://codereview.webrtc.org/2448463003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:177: 
Add DCHECKS payload_type is small enough:
RTC_DCHECK_LE(payload_type, 127);

https://codereview.webrtc.org/2448463003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.h (right):

https://codereview.webrtc.org/2448463003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:97: bool fec_enabled_
GUARDED_BY(crit_) = false;
may be rename this variable to ulpfec_enabled_

[brandtr, 2016-10-26 13:19:59]

Patchset #2 (id:20001) has been deleted

[brandtr, 2016-10-26 13:21:31]

Patchset #2 (id:40001) has been deleted

[brandtr, 2016-10-26 13:57:04]

https://codereview.webrtc.org/2448463003/diff/1/webrtc/modules/rtp_rtcp/inclu...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right):

https://codereview.webrtc.org/2448463003/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:456: // Turn on/off generic FEC.
On 2016/10/26 12:59:13, danilchap wrote:
> update comments too.

Done.

https://codereview.webrtc.org/2448463003/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:457: virtual void
SetUlpfecConfig(bool ulpfec_enabled,
On 2016/10/26 12:59:13, danilchap wrote:
> since function already called UlpfecConfig, 'ulpfec_' prefix in this variable
> name feels a bit redundant.
> May be remove it (call just 'enable')
> (this comment doesn't apply to payload_type since there are two payload type,
so
> prefix is helpful to distinguish them)

Done. (This parameter will probably removed soon though!)

https://codereview.webrtc.org/2448463003/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:462: virtual void UlpfecConfig(bool*
ulpfec_enabled,
On 2016/10/26 12:59:13, danilchap wrote:
> It seems this function is not in use any more. (e.g. no use of it updated in
> this CL)
> May be just delete it.

Ah, I wasn't careful enough in checking where this function is used. It's only
the implementation living in RTPSenderVideo that is actually called, so no need
to have it part of this general interface.

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

https://codereview.webrtc.org/2448463003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:177: 
On 2016/10/26 12:59:14, danilchap wrote:
> Add DCHECKS payload_type is small enough:
> RTC_DCHECK_LE(payload_type, 127);

Done.

https://codereview.webrtc.org/2448463003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.h (right):

https://codereview.webrtc.org/2448463003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:97: bool fec_enabled_
GUARDED_BY(crit_) = false;
On 2016/10/26 12:59:14, danilchap wrote:
> may be rename this variable to ulpfec_enabled_

This is happening in upcoming CL.

https://codereview.webrtc.org/2448463003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right):

https://codereview.webrtc.org/2448463003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.h:286: Transport* transport_;
Lint fix.

[danilchap, 2016-10-26 14:18:08]

lgtm

https://codereview.webrtc.org/2448463003/diff/60001/webrtc/modules/rtp_rtcp/m...
File webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h (right):

https://codereview.webrtc.org/2448463003/diff/60001/webrtc/modules/rtp_rtcp/m...
webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h:193: MOCK_METHOD3(UlpfecConfig,
can remove this too

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

https://codereview.webrtc.org/2448463003/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:942: if (config_->rtp.ulpfec.red_payload_type
!= -1) {
these two if block can be now reduced to
RTC_DCHECK_GE(...payload_type, -1);
RTC_DCHECK_LE(...payload_type, 127);
or may be just drop them (payload type dchecked internally anyway,
here it was necessary to ensure cast will not turn invalid value into valid.)

[brandtr, 2016-10-27 09:47:42]

https://codereview.webrtc.org/2448463003/diff/60001/webrtc/modules/rtp_rtcp/m...
File webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h (right):

https://codereview.webrtc.org/2448463003/diff/60001/webrtc/modules/rtp_rtcp/m...
webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h:193: MOCK_METHOD3(UlpfecConfig,
On 2016/10/26 14:18:08, danilchap wrote:
> can remove this too

Thanks for finding this! Done.

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

https://codereview.webrtc.org/2448463003/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:942: if (config_->rtp.ulpfec.red_payload_type
!= -1) {
On 2016/10/26 14:18:08, danilchap wrote:
> these two if block can be now reduced to
> RTC_DCHECK_GE(...payload_type, -1);
> RTC_DCHECK_LE(...payload_type, 127);
> or may be just drop them (payload type dchecked internally anyway,
> here it was necessary to ensure cast will not turn invalid value into valid.)

Right. I will make some specific changes to this function in the next (or second
next) CL, so I won't make more changes right now.

[brandtr, 2016-10-27 10:14:06]

Final.

[brandtr, 2016-10-28 07:05:02]

Patchset #5 (id:120001) has been deleted

[brandtr, 2016-10-28 07:09:04]

Noop.

[brandtr, 2016-10-28 07:09:16]

Patchset #4 (id:100001) has been deleted

[brandtr, 2016-10-28 07:29:46]

Patchset #4 (id:140001) has been deleted

[brandtr, 2016-10-28 07:39:13]

Rebase.

[stefan-webrtc, 2016-10-31 09:33:17]

https://codereview.webrtc.org/2448463003/diff/160001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.h (right):

https://codereview.webrtc.org/2448463003/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:67: void UlpfecConfig(bool*
enabled,
Perhaps GetUlpfecConfig

https://codereview.webrtc.org/2448463003/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:102: int fec_payload_type_
GUARDED_BY(crit_) = -1;
I'd prefer moving these initializations to the initializer list. Feel free to do
that...

https://codereview.webrtc.org/2448463003/diff/160001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (left):

https://codereview.webrtc.org/2448463003/diff/160001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:955:
static_cast<uint8_t>(config_->rtp.ulpfec.red_payload_type);
I think the TODO above becomes a bit strange when we remove this. The comment
means that this assignment only should happen if enable_protection_fec is true,
I think? Maybe change the comment to make that more clear.

[brandtr, 2016-10-31 12:43:24]

Patchset #5 (id:180001) has been deleted

[brandtr, 2016-10-31 12:47:04]

Addressed comments. Please have a second look :)

https://codereview.webrtc.org/2448463003/diff/160001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.h (right):

https://codereview.webrtc.org/2448463003/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:67: void UlpfecConfig(bool*
enabled,
On 2016/10/31 09:33:16, stefan-webrtc (holmer) wrote:
> Perhaps GetUlpfecConfig

Done.

https://codereview.webrtc.org/2448463003/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:102: int fec_payload_type_
GUARDED_BY(crit_) = -1;
On 2016/10/31 09:33:16, stefan-webrtc (holmer) wrote:
> I'd prefer moving these initializations to the initializer list. Feel free to
do
> that...

Done!

This was originally my plan, but then I saw this CL:
https://codereview.webrtc.org/2067673004/diff/200001/webrtc/modules/rtp_rtcp/...

But with your mandate, I'm happy to do this :)

https://codereview.webrtc.org/2448463003/diff/160001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (left):

https://codereview.webrtc.org/2448463003/diff/160001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:955:
static_cast<uint8_t>(config_->rtp.ulpfec.red_payload_type);
On 2016/10/31 09:33:16, stefan-webrtc (holmer) wrote:
> I think the TODO above becomes a bit strange when we remove this. The comment
> means that this assignment only should happen if enable_protection_fec is
true,
> I think? Maybe change the comment to make that more clear.

Yeah, you are right. This whole function will be changed in a couple of CLs in
the future.

It's my end-term goal to simplify the logic here, and remove many/all of the
TODOs. [General idea: either you use FlexFEC, or you use RED+ULPFEC, or you use
nothing.] During the process, I'm trying to make small changes here, to avoid
breaking anything.

https://codereview.webrtc.org/2448463003/diff/200001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2448463003/diff/200001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:50: clock_(clock),
Please help me check that I correctly copy-pasted the initial values...

[danilchap, 2016-10-31 13:22:00]

https://codereview.webrtc.org/2448463003/diff/160001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.h (right):

https://codereview.webrtc.org/2448463003/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:102: int fec_payload_type_
GUARDED_BY(crit_) = -1;
On 2016/10/31 09:33:16, stefan-webrtc (holmer) wrote:
> I'd prefer moving these initializations to the initializer list. Feel free to
do
> that...

just curious, what are benefits of initializer list over default Member
Initializers?
Default member initializers make it easier to add new member, reorder existent
members, make it easier to see when a POD member miss initial value.

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

Rebase.

[stefan-webrtc, 2016-11-03 13:52:26]

lgtm

https://codereview.webrtc.org/2448463003/diff/160001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.h (right):

https://codereview.webrtc.org/2448463003/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:102: int fec_payload_type_
GUARDED_BY(crit_) = -1;
On 2016/10/31 13:22:00, danilchap wrote:
> On 2016/10/31 09:33:16, stefan-webrtc (holmer) wrote:
> > I'd prefer moving these initializations to the initializer list. Feel free
to
> do
> > that...
> 
> just curious, what are benefits of initializer list over default Member
> Initializers?
> Default member initializers make it easier to add new member, reorder existent
> members, make it easier to see when a POD member miss initial value.

We discussed this a lot at some point, and I think the conclusion was that it
harms readability if some members are initialized in the initializer list while
others are initialized in the .h file. At that point I remember that there
wasn't a clear consensus on whether this also would apply for const variables,
but I tend to just prefer everything in initializer lists to keep it simple.

Feel free to bring it up for discussion again if you think it's wrong though!

https://codereview.webrtc.org/2448463003/diff/200001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2448463003/diff/200001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:50: clock_(clock),
On 2016/10/31 12:47:04, brandtr wrote:
> Please help me check that I correctly copy-pasted the initial values...

Done.

[brandtr, 2016-11-03 15:32:59]

Rebase.

[brandtr, 2016-11-07 07:41:40]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2016-11-07 07:41:40]

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

[commit-bot: I haz the power, 2016-11-07 07:41:45]

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-07 09:38:16]

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

[commit-bot: I haz the power, 2016-11-07 09:38:18]

Try jobs failed on following builders:
  win_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/12795)

[brandtr, 2016-11-07 09:58:26]

The CQ bit was checked by brandtr@webrtc.org

[commit-bot: I haz the power, 2016-11-07 09:58:34]

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-07 10:08:55]

Description was changed from

==========
Rename {,Set}GenericFECStatus to {,Set}UlpfecConfig.

At the same time, change to using int's instead of uint8_t's for the payload
type.
This allows us to signal disabled FEC or RED using the sentinel value -1, which
is commonplace in other parts of the code.

These APIs will be deprecated when ULPFEC is deprecated.

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

to

==========
Rename {,Set}GenericFECStatus to {,Set}UlpfecConfig.

At the same time, change to using int's instead of uint8_t's for the payload
type.
This allows us to signal disabled FEC or RED using the sentinel value -1, which
is commonplace in other parts of the code.

These APIs will be deprecated when ULPFEC is deprecated.

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

[commit-bot: I haz the power, 2016-11-07 10:08:56]

Committed patchset #7 (id:240001)

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

Description was changed from

==========
Rename {,Set}GenericFECStatus to {,Set}UlpfecConfig.

At the same time, change to using int's instead of uint8_t's for the payload
type.
This allows us to signal disabled FEC or RED using the sentinel value -1, which
is commonplace in other parts of the code.

These APIs will be deprecated when ULPFEC is deprecated.

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

to

==========
Rename {,Set}GenericFECStatus to {,Set}UlpfecConfig.

At the same time, change to using int's instead of uint8_t's for the payload
type.
This allows us to signal disabled FEC or RED using the sentinel value -1, which
is commonplace in other parts of the code.

These APIs will be deprecated when ULPFEC is deprecated.

BUG=webrtc:5654

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

[commit-bot: I haz the power, 2016-11-07 10:09:04]

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