Change frame length on very low bandwidth.

Change frame length on very low bandwidth.

BUG=webrtc:7199
Review-Url: https://codereview.webrtc.org/2703353002
Cr-Commit-Position: refs/heads/master@{#16777}
Committed: https://chromium.googlesource.com/external/webrtc/+/6f08d7d68d3ba41d65b61728266ad70391f009fc

[michaelt, 2017-02-21 09:44:13]

Description was changed from

==========
Change frame length on very low bandwidth.

BUG=webrtc:7199
==========

to

==========
Change frame length on very low bandwidth.

BUG=webrtc:7199
==========

[michaelt, 2017-02-21 09:44:13]

michaelt@webrtc.org changed reviewers:
+ minyue@webrtc.org

[michaelt, 2017-02-21 09:45:01]

Here the discussed fix for the low bandwidth case.

[minyue-webrtc, 2017-02-21 10:41:25]

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/audio_network_adaptor/config.proto (right):

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/audio_network_adaptor/config.proto:46: // Uplink
packet loss fraction above which frame length should decrease.
Thanks for picking this

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
File
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc
(right):

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:31:
int prevent_overuse_margin_bps,
I'd like this to be hard coded in frame length controller. WDYT?

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:102:
// 5. |uplink_bandwidth_bps| is known to be smaller or equal than
This can be misleading because of order of operations

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
File
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc
(right):

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:252:
From20MsTo60MsOnHighUplinkPacketLossFractionAndVeryLowUplinkBandwidth) {
I think we can remove move the test closer to
"IncreaseTo60MsOnMultipleConditions", and rename it to
"IncreaseTo60MsOnVeryLowUplinkBandwidth"

please add a comment before 255, say

We set packet loss fraction to kFlDecreasingPacketLossFraction, and FEC on, both
of which should have prevented frame length to increase, if the uplink bandwidth
is not this low.

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:310:
From120MsTo60MsWhenFecIsOnAndVeryLowUplinkBandwidth) {
this is somehow missleading, since very low bitrate is to either prevent fl from
decreasing or force it to increase.

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:321:
From20MsTo60MsWhenFecIsOnAndVeryLowUplinkBandwidth) {
This is sort of tested, hasn't it.

I think we would like to add From60To120WhenVeryLow...

[michaelt, 2017-02-21 15:56:42]

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
File
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc
(right):

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:31:
int prevent_overuse_margin_bps,
I put it here because of the testing. Other wise i would have to redefine the
value for the test.

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:102:
// 5. |uplink_bandwidth_bps| is known to be smaller or equal than
On 2017/02/21 10:41:24, minyue-webrtc wrote:
> This can be misleading because of order of operations

Changed order

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
File
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc
(right):

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:252:
From20MsTo60MsOnHighUplinkPacketLossFractionAndVeryLowUplinkBandwidth) {
On 2017/02/21 10:41:24, minyue-webrtc wrote:
> I think we can remove move the test closer to
> "IncreaseTo60MsOnMultipleConditions", and rename it to
> "IncreaseTo60MsOnVeryLowUplinkBandwidth"
> 
> please add a comment before 255, say
> 
> We set packet loss fraction to kFlDecreasingPacketLossFraction, and FEC on,
both
> of which should have prevented frame length to increase, if the uplink
bandwidth
> is not this low.

Done.

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:310:
From120MsTo60MsWhenFecIsOnAndVeryLowUplinkBandwidth) {
Changed according to offline discussion.

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:321:
From20MsTo60MsWhenFecIsOnAndVeryLowUplinkBandwidth) {
Changed according to offline discussion.

[minyue-webrtc, 2017-02-22 09:10:47]

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
File
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc
(right):

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:31:
int prevent_overuse_margin_bps,
On 2017/02/21 15:56:41, michaelt wrote:
> I put it here because of the testing. Other wise i would have to redefine the
> value for the test. 
> 

I think redefine in test is acceptable. otherwise, you can make it a static
public member. since I hope to keep this hard coded for now. Otherwise, it
should have become a config parameter.

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:102:
// 5. |uplink_bandwidth_bps| is known to be smaller or equal than
On 2017/02/21 15:56:41, michaelt wrote:
> On 2017/02/21 10:41:24, minyue-webrtc wrote:
> > This can be misleading because of order of operations
> 
> Changed order

not sure if this is clearly enough. may add "all the following" after "OR"

https://codereview.webrtc.org/2703353002/diff/20001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc
(right):

https://codereview.webrtc.org/2703353002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:190:
// bandwidth is not this low.
is -> was

https://codereview.webrtc.org/2703353002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:196:

and add a Maintain60MsOnVeryLowUplinkBandwidth in analogy to
Maintain120MsOnVeryLowUplinkBandwidth

https://codereview.webrtc.org/2703353002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:312:
Maintain120MsWhenFecIsOnAndVeryLowUplinkBandwidth) {
Maintain120MsOnVeryLowUplinkBandwidth

https://codereview.webrtc.org/2703353002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:314:
{20, 60, 120}, 120);
We set packet loss fraction to FlDecreasingPacketLossFraction, and FEC on, both
of which should have caused the frame length to decrease, if the uplink
bandwidth was not this low.

