Variable audio bitrate.

Variable audio bitrate.

This is a first CL wiring up AudioSendStream to BitrateAllocator. This
is still experimental and there is a test added for the audio only case,
combined audio video variable bitrate test cases will be added as a
follow up.

BUG=5079
Committed: https://crrev.com/86cc6ffc7ca14c79d760695e89ab9698ed251873
Cr-Commit-Position: refs/heads/master@{#13527}

[mflodman, 2016-07-20 10:00:38]

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

[mflodman, 2016-07-20 10:00:39]

This CL replaces the old placeholder and is intended to be submitted as a way to
experiment with variable audio bitrate and verify the functionality.

Minyue,
Can you take a look at this from the audio side?

Stefan,
As optional reviewer, feel free to remove yourself as a reviewer.

[stefan-webrtc, 2016-07-20 10:15:00]

https://codereview.webrtc.org/2165743003/diff/1/webrtc/audio/audio_send_strea...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/2165743003/diff/1/webrtc/audio/audio_send_strea...
webrtc/audio/audio_send_stream.cc:253: // Possible audio protection used is
ignored for now.
Maybe rewrite as:
TODO(mflodman): Return amount of audio protection used here.

https://codereview.webrtc.org/2165743003/diff/1/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2165743003/diff/1/webrtc/call/rampup_tests.cc#n...
webrtc/call/rampup_tests.cc:350: } else if (num_audio_streams_ > 0 &&
sender_call_ != nullptr) {
Do we have to protect sender_call_ since it's accessed from the stats thread? Or
maybe it's safe since the stats thread starts after the call is created?

https://codereview.webrtc.org/2165743003/diff/1/webrtc/call/rampup_tests.cc#n...
webrtc/call/rampup_tests.cc:413: suspended = true;
I'd prefer to have a bool called "check_for_suspended_streams" which is false
for audio only case.

https://codereview.webrtc.org/2165743003/diff/1/webrtc/call/rampup_tests.h
File webrtc/call/rampup_tests.h (right):

https://codereview.webrtc.org/2165743003/diff/1/webrtc/call/rampup_tests.h#ne...
webrtc/call/rampup_tests.h:67: Call* sender_call_  ;
Remove spaces at the end.

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

https://codereview.webrtc.org/2165743003/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:1143: config_.max_bitrate_kbps =
kOpusBitrateFb;
kOpusBitrateFb seems like a bad variable name. Could we rename it or in some way
clarify here what it means?

[minyue-webrtc, 2016-07-21 10:59:19]

thanks for the work. I have just some small comments.

https://codereview.webrtc.org/2165743003/diff/1/webrtc/audio/audio_send_strea...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/2165743003/diff/1/webrtc/audio/audio_send_strea...
webrtc/audio/audio_send_stream.cc:254: return 0;
do we need to return

max_bitrate_bps - bitrate_bps

for the case that bitrate is clamped by the max

https://codereview.webrtc.org/2165743003/diff/1/webrtc/audio/audio_send_stream.h
File webrtc/audio/audio_send_stream.h (right):

https://codereview.webrtc.org/2165743003/diff/1/webrtc/audio/audio_send_strea...
webrtc/audio/audio_send_stream.h:66: BitrateAllocator* const bitrate_allocator_;
use smart pointer if possible

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

https://codereview.webrtc.org/2165743003/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:1143: config_.max_bitrate_kbps =
kOpusBitrateFb;
On 2016/07/20 10:15:00, stefan-webrtc (holmer) wrote:
> kOpusBitrateFb seems like a bad variable name. Could we rename it or in some
way
> clarify here what it means?

is it possible to use kOpusMaxBitrate (although we may not go that high in
practice). or does max_bitrate_kbps make audio compete with video?

also add a comment "WebRTC-AdaptAudioBitrate is currently only available to
Opus"

https://codereview.webrtc.org/2165743003/diff/1/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/2165743003/diff/1/webrtc/test/call_test.cc#newc...
webrtc/test/call_test.cc:65:
receive_transport_.reset(test->CreateReceiveTransport());
what does this change do

https://codereview.webrtc.org/2165743003/diff/1/webrtc/voice_engine/channel_p...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/2165743003/diff/1/webrtc/voice_engine/channel_p...
webrtc/voice_engine/channel_proxy.cc:162: // May be called on different threads
and needs to be handled by the channel.
would you specify "different threads"?

[mflodman, 2016-07-22 13:50:29]

Thanks for the reviews, PTAL.

https://codereview.webrtc.org/2165743003/diff/1/webrtc/audio/audio_send_strea...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/2165743003/diff/1/webrtc/audio/audio_send_strea...
webrtc/audio/audio_send_stream.cc:253: // Possible audio protection used is
ignored for now.
On 2016/07/20 10:14:59, stefan-webrtc (holmer) wrote:
> Maybe rewrite as:
> TODO(mflodman): Return amount of audio protection used here.

This information isn't available to us and would require changes to audio
encoders and Opus to expose to AudioSendStream, work that there are no plans to
do when I asked. All this is currently handled internally by Opus.

I don't want to put a TODO here since there are no plans to actually do the
work.

I updated the comment though, looks ok?

https://codereview.webrtc.org/2165743003/diff/1/webrtc/audio/audio_send_strea...
webrtc/audio/audio_send_stream.cc:254: return 0;
On 2016/07/21 10:59:19, minyue-webrtc wrote:
> do we need to return
> 
> max_bitrate_bps - bitrate_bps
> 
> for the case that bitrate is clamped by the max

We should return the bitrate used for FEC, but we don't know that at this point
for Opus since that is all done internally in the encoder. So to do that
properly, we would need an API from the encoder to ask for the amount of FEC
used.

We can change this when Opus / ACM exposes this.

https://codereview.webrtc.org/2165743003/diff/1/webrtc/audio/audio_send_stream.h
File webrtc/audio/audio_send_stream.h (right):

https://codereview.webrtc.org/2165743003/diff/1/webrtc/audio/audio_send_strea...
webrtc/audio/audio_send_stream.h:66: BitrateAllocator* const bitrate_allocator_;
On 2016/07/21 10:59:19, minyue-webrtc wrote:
> use smart pointer if possible

The BitrateAllocator isn't owned by this class and the owner will always outlive
an AudioSendStream, hence no need to ref count it and I'd prefer to keep it this
way to show (lack of) ownership.

https://codereview.webrtc.org/2165743003/diff/1/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2165743003/diff/1/webrtc/call/rampup_tests.cc#n...
webrtc/call/rampup_tests.cc:350: } else if (num_audio_streams_ > 0 &&
sender_call_ != nullptr) {
On 2016/07/20 10:14:59, stefan-webrtc (holmer) wrote:
> Do we have to protect sender_call_ since it's accessed from the stats thread?
Or
> maybe it's safe since the stats thread starts after the call is created?

Discussed offline with Stefan, keeping as is.

https://codereview.webrtc.org/2165743003/diff/1/webrtc/call/rampup_tests.cc#n...
webrtc/call/rampup_tests.cc:413: suspended = true;
On 2016/07/20 10:14:59, stefan-webrtc (holmer) wrote:
> I'd prefer to have a bool called "check_for_suspended_streams" which is false
> for audio only case.

Ah, of course. Leftovers in a bad state...

https://codereview.webrtc.org/2165743003/diff/1/webrtc/call/rampup_tests.h
File webrtc/call/rampup_tests.h (right):

https://codereview.webrtc.org/2165743003/diff/1/webrtc/call/rampup_tests.h#ne...
webrtc/call/rampup_tests.h:67: Call* sender_call_  ;
On 2016/07/20 10:14:59, stefan-webrtc (holmer) wrote:
> Remove spaces at the end.

Done.

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

https://codereview.webrtc.org/2165743003/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:1143: config_.max_bitrate_kbps =
kOpusBitrateFb;
On 2016/07/21 10:59:19, minyue-webrtc wrote:
> On 2016/07/20 10:15:00, stefan-webrtc (holmer) wrote:
> > kOpusBitrateFb seems like a bad variable name. Could we rename it or in some
> way
> > clarify here what it means?
> 
> is it possible to use kOpusMaxBitrate (although we may not go that high in
> practice). or does max_bitrate_kbps make audio compete with video?

@minyue:
I didn't want to increase the bitrate more than used today at this stage, rather
focus on the the lower bitrate use case. And also like you said, keep the audio
bitrate and the video bitrate non-overlapping for now. 

Sounds ok for now and then we'll discuss and try this out together?


> also add a comment "WebRTC-AdaptAudioBitrate is currently only available to
> Opus"
Done.

https://codereview.webrtc.org/2165743003/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:1143: config_.max_bitrate_kbps =
kOpusBitrateFb;
On 2016/07/20 10:15:00, stefan-webrtc (holmer) wrote:
> kOpusBitrateFb seems like a bad variable name. Could we rename it or in some
way
> clarify here what it means?

@stefan:

There are three different Opus bitrate constants with: NB = Narrow Band, WB =
Wide Band and FB = Full Band. I'm not sure what to name it otherwise and I don't
want to go change that convention in this CL without approval from the audio
team that has chosen this.

OK?

https://codereview.webrtc.org/2165743003/diff/1/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/2165743003/diff/1/webrtc/test/call_test.cc#newc...
webrtc/test/call_test.cc:65:
receive_transport_.reset(test->CreateReceiveTransport());
On 2016/07/21 10:59:19, minyue-webrtc wrote:
> what does this change do

Reverted.

I did a few other changes I reverted before uploading, this one got left behind.

https://codereview.webrtc.org/2165743003/diff/1/webrtc/voice_engine/channel_p...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/2165743003/diff/1/webrtc/voice_engine/channel_p...
webrtc/voice_engine/channel_proxy.cc:162: // May be called on different threads
and needs to be handled by the channel.
On 2016/07/21 10:59:19, minyue-webrtc wrote:
> would you specify "different threads"?

There can be multiple threads calling BitrateAllocator::OnNetworkChanged, both
with a new incoming RTCP packet and the process thread. perkj is working on a
change to only call this on a specific task queue, but that is still in the
works.

I don't think it makes sense to document these details in the code for now.

[minyue-webrtc, 2016-07-25 14:33:47]

lgtm

https://codereview.webrtc.org/2165743003/diff/1/webrtc/audio/audio_send_strea...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/2165743003/diff/1/webrtc/audio/audio_send_strea...
webrtc/audio/audio_send_stream.cc:254: return 0;
On 2016/07/22 13:50:29, mflodman_OOO_July_18 wrote:
> On 2016/07/21 10:59:19, minyue-webrtc wrote:
> > do we need to return
> > 
> > max_bitrate_bps - bitrate_bps
> > 
> > for the case that bitrate is clamped by the max
> 
> We should return the bitrate used for FEC, but we don't know that at this
point
> for Opus since that is all done internally in the encoder. So to do that
> properly, we would need an API from the encoder to ask for the amount of FEC
> used.
> 
> We can change this when Opus / ACM exposes this.

OK, the comment on OnBitrateUpdated says
"
Returns the amount of protection used by the BitrateAllocatorObserve
"
I did not have a good understanding of "protection" in here. So you mean that it
is expected to return FEC usage within |bitrate_bps|? What is that used for
then?

It is true that Opus's FEC is included in bitrate_bps and no way to tell how
large it is for the moment.

[stefan-webrtc, 2016-07-25 15:06:20]

lgtm

https://codereview.webrtc.org/2165743003/diff/1/webrtc/audio/audio_send_strea...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/2165743003/diff/1/webrtc/audio/audio_send_strea...
webrtc/audio/audio_send_stream.cc:253: // Possible audio protection used is
ignored for now.
On 2016/07/22 13:50:29, mflodman_OOO_July_18 wrote:
> On 2016/07/20 10:14:59, stefan-webrtc (holmer) wrote:
> > Maybe rewrite as:
> > TODO(mflodman): Return amount of audio protection used here.
> 
> This information isn't available to us and would require changes to audio
> encoders and Opus to expose to AudioSendStream, work that there are no plans
to
> do when I asked. All this is currently handled internally by Opus.
> 
> I don't want to put a TODO here since there are no plans to actually do the
> work.
> 
> I updated the comment though, looks ok?

Acknowledged.

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

https://codereview.webrtc.org/2165743003/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:1143: config_.max_bitrate_kbps =
kOpusBitrateFb;
On 2016/07/22 13:50:29, mflodman_OOO_July_18 wrote:
> On 2016/07/20 10:15:00, stefan-webrtc (holmer) wrote:
> > kOpusBitrateFb seems like a bad variable name. Could we rename it or in some
> way
> > clarify here what it means?
> 
> @stefan:
> 
> There are three different Opus bitrate constants with: NB = Narrow Band, WB =
> Wide Band and FB = Full Band. I'm not sure what to name it otherwise and I
don't
> want to go change that convention in this CL without approval from the audio
> team that has chosen this.
> 
> OK?

I see. Probably good enough then, although I would have preferred
kOpusBitrateFullBand.

https://codereview.webrtc.org/2165743003/diff/20001/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2165743003/diff/20001/webrtc/call/rampup_tests....
webrtc/call/rampup_tests.cc:414: (suspended == check_suspend_state)) {
I'd prefer removing () around this.

[mflodman, 2016-07-26 09:50:52]

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

https://codereview.webrtc.org/2165743003/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:1143: config_.max_bitrate_kbps =
kOpusBitrateFb;
On 2016/07/25 15:06:20, stefan-webrtc (holmer) wrote:
> On 2016/07/22 13:50:29, mflodman_OOO_July_18 wrote:
> > On 2016/07/20 10:15:00, stefan-webrtc (holmer) wrote:
> > > kOpusBitrateFb seems like a bad variable name. Could we rename it or in
some
> > way
> > > clarify here what it means?
> > 
> > @stefan:
> > 
> > There are three different Opus bitrate constants with: NB = Narrow Band, WB
=
> > Wide Band and FB = Full Band. I'm not sure what to name it otherwise and I
> don't
> > want to go change that convention in this CL without approval from the audio
> > team that has chosen this.
> > 
> > OK?
> 
> I see. Probably good enough then, although I would have preferred
> kOpusBitrateFullBand.

I agree and I'll bring that up for discussion.

https://codereview.webrtc.org/2165743003/diff/20001/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2165743003/diff/20001/webrtc/call/rampup_tests....
webrtc/call/rampup_tests.cc:414: (suspended == check_suspend_state)) {
On 2016/07/25 15:06:20, stefan-webrtc (holmer) wrote:
> I'd prefer removing () around this.

Done.

[mflodman, 2016-07-26 10:02:34]

The CQ bit was checked by mflodman@webrtc.org

[mflodman, 2016-07-26 10:02:34]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org,
minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2165743003/#ps60001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-07-26 10:02:37]

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, 2016-07-26 11:44:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-26 11:44:21]

Description was changed from

==========
Variable audio bitrate.

This is a first CL wiring up AudioSendStream to BitrateAllocator. This
is still experimental and there is a test added for the audio only case,
combined audio video variable bitrate test cases will be added as a
follow up.

BUG=5079
==========

to

==========
Variable audio bitrate.

This is a first CL wiring up AudioSendStream to BitrateAllocator. This
is still experimental and there is a test added for the audio only case,
combined audio video variable bitrate test cases will be added as a
follow up.

BUG=5079

Committed: https://crrev.com/86cc6ffc7ca14c79d760695e89ab9698ed251873
Cr-Commit-Position: refs/heads/master@{#13527}
==========

[commit-bot: I haz the power, 2016-07-26 11:44:23]

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