Adding cbr support for Opus

Adding cbr support for Opus

BUG=webrtc:7394
Review-Url: https://codereview.webrtc.org/2772773002
Cr-Commit-Position: refs/heads/master@{#17564}
Committed: https://chromium.googlesource.com/external/webrtc/+/28dc285f2208ecbb3b79213ca8ccb608d46af654

[squashingskak, 2017-03-23 08:21:56]

Patch for adding cbr (Constant bitrate) support for Opus

[the sun, 2017-03-23 08:46:44]

solenberg@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, minyue@webrtc.org, ossu@webrtc.org,
solenberg@webrtc.org

[the sun, 2017-03-23 08:46:45]

Nice, great job!

- One comment about the VoECodec APIs.
- Adding minyue@ for checking the Opus integration bits - is CBR something we
aim to support?
- Adding ossu@ for heads up - this will need to be merged into the injectable
encoders CLs.
- Adding henrik.lundin@ as FYI.

https://codereview.webrtc.org/2772773002/diff/1/webrtc/voice_engine/include/v...
File webrtc/voice_engine/include/voe_codec.h (right):

https://codereview.webrtc.org/2772773002/diff/1/webrtc/voice_engine/include/v...
webrtc/voice_engine/include/voe_codec.h:142: virtual int SetOpusCbr(int channel,
bool enable_cbr) = 0;
The legacy VoE APIs (e.g. VoECodec) are in the process of being removed. We
shouldn't add new functions to them.

[henrika_webrtc, 2017-03-23 08:47:01]

Description was changed from

==========
Adding cbr support for Opus

fixing too long line


Fixing white spaces


Fixing white spaces


fixing audio_send_stream_unittest


updated AUTHORS file


added Opus Cbr support

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

to

==========
Adding cbr support for Opus

fixing too long line


Fixing white spaces


Fixing white spaces


fixing audio_send_stream_unittest


updated AUTHORS file


added Opus Cbr support

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

[henrika_webrtc, 2017-03-23 08:47:01]

henrika@webrtc.org changed reviewers:
+ tina.legrand@webrtc.org
- henrik.lundin@webrtc.org, ossu@webrtc.org, solenberg@webrtc.org

[tommi, 2017-03-23 09:02:02]

Can you fix the description before landing?

[squashingskak, 2017-03-23 09:03:22]

I was looking at how SetOpusDtx was done and that is why it was added to the
VoeCodec. I think it is not needed as the path will be ( after parsing the SDP)
:
audio_send_stream -> channel_proxy -> channel -> audio_coding_module

[squashingskak, 2017-03-23 09:09:26]

@tommi (webrtc)	

Can you point me to what has to be changed in the description? I used the
standard git cl that inclueded all my git commit messages. Do I have to create a
bug somewhere else to but on the BUG= line?

I am not familiar with the term "landing" what does that mean?

[minyue-webrtc, 2017-03-23 09:23:22]

On 2017/03/23 09:09:26, squashingskak wrote
> @tommi (webrtc)	
> 
> Can you point me to what has to be changed in the description? I used the
> standard git cl that inclueded all my git commit messages. Do I have to create
a
> bug somewhere else to but on the BUG= line?
> 
> I am not familiar with the term "landing" what does that mean?

Would you create a bug in https://bugs.chromium.org/p/webrtc/issues/list and put
the bug id after BUG=

[squashingskak, 2017-03-23 09:33:13]

Description was changed from

==========
Adding cbr support for Opus

fixing too long line


Fixing white spaces


Fixing white spaces


fixing audio_send_stream_unittest


updated AUTHORS file


added Opus Cbr support

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

to

==========
Adding cbr support for Opus

BUG=7294
==========

[tommi, 2017-03-23 09:38:58]

On 2017/03/23 09:09:26, squashingskak wrote:
> @tommi (webrtc)	
> 
> Can you point me to what has to be changed in the description? I used the
> standard git cl that inclueded all my git commit messages. Do I have to create
a
> bug somewhere else to but on the BUG= line?
> 
> I am not familiar with the term "landing" what does that mean?

The description should explain what the change does. At the top a single short
heading, 70 chars or less would be good, as it's often used in single line git
logs.  Here's some more detail with good recommendations:
http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

My personal preference is also to not write the CL description from the
perspective of the committer  (as in "[I'm] adding") but from the perspective of
the change itself ("Add" or "Adds"). When sending out a code review request,
there's another field that is explicitly for communicating with reviewers and
that's where I personally like to keep conversation type language.

Leave out details of each and every local commit you might keep for yourself
when writing the patch, such as "Fixing white spaces".

[tommi, 2017-03-23 09:39:17]

On 2017/03/23 09:38:58, tommi (webrtc) wrote:
> On 2017/03/23 09:09:26, squashingskak wrote:
> > @tommi (webrtc)	
> > 
> > Can you point me to what has to be changed in the description? I used the
> > standard git cl that inclueded all my git commit messages. Do I have to
create
> a
> > bug somewhere else to but on the BUG= line?
> > 
> > I am not familiar with the term "landing" what does that mean?
> 
> The description should explain what the change does. At the top a single short
> heading, 70 chars or less would be good, as it's often used in single line git
> logs.  Here's some more detail with good recommendations:
> http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
> 
> My personal preference is also to not write the CL description from the
> perspective of the committer  (as in "[I'm] adding") but from the perspective
of
> the change itself ("Add" or "Adds"). When sending out a code review request,
> there's another field that is explicitly for communicating with reviewers and
> that's where I personally like to keep conversation type language.
> 
> Leave out details of each and every local commit you might keep for yourself
> when writing the patch, such as "Fixing white spaces".

Oh and 'landing' is the same as 'committing' :)

