Pull out the PostFilter to its own NonlinearBeamformer API

Pull out the PostFilter to its own NonlinearBeamformer API

This is done to avoid having a nonlinear component in the AEC path.
Now the linear delay and sum is run before the AEC and the postfilter after it.

This change landed originally at: https://codereview.webrtc.org/1982183002/

R=peah@webrtc.org
TBR=henrik.lundin@webrtc.org

Committed: https://crrev.com/f4022ffa1a3b4597cab9d06e83dd493c246b204f
Cr-Commit-Position: refs/heads/master@{#13371}

[aluebs-webrtc, 2016-06-29 00:04:57]

peah, the first patch is identical to what already was reviewed and landed.
Please have a look at the second patch, which re-adds the old interfaces to
support dependencies.

[peah-webrtc, 2016-06-29 08:38:20]

https://codereview.webrtc.org/2110593003/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right):

https://codereview.webrtc.org/2110593003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:427: for
(size_t i = 0u; i < input.num_bands(); ++i) {
Would it be possible to handle the output number of channels in a less
error-prone way.
As it is now, there is an unnecessary dependency between how the user of the
method handles the output.

Would it be possible to either:
1) DCHECK on that the number of channels in output is 1?
2) Set the number of channels in output to 1.
3) Change the memcpy so that the input is downmixed/upmixed to the number of
channels in output.

[hlundin-webrtc, 2016-06-29 09:23:05]

https://codereview.webrtc.org/2110593003/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.h (right):

https://codereview.webrtc.org/2110593003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.h:61: explicit
NonlinearBeamformer(
Nit: should this be explicit still?

[aluebs-webrtc, 2016-06-29 23:19:55]

https://codereview.webrtc.org/2110593003/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right):

https://codereview.webrtc.org/2110593003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:427: for
(size_t i = 0u; i < input.num_bands(); ++i) {
On 2016/06/29 08:38:20, peah-webrtc wrote:
> Would it be possible to handle the output number of channels in a less
> error-prone way.
> As it is now, there is an unnecessary dependency between how the user of the
> method handles the output.
> 
> Would it be possible to either:
> 1) DCHECK on that the number of channels in output is 1?
> 2) Set the number of channels in output to 1.
> 3) Change the memcpy so that the input is downmixed/upmixed to the number of
> channels in output.
> 

To be compatible with how the beamformer was used before this CL (the whole
point of re-adding this interface) we:
1) can't enforce that the number of channels of the output is 1, since it could
be the same channel_buffer than the input.
2) can't change the number of channels of the output, since that wasn't how it
behaved and again it could be the same channel_buffer than the input.

On the old bf interface, the behavior was exactly like this and copying the
number of channels of the output (which can be the same as the input) will add
unecessary complexity. But I guess it ensures it can be used with a different
number of output channels (not supported in the old interface), so I did that.

https://codereview.webrtc.org/2110593003/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.h (right):

https://codereview.webrtc.org/2110593003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.h:61: explicit
NonlinearBeamformer(
On 2016/06/29 09:23:05, hlundin-webrtc wrote:
> Nit: should this be explicit still?

I think so, since it is valid to only receive the array_geometry.

[peah-webrtc, 2016-06-30 05:25:05]

https://codereview.webrtc.org/2110593003/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right):

https://codereview.webrtc.org/2110593003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:427: for
(size_t i = 0u; i < input.num_bands(); ++i) {
On 2016/06/29 23:19:55, aluebs-webrtc wrote:
> On 2016/06/29 08:38:20, peah-webrtc wrote:
> > Would it be possible to handle the output number of channels in a less
> > error-prone way.
> > As it is now, there is an unnecessary dependency between how the user of the
> > method handles the output.
> > 
> > Would it be possible to either:
> > 1) DCHECK on that the number of channels in output is 1?
> > 2) Set the number of channels in output to 1.
> > 3) Change the memcpy so that the input is downmixed/upmixed to the number of
> > channels in output.
> > 
> 
> To be compatible with how the beamformer was used before this CL (the whole
> point of re-adding this interface) we:
> 1) can't enforce that the number of channels of the output is 1, since it
could
> be the same channel_buffer than the input.
> 2) can't change the number of channels of the output, since that wasn't how it
> behaved and again it could be the same channel_buffer than the input.
> 
> On the old bf interface, the behavior was exactly like this and copying the
> number of channels of the output (which can be the same as the input) will add
> unecessary complexity. But I guess it ensures it can be used with a different
> number of output channels (not supported in the old interface), so I did that.

