Make NoiseSuppression not a processing component (bit exact).

Make NoiseSuppression not a processing component (bit exact).

BUG=webrtc:5298
patch from issue 1490333004 at patchset 1 (http://crrev.com/1490333004#ps1)

Committed: https://crrev.com/5e465c33cac54ed5265f18413f7afc44aae2dfca
Cr-Commit-Position: refs/heads/master@{#10944}

[the sun, 2015-12-08 13:07:00]

Description was changed from

==========
Make NoiseSuppression not a processing component.

BUG=

patch from issue 1490333004 at patchset 1 (http://crrev.com/1490333004#ps1)
==========

to

==========
Make NoiseSuppression not a processing component (bit exact).

BUG=

patch from issue 1490333004 at patchset 1 (http://crrev.com/1490333004#ps1)
==========

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

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

[hlundin-webrtc, 2015-12-08 13:31:33]

LGTM with two (optional) suggestions.

https://codereview.webrtc.org/1507683006/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/noise_suppression_impl.cc (right):

https://codereview.webrtc.org/1507683006/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/noise_suppression_impl.cc:68: #if
defined(WEBRTC_NS_FLOAT)
I would like

#ifndef WEBRTC_NS_FLOAT
return AudioProcessing::kNoError;
#endif

better.

https://codereview.webrtc.org/1507683006/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/noise_suppression_impl.cc:97:
WebRtcNs_Process(suppressors_[i]->state(),
Can you #define these two variants as well, together with the others up at lines
14-28?

https://codereview.webrtc.org/1507683006/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/noise_suppression_impl.cc:167: // TODO(peah):
Returning error code as a float! Remove this.
Oh, dear...

[the sun, 2015-12-08 13:37:27]

https://codereview.webrtc.org/1507683006/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/noise_suppression_impl.cc (right):

https://codereview.webrtc.org/1507683006/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/noise_suppression_impl.cc:68: #if
defined(WEBRTC_NS_FLOAT)
On 2015/12/08 13:31:33, hlundin-webrtc wrote:
> I would like
> 
> #ifndef WEBRTC_NS_FLOAT
> return AudioProcessing::kNoError;
> #endif
> 
> better.

I bet some compilers will complain about unreachable code unless you elaborate
on that to include an #elif. Do you still want me to change it?

https://codereview.webrtc.org/1507683006/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/noise_suppression_impl.cc:97:
WebRtcNs_Process(suppressors_[i]->state(),
On 2015/12/08 13:31:33, hlundin-webrtc wrote:
> Can you #define these two variants as well, together with the others up at
lines
> 14-28?

Theoretically. It wouldn't make any difference to the code here since in one
case the _f() methods are used to get the actual frame data. That's why I didn't
do it. I also tried wrapping everything in the Suppressor class, but the way
_Process() is called here made that messy.

https://codereview.webrtc.org/1507683006/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/noise_suppression_impl.cc:167: // TODO(peah):
Returning error code as a float! Remove this.
On 2015/12/08 13:31:33, hlundin-webrtc wrote:
> Oh, dear...

Acknowledged.

[hlundin-webrtc, 2015-12-08 14:44:07]

https://codereview.webrtc.org/1507683006/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/noise_suppression_impl.cc (right):

https://codereview.webrtc.org/1507683006/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/noise_suppression_impl.cc:68: #if
defined(WEBRTC_NS_FLOAT)
On 2015/12/08 13:37:27, the sun wrote:
> On 2015/12/08 13:31:33, hlundin-webrtc wrote:
> > I would like
> > 
> > #ifndef WEBRTC_NS_FLOAT
> > return AudioProcessing::kNoError;
> > #endif
> > 
> > better.
> 
> I bet some compilers will complain about unreachable code unless you elaborate
> on that to include an #elif. Do you still want me to change it?

Nope.

https://codereview.webrtc.org/1507683006/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/noise_suppression_impl.cc:97:
WebRtcNs_Process(suppressors_[i]->state(),
On 2015/12/08 13:37:27, the sun wrote:
> On 2015/12/08 13:31:33, hlundin-webrtc wrote:
> > Can you #define these two variants as well, together with the others up at
> lines
> > 14-28?
> 
> Theoretically. It wouldn't make any difference to the code here since in one
> case the _f() methods are used to get the actual frame data. That's why I
didn't
> do it. I also tried wrapping everything in the Suppressor class, but the way
> _Process() is called here made that messy.

Don't bother, then.

[peah-webrtc, 2015-12-08 15:08:22]

https://codereview.webrtc.org/1507683006/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/high_pass_filter_impl.cc (right):

https://codereview.webrtc.org/1507683006/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/high_pass_filter_impl.cc:92: RTC_DCHECK_LE(0,
channels);
Nit:
I'd do this check the opposite way. I think we really want to check whether
channels is greater than zero, not whether 0 is lower than channels (which it
sounds like now). Fine as it is though.

https://codereview.webrtc.org/1507683006/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/noise_suppression_impl.cc (right):

https://codereview.webrtc.org/1507683006/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/noise_suppression_impl.cc:31: class
NoiseSuppressionImpl::Suppressor {
I'd rather call it MonoNoiseSuppressor, or just NoiseSuppressor, because that is
what it is. It is not a generic suppressor.

[the sun, 2015-12-08 15:31:30]

https://codereview.webrtc.org/1507683006/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/high_pass_filter_impl.cc (right):

https://codereview.webrtc.org/1507683006/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/high_pass_filter_impl.cc:92: RTC_DCHECK_LE(0,
channels);
On 2015/12/08 15:08:22, peah-webrtc wrote:
> Nit:
> I'd do this check the opposite way. I think we really want to check whether
> channels is greater than zero, not whether 0 is lower than channels (which it
> sounds like now). Fine as it is though.

Acknowledged.

https://codereview.webrtc.org/1507683006/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/noise_suppression_impl.cc (right):

https://codereview.webrtc.org/1507683006/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/noise_suppression_impl.cc:31: class
NoiseSuppressionImpl::Suppressor {
On 2015/12/08 15:08:22, peah-webrtc wrote:
> I'd rather call it MonoNoiseSuppressor, or just NoiseSuppressor, because that
is
> what it is. It is not a generic suppressor.

Resolved offline.

[peah-webrtc, 2015-12-08 15:45:16]

On 2015/12/08 15:08:22, peah-webrtc wrote:
>
https://codereview.webrtc.org/1507683006/diff/60001/webrtc/modules/audio_proc...
> File webrtc/modules/audio_processing/high_pass_filter_impl.cc (right):
> 
>
https://codereview.webrtc.org/1507683006/diff/60001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/high_pass_filter_impl.cc:92: RTC_DCHECK_LE(0,
> channels);
> Nit:
> I'd do this check the opposite way. I think we really want to check whether
> channels is greater than zero, not whether 0 is lower than channels (which it
> sounds like now). Fine as it is though.
> 
>
https://codereview.webrtc.org/1507683006/diff/60001/webrtc/modules/audio_proc...
> File webrtc/modules/audio_processing/noise_suppression_impl.cc (right):
> 
>
https://codereview.webrtc.org/1507683006/diff/60001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/noise_suppression_impl.cc:31: class
> NoiseSuppressionImpl::Suppressor {
> I'd rather call it MonoNoiseSuppressor, or just NoiseSuppressor, because that
is
> what it is. It is not a generic suppressor.

After offline discussions, I'm happy with the name Suppressor.


LGTM

[the sun, 2015-12-08 19:11:55]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2015-12-08 19:11:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org
Link to the patchset: https://codereview.chromium.org/1507683006/#ps40002
(title: "missing explicit")

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

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

[the sun, 2015-12-08 19:13:26]

Description was changed from

==========
Make NoiseSuppression not a processing component (bit exact).

BUG=

patch from issue 1490333004 at patchset 1 (http://crrev.com/1490333004#ps1)
==========

to

==========
Make NoiseSuppression not a processing component (bit exact).

BUG=webrtc:5298

patch from issue 1490333004 at patchset 1 (http://crrev.com/1490333004#ps1)
==========

[commit-bot: I haz the power, 2015-12-08 21:22:35]

Description was changed from

==========
Make NoiseSuppression not a processing component (bit exact).

BUG=webrtc:5298

patch from issue 1490333004 at patchset 1 (http://crrev.com/1490333004#ps1)
==========

to

==========
Make NoiseSuppression not a processing component (bit exact).

BUG=webrtc:5298

patch from issue 1490333004 at patchset 1 (http://crrev.com/1490333004#ps1)
==========

[commit-bot: I haz the power, 2015-12-08 21:22:35]

Committed patchset #5 (id:40002)

[commit-bot: I haz the power, 2015-12-08 21:22:40]

Description was changed from

==========
Make NoiseSuppression not a processing component (bit exact).

BUG=webrtc:5298

patch from issue 1490333004 at patchset 1 (http://crrev.com/1490333004#ps1)
==========

to

==========
Make NoiseSuppression not a processing component (bit exact).

BUG=webrtc:5298

patch from issue 1490333004 at patchset 1 (http://crrev.com/1490333004#ps1)

Committed: https://crrev.com/5e465c33cac54ed5265f18413f7afc44aae2dfca
Cr-Commit-Position: refs/heads/master@{#10944}
==========

[commit-bot: I haz the power, 2015-12-08 21:22:41]

Patchset 5 (id:??) landed as
https://crrev.com/5e465c33cac54ed5265f18413f7afc44aae2dfca
Cr-Commit-Position: refs/heads/master@{#10944}