Make sure min and max bitrate is always set for audio.

Make sure min and max bitrate is always set for audio.

BUG=webrtc:5079
Review-Url: https://codereview.webrtc.org/2654503005
Cr-Commit-Position: refs/heads/master@{#16250}
Committed: https://chromium.googlesource.com/external/webrtc/+/e9f36d55dadda72dd31ab0b130b1441fef1397a4

[stefan-webrtc, 2017-01-24 11:55:31]

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

[stefan-webrtc, 2017-01-24 11:55:31]

PTAL, I believe there's a possible bug here if there are no packet_sizes. Then
no max and min bitrate will be set, and not the defaults.

[stefan-webrtc, 2017-01-24 11:56:29]

Description was changed from

==========
Make sure min and max bitrate is always set for audio, irrespective of
experiments.

BUG=webrtc:5079
==========

to

==========
Make sure min and max bitrate is always set for audio.

BUG=webrtc:5079
==========

[minyue-webrtc, 2017-01-24 14:22:00]

https://codereview.webrtc.org/2654503005/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine.cc (left):

https://codereview.webrtc.org/2654503005/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:1491: config_.min_bitrate_bps =
kOpusMinBitrateBps;
Then it should be outside if(webrtc::field_trial::FindFullName...) rather than
in else-branch.

But you know what, I think it might even need to just remove the outer if
(SendSideBwe).

WithOverhead can be set without SendSideBwe, right?

[stefan-webrtc, 2017-01-24 14:48:03]

https://codereview.webrtc.org/2654503005/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine.cc (left):

https://codereview.webrtc.org/2654503005/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:1491: config_.min_bitrate_bps =
kOpusMinBitrateBps;
On 2017/01/24 14:22:00, minyue-webrtc wrote:
> Then it should be outside if(webrtc::field_trial::FindFullName...) rather than
> in else-branch.

I don't follow, I moved it from the else branch to outside line 1462. Do you
mean that I should move it to line 1456? I could, but it doesn't have any effect
without BWE.

> 
> But you know what, I think it might even need to just remove the outer if
> (SendSideBwe).
> 
> WithOverhead can be set without SendSideBwe, right?

It can, but I'm not aware of any plans to do so, and I don't think it would have
any effect?

[minyue-webrtc, 2017-01-24 15:10:58]

On 2017/01/24 14:48:03, stefan-webrtc wrote:
>
https://codereview.webrtc.org/2654503005/diff/1/webrtc/media/engine/webrtcvoi...
> File webrtc/media/engine/webrtcvoiceengine.cc (left):
> 
>
https://codereview.webrtc.org/2654503005/diff/1/webrtc/media/engine/webrtcvoi...
> webrtc/media/engine/webrtcvoiceengine.cc:1491: config_.min_bitrate_bps =
> kOpusMinBitrateBps;
> On 2017/01/24 14:22:00, minyue-webrtc wrote:
> > Then it should be outside if(webrtc::field_trial::FindFullName...) rather
than
> > in else-branch.
> 
> I don't follow, I moved it from the else branch to outside line 1462. Do you
> mean that I should move it to line 1456? I could, but it doesn't have any
effect
> without BWE.
> 
> > 
> > But you know what, I think it might even need to just remove the outer if
> > (SendSideBwe).
> > 
> > WithOverhead can be set without SendSideBwe, right?
> 
> It can, but I'm not aware of any plans to do so, and I don't think it would
have
> any effect?

I was confused by myself when reviewing it. Apologies.

As you said, the change may not have any effect, but it makes the code clearer.
LGTM.

[michaelt, 2017-01-24 15:12:37]

lgtm

[stefan-webrtc, 2017-01-24 15:15:20]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2017-01-24 15:15:24]

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-01-24 16:18:46]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1485270920156310, "parent_rev":
"e256bc582a9feeeccfa8b932b3dafe8ab81401c6", "commit_rev":
"e9f36d55dadda72dd31ab0b130b1441fef1397a4"}

[commit-bot: I haz the power, 2017-01-24 16:18:48]

Description was changed from

==========
Make sure min and max bitrate is always set for audio.

BUG=webrtc:5079
==========

to

==========
Make sure min and max bitrate is always set for audio.

BUG=webrtc:5079

Review-Url: https://codereview.webrtc.org/2654503005
Cr-Commit-Position: refs/heads/master@{#16250}
Committed:
https://chromium.googlesource.com/external/webrtc/+/e9f36d55dadda72dd31ab0b13...
==========

[commit-bot: I haz the power, 2017-01-24 16:18:49]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/external/webrtc/+/e9f36d55dadda72dd31ab0b13...