I know this is how it is used to be, but it is quite error-prone and that was
why I wanted to see whether it could be changed.

By only copying the left channel to the output, you are in fact enforcing the
number of channels in the output to 1, as any other channels than the left one
will not be updated. Refraining to set the number of channels to 1 in this part
of the code does not change that. 

I don't really see why it would be a problem to change the output number of
channels when the input and the output refer to the same ChannelBuffer (via an
in-place call). The memcopy operation is valid regardless of that, and the
const-ness of input does not impose any restrictions on output just because they
happen to point to the same ChannelBuffer.

To me, since you anyway here copied only the left channel, you might as well
also set the number of channels to 1 as that is what you really do when you only
copy the left channel but I'm probably missing something.

https://codereview.webrtc.org/2110593003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right):

https://codereview.webrtc.org/2110593003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:428: for
(size_t j = 0u; j < output->num_channels(); ++j) {
This is not ok to do, since output->num_channels() could be larger than
input.num_channels(). Then it is  better to just copy the left channel.


I'm aware that the former implementation of the beamformer had the issue that
the number of channels were reduced to 1 in the output without setting the
number of channels to 1, or checking whether that was the case, and since this
change is temporary, I'm fully fine to keep it like that as that is a
cross-dependency between the user of the beamformer and the beamformer that
takes time to fix.

However, I think it still would make sense to as far as possible ensure that no
memory access error is made.

My suggestion is therefore to either:
1) Keep it as it was, (the one before the last patchset that just copy the left
channel), but add the following  CHECKs (not DCHECKs) 
  CHECK(output->num_channels() > 0);
  CHECK(output->num_frames_per_band() == input->num_frames_per_band());

2) Keep the new variant (the one in the last patchset) but add the following
CHECKs (not DCHECKs).
  CHECK(output->num_channels() <= input->num_channels());
  CHECK(output->num_frames_per_band() == input->num_frames_per_band());


Since you are using a memcpy and the beamformer does not have any control over,
and does not elsewhere in the code check, how output is allocated, I think
CHECKs are required to ensure a proper memory access in the memcpy operation.

WDYT?

[aluebs-webrtc, 2016-06-30 23:37:09]

https://codereview.webrtc.org/2110593003/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right):

https://codereview.webrtc.org/2110593003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:427: for
(size_t i = 0u; i < input.num_bands(); ++i) {
On 2016/06/30 05:25:05, peah-webrtc wrote:
> On 2016/06/29 23:19:55, aluebs-webrtc wrote:
> > On 2016/06/29 08:38:20, peah-webrtc wrote:
> > > Would it be possible to handle the output number of channels in a less
> > > error-prone way.
> > > As it is now, there is an unnecessary dependency between how the user of
the
> > > method handles the output.
> > > 
> > > Would it be possible to either:
> > > 1) DCHECK on that the number of channels in output is 1?
> > > 2) Set the number of channels in output to 1.
> > > 3) Change the memcpy so that the input is downmixed/upmixed to the number
of
> > > channels in output.
> > > 
> > 
> > To be compatible with how the beamformer was used before this CL (the whole
> > point of re-adding this interface) we:
> > 1) can't enforce that the number of channels of the output is 1, since it
> could
> > be the same channel_buffer than the input.
> > 2) can't change the number of channels of the output, since that wasn't how
it
> > behaved and again it could be the same channel_buffer than the input.
> > 
> > On the old bf interface, the behavior was exactly like this and copying the
> > number of channels of the output (which can be the same as the input) will
add
> > unecessary complexity. But I guess it ensures it can be used with a
different
> > number of output channels (not supported in the old interface), so I did
that.
> 
> I know this is how it is used to be, but it is quite error-prone and that was
> why I wanted to see whether it could be changed.
> 
> By only copying the left channel to the output, you are in fact enforcing the
> number of channels in the output to 1, as any other channels than the left one
> will not be updated. Refraining to set the number of channels to 1 in this
part
> of the code does not change that. 
> 
> I don't really see why it would be a problem to change the output number of
> channels when the input and the output refer to the same ChannelBuffer (via an
> in-place call). The memcopy operation is valid regardless of that, and the
> const-ness of input does not impose any restrictions on output just because
they
> happen to point to the same ChannelBuffer.
> 
> To me, since you anyway here copied only the left channel, you might as well
> also set the number of channels to 1 as that is what you really do when you
only
> copy the left channel but I'm probably missing something.
> 

