Cap the send bitrate for opus and iSAC before passing down to VoE.

Cap the send bitrate for opus and iSAC before passing down to VoE.

The voice engine expects send bitrates no more than the maximum for the
codec. For example, 510kbps for opus. So if "b=AS" sets a maximum above
the codec maximum, WebRtcVoiceEngine needs to cap it.

BUG=603690
Committed: https://crrev.com/8034614b817810f33ad193d8fa5c460bc6c28276
Cr-Commit-Position: refs/heads/master@{#12537}

[Taylor Brandstetter, 2016-04-26 00:14:49]

deadbeef@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[Taylor Brandstetter, 2016-04-26 00:14:50]

solenberg@, PTAL

[the sun, 2016-04-26 09:35:06]

https://codereview.webrtc.org/1920123002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1920123002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:505: {kG722CodecName, 8000, 1, 9,
false, {10, 20, 30, 40, 50, 60}},
The max_bitrate_bps field is uninitialized for the codecs below, and this works
because the field is only retrieved when is_multi_rate is also set. I find that
brittle.

Could we combine the IsCodecMultiRate() and MaxBitrateBps() iterations into one
function, so the link between these fields is made more obvious?

[Taylor Brandstetter, 2016-04-26 17:14:04]

https://codereview.webrtc.org/1920123002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1920123002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:505: {kG722CodecName, 8000, 1, 9,
false, {10, 20, 30, 40, 50, 60}},
On 2016/04/26 09:35:06, the sun wrote:
> The max_bitrate_bps field is uninitialized for the codecs below, and this
works
> because the field is only retrieved when is_multi_rate is also set. I find
that
> brittle.
> 
> Could we combine the IsCodecMultiRate() and MaxBitrateBps() iterations into
one
> function, so the link between these fields is made more obvious?

The max_bitrate_bps field is initialized to 0, it's just not explicit. Would you
prefer if it was made explicit (by adding ", 0")?

As for combining IsCodecMultiRate and MaxBitrateBps, should the combined
function return an int (or rtc::Optional<int>), where a value of 0 (or null)
means "not multi-rate"?

[the sun, 2016-04-26 18:33:34]

https://codereview.webrtc.org/1920123002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1920123002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:505: {kG722CodecName, 8000, 1, 9,
false, {10, 20, 30, 40, 50, 60}},
On 2016/04/26 17:14:04, Taylor Brandstetter wrote:
> On 2016/04/26 09:35:06, the sun wrote:
> > The max_bitrate_bps field is uninitialized for the codecs below, and this
> works
> > because the field is only retrieved when is_multi_rate is also set. I find
> that
> > brittle.
> > 
> > Could we combine the IsCodecMultiRate() and MaxBitrateBps() iterations into
> one
> > function, so the link between these fields is made more obvious?
> 
> The max_bitrate_bps field is initialized to 0, it's just not explicit. Would
you
> prefer if it was made explicit (by adding ", 0")?

No need to make that explicit. The issue to me is that unless multi_rate is set,
you'll set the codec.rate to min(.., 0).

> As for combining IsCodecMultiRate and MaxBitrateBps, should the combined
> function return an int (or rtc::Optional<int>), where a value of 0 (or null)
> means "not multi-rate"?

I think I'd go for an rtc::Optional<int>, as long as the resulting code is nice
and succinct.

[the sun, 2016-04-26 18:45:33]

On 2016/04/26 18:33:34, the sun wrote:
>
https://codereview.webrtc.org/1920123002/diff/20001/webrtc/media/engine/webrt...
> File webrtc/media/engine/webrtcvoiceengine.cc (right):
> 
>
https://codereview.webrtc.org/1920123002/diff/20001/webrtc/media/engine/webrt...
> webrtc/media/engine/webrtcvoiceengine.cc:505: {kG722CodecName, 8000, 1, 9,
> false, {10, 20, 30, 40, 50, 60}},
> On 2016/04/26 17:14:04, Taylor Brandstetter wrote:
> > On 2016/04/26 09:35:06, the sun wrote:
> > > The max_bitrate_bps field is uninitialized for the codecs below, and this
> > works
> > > because the field is only retrieved when is_multi_rate is also set. I find
> > that
> > > brittle.
> > > 
> > > Could we combine the IsCodecMultiRate() and MaxBitrateBps() iterations
into
> > one
> > > function, so the link between these fields is made more obvious?
> > 
> > The max_bitrate_bps field is initialized to 0, it's just not explicit. Would
> you
> > prefer if it was made explicit (by adding ", 0")?
> 
> No need to make that explicit. The issue to me is that unless multi_rate is
set,
> you'll set the codec.rate to min(.., 0).

