Add AudioEncoder::GetTargetBitrate

Add AudioEncoder::GetTargetBitrate

The GetTargetBitrate implementation will return the
target bitrate of the codec. This may differ from the
desired target bitrate, as set by SetTargetBitrate, depending on implementation.

Tests are updated to exercise the new functionality.

R=kwiberg@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/3e89dbf45835896c8fd89f235f396d03bc2e6065

[hlundin-webrtc, 2015-06-16 09:29:45]

henrik.lundin@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[hlundin-webrtc, 2015-06-16 09:30:03]

Karl,

Please, take a look at this cl.

Thanks!

[hlundin-webrtc, 2015-06-16 10:03:25]

Getting rid of erroneous code

[kwiberg-webrtc, 2015-06-16 19:22:54]

General note: I thought you also wanted to log the bitrate in places where
SetTargetBitrate isn't called, so a GetTargetBitrate would have been better.

https://codereview.webrtc.org/1184313002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right):

https://codereview.webrtc.org/1184313002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:63: return
full_frame_samples_;
Oooh. This isn't correct for pcm16b, so needs to be overridden there.
Alternatively, have a BytesPerSample() virtual method here that both
MaxEncodedBytes and SetTargetBitrate can use.

https://codereview.webrtc.org/1184313002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:75: return 64000 *
NumChannels();
See suggestion above about how this can be made to work for pcm16b as well.

https://codereview.webrtc.org/1184313002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc (right):

https://codereview.webrtc.org/1184313002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc:86: return 64000 *
NumChannels();
16 kHz times 4 bits per sample, right?

https://codereview.webrtc.org/1184313002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc (right):

https://codereview.webrtc.org/1184313002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc:72: case 3: case
6:  return 13333;
I'd recommend an explanation here. Otherwise these will be very hard to verify
or change.

https://codereview.webrtc.org/1184313002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h
(right):

https://codereview.webrtc.org/1184313002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:139: return
target_bitrate_bps_;
Just "return -1" might be clearer. Also, document in the interface what -1
means.

[hlundin-webrtc, 2015-06-16 21:31:42]

Taking care of first round of comments

[hlundin-webrtc, 2015-06-16 21:38:51]

Thanks, Karl.

I added a new GetTargetBitrate method, but the Set... method still returns a
value too. Do you think I should return the Set method to be void?

https://codereview.webrtc.org/1184313002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right):

https://codereview.webrtc.org/1184313002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:63: return
full_frame_samples_;
On 2015/06/16 19:22:54, kwiberg1 wrote:
> Oooh. This isn't correct for pcm16b, so needs to be overridden there.
> Alternatively, have a BytesPerSample() virtual method here that both
> MaxEncodedBytes and SetTargetBitrate can use.

Done.

https://codereview.webrtc.org/1184313002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:75: return 64000 *
NumChannels();
On 2015/06/16 19:22:54, kwiberg1 wrote:
> See suggestion above about how this can be made to work for pcm16b as well.

Done.

https://codereview.webrtc.org/1184313002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc (right):

https://codereview.webrtc.org/1184313002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc:86: return 64000 *
NumChannels();
On 2015/06/16 19:22:54, kwiberg1 wrote:
> 16 kHz times 4 bits per sample, right?

Acknowledged.

https://codereview.webrtc.org/1184313002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc (right):

https://codereview.webrtc.org/1184313002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc:72: case 3: case
6:  return 13333;
On 2015/06/16 19:22:54, kwiberg1 wrote:
> I'd recommend an explanation here. Otherwise these will be very hard to verify
> or change.

Done.

[kwiberg-webrtc, 2015-06-17 07:56:04]

On 2015/06/16 21:38:51, hlundin-webrtc wrote:
> I added a new GetTargetBitrate method, but the Set... method still returns a
> value too. Do you think I should return the Set method to be void?

From offline discussion: yes.

[hlundin-webrtc, 2015-06-17 13:33:52]

Reverting to SetTargetBitrate returning void

[hlundin-webrtc, 2015-06-17 13:34:20]

Patchset #4 (id:60001) has been deleted

[hlundin-webrtc, 2015-06-17 13:46:41]

I removed the return value from SetTargetBitrate.

PTAL again.

[kwiberg-webrtc, 2015-06-17 14:36:31]

lgtm with nits

https://codereview.webrtc.org/1184313002/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h (right):

https://codereview.webrtc.org/1184313002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h:117: int
target_bitrate_bps_ GUARDED_BY(lock_);
The lock isn't necessary, strictly speaking, since this is only written to in
the constructor. Is there some sort of annotation that can capture that? If not,
don't bother changing it.

https://codereview.webrtc.org/1184313002/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_mutable_opus_test.cc
(right):

https://codereview.webrtc.org/1184313002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_mutable_opus_test.cc:109:
}
This test is repetitive now, since GetTargetBitrate and bitrate_bps do exactly
the same thing.

https://codereview.webrtc.org/1184313002/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/interface/audio_encoder_opus.h
(right):

https://codereview.webrtc.org/1184313002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/interface/audio_encoder_opus.h:62: int
bitrate_bps() const { return bitrate_bps_; }
Do you really need this anymore? GetTargetBitrate does exactly the same thing.

[hlundin-webrtc, 2015-06-18 07:46:11]

Second round of review tackled

[hlundin-webrtc, 2015-06-18 08:03:39]

Second round of review tackled

[hlundin-webrtc, 2015-06-18 08:06:38]

Patchset #5 (id:100001) has been deleted

[hlundin-webrtc, 2015-06-18 08:41:32]

Second round of review tackled

[hlundin-webrtc, 2015-06-18 08:42:08]

Rename bit_rate to bitrate

[hlundin-webrtc, 2015-06-18 08:42:19]

Patchset #5 (id:120001) has been deleted

[hlundin-webrtc, 2015-06-18 08:51:14]

Patchset #6 (id:160001) has been deleted

[hlundin-webrtc, 2015-06-18 08:52:18]

https://codereview.webrtc.org/1184313002/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h (right):

https://codereview.webrtc.org/1184313002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h:117: int
target_bitrate_bps_ GUARDED_BY(lock_);
On 2015/06/17 14:36:30, kwiberg1 wrote:
> The lock isn't necessary, strictly speaking, since this is only written to in
> the constructor. Is there some sort of annotation that can capture that? If
not,
> don't bother changing it.

Done.

https://codereview.webrtc.org/1184313002/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_mutable_opus_test.cc
(right):

https://codereview.webrtc.org/1184313002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_mutable_opus_test.cc:109:
}
On 2015/06/17 14:36:31, kwiberg1 wrote:
> This test is repetitive now, since GetTargetBitrate and bitrate_bps do exactly
> the same thing.

Done.

https://codereview.webrtc.org/1184313002/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/interface/audio_encoder_opus.h
(right):

https://codereview.webrtc.org/1184313002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/interface/audio_encoder_opus.h:62: int
bitrate_bps() const { return bitrate_bps_; }
On 2015/06/17 14:36:31, kwiberg1 wrote:
> Do you really need this anymore? GetTargetBitrate does exactly the same thing.

Done.

[kwiberg-webrtc, 2015-06-18 09:39:33]

lgtm

[hlundin-webrtc, 2015-06-18 12:58:53]

Committed patchset #6 (id:180001) manually as
3e89dbf45835896c8fd89f235f396d03bc2e6065 (presubmit successful).