This is a temporary interface to be able to migrate the dependencies without
breaking them and will be removed as soon as we migrate the dependencies. We
shouldn't change the behavior, since that could break the dependencies that were
not expecting the BF to change the channels of its ChannelBuffer.

https://codereview.webrtc.org/2110593003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right):

https://codereview.webrtc.org/2110593003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:428: for
(size_t j = 0u; j < output->num_channels(); ++j) {
On 2016/06/30 05:25:05, peah-webrtc wrote:
> This is not ok to do, since output->num_channels() could be larger than
> input.num_channels(). Then it is  better to just copy the left channel.
> 
> 
> I'm aware that the former implementation of the beamformer had the issue that
> the number of channels were reduced to 1 in the output without setting the
> number of channels to 1, or checking whether that was the case, and since this
> change is temporary, I'm fully fine to keep it like that as that is a
> cross-dependency between the user of the beamformer and the beamformer that
> takes time to fix.
> 
> However, I think it still would make sense to as far as possible ensure that
no
> memory access error is made.
> 
> My suggestion is therefore to either:
> 1) Keep it as it was, (the one before the last patchset that just copy the
left
> channel), but add the following  CHECKs (not DCHECKs) 
>   CHECK(output->num_channels() > 0);
>   CHECK(output->num_frames_per_band() == input->num_frames_per_band());
> 
> 2) Keep the new variant (the one in the last patchset) but add the following
> CHECKs (not DCHECKs).
>   CHECK(output->num_channels() <= input->num_channels());
>   CHECK(output->num_frames_per_band() == input->num_frames_per_band());
> 
> 
> Since you are using a memcpy and the beamformer does not have any control
over,
> and does not elsewhere in the code check, how output is allocated, I think
> CHECKs are required to ensure a proper memory access in the memcpy operation.
> 
> WDYT?

I am going with version 1, since it is what I originally proposed because it is
what was done originally and doesn't add unnecessary complexity by memcpying all
channels.

[peah-webrtc, 2016-07-01 04:47:41]