https://codereview.webrtc.org/2703353002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:322:
From60MsTo120MsWhenFecIsOnAndVeryLowUplinkBandwidth) {
From60MsTo120MsOnVeryLowUplinkBandwidth

https://codereview.webrtc.org/2703353002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:325:
UpdateNetworkMetrics(controller.get(), rtc::Optional<int>(VeryLowBitrate(60)),
add

We set packet loss fraction to FlDecreasingPacketLossFraction, and FEC on, both
of which should have prevented frame length to increase, if the uplink bandwidth
was not this low.

[michaelt, 2017-02-22 13:57:07]

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
File
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc
(right):

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:31:
int prevent_overuse_margin_bps,
On 2017/02/22 09:10:47, minyue-webrtc wrote:
> On 2017/02/21 15:56:41, michaelt wrote:
> > I put it here because of the testing. Other wise i would have to redefine
the
> > value for the test. 
> > 
> 
> I think redefine in test is acceptable. otherwise, you can make it a static
> public member. since I hope to keep this hard coded for now. Otherwise, it
> should have become a config parameter.

Acknowledged.

https://codereview.webrtc.org/2703353002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:102:
// 5. |uplink_bandwidth_bps| is known to be smaller or equal than
On 2017/02/22 09:10:47, minyue-webrtc wrote:
> On 2017/02/21 15:56:41, michaelt wrote:
> > On 2017/02/21 10:41:24, minyue-webrtc wrote:
> > > This can be misleading because of order of operations
> > 
> > Changed order
> 
> not sure if this is clearly enough. may add "all the following" after "OR"

Done.

https://codereview.webrtc.org/2703353002/diff/20001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc
(right):

https://codereview.webrtc.org/2703353002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:190:
// bandwidth is not this low.
On 2017/02/22 09:10:47, minyue-webrtc wrote:
> is -> was

Done.

https://codereview.webrtc.org/2703353002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:196:

The test already existed line 161 => moved it to this place.

https://codereview.webrtc.org/2703353002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:312:
Maintain120MsWhenFecIsOnAndVeryLowUplinkBandwidth) {
On 2017/02/22 09:10:47, minyue-webrtc wrote:
> Maintain120MsOnVeryLowUplinkBandwidth

Done.

https://codereview.webrtc.org/2703353002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:314:
{20, 60, 120}, 120);
On 2017/02/22 09:10:47, minyue-webrtc wrote:
> We set packet loss fraction to FlDecreasingPacketLossFraction, and FEC on,
both
> of which should have caused the frame length to decrease, if the uplink
> bandwidth was not this low.

Done.

https://codereview.webrtc.org/2703353002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:322:
From60MsTo120MsWhenFecIsOnAndVeryLowUplinkBandwidth) {
On 2017/02/22 09:10:47, minyue-webrtc wrote:
> From60MsTo120MsOnVeryLowUplinkBandwidth

Done.

https://codereview.webrtc.org/2703353002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:325:
UpdateNetworkMetrics(controller.get(), rtc::Optional<int>(VeryLowBitrate(60)),
On 2017/02/22 09:10:47, minyue-webrtc wrote:
> add
> 
> We set packet loss fraction to FlDecreasingPacketLossFraction, and FEC on,
both
> of which should have prevented frame length to increase, if the uplink
bandwidth
> was not this low.

Done.

[minyue-webrtc, 2017-02-22 14:01:19]

lgtm

[michaelt, 2017-02-22 14:05:58]

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

[commit-bot: I haz the power, 2017-02-22 14:06:08]

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-02-22 14:57:57]

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

[commit-bot: I haz the power, 2017-02-22 14:57:58]

Dry run: This issue passed the CQ dry run.

[michaelt, 2017-02-22 15:31:00]

The CQ bit was checked by michaelt@webrtc.org

[commit-bot: I haz the power, 2017-02-22 15:31:10]

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-02-22 15:35:06]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487777460042010,
"parent_rev": "6e5b2195d72fd469b7e222fd06d7612a09727b6f", "commit_rev":
"6f08d7d68d3ba41d65b61728266ad70391f009fc"}

[commit-bot: I haz the power, 2017-02-22 15:35:08]

Description was changed from

==========
Change frame length on very low bandwidth.

BUG=webrtc:7199
==========

to

==========
Change frame length on very low bandwidth.

BUG=webrtc:7199

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

[commit-bot: I haz the power, 2017-02-22 15:35:09]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/6f08d7d68d3ba41d65b617282...