[minyue-webrtc, 2017-03-23 10:35:29]

On 2017/03/23 09:39:17, tommi (webrtc) wrote:
> On 2017/03/23 09:38:58, tommi (webrtc) wrote:
> > On 2017/03/23 09:09:26, squashingskak wrote:
> > > @tommi (webrtc)	
> > > 
> > > Can you point me to what has to be changed in the description? I used the
> > > standard git cl that inclueded all my git commit messages. Do I have to
> create
> > a
> > > bug somewhere else to but on the BUG= line?
> > > 
> > > I am not familiar with the term "landing" what does that mean?
> > 
> > The description should explain what the change does. At the top a single
short
> > heading, 70 chars or less would be good, as it's often used in single line
git
> > logs.  Here's some more detail with good recommendations:
> > http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
> > 
> > My personal preference is also to not write the CL description from the
> > perspective of the committer  (as in "[I'm] adding") but from the
perspective
> of
> > the change itself ("Add" or "Adds"). When sending out a code review request,
> > there's another field that is explicitly for communicating with reviewers
and
> > that's where I personally like to keep conversation type language.
> > 
> > Leave out details of each and every local commit you might keep for yourself
> > when writing the patch, such as "Fixing white spaces".
> 
> Oh and 'landing' is the same as 'committing' :)

The bug id does not look right. Please add "webrtc:" before the number if it is
in https://bugs.chromium.org/p/webrtc/issues, otherwise, it links to chromium.
But it looks like that webrtc:7294 is still not right, please check.

[squashingskak, 2017-03-23 10:51:27]

Description was changed from

==========
Adding cbr support for Opus

BUG=7294
==========

to

==========
Adding cbr support for Opus

BUG=webrtc:7394
==========

[minyue-webrtc, 2017-03-23 11:46:05]

On 2017/03/23 10:35:29, minyue-webrtc wrote:
> On 2017/03/23 09:39:17, tommi (webrtc) wrote:
> > On 2017/03/23 09:38:58, tommi (webrtc) wrote:
> > > On 2017/03/23 09:09:26, squashingskak wrote:
> > > > @tommi (webrtc)	
> > > > 
> > > > Can you point me to what has to be changed in the description? I used
the
> > > > standard git cl that inclueded all my git commit messages. Do I have to
> > create
> > > a
> > > > bug somewhere else to but on the BUG= line?
> > > > 
> > > > I am not familiar with the term "landing" what does that mean?
> > > 
> > > The description should explain what the change does. At the top a single
> short
> > > heading, 70 chars or less would be good, as it's often used in single line
> git
> > > logs.  Here's some more detail with good recommendations:
> > > http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
> > > 
> > > My personal preference is also to not write the CL description from the
> > > perspective of the committer  (as in "[I'm] adding") but from the
> perspective
> > of
> > > the change itself ("Add" or "Adds"). When sending out a code review
request,
> > > there's another field that is explicitly for communicating with reviewers
> and
> > > that's where I personally like to keep conversation type language.
> > > 
> > > Leave out details of each and every local commit you might keep for
yourself
> > > when writing the patch, such as "Fixing white spaces".
> > 
> > Oh and 'landing' is the same as 'committing' :)
> 
> The bug id does not look right. Please add "webrtc:" before the number if it
is
> in https://bugs.chromium.org/p/webrtc/issues, otherwise, it links to chromium.
> But it looks like that webrtc:7294 is still not right, please check.