On 2016/06/30 23:37:09, aluebs-webrtc wrote:
>
https://codereview.webrtc.org/2110593003/diff/20001/webrtc/modules/audio_proc...
> File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc
(right):
> 
>
https://codereview.webrtc.org/2110593003/diff/20001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:427: for
> (size_t i = 0u; i < input.num_bands(); ++i) {
> On 2016/06/30 05:25:05, peah-webrtc wrote:
> > On 2016/06/29 23:19:55, aluebs-webrtc wrote:
> > > On 2016/06/29 08:38:20, peah-webrtc wrote:
> > > > Would it be possible to handle the output number of channels in a less
> > > > error-prone way.
> > > > As it is now, there is an unnecessary dependency between how the user of
> the
> > > > method handles the output.
> > > > 
> > > > Would it be possible to either:
> > > > 1) DCHECK on that the number of channels in output is 1?
> > > > 2) Set the number of channels in output to 1.
> > > > 3) Change the memcpy so that the input is downmixed/upmixed to the
number
> of
> > > > channels in output.
> > > > 
> > > 
> > > To be compatible with how the beamformer was used before this CL (the
whole
> > > point of re-adding this interface) we:
> > > 1) can't enforce that the number of channels of the output is 1, since it
> > could
> > > be the same channel_buffer than the input.
> > > 2) can't change the number of channels of the output, since that wasn't
how
> it
> > > behaved and again it could be the same channel_buffer than the input.
> > > 
> > > On the old bf interface, the behavior was exactly like this and copying
the
> > > number of channels of the output (which can be the same as the input) will
> add
> > > unecessary complexity. But I guess it ensures it can be used with a
> different
> > > number of output channels (not supported in the old interface), so I did
> that.
> > 
> > I know this is how it is used to be, but it is quite error-prone and that
was
> > why I wanted to see whether it could be changed.
> > 
> > By only copying the left channel to the output, you are in fact enforcing
the
> > number of channels in the output to 1, as any other channels than the left
one
> > will not be updated. Refraining to set the number of channels to 1 in this
> part
> > of the code does not change that. 
> > 
> > I don't really see why it would be a problem to change the output number of
> > channels when the input and the output refer to the same ChannelBuffer (via
an
> > in-place call). The memcopy operation is valid regardless of that, and the
> > const-ness of input does not impose any restrictions on output just because
> they
> > happen to point to the same ChannelBuffer.
> > 
> > To me, since you anyway here copied only the left channel, you might as well
> > also set the number of channels to 1 as that is what you really do when you
> only
> > copy the left channel but I'm probably missing something.
> > 
> 
> This is a temporary interface to be able to migrate the dependencies without
> breaking them and will be removed as soon as we migrate the dependencies. We
> shouldn't change the behavior, since that could break the dependencies that
were
> not expecting the BF to change the channels of its ChannelBuffer.
> 
>
https://codereview.webrtc.org/2110593003/diff/40001/webrtc/modules/audio_proc...
> File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc
(right):
> 
>
https://codereview.webrtc.org/2110593003/diff/40001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:428: for
> (size_t j = 0u; j < output->num_channels(); ++j) {
> On 2016/06/30 05:25:05, peah-webrtc wrote:
> > This is not ok to do, since output->num_channels() could be larger than
> > input.num_channels(). Then it is  better to just copy the left channel.
> > 
> > 
> > I'm aware that the former implementation of the beamformer had the issue
that
> > the number of channels were reduced to 1 in the output without setting the
> > number of channels to 1, or checking whether that was the case, and since
this
> > change is temporary, I'm fully fine to keep it like that as that is a
> > cross-dependency between the user of the beamformer and the beamformer that
> > takes time to fix.
> > 
> > However, I think it still would make sense to as far as possible ensure that
> no
> > memory access error is made.
> > 
> > My suggestion is therefore to either:
> > 1) Keep it as it was, (the one before the last patchset that just copy the
> left
> > channel), but add the following  CHECKs (not DCHECKs) 
> >   CHECK(output->num_channels() > 0);
> >   CHECK(output->num_frames_per_band() == input->num_frames_per_band());
> > 
> > 2) Keep the new variant (the one in the last patchset) but add the following
> > CHECKs (not DCHECKs).
> >   CHECK(output->num_channels() <= input->num_channels());
> >   CHECK(output->num_frames_per_band() == input->num_frames_per_band());
> > 
> > 
> > Since you are using a memcpy and the beamformer does not have any control
> over,
> > and does not elsewhere in the code check, how output is allocated, I think
> > CHECKs are required to ensure a proper memory access in the memcpy
operation.
> > 
> > WDYT?
> 
> I am going with version 1, since it is what I originally proposed because it
is
> what was done originally and doesn't add unnecessary complexity by memcpying
all
> channels.

lgtm, but please consider my comment about the number of channels and about
potential uninitialized memory access.

[peah-webrtc, 2016-07-01 05:31:48]

https://codereview.webrtc.org/2110593003/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right):

https://codereview.webrtc.org/2110593003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:433:
PostFilter(output);
I'll lgtm this since this is just temporary, the DCHECKS are in place, there
cannot be any memory corruption, and there cannot be any out-of-bounds memory
access.

In PostFilter, there is a DCHECK with
RTC_DCHECK_GE(data->num_channels(), num_postfilter_channels_);

That means that the number of channels in output must be predefined during
beamformer construction to num_postfilter_channels_. This in turn means that it
should be fully safe for the beamformer to explicitly set the number of channels
 specified by num_postfilter_channels_.

The only case when the output would need to be set explicitly to 1 channel
(which was how it was done previously in APM) is if 
a) output has 2 channels before the call to ProcessChunk and
b) the beamformer has been specified to have 2 num_postfilter_channels
That does not make sense, as then anyway num_postfilter_channels could as easily
have been specified to be 1 in the first place.

Furthermore, if output has the number of channels set to 2, I'd expect
PostFilter to possibly access uninitialized data as it processes all channels
present in output, but the memcopy only copies data into the left channel.

I'll leave it up to you to decide, but I cannot see why we cannot set the number
of channels inside the beamformer in the current beamformer implementation (as
we anyway DCHECK against it). Furthermore, if the number of channels would be
preset to anything by 1, I'd expect the memory access tests to fail for this.

