Introduced the new parameter setting scheme for activating the high-pass filter in APM

This CL introduces the new functionality for setting
the APM parameters to the high-pass filter.

The introduction will be done in three steps:
1) This CL which introduces the new scheme and
 changes the code in webrtcvoiceengine.cc to use it.
2) Introduce the scheme into upstream code.
3) Remove the HighPassFilter interface in APM.

BUG=webrtc::6220, webrtc::6296, webrtc::6297, webrtc::6181, webrtc::5298

Committed: https://crrev.com/8271d04009bbddbda35e12c7645185cea50a6312
Cr-Commit-Position: refs/heads/master@{#15197}

[peah-webrtc, 2016-10-14 11:23:33]

Patchset #1 (id:1) has been deleted

[peah-webrtc, 2016-10-14 11:28:15]

Description was changed from

==========
Introduced the new functionality for setting the APM parameters to the high-pass
filter

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

to

==========
This CL introduces the new functionality for setting 
the APM parameters to the high-pass filter. 

The introduction will be done in three steps:
1) This CL which introduces the new scheme and
 changes the code in webrtcvoiceengine.cc to use it.
2) Introduce the scheme into upstream code.
3) Remove the HighPassFilter interface in APM.

BUG=webrtc::6220,webrtc::6296,webrtc::6297,webrtc::6181,webrtc::5298
==========

[peah-webrtc, 2016-10-14 11:28:24]

peah@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[peah-webrtc, 2016-10-21 09:45:28]

peah@webrtc.org changed reviewers:
- solenberg@webrtc.org

[peah-webrtc, 2016-10-24 16:04:13]

Patchset #6 (id:120001) has been deleted

[peah-webrtc, 2016-10-24 16:21:51]

Patchset #6 (id:140001) has been deleted

[peah-webrtc, 2016-10-24 16:24:09]

peah@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[peah-webrtc, 2016-10-24 16:24:10]

Now the changes discussed have been introduced for this CL. It would be great if
you could take a second look!

https://codereview.chromium.org/2415403002/diff/160001/webrtc/modules/audio_p...
File webrtc/modules/audio_processing/biquad_high_pass_filter.h (right):