Thanks! Another thought, I hope you can rebase on
https://codereview.webrtc.org/2695243005/

[squashingskak, 2017-03-27 10:17:28]

@minyue. I might need some help to do that rebasing. How do I know the branch
names I have to rebase against?

[minyue-webrtc, 2017-03-27 10:50:48]

On 2017/03/27 10:17:28, squashingskak wrote:
> @minyue. I might need some help to do that rebasing. How do I know the branch
> names I have to rebase against?

Don't worry, Soren!

I think https://codereview.webrtc.org/2695243005/  is landing soon. You may wait
a bit then just rebase on master.

To prepare locally, you may

git cl patch 2695243005 -b xxx (under master)

to get the patch.

[squashingskak, 2017-03-28 07:55:08]

I tried running git cl patch 2695243005 -b xxx but got a conflict. Maybe I wait
untill it is on master. Will you ping me when that is the case?

[minyue-webrtc, 2017-03-28 08:01:13]

On 2017/03/28 07:55:08, squashingskak wrote:
> I tried running git cl patch 2695243005 -b xxx but got a conflict. Maybe I
wait
> untill it is on master. Will you ping me when that is the case?

Emm, I think the CL has to be rebased on master first. I will ping the author
there. Anyways, the best is that I ping you when it is done.

[minyue-webrtc, 2017-03-29 20:45:26]

The author of https://codereview.webrtc.org/2695243005/, and suggest that we
don't wait.

Please see my comments +
1. please revert changes on voe_codec_*.* as the-sun suggested
2. would you plase add cbr to neteq_opus_quality_test.cc

https://codereview.webrtc.org/2772773002/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.webrtc.org/2772773002/diff/1/AUTHORS#newcode64
AUTHORS:64: Wire Swiss GmbH <*@wire.com>
I need someone to help review this.

https://codereview.webrtc.org/2772773002/diff/1/webrtc/media/base/mediaconsta...
File webrtc/media/base/mediaconstants.h (right):

https://codereview.webrtc.org/2772773002/diff/1/webrtc/media/base/mediaconsta...
webrtc/media/base/mediaconstants.h:79: extern const int kOpusDefaultUseCbr;
nit: if you place Cbr before Dtx everywhere else, do the same here

https://codereview.webrtc.org/2772773002/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/audio_coding_module.cc (right):

https://codereview.webrtc.org/2772773002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:193: int EnableOpusCbr()
override;
nit: make order between Cbr and Dtx consistent

https://codereview.webrtc.org/2772773002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/audio_encoder.h (right):

https://codereview.webrtc.org/2772773002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_encoder.h:138: // Enables or disables
codec-internal CBR (constant bitrate mode). Returns
remove codec-internal, since there is no codec-external CBR, as far as I know

https://codereview.webrtc.org/2772773002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc
(right):