Sorry, that was a bit confused. What I meant was that codec.rate would be set to
0, unless there was a condition on multi_rate to be set, so there's an implicit
dependency on multi_rate.

> > As for combining IsCodecMultiRate and MaxBitrateBps, should the combined
> > function return an int (or rtc::Optional<int>), where a value of 0 (or null)
> > means "not multi-rate"?
> 
> I think I'd go for an rtc::Optional<int>, as long as the resulting code is
nice
> and succinct.

[Taylor Brandstetter, 2016-04-26 20:46:44]

Alright, take a look. I still think it may be clearer if there were separate
"IsMultiRate" and "MaxBitrate" functions but I'll let you decide. And I'm sure
the name of the combined function could be improved.

[The Sun (google.com), 2016-04-26 22:18:07]

solenberg@google.com changed reviewers:
+ solenberg@google.com

[The Sun (google.com), 2016-04-26 22:18:08]

https://codereview.webrtc.org/1920123002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1920123002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:426: // Nonempty if it's a multirate
codec.
Ah, what I had in mind was just to make the return type of the combined function
an Optional. I agree that keeping an Optional in CodecPref makes things rather
messy.

You know what, just revert and we'll go with your initial two-function version.

[Taylor Brandstetter, 2016-04-26 23:00:38]

On 2016/04/26 22:18:08, The Sun (google.com) wrote:
>
https://codereview.webrtc.org/1920123002/diff/40001/webrtc/media/engine/webrt...
> File webrtc/media/engine/webrtcvoiceengine.cc (right):
> 
>
https://codereview.webrtc.org/1920123002/diff/40001/webrtc/media/engine/webrt...
> webrtc/media/engine/webrtcvoiceengine.cc:426: // Nonempty if it's a multirate
> codec.
> Ah, what I had in mind was just to make the return type of the combined
function
> an Optional. I agree that keeping an Optional in CodecPref makes things rather
> messy.
> 
> You know what, just revert and we'll go with your initial two-function
version.

Ok, reverted.

[the sun, 2016-04-27 08:06:18]

solenberg@webrtc.org changed reviewers:
- solenberg@google.com

[the sun, 2016-04-27 08:06:18]

lgtm

[Taylor Brandstetter, 2016-04-27 19:51:58]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2016-04-27 19:52:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1920123002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1920123002/60001

[commit-bot: I haz the power, 2016-04-27 21:17:15]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-01 22:00:59]

Description was changed from

==========
Cap the send bitrate for opus and iSAC before passing down to VoE.

The voice engine expects send bitrates no more than the maximum for the
codec. For example, 510kbps for opus. So if "b=AS" sets a maximum above
the codec maximum, WebRtcVoiceEngine needs to cap it.

BUG=603690
==========

to

==========
Cap the send bitrate for opus and iSAC before passing down to VoE.

The voice engine expects send bitrates no more than the maximum for the
codec. For example, 510kbps for opus. So if "b=AS" sets a maximum above
the codec maximum, WebRtcVoiceEngine needs to cap it.

BUG=603690

Committed: https://crrev.com/8034614b817810f33ad193d8fa5c460bc6c28276
Cr-Commit-Position: refs/heads/master@{#12537}
==========

[commit-bot: I haz the power, 2016-05-01 22:01:00]

Patchset 4 (id:??) landed as
https://crrev.com/8034614b817810f33ad193d8fa5c460bc6c28276
Cr-Commit-Position: refs/heads/master@{#12537}