[aluebs-webrtc, 2016-07-01 22:15:34]

https://codereview.webrtc.org/2110593003/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right):

https://codereview.webrtc.org/2110593003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:433:
PostFilter(output);
On 2016/07/01 05:31:48, peah-webrtc wrote:
> I'll lgtm this since this is just temporary, the DCHECKS are in place, there
> cannot be any memory corruption, and there cannot be any out-of-bounds memory
> access.
> 
> In PostFilter, there is a DCHECK with
> RTC_DCHECK_GE(data->num_channels(), num_postfilter_channels_);
> 
> That means that the number of channels in output must be predefined during
> beamformer construction to num_postfilter_channels_. This in turn means that
it
> should be fully safe for the beamformer to explicitly set the number of
channels
>  specified by num_postfilter_channels_.
> 
> The only case when the output would need to be set explicitly to 1 channel
> (which was how it was done previously in APM) is if 
> a) output has 2 channels before the call to ProcessChunk and
> b) the beamformer has been specified to have 2 num_postfilter_channels
> That does not make sense, as then anyway num_postfilter_channels could as
easily
> have been specified to be 1 in the first place.
> 
> Furthermore, if output has the number of channels set to 2, I'd expect
> PostFilter to possibly access uninitialized data as it processes all channels
> present in output, but the memcopy only copies data into the left channel.
> 
> I'll leave it up to you to decide, but I cannot see why we cannot set the
number
> of channels inside the beamformer in the current beamformer implementation (as
> we anyway DCHECK against it). Furthermore, if the number of channels would be
> preset to anything by 1, I'd expect the memory access tests to fail for this.
> 

The CHECK is to see that it is greater or equal. I want to leave the temporary
interface to have the same behavior as the original.

[aluebs-webrtc, 2016-07-01 22:15:38]

The CQ bit was checked by aluebs@webrtc.org

[commit-bot: I haz the power, 2016-07-01 22:15:51]

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

[commit-bot: I haz the power, 2016-07-02 00:16:17]

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

[commit-bot: I haz the power, 2016-07-02 00:16:18]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[aluebs-webrtc, 2016-07-02 00:19:38]

Description was changed from

==========
Pull out the PostFilter to its own NonlinearBeamformer API

This is done to avoid having a nonlinear component in the AEC path.
Now the linear delay and sum is run before the AEC and the postfilter after it.

This change landed originally at: https://codereview.webrtc.org/1982183002/

R=peah@webrtc.org
TBR=henrik.lundin@webrtc.org
==========

to

==========
Pull out the PostFilter to its own NonlinearBeamformer API

This is done to avoid having a nonlinear component in the AEC path.
Now the linear delay and sum is run before the AEC and the postfilter after it.

This change landed originally at: https://codereview.webrtc.org/1982183002/

R=peah@webrtc.org
TBR=henrik.lundin@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/f4022ffa1a3b4597cab9d06e8...
==========

[commit-bot: I haz the power, 2016-07-02 00:19:38]

Description was changed from

==========
Pull out the PostFilter to its own NonlinearBeamformer API

This is done to avoid having a nonlinear component in the AEC path.
Now the linear delay and sum is run before the AEC and the postfilter after it.

This change landed originally at: https://codereview.webrtc.org/1982183002/

R=peah@webrtc.org
TBR=henrik.lundin@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/f4022ffa1a3b4597cab9d06e8...
==========

to

==========
Pull out the PostFilter to its own NonlinearBeamformer API

This is done to avoid having a nonlinear component in the AEC path.
Now the linear delay and sum is run before the AEC and the postfilter after it.

This change landed originally at: https://codereview.webrtc.org/1982183002/

R=peah@webrtc.org
TBR=henrik.lundin@webrtc.org

Committed: https://crrev.com/f4022ffa1a3b4597cab9d06e83dd493c246b204f
Cr-Commit-Position: refs/heads/master@{#13371}
==========

[aluebs-webrtc, 2016-07-02 00:19:39]

Committed patchset #5 (id:80001) manually as
f4022ffa1a3b4597cab9d06e83dd493c246b204f (presubmit successful).

[commit-bot: I haz the power, 2016-07-02 00:19:39]

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