Remove usage of VoEAudioProcessing from WVoE/MC.

Remove usage of VoEAudioProcessing from WVoE/MC.

Calling APM and TransmitMixer directly instead.

BUG=webrtc:4690
TBR=peah@webrtc.org

Review-Url: https://codereview.webrtc.org/2681033010
Cr-Commit-Position: refs/heads/master@{#16734}
Committed: https://chromium.googlesource.com/external/webrtc/+/76377c55b7e07613ff4f5195668a3797ddc834a5

[the sun, 2017-02-10 21:27:00]

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

[commit-bot: I haz the power, 2017-02-10 21:27:07]

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-02-10 22:51:11]

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

[commit-bot: I haz the power, 2017-02-10 22:51:12]

Dry run: This issue passed the CQ dry run.

[the sun, 2017-02-11 00:17:05]

Description was changed from

==========
Remove usage of VoEAudioProcessing from WVoE/MC.

Calling APM and TransmitMixer directly instead.

WIP

BUG=webrtc:4690
==========

to

==========
Remove usage of VoEAudioProcessing from WVoE/MC.

Calling APM and TransmitMixer directly instead.

BUG=webrtc:4690
==========

[the sun, 2017-02-11 00:17:37]

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

[the sun, 2017-02-11 00:17:52]

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

[commit-bot: I haz the power, 2017-02-11 00:17:59]

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-02-11 01:35:43]

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

[commit-bot: I haz the power, 2017-02-11 01:35:43]

Dry run: This issue passed the CQ dry run.

[hlundin-webrtc, 2017-02-13 21:03:58]