https://codereview.chromium.org/2415403002/diff/160001/webrtc/modules/audio_p...
webrtc/modules/audio_processing/biquad_high_pass_filter.h:23: class
BiquadHighPassFilter {
The reason this is not named HighPassFilter is that there is an interface in
audio_processing.h that is already called that.

[the sun, 2016-10-26 09:06:43]

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

https://codereview.webrtc.org/2415403002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:872: LOG(LS_INFO) << "High pass filter:
"
If the AudioProcessing::Config has a ToString() method, and that is used to log
the config in ApplyConfig(), do you really need these log lines?

Also, the AudioOptions are being logged at the beginning of this method.

https://codereview.webrtc.org/2415403002/diff/160001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/2415403002/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.cc:222:
std::unique_ptr<HighPassFilterImpl> high_pass_filter_impl;
Since the HPFImpl is now just redirecting calls onto APM, I think it is a bit
confusing that it sits among the other submodules here. IIRC the reason for this
sub grouping was to make locking semantics explicit.

How about you instead add it directly in the APM class?

You could even keep the redirecting HPFImpl in an anonymous namespace right here
in the .cc.

https://codereview.webrtc.org/2415403002/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.cc:592: LOG(LS_INFO) <<
"Highpass filter activated: "
Move log line into InitializeHighPassFilter?

https://codereview.webrtc.org/2415403002/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.cc:1382:
config_.high_pass_filter.enabled = enable;
Thread checks?

https://codereview.webrtc.org/2415403002/diff/160001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/2415403002/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.h:134: void
EnableHighPassFilter(bool enable);
How about you simply expose a "const AudioProcessing::Config& config() const;"
method and use that +ApplyConfig() in you HPFImpl? I think it will scale better
when you update the other submodule APIs.

[peah-webrtc, 2016-10-28 05:48:58]

Patchset #7 (id:180001) has been deleted

[peah-webrtc, 2016-10-28 05:50:28]

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

https://codereview.webrtc.org/2415403002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:872: LOG(LS_INFO) << "High pass filter:
"
On 2016/10/26 09:06:43, the sun wrote:
> If the AudioProcessing::Config has a ToString() method, and that is used to
log
> the config in ApplyConfig(), do you really need these log lines?
> 
> Also, the AudioOptions are being logged at the beginning of this method.

I totally agree! I remove this log line as well as the one for the level
controller. 

Note that the AudioProcessing::Config has no ToString method, but the activation
is logged in in the ApplyConfig method which is a much better place to put the
logging of the activation of the audio processing functionality.

https://codereview.webrtc.org/2415403002/diff/160001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/2415403002/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.cc:222:
std::unique_ptr<HighPassFilterImpl> high_pass_filter_impl;
On 2016/10/26 09:06:43, the sun wrote:
> Since the HPFImpl is now just redirecting calls onto APM, I think it is a bit
> confusing that it sits among the other submodules here. IIRC the reason for
this
> sub grouping was to make locking semantics explicit.
> 
> How about you instead add it directly in the APM class?
> 
> You could even keep the redirecting HPFImpl in an anonymous namespace right
here
> in the .cc.

Great suggestion!
I added it to the APM class. I did, however, need to put it as a unique pointer
as I otherwise gets issues with the constness of the high_pass_filter()
interface function.

Done.

https://codereview.webrtc.org/2415403002/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.cc:592: LOG(LS_INFO) <<
"Highpass filter activated: "
On 2016/10/26 09:06:43, the sun wrote:
> Move log line into InitializeHighPassFilter?

That would make sense, but currently InitializeHighPassFilter may be called
multiple times during a call and is not really tied to the activation of the
highpassfilter. I'd rather have it such that there is one logline when the
highpassfilter is activated and not one for each time it is initialized.

Therefore, I keep it as it is. WDYT?

https://codereview.webrtc.org/2415403002/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.cc:1382:
config_.high_pass_filter.enabled = enable;
On 2016/10/26 09:06:43, the sun wrote:
> Thread checks?

I see now that there is a lock missing here. I fixed that.

Regarding thread checkers, that would make sense. But I won't include it since
1) It does not change the way that this method is called. Either it works, or
not. We could try it to see whether we are called in a consistent manner and
change that scheme if it is not how we want it but I think that should be a
separate CL.
2) The code is designed to work  without any restrictions on what thread is
calling this method. Adding a thread check that passes would merely lock the
current calling scheme so that it cannot be changed. I don't think that we
should do that until we see a need for it from an APM perspective or until we
have explicitly designed our APIs to enforce a certain scheme for being called.
3) There is nothing in our APIs that states that the HighPassFilter must be used
from a single thread. Therefore I don't think we can/should impose that with
thread checkers.

WDYT?

https://codereview.webrtc.org/2415403002/diff/160001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/2415403002/diff/160001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.h:134: void
EnableHighPassFilter(bool enable);
On 2016/10/26 09:06:43, the sun wrote:
> How about you simply expose a "const AudioProcessing::Config& config() const;"
> method and use that +ApplyConfig() in you HPFImpl? I think it will scale
better
> when you update the other submodule APIs.

Great suggestion! 
Done.

[peah-webrtc, 2016-10-28 07:18:03]

Added the proposed change for GetConfig to return a copy instead of a reference

[peah-webrtc, 2016-10-28 07:20:11]

Removed a leftover const