https://codereview.webrtc.org/2772773002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:549:
TEST(AudioEncoderOpusTest, EncodeCbr) {
I am not sure that we have to test Cbr is Cbr in AudioEncoderOpusTest.

On AudioEncoderOpus level, we want to make sure that SetCbr can be called
properly, Take ToggleDtx as an example.

But definitely need this test on the opus_interface level. See opus_unittest.cc

https://codereview.webrtc.org/2772773002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:555:
Create10msAudioBlocks(states.encoder, kNumPacketsToEncode * 20);
needs indent. you can do

git cl format

https://codereview.webrtc.org/2772773002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:563: bool
use_cbr = (run & 1) ? true : false;
are you trying combinations of {use_cbr, bitrate}?

I hope you do it more explicitly

say (ignore syntax)

set<bool> cbr_set = {true, false}
set<int> bitrate_set = {0, 10000}
for (bool cbr : cbr_set) {
  for (int bitrate : bitrate_set) {
  }
}

https://codereview.webrtc.org/2772773002/diff/1/webrtc/modules/audio_coding/i...
File webrtc/modules/audio_coding/include/audio_coding_module.h (right):

https://codereview.webrtc.org/2772773002/diff/1/webrtc/modules/audio_coding/i...
webrtc/modules/audio_coding/include/audio_coding_module.h:774: //
EnableOpusCbr()
int

[kwiberg-webrtc, 2017-03-29 21:54:17]

https://codereview.webrtc.org/2772773002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc
(right):

https://codereview.webrtc.org/2772773002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:563: bool
use_cbr = (run & 1) ? true : false;
On 2017/03/29 20:45:26, minyue-webrtc wrote:
> are you trying combinations of {use_cbr, bitrate}?
> 
> I hope you do it more explicitly
> 
> say (ignore syntax)
> 
> set<bool> cbr_set = {true, false}
> set<int> bitrate_set = {0, 10000}
> for (bool cbr : cbr_set) {
>   for (int bitrate : bitrate_set) {
>   }
> }

Nowadays, it should be legal to do simply

  for (const bool cbr : {true, false}) {
    for (const int bitrate : {0, 10000}) {
      ...
    }
  }

[squashingskak, 2017-03-30 08:25:54]

https://codereview.webrtc.org/2772773002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc
(right):

https://codereview.webrtc.org/2772773002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:549:
TEST(AudioEncoderOpusTest, EncodeCbr) {
On 2017/03/29 20:45:26, minyue-webrtc wrote:
> I am not sure that we have to test Cbr is Cbr in AudioEncoderOpusTest.
> 
> On AudioEncoderOpus level, we want to make sure that SetCbr can be called
> properly, Take ToggleDtx as an example.
> 
> But definitely need this test on the opus_interface level. See
opus_unittest.cc

Main reason to do the test is to check that enable actually does enable cbr. The
first time I ran the test I saw that I had reversed the logic so SetCbr(true)
was disabeling cbr. Maybe it is not needed in a unit test. It is your call if
you think it can be removed.

[minyue-webrtc, 2017-03-30 08:36:37]

On 2017/03/30 08:25:54, squashingskak wrote:
>
https://codereview.webrtc.org/2772773002/diff/1/webrtc/modules/audio_coding/c...
> File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc
> (right):
> 
>
https://codereview.webrtc.org/2772773002/diff/1/webrtc/modules/audio_coding/c...
> webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:549:
> TEST(AudioEncoderOpusTest, EncodeCbr) {
> On 2017/03/29 20:45:26, minyue-webrtc wrote:
> > I am not sure that we have to test Cbr is Cbr in AudioEncoderOpusTest.
> > 
> > On AudioEncoderOpus level, we want to make sure that SetCbr can be called
> > properly, Take ToggleDtx as an example.
> > 
> > But definitely need this test on the opus_interface level. See
> opus_unittest.cc
> 
> Main reason to do the test is to check that enable actually does enable cbr.
The
> first time I ran the test I saw that I had reversed the logic so SetCbr(true)
> was disabeling cbr. Maybe it is not needed in a unit test. It is your call if
> you think it can be removed.

I don't think we should add unittests on the ultimate fact on every level. To
me, we may add integration test on the high level.

A browser test would be nice. An example is
https://codereview.chromium.org/2190533002/

[squashingskak, 2017-03-30 08:48:37]

Adding a browser test makes sense if that one can check that the packet size is
constant. I will remove AudioEncoderOpusTest. EncodeCbr.

[squashingskak, 2017-03-30 12:53:35]

Im working to move the cbr testing from AudioEncoderOpusTest to OpusTest (
opus_unittest.cc ). if I do :

./out/Default/modules_unittests --gtest_filter=OpusTest.*

I only get two of the tests. How can I run all of them ?

[minyue-webrtc, 2017-03-30 13:10:48]

On 2017/03/30 12:53:35, squashingskak wrote:
> Im working to move the cbr testing from AudioEncoderOpusTest to OpusTest (
> opus_unittest.cc ). if I do :
> 
> ./out/Default/modules_unittests --gtest_filter=OpusTest.*
> 
> I only get two of the tests. How can I run all of them ?

Because OpusTest are parameterized and prefixed with "VariousMode/"

try ./out/Default/modules_unittests --gtest_filter=*OpusTest*

[squashingskak, 2017-03-30 15:47:02]

I have addressed most of the review comments. I havent modified the
neteq_opus_quality_test what would be the main thing to test here? Does it run
some audio quality tool to check the audio quality?

[minyue-webrtc, 2017-03-30 20:36:05]

On 2017/03/30 15:47:02, squashingskak wrote:
> I have addressed most of the review comments. I havent modified the
> neteq_opus_quality_test what would be the main thing to test here? Does it run
> some audio quality tool to check the audio quality?

Thanks! please see some remaining comments.

neteq_opus_quality_test is simple. You may just add a command line arg as DTX or
FEC there.

[minyue-webrtc, 2017-03-30 20:36:17]

https://codereview.webrtc.org/2772773002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/audio_encoder.h (right):

https://codereview.webrtc.org/2772773002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder.h:138: // Enables or
disablesCBR (constant bitrate mode). Returns
nit: needs a space

https://codereview.webrtc.org/2772773002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc
(right):

https://codereview.webrtc.org/2772773002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:179: //
Verify that the mode is still kAudio.
test kAudio was needed because we used to force kSpeech when setting Dtx. This
is to show we don't force kSpeech anymore.. But this is not needed.

I will fix this in a follow up CL

https://codereview.webrtc.org/2772773002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:190:
EXPECT_EQ(AudioEncoderOpus::kAudio, states.encoder->application());
please don't check application here, (see above comment)

Check Cbr with GetCbr

https://codereview.webrtc.org/2772773002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc (right):

https://codereview.webrtc.org/2772773002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:306: void
OpusTest::TestCbrEffect(bool cbr, int block_length_ms) {
it looks like that TestCbrEffect and TestDtxEffect has a lot of similarity. Good
if merge them somehow to avoid duplication.

https://codereview.webrtc.org/2772773002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:327: int16_t*
output_data_decode = new int16_t[samples * channels_];
std::vector<int16_t> output_data_decode(samples * channels_);

[minyue-webrtc, 2017-03-31 07:49:30]

https://codereview.webrtc.org/2772773002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc
(right):

https://codereview.webrtc.org/2772773002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:179: //
Verify that the mode is still kAudio.
On 2017/03/30 20:36:17, minyue-webrtc wrote:
> test kAudio was needed because we used to force kSpeech when setting Dtx. This
> is to show we don't force kSpeech anymore.. But this is not needed.
> 
> I will fix this in a follow up CL

I have made a fix and added you to review. see
https://codereview.webrtc.org/2785363002/

[squashingskak, 2017-03-31 12:39:12]

Updated again based on review comments. I have merged the dtx and cbr testing. I
notice that with both cbr and dtx on the dtx testing part fails. Currently that
combination is not tested.

[squashingskak, 2017-03-31 12:43:01]

The reason to use cbr is to hide any information about the speech signal. Maybe
for this reason the combination of cbr and dtx dosnt make much sense.

[minyue-webrtc, 2017-03-31 13:35:53]

On 2017/03/31 12:39:12, squashingskak wrote:
> Updated again based on review comments. I have merged the dtx and cbr testing.
I
> notice that with both cbr and dtx on the dtx testing part fails. Currently
that
> combination is not tested.

no need to test that. I don't think DTX can have effect is CBR is on

[kwiberg-webrtc, 2017-03-31 16:07:47]

On 2017/03/31 13:35:53, minyue-webrtc wrote:
> On 2017/03/31 12:39:12, squashingskak wrote:
> > Updated again based on review comments. I have merged the dtx and cbr
testing.
> I
> > notice that with both cbr and dtx on the dtx testing part fails. Currently
> that
> > combination is not tested.
> 
> no need to test that. I don't think DTX can have effect is CBR is on

That sounds like an excellent thing to test... :-)

[minyue-webrtc, 2017-03-31 18:37:43]

https://codereview.webrtc.org/2772773002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc (right):

https://codereview.webrtc.org/2772773002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/opus_unittest.cc:41: void
TestDtxAndCbrEffect(bool dtx, bool cbr, int block_length_ms);
Merging DTX and CBR into this function isn't good. as you have seen, the test
fails if I call it with (true, true).

I see that my suggestion make it bad. My apologies. The common part of Dtx test
and Cbr test is only just a small portion. Maybe one or two helper functions are
enough. What you had in your last patch set was also good. Do you mind revert
this file back to last patch set. But good to use vector instead of raw pointer
on |output_data_decode|

[squashingskak, 2017-04-02 11:10:34]

Reverted the merging of cbr and dtx testing

[minyue-webrtc, 2017-04-02 11:18:44]

On 2017/04/02 11:10:34, squashingskak wrote:
> Reverted the merging of cbr and dtx testing

thank you and lgtm

[minyue-webrtc, 2017-04-03 10:41:59]

minyue@webrtc.org changed reviewers:
+ ossu@webrtc.org

[minyue-webrtc, 2017-04-03 10:42:25]

add ossu@ since he comes back to work.

[ossu, 2017-04-04 16:38:38]

This looks pretty good!

However, a _lot_ of the plumbing will disappear once I land my set of CLs
starting with https://codereview.webrtc.org/2695243005/. I'm hoping this will
happen this week.

Essentially, everything that isn't in AudioEncoderOpus (and opus_interface,
opus_unittest) will get bypassed, and all the stuff in webrtcvoiceengine will be
removed.

Instead, AudioEncoderOpus will get the fmtp parameters during construction, and
can set up CBR directly from that.

If you'd like to, and aren't in a massive hurry to get this landed, you could
minimize this CL by leaving only the changes to AudioEncoderOpus (and its
constituent parts and tests) and I'll hook it up "the new way" in the CL above.

WDYT?

[squashingskak, 2017-04-04 17:34:28]

That sounds good to me. I will update accordingly within the next few days.

[squashingskak, 2017-04-05 12:45:30]

Just to clearify. Should AudioEncoderOpus::SetCbr be removed ? And you will call
WebRtcOpus_EnableCbr / WebRtcOpus_EnableCbr accordingly in the constructor ? In
that case I guess the only thing needed is the opus_interface.c / h changes.

[ossu, 2017-04-05 12:52:46]

On 2017/04/04 17:34:28, squashingskak wrote:
> That sounds good to me. I will update accordingly within the next few days.

Great! Thanks!

On 2017/04/05 12:45:30, squashingskak wrote:
> Just to clearify. Should AudioEncoderOpus::SetCbr be removed ? And you will
call
> WebRtcOpus_EnableCbr / WebRtcOpus_EnableCbr accordingly in the constructor ?
In
> that case I guess the only thing needed is the opus_interface.c / h changes.

Yes, the AudioEncoder interface should remain unchanged, so there would be no
need for Get/SetCbr on AudioEncoderOpus.
cbr_enabled in AudioEncoderOpus::Config still makes sense, though!

[squashingskak, 2017-04-05 13:41:56]

Updated

[ossu, 2017-04-05 13:53:02]

On 2017/04/05 13:41:56, squashingskak wrote:
> Updated

Great! lgtm

[ossu, 2017-04-06 09:01:49]

The CQ bit was checked by ossu@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 09:01:57]

Dry run: 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-04-06 09:26:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 09:26:35]

Dry run: This issue passed the CQ dry run.

[ossu, 2017-04-06 12:44:35]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2017-04-06 12:44:36]

The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2772773002/#ps80001 (title:
"Reverted everything but audio_encoder_opus and opus_interface")

[commit-bot: I haz the power, 2017-04-06 12:44:45]

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-04-06 12:48:37]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491482675539760,
"parent_rev": "388fe425c783b1994608789842503ec756327913", "commit_rev":
"28dc285f2208ecbb3b79213ca8ccb608d46af654"}

[commit-bot: I haz the power, 2017-04-06 12:48:40]

Description was changed from

==========
Adding cbr support for Opus

BUG=webrtc:7394
==========

to

==========
Adding cbr support for Opus

BUG=webrtc:7394

Review-Url: https://codereview.webrtc.org/2772773002
Cr-Commit-Position: refs/heads/master@{#17564}
Committed:
https://chromium.googlesource.com/external/webrtc/+/28dc285f2208ecbb3b79213ca...
==========

[commit-bot: I haz the power, 2017-04-06 12:48:42]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/28dc285f2208ecbb3b79213ca...