Simplify {,Set}UlpfecConfig interface.

Simplify {,Set}UlpfecConfig interface.

Prior to this change, we signalled that ULPFEC was disabled
through a bool, but that RED was disabled by setting its
payload type to -1. The latter is consistent with how we
disable RED/ULPFEC in the config, so this CL removes the
ULPFEC bool from the {,Set}UlpfecConfig chain of member
functions.

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

[brandtr, 2016-10-27 11:06:23]

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

[brandtr, 2016-10-27 11:06:24]

Follow-up CL to 2448463003.

The bot failure is due to a mysterious patch failure.

https://codereview.webrtc.org/2460533002/diff/1/webrtc/video/video_send_strea...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2460533002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:922: RTC_DCHECK_RUN_ON(worker_queue_);
This function will see further changes in future CLs. Until then, I try to keep
the changes to a minimum.

[danilchap, 2016-10-27 11:53:41]

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

https://codereview.webrtc.org/2460533002/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:456: // Set RED and ULPFEC payload
types. A payload type of -1
which of the two payload types should be -1 to turn ulpfec off?

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

https://codereview.webrtc.org/2460533002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:92: bool red_enabled_
GUARDED_BY(crit_) = false;
why do you want red_enabled_ and fec_enabled_ members?
thay can be simple accessors instead if you want that logic centralized:
bool red_enabled() const { return red_payload_type >= 0; }

[brandtr, 2016-10-28 07:07:41]

Noop.

[brandtr, 2016-10-28 07:08:25]

Patchset #3 (id:40001) has been deleted

[brandtr, 2016-10-28 07:08:30]

Patchset #2 (id:20001) has been deleted

[brandtr, 2016-10-28 07:10:17]

Feedback response 1.

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

Test.

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

Patchset #3 (id:80001) has been deleted

[brandtr, 2016-10-28 07:35:44]

Feedback response 1.

[brandtr, 2016-10-28 07:35:47]

Patchset #2 (id:60001) has been deleted

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

Rebase.

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

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

https://codereview.webrtc.org/2460533002/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:456: // Set RED and ULPFEC payload
types. A payload type of -1
On 2016/10/27 11:53:40, danilchap wrote:
> which of the two payload types should be -1 to turn ulpfec off?

ulpfec_payload_type == -1 means that ULPFEC is off and red_payload_type == -1
means that RED is off. I have expanded the comment to clarify this.

In a consistent system, these should always be enabled/disabled together.
However, due to a workaround involving RTX (see
https://codereview.webrtc.org/1649493004), for now we will always send RED if it
has been negotiated in the SDP, regardless if we are using ULPFEC or not. I will
most likely remove this workaround, but for now we need the ability to turn
these off individually.

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

https://codereview.webrtc.org/2460533002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:92: bool red_enabled_
GUARDED_BY(crit_) = false;
On 2016/10/27 11:53:40, danilchap wrote:
> why do you want red_enabled_ and fec_enabled_ members?
> thay can be simple accessors instead if you want that logic centralized:
> bool red_enabled() const { return red_payload_type >= 0; }

Yep, this makes sense!

These variables are currently under locks, so I'm adding the member functions as
"heavy" (i.e. CamelCase) non-inline functions.

[danilchap, 2016-10-28 09:29:43]

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

https://codereview.webrtc.org/2460533002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:174: rtc::CritScope
cs(&crit_);
do not (re)take lock, instead annotate these functions with
EXCLUSIVE_LOCKS_REQUIRED(crit_)

[brandtr, 2016-10-28 11:05:06]

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

https://codereview.webrtc.org/2460533002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:174: rtc::CritScope
cs(&crit_);
On 2016/10/28 09:29:43, danilchap wrote:
> do not (re)take lock, instead annotate these functions with
> EXCLUSIVE_LOCKS_REQUIRED(crit_)

Got it! Done. Also changed name to cheap_accessor_style().

[danilchap, 2016-10-28 11:35:46]

https://codereview.webrtc.org/2460533002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right):

https://codereview.webrtc.org/2460533002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:458: // That is, RED and ULPFEC can
be enabled/disabled individually.
may be rephrase to note one can't use ULPFEC without RED.

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

https://codereview.webrtc.org/2460533002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:285: bool _red_enabled;
may be name it 'is_red_enabled' or 'send_as_red'
probably you can name it red_enabled shadowing the accessor name
then a line below will be
red_enabled = this->red_enabled();

[brandtr, 2016-10-28 12:11:45]

https://codereview.webrtc.org/2460533002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right):

https://codereview.webrtc.org/2460533002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:458: // That is, RED and ULPFEC can
be enabled/disabled individually.
On 2016/10/28 11:35:46, danilchap wrote:
> may be rephrase to note one can't use ULPFEC without RED.

Added longer explanation.

Within WebRTC, this function will be called from
VideoSendStream::ConfigureProtection, where all the details will be described.

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

https://codereview.webrtc.org/2460533002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:285: bool _red_enabled;
On 2016/10/28 11:35:46, danilchap wrote:
> may be name it 'is_red_enabled' or 'send_as_red'
> probably you can name it red_enabled shadowing the accessor name
> then a line below will be
> red_enabled = this->red_enabled();

Going with 'red_enabled'. This is symmetric with 'ulpfec_enabled', and the
coming 'flexfec_enabled'.

[danilchap, 2016-10-28 12:22:32]

lgtm

[stefan-webrtc, 2016-10-31 09:41:03]

lgtm % nit

https://codereview.webrtc.org/2460533002/diff/160001/webrtc/video/rtp_stream_...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2460533002/diff/160001/webrtc/video/rtp_stream_...
webrtc/video/rtp_stream_receiver.cc:181: // TODO(brandtr): Investigate whether
the call below can be removed.
Could you comment on why it might be possible to remove?

[brandtr, 2016-10-31 12:05:17]

https://codereview.webrtc.org/2460533002/diff/160001/webrtc/video/rtp_stream_...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2460533002/diff/160001/webrtc/video/rtp_stream_...
webrtc/video/rtp_stream_receiver.cc:181: // TODO(brandtr): Investigate whether
the call below can be removed.
On 2016/10/31 09:41:02, stefan-webrtc (holmer) wrote:
> Could you comment on why it might be possible to remove?

Done.

[brandtr, 2016-11-02 10:39:45]

Rebase.

[brandtr, 2016-11-03 15:33:14]

Rebase.

[brandtr, 2016-11-07 10:41:52]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2016-11-07 10:41:53]

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/2460533002/#ps260001 (title:
"git cl format.")

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

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 11:05:09]

Committed patchset #10 (id:260001)

[commit-bot: I haz the power, 2016-11-07 11:05:14]

Description was changed from

==========
Simplify {,Set}UlpfecConfig interface.

Prior to this change, we signalled that ULPFEC was disabled
through a bool, but that RED was disabled by setting its
payload type to -1. The latter is consistent with how we
disable RED/ULPFEC in the config, so this CL removes the
ULPFEC bool from the {,Set}UlpfecConfig chain of member
functions.

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

to

==========
Simplify {,Set}UlpfecConfig interface.

Prior to this change, we signalled that ULPFEC was disabled
through a bool, but that RED was disabled by setting its
payload type to -1. The latter is consistent with how we
disable RED/ULPFEC in the config, so this CL removes the
ULPFEC bool from the {,Set}UlpfecConfig chain of member
functions.

BUG=webrtc:5654

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

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

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