[the sun, 2016-10-28 10:52:53]

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

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:880: if (audioproc) {
Revert back to "apm()->" for these two calls. Remove the conditional. apm()
never returns NULL.

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.h:136: void
MutateConfig(MutatorFunction mutator) {
Nice! Can we do without it being a template though? The function prototype
should be the same in all cases. You should probably use rtc::FunctionView<>, or
possibly std::function - check with Karl what is the preferred way.

(See
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud...
for an example use of rtc::FunctionView<>)

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.h:164:
std::unique_ptr<HighPassFilterImpl> high_pass_filter_impl_;
I don't see why you couldn't have a pointer to a HighPassFilter here instead,
provided you make the protected dtor of HighPassFilter public. Then you could
hide the entire impl in the .cc.

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_unittest.cc (left):

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_unittest.cc:1531:
EXPECT_FALSE(apm_->high_pass_filter()->is_enabled());
Don't you need a similar test, which just looks at the default config?

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/biquad_high_pass_filter.cc (right):

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/biquad_high_pass_filter.cc:23: class
BiquadHighPassFilter::BiquadFilter {
What do we do about the name? Should we use LowCutFilter?

[kwiberg-webrtc, 2016-10-28 11:07:00]

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.h:136: void
MutateConfig(MutatorFunction mutator) {
On 2016/10/28 10:52:53, the sun wrote:
> Nice! Can we do without it being a template though? The function prototype
> should be the same in all cases. You should probably use rtc::FunctionView<>,
or
> possibly std::function - check with Karl what is the preferred way.
> 
> (See
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud...
> for an example use of rtc::FunctionView<>)

rtc::FunctionView is the right thing to use in a case like this, when the callee
isn't allowed to save the function. However, since this method is simple and not
virtual, I think using a template is fine too (but consider using const& or &&
so that you pass the mutator by reference instead of by value).

[peah-webrtc, 2016-10-28 12:19:23]

Patchset #10 (id:260001) has been deleted

[peah-webrtc, 2016-10-28 12:19:31]

Patchset #10 (id:280001) has been deleted

[peah-webrtc, 2016-10-28 12:19:38]

Patchset #10 (id:300001) has been deleted

[peah-webrtc, 2016-10-28 12:19:52]

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

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:880: if (audioproc) {
On 2016/10/28 10:52:53, the sun wrote:
> Revert back to "apm()->" for these two calls. Remove the conditional. apm()
> never returns NULL.

Sorry, this was caused by my manually reverting this during a rebase. That
should not have happened.
Done.

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.h:136: void
MutateConfig(MutatorFunction mutator) {
On 2016/10/28 10:52:53, the sun wrote:
> Nice! Can we do without it being a template though? The function prototype
> should be the same in all cases. You should probably use rtc::FunctionView<>,
or
> possibly std::function - check with Karl what is the preferred way.
> 
> (See
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud...
> for an example use of rtc::FunctionView<>)

Great suggestion!
Done.

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.h:136: void
MutateConfig(MutatorFunction mutator) {
On 2016/10/28 11:07:00, kwiberg-webrtc wrote:
> On 2016/10/28 10:52:53, the sun wrote:
> > Nice! Can we do without it being a template though? The function prototype
> > should be the same in all cases. You should probably use
rtc::FunctionView<>,
> or
> > possibly std::function - check with Karl what is the preferred way.
> > 
> > (See
> >
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud...
> > for an example use of rtc::FunctionView<>)
> 
> rtc::FunctionView is the right thing to use in a case like this, when the
callee
> isn't allowed to save the function. However, since this method is simple and
not
> virtual, I think using a template is fine too (but consider using const& or &&
> so that you pass the mutator by reference instead of by value).

Thanks! I think the FunctionView became really nice in this case so I used that.

Done.

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.h:164:
std::unique_ptr<HighPassFilterImpl> high_pass_filter_impl_;
On 2016/10/28 10:52:53, the sun wrote:
> I don't see why you couldn't have a pointer to a HighPassFilter here instead,
> provided you make the protected dtor of HighPassFilter public. Then you could
> hide the entire impl in the .cc.

That is an awesome suggestion!!!
Done.

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_unittest.cc (left):

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_unittest.cc:1531:
EXPECT_FALSE(apm_->high_pass_filter()->is_enabled());
On 2016/10/28 10:52:53, the sun wrote:
> Don't you need a similar test, which just looks at the default config?

You are right. As we still are keeping and using the HighPassFilter interface
I'll readd this line and wait with rewriting it until we remove the interface.

Doing this test in another way would require rewriting either the test or the
AudioProcessing interface and I don't think we should do that now because of
this.

Done.

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/biquad_high_pass_filter.cc (right):

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/biquad_high_pass_filter.cc:23: class
BiquadHighPassFilter::BiquadFilter {
On 2016/10/28 10:52:53, the sun wrote:
> What do we do about the name? Should we use LowCutFilter?

Let's go with that. That name is awesome!

Done.

[peah-webrtc, 2016-10-28 12:21:03]

peah@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[peah-webrtc, 2016-10-28 12:21:04]

+kjellander@ as an OWNER of webrtc/modules/BUILD.gn

[kwiberg-webrtc, 2016-10-28 12:28:56]

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.h:136: void
MutateConfig(MutatorFunction mutator) {
On 2016/10/28 12:19:52, peah-webrtc wrote:
> On 2016/10/28 11:07:00, kwiberg-webrtc wrote:
> > On 2016/10/28 10:52:53, the sun wrote:
> > > Nice! Can we do without it being a template though? The function prototype
> > > should be the same in all cases. You should probably use
> rtc::FunctionView<>,
> > or
> > > possibly std::function - check with Karl what is the preferred way.
> > > 
> > > (See
> > >
> >
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud...
> > > for an example use of rtc::FunctionView<>)
> > 
> > rtc::FunctionView is the right thing to use in a case like this, when the
> callee
> > isn't allowed to save the function. However, since this method is simple and
> not
> > virtual, I think using a template is fine too (but consider using const& or
&&
> > so that you pass the mutator by reference instead of by value).
> 
> Thanks! I think the FunctionView became really nice in this case so I used
that.
> 
> Done.

No, you used std::function instead (which will also work, although it's a bit
more expensive since it copies the lambda).

[kwiberg-webrtc, 2016-10-28 12:32:56]

Description was changed from

==========
This CL introduces the new functionality for setting 
the APM parameters to the high-pass filter. 

The introduction will be done in three steps:
1) This CL which introduces the new scheme and
 changes the code in webrtcvoiceengine.cc to use it.
2) Introduce the scheme into upstream code.
3) Remove the HighPassFilter interface in APM.

BUG=webrtc::6220,webrtc::6296,webrtc::6297,webrtc::6181,webrtc::5298
==========

to

==========
This CL introduces the new functionality for setting 
the APM parameters to the high-pass filter. 

The introduction will be done in three steps:
1) This CL which introduces the new scheme and
 changes the code in webrtcvoiceengine.cc to use it.
2) Introduce the scheme into upstream code.
3) Remove the HighPassFilter interface in APM.

BUG=webrtc::6220, webrtc::6296, webrtc::6297, webrtc::6181, webrtc::5298
==========

[peah-webrtc, 2016-10-28 12:36:12]

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/2415403002/diff/240001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.h:136: void
MutateConfig(MutatorFunction mutator) {
On 2016/10/28 12:28:56, kwiberg-webrtc wrote:
> On 2016/10/28 12:19:52, peah-webrtc wrote:
> > On 2016/10/28 11:07:00, kwiberg-webrtc wrote:
> > > On 2016/10/28 10:52:53, the sun wrote:
> > > > Nice! Can we do without it being a template though? The function
prototype
> > > > should be the same in all cases. You should probably use
> > rtc::FunctionView<>,
> > > or
> > > > possibly std::function - check with Karl what is the preferred way.
> > > > 
> > > > (See
> > > >
> > >
> >
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud...
> > > > for an example use of rtc::FunctionView<>)
> > > 
> > > rtc::FunctionView is the right thing to use in a case like this, when the
> > callee
> > > isn't allowed to save the function. However, since this method is simple
and
> > not
> > > virtual, I think using a template is fine too (but consider using const&
or
> &&
> > > so that you pass the mutator by reference instead of by value).
> > 
> > Thanks! I think the FunctionView became really nice in this case so I used
> that.
> > 
> > Done.
> 
> No, you used std::function instead (which will also work, although it's a bit
> more expensive since it copies the lambda).

You are right! I tried both locally and removed the wrong one.
I changed to the FunctionView instead.

Thanks for checking!!!!

[the sun, 2016-10-28 12:38:33]

You forgot to add the low_cut_filter files.

https://codereview.webrtc.org/2415403002/diff/320001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/2415403002/diff/320001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.h:159: bool Update(bool
low_cut_filter_enabled,
I'm not so sure about this use of low cut vs high pass - it is a bit confusing.
Maybe it is better to just stick to one name, or use LowCutFilter only at the
very lowest level, where the filter is actually being owned and instantiated.

[kjellander_webrtc, 2016-10-28 12:40:29]

where are the low_cut_filter* files?

https://codereview.webrtc.org/2415403002/diff/320001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2415403002/diff/320001/webrtc/modules/BUILD.gn#...
webrtc/modules/BUILD.gn:591: "audio_processing/low_cut_filter_unittest.cc",
I can't find this file, where is it?

https://codereview.webrtc.org/2415403002/diff/320001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/BUILD.gn (right):

https://codereview.webrtc.org/2415403002/diff/320001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/BUILD.gn:97: "low_cut_filter.cc",
where are these files? did you forget to add them?

[kjellander_webrtc, 2016-10-28 12:41:22]

On 2016/10/28 12:40:29, kjellander_webrtc wrote:
> where are the low_cut_filter* files?

Ah, in PS#12.

lgtm on *.gn+.gypi then.
> 
> https://codereview.webrtc.org/2415403002/diff/320001/webrtc/modules/BUILD.gn
> File webrtc/modules/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2415403002/diff/320001/webrtc/modules/BUILD.gn#...
> webrtc/modules/BUILD.gn:591: "audio_processing/low_cut_filter_unittest.cc",
> I can't find this file, where is it?
> 
>
https://codereview.webrtc.org/2415403002/diff/320001/webrtc/modules/audio_pro...
> File webrtc/modules/audio_processing/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2415403002/diff/320001/webrtc/modules/audio_pro...
> webrtc/modules/audio_processing/BUILD.gn:97: "low_cut_filter.cc",
> where are these files? did you forget to add them?

[peah-webrtc, 2016-10-28 12:47:46]

Thanks!
Added the new files!

https://codereview.webrtc.org/2415403002/diff/320001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/2415403002/diff/320001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.h:159: bool Update(bool
low_cut_filter_enabled,
On 2016/10/28 12:38:32, the sun wrote:
> I'm not so sure about this use of low cut vs high pass - it is a bit
confusing.
> Maybe it is better to just stick to one name, or use LowCutFilter only at the
> very lowest level, where the filter is actually being owned and instantiated.

I agree that it is confusing. But I think using the name high_pass_filter in
general is deceivingly simple for now and 

I view it as this
-High pass filtering is an effect that is provided by the APM.
-The high pass filtering effect is achieved using the low cut filter
functionality.
-The low cut filter functionality is in some scenarios needed/needs to be
activated even though the high pass filtering effect is turned off (that is what
upcoming CLs will change this to be).

Therefore I think it makes sens to differ between the effect provided by the APM
and the functionality we use to achieve the effect.

In this case, the the low_cut_filter_enabled flag is used in the
ApmSubmoduleStates class to determine when conditional reinitialization is
needed. For that, it makes much more sense to use the term low cut filter rather
than the high pass filter, as it is not the activation of the high pass
filtering effect but rather the activation of the low cut processing
functionality that possibly triggers the conditional reinitialization.

I did the partitioning so that everything that is publicly visible outside APM
still uses the high pass filter term. The log lines is one such example.

[the sun, 2016-10-28 18:04:38]

On 2016/10/28 12:47:46, peah-webrtc wrote:
> Thanks!
> Added the new files!
> 
>
https://codereview.webrtc.org/2415403002/diff/320001/webrtc/modules/audio_pro...
> File webrtc/modules/audio_processing/audio_processing_impl.h (right):
> 
>
https://codereview.webrtc.org/2415403002/diff/320001/webrtc/modules/audio_pro...
> webrtc/modules/audio_processing/audio_processing_impl.h:159: bool Update(bool
> low_cut_filter_enabled,
> On 2016/10/28 12:38:32, the sun wrote:
> > I'm not so sure about this use of low cut vs high pass - it is a bit
> confusing.
> > Maybe it is better to just stick to one name, or use LowCutFilter only at
the
> > very lowest level, where the filter is actually being owned and
instantiated.
> 
> I agree that it is confusing. But I think using the name high_pass_filter in
> general is deceivingly simple for now and 
> 
> I view it as this
> -High pass filtering is an effect that is provided by the APM.
> -The high pass filtering effect is achieved using the low cut filter
> functionality.
> -The low cut filter functionality is in some scenarios needed/needs to be
> activated even though the high pass filtering effect is turned off (that is
what
> upcoming CLs will change this to be).
> 
> Therefore I think it makes sens to differ between the effect provided by the
APM
> and the functionality we use to achieve the effect.
> 
> In this case, the the low_cut_filter_enabled flag is used in the
> ApmSubmoduleStates class to determine when conditional reinitialization is
> needed. For that, it makes much more sense to use the term low cut filter
rather
> than the high pass filter, as it is not the activation of the high pass
> filtering effect but rather the activation of the low cut processing
> functionality that possibly triggers the conditional reinitialization.
> 
> I did the partitioning so that everything that is publicly visible outside APM
> still uses the high pass filter term. The log lines is one such example.

Well, while this distinction may be perfectly clear to you, but someone else
reading the code is likely going to find it pretty strange.

I'm not going to hold the CL on it, but unless you change it I'll take the
liberty to reserve eternal douche-rights to pester you with a "told you so",
each time you need to give this explanation. :) (No, you can't dodge that by
adding extensive comments - that's a sign as good as any that the code needs to
be clarified.)

Very good improvement in any case - thanks for doing this!

LGTM

[peah-webrtc, 2016-10-30 22:41:55]

On 2016/10/28 18:04:38, the sun wrote:
> On 2016/10/28 12:47:46, peah-webrtc wrote:
> > Thanks!
> > Added the new files!
> > 
> >
>
https://codereview.webrtc.org/2415403002/diff/320001/webrtc/modules/audio_pro...
> > File webrtc/modules/audio_processing/audio_processing_impl.h (right):
> > 
> >
>
https://codereview.webrtc.org/2415403002/diff/320001/webrtc/modules/audio_pro...
> > webrtc/modules/audio_processing/audio_processing_impl.h:159: bool
Update(bool
> > low_cut_filter_enabled,
> > On 2016/10/28 12:38:32, the sun wrote:
> > > I'm not so sure about this use of low cut vs high pass - it is a bit
> > confusing.
> > > Maybe it is better to just stick to one name, or use LowCutFilter only at
> the
> > > very lowest level, where the filter is actually being owned and
> instantiated.
> > 
> > I agree that it is confusing. But I think using the name high_pass_filter in
> > general is deceivingly simple for now and 
> > 
> > I view it as this
> > -High pass filtering is an effect that is provided by the APM.
> > -The high pass filtering effect is achieved using the low cut filter
> > functionality.
> > -The low cut filter functionality is in some scenarios needed/needs to be
> > activated even though the high pass filtering effect is turned off (that is
> what
> > upcoming CLs will change this to be).
> > 
> > Therefore I think it makes sens to differ between the effect provided by the
> APM
> > and the functionality we use to achieve the effect.
> > 
> > In this case, the the low_cut_filter_enabled flag is used in the
> > ApmSubmoduleStates class to determine when conditional reinitialization is
> > needed. For that, it makes much more sense to use the term low cut filter
> rather
> > than the high pass filter, as it is not the activation of the high pass
> > filtering effect but rather the activation of the low cut processing
> > functionality that possibly triggers the conditional reinitialization.
> > 
> > I did the partitioning so that everything that is publicly visible outside
APM
> > still uses the high pass filter term. The log lines is one such example.
> 
> Well, while this distinction may be perfectly clear to you, but someone else
> reading the code is likely going to find it pretty strange.
> 
> I'm not going to hold the CL on it, but unless you change it I'll take the
> liberty to reserve eternal douche-rights to pester you with a "told you so",
> each time you need to give this explanation. :) (No, you can't dodge that by
> adding extensive comments - that's a sign as good as any that the code needs
to
> be clarified.)
> 
> Very good improvement in any case - thanks for doing this!
> 
> LGTM

I think your comments about the naming are very valid and I agree.  I won't
change it now, but I'll follow up this CL with other CLs that disconnects the
1-1 mapping of the high_pass_filter interface effect with the low-cut filter
functionality. I'll try to make this more clear then and, depending upon how the
code turns out, possibly revert to using the name high_pass_filter in the code.

Thanks for the review! Your suggestions made the last patch way much better than
the original CL! This is a great step in the APM refactoring.

[peah-webrtc, 2016-10-30 22:42:09]

The CQ bit was checked by peah@webrtc.org

[commit-bot: I haz the power, 2016-10-30 22:42:22]

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-10-30 22:43:30]

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

[commit-bot: I haz the power, 2016-10-30 22:43:31]

Try jobs failed on following builders:
  ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/11985)
  ios64_gyp_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_dbg/builds/1879)
  ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12005)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14340)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/14225)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/19571)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)

