Regression test for issue where Opus DTX status was being forgotten.

Regression test for issue where Opus DTX status was being forgotten.

BUG=webrtc:6020
Committed: https://crrev.com/85228d6af6c13c0a63ea11d8fe0bf240b752cfd0
Cr-Commit-Position: refs/heads/master@{#13539}

[ivoc, 2016-07-25 14:52:25]

Patchset #1 (id:1) has been deleted

[ivoc, 2016-07-25 14:52:43]

Description was changed from

==========
Regression test for issue where Opus DTX status was being forgotten.

BUG=webrtc:6020
==========

to

==========
Regression test for issue where Opus DTX status was being forgotten.

BUG=webrtc:6020
==========

[ivoc, 2016-07-25 14:52:43]

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

[ivoc, 2016-07-25 14:53:52]

Hi Minyue, PTAL at this regression test for the issue where the Opus DTX status
was being forgotten.

[minyue-webrtc, 2016-07-25 16:13:01]

Good catch!

Some small comments.

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

https://codereview.webrtc.org/2177263002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:155: bool
AudioEncoderOpus::GetDtx() {
why to add another function since dtx_enabled() exists?

https://codereview.webrtc.org/2177263002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/include/audio_coding_module.h (right):

https://codereview.webrtc.org/2177263002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/include/audio_coding_module.h:769: // int
GetOpusDtx()
I think we can avoid introducing this function by using

ModifyEncoder() in the test code itself. I think that we are moving away from
using ACM as relay on requests like this.

It would be even better if we add QueryEncoder() in analogy to ModifyEncoder(),
but allows it to call const methods of AudioEncoder. If this is irrelevant of
this CL, consider using ModifyEncoder() and add a TODO.

https://codereview.webrtc.org/2177263002/diff/20001/webrtc/voice_engine/voe_c...
File webrtc/voice_engine/voe_codec_unittest.cc (right):

https://codereview.webrtc.org/2177263002/diff/20001/webrtc/voice_engine/voe_c...
webrtc/voice_engine/voe_codec_unittest.cc:156: // The Opus DTX setting was being
forgotten after unrelated VoE calls.
do you know what the fixture "VoECodecTest" does?

Can we reuse it? or should be maybe remove it.

[ivoc, 2016-07-25 17:11:10]

Thanks for the comments, PTAL.

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

https://codereview.webrtc.org/2177263002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:155: bool
AudioEncoderOpus::GetDtx() {
On 2016/07/25 16:13:01, minyue-webrtc wrote:
> why to add another function since dtx_enabled() exists?

The function needs to be on the AudioEncoder interface so it can be called from
the outside world, and dtx_enabled() is not on that interface. I can merge the
two functions into one, but I think it should be called GetDtx(), since it fits
better with the naming system on the AudioEncoder interface.

https://codereview.webrtc.org/2177263002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/include/audio_coding_module.h (right):

https://codereview.webrtc.org/2177263002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/include/audio_coding_module.h:769: // int
GetOpusDtx()
On 2016/07/25 16:13:01, minyue-webrtc wrote:
> I think we can avoid introducing this function by using
> 
> ModifyEncoder() in the test code itself. I think that we are moving away from
> using ACM as relay on requests like this.
> 
> It would be even better if we add QueryEncoder() in analogy to
ModifyEncoder(),
> but allows it to call const methods of AudioEncoder. If this is irrelevant of
> this CL, consider using ModifyEncoder() and add a TODO.

Good point, that is indeed a better approach. I added a QueryEncoder function
like you suggested.

https://codereview.webrtc.org/2177263002/diff/20001/webrtc/voice_engine/voe_c...
File webrtc/voice_engine/voe_codec_unittest.cc (right):

https://codereview.webrtc.org/2177263002/diff/20001/webrtc/voice_engine/voe_c...
webrtc/voice_engine/voe_codec_unittest.cc:156: // The Opus DTX setting was being
forgotten after unrelated VoE calls.
On 2016/07/25 16:13:01, minyue-webrtc wrote:
> do you know what the fixture "VoECodecTest" does?
> 
> Can we reuse it? or should be maybe remove it.

I tried reusing it, but it causes a crash (it's calling a virtual function
somewhere). Since it doesn't seem to be used, I guess I can remove it.

[minyue-webrtc, 2016-07-26 09:37:01]

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

https://codereview.webrtc.org/2177263002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:155: bool
AudioEncoderOpus::GetDtx() {
On 2016/07/25 17:11:09, ivoc wrote:
> On 2016/07/25 16:13:01, minyue-webrtc wrote:
> > why to add another function since dtx_enabled() exists?
> 
> The function needs to be on the AudioEncoder interface so it can be called
from
> the outside world, and dtx_enabled() is not on that interface. I can merge the
> two functions into one, but I think it should be called GetDtx(), since it
fits
> better with the naming system on the AudioEncoder interface. 

Ok. Can you remove AudioEncoderOpus::dtx_enabled()

https://codereview.webrtc.org/2177263002/diff/20001/webrtc/voice_engine/voe_c...
File webrtc/voice_engine/voe_codec_unittest.cc (right):

https://codereview.webrtc.org/2177263002/diff/20001/webrtc/voice_engine/voe_c...
webrtc/voice_engine/voe_codec_unittest.cc:156: // The Opus DTX setting was being
forgotten after unrelated VoE calls.
On 2016/07/25 17:11:10, ivoc wrote:
> On 2016/07/25 16:13:01, minyue-webrtc wrote:
> > do you know what the fixture "VoECodecTest" does?
> > 
> > Can we reuse it? or should be maybe remove it.
> 
> I tried reusing it, but it causes a crash (it's calling a virtual function
> somewhere). Since it doesn't seem to be used, I guess I can remove it.

sure, we can remove it in a following CL. Or you can change it and use it as a
base for this new TEST in this CL

https://codereview.webrtc.org/2177263002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/acm2/audio_coding_module.cc (right):

https://codereview.webrtc.org/2177263002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:601: void
AudioCodingModuleImpl::QueryEncoder(
yes, I like it. but preferrably a std::unique_ptr<const AudioEncoder> as
argument

[ivoc, 2016-07-26 12:28:31]

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

https://codereview.webrtc.org/2177263002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:155: bool
AudioEncoderOpus::GetDtx() {
On 2016/07/26 09:37:01, minyue-webrtc wrote:
> On 2016/07/25 17:11:09, ivoc wrote:
> > On 2016/07/25 16:13:01, minyue-webrtc wrote:
> > > why to add another function since dtx_enabled() exists?
> > 
> > The function needs to be on the AudioEncoder interface so it can be called
> from
> > the outside world, and dtx_enabled() is not on that interface. I can merge
the
> > two functions into one, but I think it should be called GetDtx(), since it
> fits
> > better with the naming system on the AudioEncoder interface. 
> 
> Ok. Can you remove AudioEncoderOpus::dtx_enabled()

It's already gone :-)

https://codereview.webrtc.org/2177263002/diff/20001/webrtc/voice_engine/voe_c...
File webrtc/voice_engine/voe_codec_unittest.cc (right):

https://codereview.webrtc.org/2177263002/diff/20001/webrtc/voice_engine/voe_c...
webrtc/voice_engine/voe_codec_unittest.cc:156: // The Opus DTX setting was being
forgotten after unrelated VoE calls.
On 2016/07/26 09:37:01, minyue-webrtc wrote:
> On 2016/07/25 17:11:10, ivoc wrote:
> > On 2016/07/25 16:13:01, minyue-webrtc wrote:
> > > do you know what the fixture "VoECodecTest" does?
> > > 
> > > Can we reuse it? or should be maybe remove it.
> > 
> > I tried reusing it, but it causes a crash (it's calling a virtual function
> > somewhere). Since it doesn't seem to be used, I guess I can remove it.
> 
> sure, we can remove it in a following CL. Or you can change it and use it as a
> base for this new TEST in this CL

I think removing is better in this case, since I don't think that the
setup/teardown code in this test is complicated enough to need a class, and I
don't expect that we'll add a lot of new tests here in the future. I removed the
class in this CL, is that okay with you?

https://codereview.webrtc.org/2177263002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/acm2/audio_coding_module.cc (right):

https://codereview.webrtc.org/2177263002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:601: void
AudioCodingModuleImpl::QueryEncoder(
On 2016/07/26 09:37:01, minyue-webrtc wrote:
> yes, I like it. but preferrably a std::unique_ptr<const AudioEncoder> as
> argument

I don't think it's possible to cast a unique_ptr<X> to unique_ptr<const X>. The
only way to do this is to use std::move, but then the new pointer would take the
ownership of the object, which means that the function has to return it as well
(which would not be a very nice solution in my opinion).

[ivoc, 2016-07-26 13:55:21]

https://codereview.webrtc.org/2177263002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/acm2/audio_coding_module.cc (right):

https://codereview.webrtc.org/2177263002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/audio_coding_module.cc:601: void
AudioCodingModuleImpl::QueryEncoder(
On 2016/07/26 12:28:31, ivoc wrote:
> On 2016/07/26 09:37:01, minyue-webrtc wrote:
> > yes, I like it. but preferrably a std::unique_ptr<const AudioEncoder> as
> > argument
> 
> I don't think it's possible to cast a unique_ptr<X> to unique_ptr<const X>.
The
> only way to do this is to use std::move, but then the new pointer would take
the
> ownership of the object, which means that the function has to return it as
well
> (which would not be a very nice solution in my opinion).

I thought about this a little more, and I think we should stick to a const
pointer. I think unique_ptr is good for managing object ownership/lifetime, but
in this function we don't really want to change anything about the ownership of
the object, just provide access to its const functions. 

I also found this in the style guide: "If dynamic allocation is necessary,
prefer to keep ownership with the code that allocated it. If other code needs
access to the object, consider passing it a copy, or passing a pointer or
reference without transferring ownership. Prefer to use std::unique_ptr to make
ownership transfer explicit."
Let me know if you disagree, we can also ask another reviewer to have a look at
this if you're not sure.

[minyue-webrtc, 2016-07-26 15:04:54]

lgtm

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

https://codereview.webrtc.org/2177263002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:155: bool
AudioEncoderOpus::GetDtx() {
On 2016/07/26 12:28:31, ivoc wrote:
> On 2016/07/26 09:37:01, minyue-webrtc wrote:
> > On 2016/07/25 17:11:09, ivoc wrote:
> > > On 2016/07/25 16:13:01, minyue-webrtc wrote:
> > > > why to add another function since dtx_enabled() exists?
> > > 
> > > The function needs to be on the AudioEncoder interface so it can be called
> > from
> > > the outside world, and dtx_enabled() is not on that interface. I can merge
> the
> > > two functions into one, but I think it should be called GetDtx(), since it
> > fits
> > > better with the naming system on the AudioEncoder interface. 
> > 
> > Ok. Can you remove AudioEncoderOpus::dtx_enabled()
> 
> It's already gone :-)

Oh, yes, sorry. I missed that one

[ivoc, 2016-07-26 15:19:37]

ivoc@webrtc.org changed reviewers:
+ henrika@webrtc.org

[ivoc, 2016-07-26 15:19:38]

Hi Henrik, please have a look at this CL to introduce a regression test for our
recent Opus Dtx issue. Feel free to limit yourself to the changes in
webrtc/voice_engine/ if you're busy.

[henrika_webrtc, 2016-07-27 09:32:59]

LGTM

https://codereview.webrtc.org/2177263002/diff/40001/webrtc/voice_engine/inclu...
File webrtc/voice_engine/include/voe_codec.h (right):

https://codereview.webrtc.org/2177263002/diff/40001/webrtc/voice_engine/inclu...
webrtc/voice_engine/include/voe_codec.h:136: virtual int GetOpusDtx(int channel,
bool* enabled) { return -1; }
Why default implementation?

[ivoc, 2016-07-27 09:48:32]

https://codereview.webrtc.org/2177263002/diff/40001/webrtc/voice_engine/inclu...
File webrtc/voice_engine/include/voe_codec.h (right):

https://codereview.webrtc.org/2177263002/diff/40001/webrtc/voice_engine/inclu...
webrtc/voice_engine/include/voe_codec.h:136: virtual int GetOpusDtx(int channel,
bool* enabled) { return -1; }
On 2016/07/27 09:32:59, henrika_webrtc wrote:
> Why default implementation?

This is a public API, and there is code outside of WebRTC that implements this
interface (for example for tests). When adding functions, I think it's good to
add a default implementation first before all the deriving classes are updated.
I will add a TODO here to remind myself to make it pure virtual later.

[henrika_webrtc, 2016-07-27 09:57:04]

LGTM++

[ivoc, 2016-07-27 10:02:12]

The CQ bit was checked by ivoc@webrtc.org

[ivoc, 2016-07-27 10:02:12]

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/2177263002/#ps60001 (title:
"Added TODO for making function pure virtual.")

[commit-bot: I haz the power, 2016-07-27 10:02:19]

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-27 10:07:35]

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

[commit-bot: I haz the power, 2016-07-27 10:07:36]

Try jobs failed on following builders:
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/12909)

[ivoc, 2016-07-27 11:09:39]

The CQ bit was checked by ivoc@webrtc.org

[ivoc, 2016-07-27 11:09:40]

The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org,
henrika@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2177263002/#ps80001 (title:
"Renamed GetOpusDtx to GetOpusDtxStatus to avoid naming conflict with
FakeWebRtcVoiceEngine.")

[commit-bot: I haz the power, 2016-07-27 11:09: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, 2016-07-27 11:53:50]

Description was changed from

==========
Regression test for issue where Opus DTX status was being forgotten.

BUG=webrtc:6020
==========

to

==========
Regression test for issue where Opus DTX status was being forgotten.

BUG=webrtc:6020
==========

[commit-bot: I haz the power, 2016-07-27 11:53:52]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-07-27 11:53:58]

Description was changed from

==========
Regression test for issue where Opus DTX status was being forgotten.

BUG=webrtc:6020
==========

to

==========
Regression test for issue where Opus DTX status was being forgotten.

BUG=webrtc:6020

Committed: https://crrev.com/85228d6af6c13c0a63ea11d8fe0bf240b752cfd0
Cr-Commit-Position: refs/heads/master@{#13539}
==========

[commit-bot: I haz the power, 2016-07-27 11:53:59]

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