Introduce FecControllerRplrBased

Introduce FecControllerRplrBased

1. Rename FecController to FecControllerPlrBased.
2. Introduce FecControllerRplrBased - a version of FecController that makes its decision based on RPLR instead of PLR.

BUG=webrtc:7058
Review-Url: https://codereview.webrtc.org/2672933003
Cr-Commit-Position: refs/heads/master@{#17373}
Committed: https://chromium.googlesource.com/external/webrtc/+/6d7900de662ea06c8a1d112a6ed71b215237173d

[elad.alon_webrtc.org, 2017-02-03 15:23:44]

Description was changed from

==========
Introduce FecControllerRplrBased

BUG=webrtc:7058
==========

to

==========
Introduce FecControllerRplrBased

BUG=webrtc:7058
==========

[elad.alon_webrtc.org, 2017-02-03 15:23:44]

elad.alon@webrtc.org changed reviewers:
+ michaelt@webrtc.org, minyue@webrtc.org, sprang@webrtc.org, stefan@webrtc.org

[elad.alon_webrtc.org, 2017-02-07 23:15:14]

Description was changed from

==========
Introduce FecControllerRplrBased

BUG=webrtc:7058
==========

to

==========
Introduce FecControllerRplrBased

1. Rename FecController to FecControllerPlrBased.
2. Introduce FecControllerRplrBased - a version of FecController that makes its
decision based on RPLR instead of PLR.

BUG=webrtc:7058
==========

[elad.alon_webrtc.org, 2017-02-07 23:19:45]

Ready for review.

[michaelt, 2017-02-09 13:59:16]

https://codereview.webrtc.org/2672933003/diff/40001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc
(right):

https://codereview.webrtc.org/2672933003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc:115:
} else if (bandwidth_bps >= threshold.high_bandwidth_bps) {
why did you introduced the "else if" the logic seams to be the same. 
And we have more code now.

https://codereview.webrtc.org/2672933003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc:118:
float rc = threshold_info.offset + threshold_info.slope * bandwidth_bps;
"rc" ? 
Would prefere "packet_loss_threshold"

https://codereview.webrtc.org/2672933003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc:119:
RTC_DCHECK_LE(rc, threshold.low_bandwidth_packet_loss);
I think this should be checked with unit test not with RTC_DCHECK.

https://codereview.webrtc.org/2672933003/diff/40001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.h
(left):

https://codereview.webrtc.org/2672933003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.h:97:
const std::unique_ptr<SmoothingFilter> packet_loss_smoother_;
Depending on the window size of the rplr_tracker we still would need a smoother.
Or another way which will prevent us from toggling to much the FEC state.

[elad.alon_webrtc.org, 2017-02-09 14:55:12]

https://codereview.webrtc.org/2672933003/diff/40001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc
(right):

https://codereview.webrtc.org/2672933003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc:115:
} else if (bandwidth_bps >= threshold.high_bandwidth_bps) {
On 2017/02/09 13:59:16, michaelt wrote:
> why did you introduced the "else if" the logic seams to be the same. 
> And we have more code now.
>  

The else-statement makes it clearer that only one condition can hold, imho.

https://codereview.webrtc.org/2672933003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc:118:
float rc = threshold_info.offset + threshold_info.slope * bandwidth_bps;
On 2017/02/09 13:59:16, michaelt wrote:
> "rc" ? 
> Would prefere "packet_loss_threshold"

Stands for "return code", as far as I can tell. It's common in some code bases,
but upon closer inspection, not in ours. I'll rename.

https://codereview.webrtc.org/2672933003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc:119:
RTC_DCHECK_LE(rc, threshold.low_bandwidth_packet_loss);
On 2017/02/09 13:59:16, michaelt wrote:
> I think this should be checked with unit test not with RTC_DCHECK.

Checking with the UT is important, no doubt. I see it as valuable for a reader
of the code, though. Serves as self-proving documentation. But since Minyue was
skeptical as well, I don't mind removing it.

https://codereview.webrtc.org/2672933003/diff/40001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.h
(left):

https://codereview.webrtc.org/2672933003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.h:97:
const std::unique_ptr<SmoothingFilter> packet_loss_smoother_;
On 2017/02/09 13:59:16, michaelt wrote:
> Depending on the window size of the rplr_tracker we still would need a
smoother.
> Or another way which will prevent us from toggling to much the FEC state. 

The current window sizes we're aiming at do not make smoothing necessary. The
values we get are stable, and change gradually. We might, however, want to use a
function of them as the source for the encoder's PL, as we've discussed
yesterday. I'll keep this in mind and come back to this issue later. I think it
would be best o explore this after the time-based window size issue.

[minyue-webrtc, 2017-03-22 09:29:05]

https://codereview.webrtc.org/2672933003/diff/80001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.h
(right):

https://codereview.webrtc.org/2672933003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.h:82:
struct ThresholdInfo {
did we agree on making this a util?

[elad.alon_webrtc.org, 2017-03-22 10:11:38]

https://codereview.webrtc.org/2672933003/diff/80001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.h
(right):

https://codereview.webrtc.org/2672933003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.h:82:
struct ThresholdInfo {
On 2017/03/22 09:29:05, minyue-webrtc wrote:
> did we agree on making this a util?

We did, and I have - https://codereview.webrtc.org/2688613003/

[minyue-webrtc, 2017-03-24 10:17:29]

https://codereview.webrtc.org/2672933003/diff/80001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.h
(right):

https://codereview.webrtc.org/2672933003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.h:82:
struct ThresholdInfo {
On 2017/03/22 10:11:38, elad.alon wrote:
> On 2017/03/22 09:29:05, minyue-webrtc wrote:
> > did we agree on making this a util?
> 
> We did, and I have - https://codereview.webrtc.org/2688613003/

Please add a TODO to make this CL complete by itself.

https://codereview.webrtc.org/2672933003/diff/80001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h
(right):

https://codereview.webrtc.org/2672933003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:29:
// Note: This is the packet loss fraction that's given to the encoder.
add an empty line between 28 and 29, and remote "Note:"

make it the same format is to num_channels.

[minyue-webrtc, 2017-03-24 10:20:30]

On 2017/03/24 10:17:29, minyue-webrtc wrote:
>
https://codereview.webrtc.org/2672933003/diff/80001/webrtc/modules/audio_codi...
> File
> webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.h
> (right):
> 
>
https://codereview.webrtc.org/2672933003/diff/80001/webrtc/modules/audio_codi...
>
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.h:82:
> struct ThresholdInfo {
> On 2017/03/22 10:11:38, elad.alon wrote:
> > On 2017/03/22 09:29:05, minyue-webrtc wrote:
> > > did we agree on making this a util?
> > 
> > We did, and I have - https://codereview.webrtc.org/2688613003/
> 
> Please add a TODO to make this CL complete by itself.
> 
>
https://codereview.webrtc.org/2672933003/diff/80001/webrtc/modules/audio_codi...
> File
>
webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h
> (right):
> 
>
https://codereview.webrtc.org/2672933003/diff/80001/webrtc/modules/audio_codi...
>
webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:29:
> // Note: This is the packet loss fraction that's given to the encoder.
> add an empty line between 28 and 29, and remote "Note:"
> 
> make it the same format is to num_channels.

This CL has been dependent on https://codereview.webrtc.org/2673543004, which
was very helpful for the reviewing. But https://codereview.webrtc.org/2673543004
is not a good CL by itself. Please consider rebasing this CL to master after
lgtm, and then land the two altogether.

[elad.alon_webrtc.org, 2017-03-24 10:48:02]

The CQ bit was checked by elad.alon@webrtc.org

[elad.alon_webrtc.org, 2017-03-24 10:48:02]

The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2672933003/#ps100001 (title:
"Merged")

[commit-bot: I haz the power, 2017-03-24 10:48:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[minyue-webrtc, 2017-03-24 10:49:33]

On 2017/03/24 10:48:10, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

missed two small comments from me

[minyue-webrtc, 2017-03-24 10:50:06]

On 2017/03/24 10:49:33, minyue-webrtc wrote:
> On 2017/03/24 10:48:10, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
> 
> missed two small comments from me

But it should be fine to omit them for now.

[commit-bot: I haz the power, 2017-03-24 11:12:57]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490352482071070,
"parent_rev": "8ed482e6d783c15ae77d0e63caa3f6a15f2bb149", "commit_rev":
"6d7900de662ea06c8a1d112a6ed71b215237173d"}

[commit-bot: I haz the power, 2017-03-24 11:12:59]

Description was changed from

==========
Introduce FecControllerRplrBased

1. Rename FecController to FecControllerPlrBased.
2. Introduce FecControllerRplrBased - a version of FecController that makes its
decision based on RPLR instead of PLR.

BUG=webrtc:7058
==========

to

==========
Introduce FecControllerRplrBased

1. Rename FecController to FecControllerPlrBased.
2. Introduce FecControllerRplrBased - a version of FecController that makes its
decision based on RPLR instead of PLR.

BUG=webrtc:7058

Review-Url: https://codereview.webrtc.org/2672933003
Cr-Commit-Position: refs/heads/master@{#17373}
Committed:
https://chromium.googlesource.com/external/webrtc/+/6d7900de662ea06c8a1d112a6...
==========

[commit-bot: I haz the power, 2017-03-24 11:13:01]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/6d7900de662ea06c8a1d112a6...