[peah-webrtc, 2016-11-01 22:15:17]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2016-11-01 22:15:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2415403002/#ps380001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-11-01 22:15:20]

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-11-01 22:25:07]

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

[commit-bot: I haz the power, 2016-11-01 22:25:08]

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

[peah-webrtc, 2016-11-02 19:50:41]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2016-11-02 19:50:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2415403002/#ps400001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-11-02 19:50:52]

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-11-02 20:01:21]

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

[commit-bot: I haz the power, 2016-11-02 20:01:22]

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

[peah-webrtc, 2016-11-06 22:12:30]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2016-11-06 22:12:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2415403002/#ps420001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-11-06 22:12:36]

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-11-06 22:20:05]

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

[commit-bot: I haz the power, 2016-11-06 22:20:06]

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

[peah-webrtc, 2016-11-10 08:46:03]

Patchset #16 (id:440001) has been deleted

[peah-webrtc, 2016-11-10 09:32:32]

Hi,
Here comes an update to the CL. The CL for several reasons caused
WebRtcVoiceEngineTestFake.* to fail. 

The reasons were
1) APM was mocked as well as the calls to VoEAudioProcessing. That meant that
the check for whether the high pass filter was active was done inside the mock
of VoEAudioProcessing and could therefore no longer be used when this
functionality was moved into APM.