Mostly LG, but I have some comments and questions.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:563: } // namespace {
Why the extra '{'?
I think it fooled the automatic formatter to remove one space between } and '}'
'//'.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:576: void
SetAgcConfig(webrtc::AudioProcessing* apm,
Swap order of parameters; input before input/output.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:592: void
SetEcStatus(webrtc::AudioProcessing* apm,
apm last

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:600: // Disable the AECM before enable
the AEC
enable -> enabling

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:601: if (enable && ecm->is_enabled() &&
ecm->Enable(false) != 0) {
If we set mode=webrtc::kEcConference and enable=false, we may end up with AEC
Mobile being (still) enabled. Is this desired?

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:615: // Disable the AEC before enable
the AECM
enable -> enabling

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:616: if (enable && ec->is_enabled() &&
ec->Enable(false) != 0) {
Similar snag here as above.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:628: void
SetEcMetricsStatus(webrtc::AudioProcessing* apm, bool enable) {
Swap parameter order.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:636: void
SetAecmMode(webrtc::AudioProcessing* apm, bool enable_cng) {
Swap parameter order.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:649: void
SetAgcStatus(webrtc::AudioProcessing* apm,
Input before output.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:657: #if defined(WEBRTC_IOS) ||
defined(ATA) || defined(WEBRTC_ANDROID)
Are these defined to either 1 or 0? If so, skip the defined.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:658: RTC_DCHECK(mode ==
webrtc::kAgcFixedDigital);
RTC_DCHECK_EQ

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:661: RTC_DCHECK(mode ==
webrtc::kAgcAdaptiveAnalog);
RTC_DCHECK_EQ

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:686: void
SetNsStatus(webrtc::AudioProcessing* apm, bool enable) {
Swap.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:700: void
SetTypingDetectionStatus(webrtc::AudioProcessing* apm, bool enable) {
Swap.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:700: void
SetTypingDetectionStatus(webrtc::AudioProcessing* apm, bool enable) {
This is confusing. The method name speaks of typing detection, while the code
mentions voice detection and VAD.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:981: if (options.stereo_swapping) {
Hmmm. When is this used? Just curious...

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left):

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:3201: channel2->SetSend(true);
I may be missing something, but the comment for this part of the test is "Make
sure settings take effect while we are sending." Would you not have to call
SetSend(true) then?

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/voice_engine/tran...
File webrtc/voice_engine/transmit_mixer.h (right):

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/voice_engine/tran...
webrtc/voice_engine/transmit_mixer.h:150: void PlayNotification(const int32_t
id,
Why the consts?

[the sun, 2017-02-13 23:34:47]

Thanks for the review! I realize this isn't an easy CL...

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:563: } // namespace {
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> Why the extra '{'?
> I think it fooled the automatic formatter to remove one space between } and
'}'
> '//'.

Done.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:576: void
SetAgcConfig(webrtc::AudioProcessing* apm,
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> Swap order of parameters; input before input/output.

<bikeshed>
Hm.
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering
"This is not a hard and fast rule"

In this context the apm is the object we're operating on, a.k.a. "this", which
is why it feels more natural for me to let it be the first parameter. It's also
common among these helper functions, so keeping it the first parameter makes it
easy to see they belong together.
</bikeshed>

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:592: void
SetEcStatus(webrtc::AudioProcessing* apm,
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> apm last

Acknowledged.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:600: // Disable the AECM before enable
the AEC
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> enable -> enabling

Done.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:601: if (enable && ecm->is_enabled() &&
ecm->Enable(false) != 0) {
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> If we set mode=webrtc::kEcConference and enable=false, we may end up with AEC
> Mobile being (still) enabled. Is this desired?

I've tried to faithfully replicate how this method works (but only keeping the
logic we actually execute):
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/voice_engin...
It isn't entirely out of the question that I've got it wrong, but I think this
is what the code used to do.

Eventually this should be configurable through the APM::Config. How about we try
to maintain the current logic in this CL, then try and refine things in a
separate one (I know peah@ is itching to work this over :)?

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:615: // Disable the AEC before enable
the AECM
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> enable -> enabling

Done.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:616: if (enable && ec->is_enabled() &&
ec->Enable(false) != 0) {
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> Similar snag here as above.

Acknowledged.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:628: void
SetEcMetricsStatus(webrtc::AudioProcessing* apm, bool enable) {
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> Swap parameter order.

Acknowledged.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:636: void
SetAecmMode(webrtc::AudioProcessing* apm, bool enable_cng) {
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> Swap parameter order.

Acknowledged.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:649: void
SetAgcStatus(webrtc::AudioProcessing* apm,
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> Input before output.

Acknowledged.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:657: #if defined(WEBRTC_IOS) ||
defined(ATA) || defined(WEBRTC_ANDROID)
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> Are these defined to either 1 or 0? If so, skip the defined.

defined(nn) is used in other parts of the code.

Do you have any idea what "ATA" is? I've just copied from
voe_audio_processing_impl.cc

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:658: RTC_DCHECK(mode ==
webrtc::kAgcFixedDigital);
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> RTC_DCHECK_EQ

Done.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:661: RTC_DCHECK(mode ==
webrtc::kAgcAdaptiveAnalog);
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> RTC_DCHECK_EQ

Done.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:686: void
SetNsStatus(webrtc::AudioProcessing* apm, bool enable) {
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> Swap.

Acknowledged.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:700: void
SetTypingDetectionStatus(webrtc::AudioProcessing* apm, bool enable) {
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> Swap.

Acknowledged.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:700: void
SetTypingDetectionStatus(webrtc::AudioProcessing* apm, bool enable) {
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> This is confusing. The method name speaks of typing detection, while the code
> mentions voice detection and VAD.

Yeah. See:
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/voice_engin...

I've tried to maintain the current terminology and logic, but perhaps it's time
to draw a line somewhere...

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:981: if (options.stereo_swapping) {
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> Hmmm. When is this used? Just curious...

It can be set from a constraint:
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/api/mediaco...

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left):

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:3201: channel2->SetSend(true);
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> I may be missing something, but the comment for this part of the test is "Make
> sure settings take effect while we are sending." Would you not have to call
> SetSend(true) then?

That channel is already sending. See line 3183/3122.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/voice_engine/tran...
File webrtc/voice_engine/transmit_mixer.h (right):

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/voice_engine/tran...
webrtc/voice_engine/transmit_mixer.h:150: void PlayNotification(const int32_t
id,
On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> Why the consts?

Because that's how the FileCallback interface defines them. This is going away
"Real Soon"(tm), and changing the impl here was simply less work than changing
the interface.

[hlundin-webrtc, 2017-02-14 07:50:44]

LGTM with one comment.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:576: void
SetAgcConfig(webrtc::AudioProcessing* apm,
On 2017/02/13 23:34:47, the sun wrote:
> On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> > Swap order of parameters; input before input/output.
> 
> <bikeshed>
> Hm.
> https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering
> "This is not a hard and fast rule"
> 
> In this context the apm is the object we're operating on, a.k.a. "this", which
> is why it feels more natural for me to let it be the first parameter. It's
also
> common among these helper functions, so keeping it the first parameter makes
it
> easy to see they belong together.
> </bikeshed>

I accept that. But this bikeshedding increased my total comment-count on this CL
quite dramatically... :)

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:700: void
SetTypingDetectionStatus(webrtc::AudioProcessing* apm, bool enable) {
On 2017/02/13 23:34:47, the sun wrote:
> On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> > This is confusing. The method name speaks of typing detection, while the
code
> > mentions voice detection and VAD.
> 
> Yeah. See:
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/voice_engin...
> 
> I've tried to maintain the current terminology and logic, but perhaps it's
time
> to draw a line somewhere...

I see. Could you snatch a few of the comments in the code you refer to? That
would make this code look less like a copy-paste error, and also document some
of the requirements for using the typing detection.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:981: if (options.stereo_swapping) {
On 2017/02/13 23:34:46, the sun wrote:
> On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> > Hmmm. When is this used? Just curious...
> 
> It can be set from a constraint:
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/api/mediaco...

Acknowledged.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/voice_engine/tran...
File webrtc/voice_engine/transmit_mixer.h (right):

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/voice_engine/tran...
webrtc/voice_engine/transmit_mixer.h:150: void PlayNotification(const int32_t
id,
On 2017/02/13 23:34:47, the sun wrote:
> On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> > Why the consts?
> 
> Because that's how the FileCallback interface defines them. This is going away
> "Real Soon"(tm), and changing the impl here was simply less work than changing
> the interface.

Acknowledged.

[the sun, 2017-02-14 19:25:57]

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:700: void
SetTypingDetectionStatus(webrtc::AudioProcessing* apm, bool enable) {
On 2017/02/14 07:50:43, hlundin-webrtc wrote:
> On 2017/02/13 23:34:47, the sun wrote:
> > On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> > > This is confusing. The method name speaks of typing detection, while the
> code
> > > mentions voice detection and VAD.
> > 
> > Yeah. See:
> >
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/voice_engin...
> > 
> > I've tried to maintain the current terminology and logic, but perhaps it's
> time
> > to draw a line somewhere...
> 
> I see. Could you snatch a few of the comments in the code you refer to? That
> would make this code look less like a copy-paste error, and also document some
> of the requirements for using the typing detection.

Not much commentary to copy, so I wrote my own essay. WDYT?

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:905: apm_helpers::SetEcStatus(apm(),
*options.echo_cancellation, ec_mode);
In case you didn't notice, I feel I should point out that the control flow here
has changed slightly.

Instead of dropping out of ApplyOptions() whenever a VoE::AudioProcessing call
fails, the new helper function will log the condition and return, but then
ApplyOptions() will continue and try to configure the next thing.

This is why I brought in both of you guys on this CL - it's not abundantly clear
to me which combinations of processing are legal/sensible. Perhaps (since I
already changed the logic), the points Henrik raised about e.g. SetEcStatus()
*should* be addressed in this CL?

[hlundin-webrtc, 2017-02-15 13:29:04]

Still LGTM

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:700: void
SetTypingDetectionStatus(webrtc::AudioProcessing* apm, bool enable) {
On 2017/02/14 19:25:57, the sun wrote:
> On 2017/02/14 07:50:43, hlundin-webrtc wrote:
> > On 2017/02/13 23:34:47, the sun wrote:
> > > On 2017/02/13 21:03:57, hlundin-webrtc wrote:
> > > > This is confusing. The method name speaks of typing detection, while the
> > code
> > > > mentions voice detection and VAD.
> > > 
> > > Yeah. See:
> > >
> >
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/voice_engin...
> > > 
> > > I've tried to maintain the current terminology and logic, but perhaps it's
> > time
> > > to draw a line somewhere...
> > 
> > I see. Could you snatch a few of the comments in the code you refer to? That
> > would make this code look less like a copy-paste error, and also document
some
> > of the requirements for using the typing detection.
> 
> Not much commentary to copy, so I wrote my own essay. WDYT?

Good.

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:905: apm_helpers::SetEcStatus(apm(),
*options.echo_cancellation, ec_mode);
On 2017/02/14 19:25:57, the sun wrote:
> In case you didn't notice, I feel I should point out that the control flow
here
> has changed slightly.
> 
> Instead of dropping out of ApplyOptions() whenever a VoE::AudioProcessing call
> fails, the new helper function will log the condition and return, but then
> ApplyOptions() will continue and try to configure the next thing.
> 
> This is why I brought in both of you guys on this CL - it's not abundantly
clear
> to me which combinations of processing are legal/sensible. Perhaps (since I
> already changed the logic), the points Henrik raised about e.g. SetEcStatus()
> *should* be addressed in this CL?

I'm not at all sure how this code is supposed to balk when given a bad config.
So, your design is as good as mine.

[the sun, 2017-02-15 15:12:06]

Per, would you mind taking a look?

[the sun, 2017-02-15 15:13:30]

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2681033010/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:905: apm_helpers::SetEcStatus(apm(),
*options.echo_cancellation, ec_mode);
On 2017/02/15 13:29:04, hlundin-webrtc wrote:
> On 2017/02/14 19:25:57, the sun wrote:
> > In case you didn't notice, I feel I should point out that the control flow
> here
> > has changed slightly.
> > 
> > Instead of dropping out of ApplyOptions() whenever a VoE::AudioProcessing
call
> > fails, the new helper function will log the condition and return, but then
> > ApplyOptions() will continue and try to configure the next thing.
> > 
> > This is why I brought in both of you guys on this CL - it's not abundantly
> clear
> > to me which combinations of processing are legal/sensible. Perhaps (since I
> > already changed the logic), the points Henrik raised about e.g.
SetEcStatus()
> > *should* be addressed in this CL?
> 
> I'm not at all sure how this code is supposed to balk when given a bad config.
> So, your design is as good as mine.

Acknowledged.

[peah-webrtc, 2017-02-15 15:24:31]

On 2017/02/15 15:12:06, the sun wrote:
> Per, would you mind taking a look?

Absolutely, I'm half-way through.

[the sun, 2017-02-19 22:33:55]

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

[commit-bot: I haz the power, 2017-02-19 22:33:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[the sun, 2017-02-19 22:54:16]

On 2017/02/15 15:24:31, peah-webrtc wrote:
> On 2017/02/15 15:12:06, the sun wrote:
> > Per, would you mind taking a look?
> 
> Absolutely, I'm half-way through.

Pulled out apm_helpers and added tests. PTAL.

[hlundin-webrtc, 2017-02-20 09:01:08]

Nice. Got a few minor comments for you.

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
File webrtc/media/engine/apm_helpers.cc (right):

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers.cc:15: #include
"webrtc/modules/audio_device/include/audio_device.h"
Wrong order

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers.cc:52: RTC_DCHECK(mode == kAgcFixedDigital ||
mode == kAgcAdaptiveAnalog);
This DCHECK is superfluous, since you are checking this in detail in the
#if-#else below.

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers.cc:69: if (agc_mode !=
GainControl::kFixedDigital) {
Are we somehow depending on that the ADM AGC is already disabled when we select
kFixedDigital? Can we query it? DCHECK it?

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers.cc:86: RTC_DCHECK(mode == kEcConference || mode
== kEcAecm);
Consider RTC_DCHECK(...) << "mode: " << mode;
to get a printout of actual mode on failure.

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers.cc:131: LOG(LS_ERROR) << "Failed to
enable/disable CNG: " << enable_cng;
Alignment is off.

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers.cc:132: return;
Unnecessary return statement.

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers.cc:172: LOG(LS_INFO) << "VAD set to " << enable;
Let the log text elaborate that VAD was set because of typing detection.

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
File webrtc/media/engine/apm_helpers_unittest.cc (right):

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers_unittest.cc:29: // TODO(solenberg): This
replicates the conditions from voe_auto_test.
This is a statement, not a TODO. :)

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers_unittest.cc:32: RTC_CHECK_EQ(0,
voe_wrapper_.base()->Init(
You should typically avoid (D)CHECKs in test code, since it will crash the
entire binary, not only that test case. Can you do an ASSERT_EQ instead?

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers_unittest.cc:91: const AgcConfig agc_config = {
11,  17, false };
Double space before "17".

[the sun, 2017-02-20 19:37:58]

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
File webrtc/media/engine/apm_helpers.cc (right):

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers.cc:15: #include
"webrtc/modules/audio_device/include/audio_device.h"
On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> Wrong order

Done.

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers.cc:52: RTC_DCHECK(mode == kAgcFixedDigital ||
mode == kAgcAdaptiveAnalog);
On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> This DCHECK is superfluous, since you are checking this in detail in the
> #if-#else below.

Done.

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers.cc:69: if (agc_mode !=
GainControl::kFixedDigital) {
On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> Are we somehow depending on that the ADM AGC is already disabled when we
select
> kFixedDigital? Can we query it? DCHECK it?

Not sure what we depend on in reality - the SetAGC() impls on the Android/iOS
ADM are empty, except for check that we never try to enable AGC. I've updated
the comment and made this function always call SetAGC(). I think it is more
clear.

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers.cc:86: RTC_DCHECK(mode == kEcConference || mode
== kEcAecm);
On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> Consider RTC_DCHECK(...) << "mode: " << mode;
> to get a printout of actual mode on failure.

Done.

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers.cc:131: LOG(LS_ERROR) << "Failed to
enable/disable CNG: " << enable_cng;
On 2017/02/20 09:01:07, hlundin-webrtc wrote:
> Alignment is off.

Done.

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers.cc:132: return;
On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> Unnecessary return statement.

Added log statement in the success path instead.

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers.cc:172: LOG(LS_INFO) << "VAD set to " << enable;
On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> Let the log text elaborate that VAD was set because of typing detection.

Done.

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
File webrtc/media/engine/apm_helpers_unittest.cc (right):

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers_unittest.cc:29: // TODO(solenberg): This
replicates the conditions from voe_auto_test.
On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> This is a statement, not a TODO. :)

Done.

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers_unittest.cc:32: RTC_CHECK_EQ(0,
voe_wrapper_.base()->Init(
On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> You should typically avoid (D)CHECKs in test code, since it will crash the
> entire binary, not only that test case. Can you do an ASSERT_EQ instead?

Unfortunately ASSERT_EQ cannot be used. IIUC this is because the assert macro is
intended to be run inside TEST() and returns from the test function with an
error description, unless we build with exception support. Hence, cannot run in
the ctor. Using EXPECT_EQ instead.

https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
webrtc/media/engine/apm_helpers_unittest.cc:91: const AgcConfig agc_config = {
11,  17, false };
On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> Double space before "17".

Done.

[the sun, 2017-02-20 19:38:32]

On 2017/02/20 19:37:58, the sun wrote:
>
https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
> File webrtc/media/engine/apm_helpers.cc (right):
> 
>
https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
> webrtc/media/engine/apm_helpers.cc:15: #include
> "webrtc/modules/audio_device/include/audio_device.h"
> On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> > Wrong order
> 
> Done.
> 
>
https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
> webrtc/media/engine/apm_helpers.cc:52: RTC_DCHECK(mode == kAgcFixedDigital ||
> mode == kAgcAdaptiveAnalog);
> On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> > This DCHECK is superfluous, since you are checking this in detail in the
> > #if-#else below.
> 
> Done.
> 
>
https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
> webrtc/media/engine/apm_helpers.cc:69: if (agc_mode !=
> GainControl::kFixedDigital) {
> On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> > Are we somehow depending on that the ADM AGC is already disabled when we
> select
> > kFixedDigital? Can we query it? DCHECK it?
> 
> Not sure what we depend on in reality - the SetAGC() impls on the Android/iOS
> ADM are empty, except for check that we never try to enable AGC. I've updated
> the comment and made this function always call SetAGC(). I think it is more
> clear.
> 
>
https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
> webrtc/media/engine/apm_helpers.cc:86: RTC_DCHECK(mode == kEcConference ||
mode
> == kEcAecm);
> On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> > Consider RTC_DCHECK(...) << "mode: " << mode;
> > to get a printout of actual mode on failure.
> 
> Done.
> 
>
https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
> webrtc/media/engine/apm_helpers.cc:131: LOG(LS_ERROR) << "Failed to
> enable/disable CNG: " << enable_cng;
> On 2017/02/20 09:01:07, hlundin-webrtc wrote:
> > Alignment is off.
> 
> Done.
> 
>
https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
> webrtc/media/engine/apm_helpers.cc:132: return;
> On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> > Unnecessary return statement.
> 
> Added log statement in the success path instead.
> 
>
https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
> webrtc/media/engine/apm_helpers.cc:172: LOG(LS_INFO) << "VAD set to " <<
enable;
> On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> > Let the log text elaborate that VAD was set because of typing detection.
> 
> Done.
> 
>
https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
> File webrtc/media/engine/apm_helpers_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
> webrtc/media/engine/apm_helpers_unittest.cc:29: // TODO(solenberg): This
> replicates the conditions from voe_auto_test.
> On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> > This is a statement, not a TODO. :)
> 
> Done.
> 
>
https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
> webrtc/media/engine/apm_helpers_unittest.cc:32: RTC_CHECK_EQ(0,
> voe_wrapper_.base()->Init(
> On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> > You should typically avoid (D)CHECKs in test code, since it will crash the
> > entire binary, not only that test case. Can you do an ASSERT_EQ instead?
> 
> Unfortunately ASSERT_EQ cannot be used. IIUC this is because the assert macro
is
> intended to be run inside TEST() and returns from the test function with an
> error description, unless we build with exception support. Hence, cannot run
in
> the ctor. Using EXPECT_EQ instead.
> 
>
https://codereview.webrtc.org/2681033010/diff/270001/webrtc/media/engine/apm_...
> webrtc/media/engine/apm_helpers_unittest.cc:91: const AgcConfig agc_config = {
> 11,  17, false };
> On 2017/02/20 09:01:08, hlundin-webrtc wrote:
> > Double space before "17".
> 
> Done.

Thanks! Per, could you take a look?

[the sun, 2017-02-20 21:41:21]

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

[commit-bot: I haz the power, 2017-02-20 21:41:27]

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-02-20 22:32:58]

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

[commit-bot: I haz the power, 2017-02-20 22:32:59]

Dry run: This issue passed the CQ dry run.

[the sun, 2017-02-21 08:50:28]

Description was changed from

==========
Remove usage of VoEAudioProcessing from WVoE/MC.

Calling APM and TransmitMixer directly instead.

BUG=webrtc:4690
==========

to

==========
Remove usage of VoEAudioProcessing from WVoE/MC.

Calling APM and TransmitMixer directly instead.

BUG=webrtc:4690
TBR=peah@webrtc.org
==========

[the sun, 2017-02-21 08:51:35]

Per is out this week so I'm landing this CL now with a TBR. I'd really
appreciate if you could take a look when you have time, Per.

[the sun, 2017-02-21 08:51:44]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2017-02-21 08:51:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2681033010/#ps310001 (title:
"one more")

[commit-bot: I haz the power, 2017-02-21 08:51:57]

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-02-21 08:54:31]

CQ is committing da patch.

Bot data: {"patchset_id": 310001, "attempt_start_ts": 1487667104915940,
"parent_rev": "11c9eafc69251d494186e12be15bde426c447240", "commit_rev":
"76377c55b7e07613ff4f5195668a3797ddc834a5"}

[commit-bot: I haz the power, 2017-02-21 08:54:35]

Description was changed from

==========
Remove usage of VoEAudioProcessing from WVoE/MC.

Calling APM and TransmitMixer directly instead.

BUG=webrtc:4690
TBR=peah@webrtc.org
==========

to

==========
Remove usage of VoEAudioProcessing from WVoE/MC.

Calling APM and TransmitMixer directly instead.

BUG=webrtc:4690
TBR=peah@webrtc.org

Review-Url: https://codereview.webrtc.org/2681033010
Cr-Commit-Position: refs/heads/master@{#16734}
Committed:
https://chromium.googlesource.com/external/webrtc/+/76377c55b7e07613ff4f51956...
==========

[commit-bot: I haz the power, 2017-02-21 08:54:37]

Committed patchset #17 (id:310001) as
https://chromium.googlesource.com/external/webrtc/+/76377c55b7e07613ff4f51956...

[peah-webrtc, 2017-05-17 05:07:06]

On 2017/02/21 08:54:37, commit-bot: I haz the power wrote:
> Committed patchset #17 (id:310001) as
>
https://chromium.googlesource.com/external/webrtc/+/76377c55b7e07613ff4f51956...

Sorry, I missed that I was TBR on this. I've looked at the changes before and
they seem great!

lgtm