HighPassFilter not a ProcessingComponent anymore (bit exact).

Make HighPassFilter not a ProcessingComponent anymore (bit exact).

BUG=webrtc:5298
Committed: https://crrev.com/70f9903e570931831a027ba6f91c164efc604a85
Cr-Commit-Position: refs/heads/master@{#10939}

[the sun, 2015-12-02 23:27:21]

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

[the sun, 2015-12-03 07:06:43]

Description was changed from

==========
HighPassFilter not a ProcessingComponent anymore.

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

to

==========
HighPassFilter not a ProcessingComponent anymore.

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

[the sun, 2015-12-03 10:40:39]

On 2015/12/02 23:27:21, the sun wrote:

Compilers seem to have bought it now. PTAL.

[the sun, 2015-12-03 12:03:38]

Description was changed from

==========
HighPassFilter not a ProcessingComponent anymore.

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

to

==========
Make HighPassFilter not a ProcessingComponent anymore.

BUG=5298
==========

[the sun, 2015-12-03 12:03:59]

Description was changed from

==========
Make HighPassFilter not a ProcessingComponent anymore.

BUG=5298
==========

to

==========
Make HighPassFilter not a ProcessingComponent anymore.

BUG=webrtc:5298
==========

[the sun, 2015-12-03 13:32:00]

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

[hlundin-webrtc, 2015-12-04 12:30:09]

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/high_pass_filter_impl.cc (right):

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/high_pass_filter_impl.cc:25: explicit
MonoFilter(int sample_rate_hz) {
If you initialize ba_ as
explicit MonoFilter(int sample_rate_hz)
    : ba_(sample_rate_hz == AudioProcessing::kSampleRate8kHz ?
kFilterCoefficients8kHz : kFilterCoefficients) { ...

you can const-declare ba_:
const int16_t* const ba_ = nullptr;

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/high_pass_filter_impl.cc:36: const int16_t* ba =
ba_;
Why these local variables? ba, x, and y. (I guess because copy-paste, but I
think you shouldn't.)

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/high_pass_filter_impl.cc:45: tmp_int32 = y[1] *
ba[3];  // -a[1] * y[i-1] (low part)
Did you git cl format this? I was expecting the comments to align.

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/high_pass_filter_impl.cc:67: // Rounding in Q12,
i.e. add 2^11
End with .

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/high_pass_filter_impl.cc:70: // Saturate (to
2^27) so that the HP filtered signal does not overflow
End with .

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/high_pass_filter_impl.h (right):

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/high_pass_filter_impl.h:37:
std::vector<rtc::scoped_ptr<MonoFilter>> filters_;
Shouldn't filters_ be GUARDED_BY(crit_)?

[the sun, 2015-12-04 14:53:31]

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/high_pass_filter_impl.cc (right):

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/high_pass_filter_impl.cc:25: explicit
MonoFilter(int sample_rate_hz) {
On 2015/12/04 12:30:09, hlundin-webrtc wrote:
> If you initialize ba_ as
> explicit MonoFilter(int sample_rate_hz)
>     : ba_(sample_rate_hz == AudioProcessing::kSampleRate8kHz ?
> kFilterCoefficients8kHz : kFilterCoefficients) { ...
> 
> you can const-declare ba_:
> const int16_t* const ba_ = nullptr;

Done.

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/high_pass_filter_impl.cc:36: const int16_t* ba =
ba_;
On 2015/12/04 12:30:09, hlundin-webrtc wrote:
> Why these local variables? ba, x, and y. (I guess because copy-paste, but I
> think you shouldn't.)

I've tried not to touch the actual implementation too much...

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/high_pass_filter_impl.cc:45: tmp_int32 = y[1] *
ba[3];  // -a[1] * y[i-1] (low part)
On 2015/12/04 12:30:09, hlundin-webrtc wrote:
> Did you git cl format this? I was expecting the comments to align.

Done.

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/high_pass_filter_impl.cc:67: // Rounding in Q12,
i.e. add 2^11
On 2015/12/04 12:30:09, hlundin-webrtc wrote:
> End with .

Done.

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/high_pass_filter_impl.cc:70: // Saturate (to
2^27) so that the HP filtered signal does not overflow
On 2015/12/04 12:30:09, hlundin-webrtc wrote:
> End with .

Done.

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/high_pass_filter_impl.h (right):

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/high_pass_filter_impl.h:37:
std::vector<rtc::scoped_ptr<MonoFilter>> filters_;
On 2015/12/04 12:30:09, hlundin-webrtc wrote:
> Shouldn't filters_ be GUARDED_BY(crit_)?

Done.

[hlundin-webrtc, 2015-12-04 15:11:09]

lgtm, for what it's worth...

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/high_pass_filter_impl.cc (right):

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/high_pass_filter_impl.cc:36: const int16_t* ba =
ba_;
On 2015/12/04 14:53:31, the sun wrote:
> On 2015/12/04 12:30:09, hlundin-webrtc wrote:
> > Why these local variables? ba, x, and y. (I guess because copy-paste, but I
> > think you shouldn't.)
> 
> I've tried not to touch the actual implementation too much...

Acknowledged.

[the sun, 2015-12-08 13:05:59]

Description was changed from

==========
Make HighPassFilter not a ProcessingComponent anymore.

BUG=webrtc:5298
==========

to

==========
Make HighPassFilter not a ProcessingComponent anymore (bit exact).

BUG=webrtc:5298
==========

[peah-webrtc, 2015-12-08 14:52:27]

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

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.cc:1297: int channels =
num_output_channels();
Why are the number of output channels and sample rate cached in local variables
before doing the function call?

(and if there is a need to do that, I guess they should be const).

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/high_pass_filter_impl.h (right):

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/high_pass_filter_impl.h:37:
std::vector<rtc::scoped_ptr<MonoFilter>> filters_;
We should have an inclusion guard for filters_ as well.

https://codereview.webrtc.org/1490333004/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/high_pass_filter_impl.cc (right):

https://codereview.webrtc.org/1490333004/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/high_pass_filter_impl.cc:23: class
HighPassFilterImpl::MonoFilter {
I think we should call this a MonoBiquadFilter, or just BiquadFilter because
that is what this is. It is not a generic Filter class.

[the sun, 2015-12-08 15:08:06]

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

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.cc:1297: int channels =
num_output_channels();
On 2015/12/08 14:52:27, peah-webrtc wrote:
> Why are the number of output channels and sample rate cached in local
variables
> before doing the function call?
> 
> (and if there is a need to do that, I guess they should be const).

Done.

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/high_pass_filter_impl.h (right):

https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/high_pass_filter_impl.h:37:
std::vector<rtc::scoped_ptr<MonoFilter>> filters_;
On 2015/12/08 14:52:27, peah-webrtc wrote:
> We should have an inclusion guard for filters_ as well.

Done.

https://codereview.webrtc.org/1490333004/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/high_pass_filter_impl.cc (right):

https://codereview.webrtc.org/1490333004/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/high_pass_filter_impl.cc:23: class
HighPassFilterImpl::MonoFilter {
On 2015/12/08 14:52:27, peah-webrtc wrote:
> I think we should call this a MonoBiquadFilter, or just BiquadFilter because
> that is what this is. It is not a generic Filter class.

Good idea. I changed it to BiquadFilter, and the processing function is now
"Process"

[peah-webrtc, 2015-12-08 15:09:35]

On 2015/12/08 15:08:06, the sun wrote:
>
https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
> File webrtc/modules/audio_processing/audio_processing_impl.cc (right):
> 
>
https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
> webrtc/modules/audio_processing/audio_processing_impl.cc:1297: int channels =
> num_output_channels();
> On 2015/12/08 14:52:27, peah-webrtc wrote:
> > Why are the number of output channels and sample rate cached in local
> variables
> > before doing the function call?
> > 
> > (and if there is a need to do that, I guess they should be const).
> 
> Done.
> 
>
https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
> File webrtc/modules/audio_processing/high_pass_filter_impl.h (right):
> 
>
https://codereview.webrtc.org/1490333004/diff/100001/webrtc/modules/audio_pro...
> webrtc/modules/audio_processing/high_pass_filter_impl.h:37:
> std::vector<rtc::scoped_ptr<MonoFilter>> filters_;
> On 2015/12/08 14:52:27, peah-webrtc wrote:
> > We should have an inclusion guard for filters_ as well.
> 
> Done.
> 
>
https://codereview.webrtc.org/1490333004/diff/180001/webrtc/modules/audio_pro...
> File webrtc/modules/audio_processing/high_pass_filter_impl.cc (right):
> 
>
https://codereview.webrtc.org/1490333004/diff/180001/webrtc/modules/audio_pro...
> webrtc/modules/audio_processing/high_pass_filter_impl.cc:23: class
> HighPassFilterImpl::MonoFilter {
> On 2015/12/08 14:52:27, peah-webrtc wrote:
> > I think we should call this a MonoBiquadFilter, or just BiquadFilter because
> > that is what this is. It is not a generic Filter class.
> 
> Good idea. I changed it to BiquadFilter, and the processing function is now
> "Process"

Awesome CL!!

LGTM

[the sun, 2015-12-08 15:10:53]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2015-12-08 15:10:53]

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/1490333004/#ps200001 (title:
"comments+rebase")

[commit-bot: I haz the power, 2015-12-08 15:11:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1490333004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1490333004/200001

[commit-bot: I haz the power, 2015-12-08 16:11:33]

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

[commit-bot: I haz the power, 2015-12-08 16:11:34]

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

[the sun, 2015-12-08 17:47:34]

The CQ bit was checked by solenberg@webrtc.org

[commit-bot: I haz the power, 2015-12-08 17:47:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1490333004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1490333004/200001

[commit-bot: I haz the power, 2015-12-08 19:07:35]

Description was changed from

==========
Make HighPassFilter not a ProcessingComponent anymore (bit exact).

BUG=webrtc:5298
==========

to

==========
Make HighPassFilter not a ProcessingComponent anymore (bit exact).

BUG=webrtc:5298
==========

[commit-bot: I haz the power, 2015-12-08 19:07:35]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-12-08 19:07:44]

Description was changed from

==========
Make HighPassFilter not a ProcessingComponent anymore (bit exact).

BUG=webrtc:5298
==========

to

==========
Make HighPassFilter not a ProcessingComponent anymore (bit exact).

BUG=webrtc:5298

Committed: https://crrev.com/70f9903e570931831a027ba6f91c164efc604a85
Cr-Commit-Position: refs/heads/master@{#10939}
==========

[commit-bot: I haz the power, 2015-12-08 19:07:45]

Patchset 11 (id:??) landed as
https://crrev.com/70f9903e570931831a027ba6f91c164efc604a85
Cr-Commit-Position: refs/heads/master@{#10939}