2) When moving the above test for whether the high pass filter is active, it
turned out that the test is incorrect and as the default options are not
applied. That caused the test to fail when using a non-mocked APM to verify
whether the high pass filter is active.

[peah-webrtc, 2016-11-22 10:29:40]

Patchset #16 (id:460001) has been deleted

[the sun, 2016-11-22 14:37:53]

lgtm % comments

https://codereview.webrtc.org/2415403002/diff/480001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/2415403002/diff/480001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.h:93: webrtc::AudioProcessing::Config
GetApmConfig() const { return apm_config_; }
Rename to GetApmConfigForTest() and remove comment.

Make return type const &

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

https://codereview.webrtc.org/2415403002/diff/480001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2796: bool
highpass_filter_enabled;
Remove this local and make the call to IsHighPassFilterEnabled() directly in the
EXPECT_... macro.

[peah-webrtc, 2016-11-22 14:54:56]

On 2016/11/22 14:37:53, the sun wrote:
> lgtm % comments
> 
>
https://codereview.webrtc.org/2415403002/diff/480001/webrtc/media/engine/webr...
> File webrtc/media/engine/webrtcvoiceengine.h (right):
> 
>
https://codereview.webrtc.org/2415403002/diff/480001/webrtc/media/engine/webr...
> webrtc/media/engine/webrtcvoiceengine.h:93: webrtc::AudioProcessing::Config
> GetApmConfig() const { return apm_config_; }
> Rename to GetApmConfigForTest() and remove comment.
> 
> Make return type const &
> 
>
https://codereview.webrtc.org/2415403002/diff/480001/webrtc/media/engine/webr...
> File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left):
> 
>
https://codereview.webrtc.org/2415403002/diff/480001/webrtc/media/engine/webr...
> webrtc/media/engine/webrtcvoiceengine_unittest.cc:2796: bool
> highpass_filter_enabled;
> Remove this local and make the call to IsHighPassFilterEnabled() directly in
the
> EXPECT_... macro.

