Allow applications to control audio send bitrate through RtpParameters.

Allow applications to control audio send bitrate through RtpParameters.

This change builds on top of the refactoring in https://codereview.webrtc.org/1841083008/, and enables WebRTC client applications to control the max send bitrate for every audio stream through RtpParameters.

The AudioSendStream now stores the last codec spec, and whenever a global or per-stream bitrate limit changes, the effective limit (smaller of the two) is recomputed and the codec is reconfigured with that bitrate.

TBR=pthatcher
BUG=

Committed: https://crrev.com/e0d4637bea1c5914de73036006c2e4a34e297791
Cr-Commit-Position: refs/heads/master@{#12290}

[skvlad, 2016-04-01 20:03:06]

skvlad@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, mflodman@webrtc.org, pthatcher@webrtc.org,
solenberg@webrtc.org

[skvlad, 2016-04-04 20:50:48]

Redesigned the change to keep the logic inside WebRtcVoiceMediaChannel, as
requested by solenberg@.

[Taylor Brandstetter, 2016-04-05 22:05:07]

lgtm with one trivial comment

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

https://codereview.webrtc.org/1847353004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:862: //  that we apply codecs
properly.
Add back the word "Test" here

[the sun, 2016-04-06 09:27:41]

Looks good - just some minor comments!

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

https://codereview.webrtc.org/1847353004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2344: for (const auto& ch :
send_streams_) {
Great to remove this!

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

https://codereview.webrtc.org/1847353004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1204: webrtc::RtpParameters
rtp_parameters() const { return rtp_parameters_; }
Return a const &

https://codereview.webrtc.org/1847353004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1206: void set_rtp_parameters(const
webrtc::RtpParameters& parameters) {
RTC_DCHECK_EQ(1, parameters.encodings.size());
or is it LE?

https://codereview.webrtc.org/1847353004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1745: SetSendBitrate(channel,
Add:
TODO(solenberg): SetSendBitrate() yields another call to SetSendCodec(). Check
if it is possible to fuse with the previous call in this function.

https://codereview.webrtc.org/1847353004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2377: for (const auto& ch :
send_streams_) {
super nit: use 'kv' for the variable instead (sorry, the code isn't all updated
to reflect we're iterating over ssrc+stream key-value pairs, instead of
channels, as it use to be).

https://codereview.webrtc.org/1847353004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2389: bool
WebRtcVoiceMediaChannel::SetSendBitrate(int channel, int bps) {
How about you send in the const RtpParameters& here so you don't have to repeat
the channel_bitrate computation?

[skvlad, 2016-04-07 00:50:50]

I've addressed the code review comments. Please take another look when you have
time.

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

https://codereview.webrtc.org/1847353004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1204: webrtc::RtpParameters
rtp_parameters() const { return rtp_parameters_; }
On 2016/04/06 09:27:41, the sun wrote:
> Return a const &

Done. It's a bit dangerous though - if the caller stores the reference or gets a
pointer to it, it may change unexpectedly.

https://codereview.webrtc.org/1847353004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1206: void set_rtp_parameters(const
webrtc::RtpParameters& parameters) {
On 2016/04/06 09:27:41, the sun wrote:
> RTC_DCHECK_EQ(1, parameters.encodings.size());
> or is it LE?

It's supposed to be exactly 1 at the moment. Added the check.

https://codereview.webrtc.org/1847353004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1745: SetSendBitrate(channel,
On 2016/04/06 09:27:41, the sun wrote:
> Add:
> TODO(solenberg): SetSendBitrate() yields another call to SetSendCodec(). Check
> if it is possible to fuse with the previous call in this function.

Done.

https://codereview.webrtc.org/1847353004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2377: for (const auto& ch :
send_streams_) {
On 2016/04/06 09:27:41, the sun wrote:
> super nit: use 'kv' for the variable instead (sorry, the code isn't all
updated
> to reflect we're iterating over ssrc+stream key-value pairs, instead of
> channels, as it use to be).

Done.

https://codereview.webrtc.org/1847353004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2389: bool
WebRtcVoiceMediaChannel::SetSendBitrate(int channel, int bps) {
On 2016/04/06 09:27:41, the sun wrote:
> How about you send in the const RtpParameters& here so you don't have to
repeat
> the channel_bitrate computation?

Done.

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

https://codereview.webrtc.org/1847353004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:862: //  that we apply codecs
properly.
On 2016/04/05 22:05:07, Taylor Brandstetter wrote:
> Add back the word "Test" here

Done.

[skvlad, 2016-04-07 00:54:43]

Patchset #3 (id:40001) has been deleted

[the sun, 2016-04-07 08:22:21]

Looks good, thanks for your patience!

Test coverage is good too, but would it be useful to test the feature with two
separate streams?

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

https://codereview.webrtc.org/1847353004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1204: webrtc::RtpParameters
rtp_parameters() const { return rtp_parameters_; }
On 2016/04/07 00:50:49, skvlad wrote:
> On 2016/04/06 09:27:41, the sun wrote:
> > Return a const &
> 
> Done. It's a bit dangerous though - if the caller stores the reference or gets
a
> pointer to it, it may change unexpectedly.

True, which is why I didn't comment on WVoMC::GetRtpParameters() returning by
value. This class is internal to WVoMC though so we can be a bit more picky with
what to expect of the user.

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

https://codereview.webrtc.org/1847353004/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1747: SetSendBitrate(channel,
Can we use SetChannelParameters() here as well?

[skvlad, 2016-04-08 03:46:56]

I've added unit tests for multiple streams. Hope it's all good now.

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

https://codereview.webrtc.org/1847353004/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1747: SetSendBitrate(channel,
On 2016/04/07 08:22:21, the sun wrote:
> Can we use SetChannelParameters() here as well?

Oops, somehow I missed this call.

[the sun, 2016-04-08 05:31:09]

lgtm

[skvlad, 2016-04-08 05:42:24]

The CQ bit was checked by skvlad@webrtc.org

[skvlad, 2016-04-08 05:42:25]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1847353004/#ps80001 (title:
"More CR feedback")

[commit-bot: I haz the power, 2016-04-08 05:42:34]

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

[skvlad, 2016-04-08 05:50:22]

The CQ bit was unchecked by skvlad@webrtc.org

[skvlad, 2016-04-08 05:50:50]

Description was changed from

==========
Allow applications to control audio send bitrate through RtpParameters.

This change builds on top of the refactoring in
https://codereview.webrtc.org/1841083008/, and enables WebRTC client
applications to control the max send bitrate for every audio stream through
RtpParameters.

The AudioSendStream now stores the last codec spec, and whenever a global or
per-stream bitrate limit changes, the effective limit (smaller of the two) is
recomputed and the codec is reconfigured with that bitrate.

BUG=
==========

to

==========
Allow applications to control audio send bitrate through RtpParameters.

This change builds on top of the refactoring in
https://codereview.webrtc.org/1841083008/, and enables WebRTC client
applications to control the max send bitrate for every audio stream through
RtpParameters.

The AudioSendStream now stores the last codec spec, and whenever a global or
per-stream bitrate limit changes, the effective limit (smaller of the two) is
recomputed and the codec is reconfigured with that bitrate.

TBR=pthatcher
BUG=
==========

[skvlad, 2016-04-08 05:53:46]

The CQ bit was checked by skvlad@webrtc.org

[commit-bot: I haz the power, 2016-04-08 05:53:55]

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

[commit-bot: I haz the power, 2016-04-08 05:59:29]

Description was changed from

==========
Allow applications to control audio send bitrate through RtpParameters.

This change builds on top of the refactoring in
https://codereview.webrtc.org/1841083008/, and enables WebRTC client
applications to control the max send bitrate for every audio stream through
RtpParameters.

The AudioSendStream now stores the last codec spec, and whenever a global or
per-stream bitrate limit changes, the effective limit (smaller of the two) is
recomputed and the codec is reconfigured with that bitrate.

TBR=pthatcher
BUG=
==========

to

==========
Allow applications to control audio send bitrate through RtpParameters.

This change builds on top of the refactoring in
https://codereview.webrtc.org/1841083008/, and enables WebRTC client
applications to control the max send bitrate for every audio stream through
RtpParameters.

The AudioSendStream now stores the last codec spec, and whenever a global or
per-stream bitrate limit changes, the effective limit (smaller of the two) is
recomputed and the codec is reconfigured with that bitrate.

TBR=pthatcher
BUG=
==========

[commit-bot: I haz the power, 2016-04-08 05:59:30]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-04-08 05:59:37]

Description was changed from

==========
Allow applications to control audio send bitrate through RtpParameters.

This change builds on top of the refactoring in
https://codereview.webrtc.org/1841083008/, and enables WebRTC client
applications to control the max send bitrate for every audio stream through
RtpParameters.

The AudioSendStream now stores the last codec spec, and whenever a global or
per-stream bitrate limit changes, the effective limit (smaller of the two) is
recomputed and the codec is reconfigured with that bitrate.

TBR=pthatcher
BUG=
==========

to

==========
Allow applications to control audio send bitrate through RtpParameters.

This change builds on top of the refactoring in
https://codereview.webrtc.org/1841083008/, and enables WebRTC client
applications to control the max send bitrate for every audio stream through
RtpParameters.

The AudioSendStream now stores the last codec spec, and whenever a global or
per-stream bitrate limit changes, the effective limit (smaller of the two) is
recomputed and the codec is reconfigured with that bitrate.

TBR=pthatcher
BUG=

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

[commit-bot: I haz the power, 2016-04-08 05:59:38]

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

[pthatcher1, 2016-04-12 18:04:23]

https://codereview.webrtc.org/1847353004/diff/80001/webrtc/api/webrtcsession_...
File webrtc/api/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1847353004/diff/80001/webrtc/api/webrtcsession_...
webrtc/api/webrtcsession_unittest.cc:3397: EXPECT_EQ(1,
params.encodings.size());
This should be ASSERT_EQ since the next line may crash if it's false.

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

https://codereview.webrtc.org/1847353004/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2400: return true;
{}s please

https://codereview.webrtc.org/1847353004/diff/80001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1847353004/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.h:213: bool SetSendCodecs(int channel,
const webrtc::RtpParameters& rtp_parameters);
Should this be SetChannelSendCodecsAndParameters?

https://codereview.webrtc.org/1847353004/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.h:231: const webrtc::RtpParameters&
parameters);
Shouldn't this be called SetChannelSendParameters?

https://codereview.webrtc.org/1847353004/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.h:232: bool SetSendBitrate(int channel,
int bps);
And should this be SetChannelSendBitrate?