Great! Thanks for the review!

[peah-webrtc, 2016-11-22 14:56:05]

https://codereview.webrtc.org/2415403002/diff/480001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/2415403002/diff/480001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.h:93: webrtc::AudioProcessing::Config
GetApmConfig() const { return apm_config_; }
On 2016/11/22 14:37:53, the sun wrote:
> Rename to GetApmConfigForTest() and remove comment.
> 
> Make return type const &

Done.

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

https://codereview.webrtc.org/2415403002/diff/480001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:2796: bool
highpass_filter_enabled;
On 2016/11/22 14:37:53, the sun wrote:
> Remove this local and make the call to IsHighPassFilterEnabled() directly in
the
> EXPECT_... macro.

Done.

[peah-webrtc, 2016-11-22 14:56:12]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2016-11-22 14:56:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org, solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2415403002/#ps500001 (title:
"Changes in response to reviewer comments")

[commit-bot: I haz the power, 2016-11-22 14:56:24]

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-11-22 15:24:54]

CQ is committing da patch.

Bot data: {"patchset_id": 500001, "attempt_start_ts": 1479826572372140,
"parent_rev": "18e563ecf6817bdf7bbeb3b7d2cb0fd0cc30107e", "commit_rev":
"ec31458b089269119ad81d795bdb19dac70dbe10"}

[commit-bot: I haz the power, 2016-11-22 15:24:56]

Description was changed from

==========
This CL introduces the new functionality for setting 
the APM parameters to the high-pass filter. 

The introduction will be done in three steps:
1) This CL which introduces the new scheme and
 changes the code in webrtcvoiceengine.cc to use it.
2) Introduce the scheme into upstream code.
3) Remove the HighPassFilter interface in APM.

BUG=webrtc::6220, webrtc::6296, webrtc::6297, webrtc::6181, webrtc::5298
==========

to

==========
This CL introduces the new functionality for setting 
the APM parameters to the high-pass filter. 

The introduction will be done in three steps:
1) This CL which introduces the new scheme and
 changes the code in webrtcvoiceengine.cc to use it.
2) Introduce the scheme into upstream code.
3) Remove the HighPassFilter interface in APM.

BUG=webrtc::6220, webrtc::6296, webrtc::6297, webrtc::6181, webrtc::5298
==========

[commit-bot: I haz the power, 2016-11-22 15:24:58]

Committed patchset #17 (id:500001)

[commit-bot: I haz the power, 2016-11-22 15:25:03]

Description was changed from

==========
This CL introduces the new functionality for setting 
the APM parameters to the high-pass filter. 

The introduction will be done in three steps:
1) This CL which introduces the new scheme and
 changes the code in webrtcvoiceengine.cc to use it.
2) Introduce the scheme into upstream code.
3) Remove the HighPassFilter interface in APM.

BUG=webrtc::6220, webrtc::6296, webrtc::6297, webrtc::6181, webrtc::5298
==========

to

==========
This CL introduces the new functionality for setting
the APM parameters to the high-pass filter.

The introduction will be done in three steps:
1) This CL which introduces the new scheme and
 changes the code in webrtcvoiceengine.cc to use it.
2) Introduce the scheme into upstream code.
3) Remove the HighPassFilter interface in APM.

BUG=webrtc::6220, webrtc::6296, webrtc::6297, webrtc::6181, webrtc::5298

Committed: https://crrev.com/8271d04009bbddbda35e12c7645185cea50a6312
Cr-Commit-Position: refs/heads/master@{#15197}
==========

[commit-bot: I haz the power, 2016-11-22 15:25:05]

Patchset 17 (id:??) landed as
https://crrev.com/8271d04009bbddbda35e12c7645185cea50a6312
Cr-Commit-Position: refs/heads/master@{#15197}