Ducking fix #1:Initial refactoring preparing for further AEC work (changes are bitexact).

First part of the preparatory work before the actual work for solving the ducking problem starts.

This works aims to:
-More clearly separate the functionalities in the AEC.
-Make the inputs and outputs to functions more clear (currently the state struct is often passed as a parameter to the functions and the functions use members of the state both as inputs and outputs, which reduces the readability of the code and makes it difficult to change/refactor.

What is done in this CL:
-Most of what belongs to the echo subtraction functionality has been moved to a separate function.
-The NonLinearProcessing function has been renamed to EchoSuppressor which I think is more appropriate.
-Part of the code was replaced by a call to the TimeToFrequency function (which was also suggested by an existing todo).
-For consistency, a function FrequencyToTime doing the opposite of TimeToFrequency was added and part of the code was moved to that.
-The ScaleErrorSignal function was changed to no longer have the state as an input parameter. This entailed also changing the corresponding assembly optimized files accordingly.

Testing:
-The changes have been tested for bitexactness on Linux using a fairly extensive test.
-All the unittests pass on linux.

BUG=webrtc:5201
Committed: https://crrev.com/d860523112263f9c8f29b95658c89215fbd95a16
Cr-Commit-Position: refs/heads/master@{#10764}

[peah-webrtc, 2015-11-19 10:18:16]

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

[peah-webrtc, 2015-11-19 10:18:16]

First part of the preparatory work before the actual work for solving the
ducking problem starts.

This works aims to:
-More clearly separate the functionalities in the AEC.
-Make the inputs and outputs to functions more clear (currently the state struct
is often passed as a parameter to the functions and the functions use members of
the state both as inputs and outputs, which reduces the readability of the code
and makes it difficult to change/refactor.

What is done in this CL:
-Most of what belongs to the echo subtraction functionality has been moved to a
separate function.
-The NonLinearProcessing function has been renamed to EchoSuppressor which I
think is more appropriate.
-Part of the code was replaced by a call to the TimeToFrequency function (which
was also suggested by an existing todo).
-For consistency, a function FrequencyToTime doing the opposite of
TimeToFrequency was added and part of the code was moved to that. 
-The ScaleErrorSignal function was changed to no longer have the state as an
input parameter. This entailed also changing the corresponding assembly
optimized files accordingly.

Testing:
-The changes have been tested for bitexactness on Linux using a fairly extensive
test.
-All the unittests pass on linux.

[peah-webrtc, 2015-11-19 10:49:16]

Description was changed from

==========
Initial refactoring preparing for further AEC work (changes are bitexact).

BUG=webrtc:5201
==========

to

==========
Initial refactoring preparing for further AEC work (changes are bitexact).

BUG=webrtc:5201
==========

[hlundin-webrtc, 2015-11-20 10:39:56]

On 2015/11/19 10:18:16, peah-webrtc wrote:
> First part of the preparatory work before the actual work for solving the
> ducking problem starts.
> 
> This works aims to:
> -More clearly separate the functionalities in the AEC.
> -Make the inputs and outputs to functions more clear (currently the state
struct
> is often passed as a parameter to the functions and the functions use members
of
> the state both as inputs and outputs, which reduces the readability of the
code
> and makes it difficult to change/refactor.
> 
> What is done in this CL:
> -Most of what belongs to the echo subtraction functionality has been moved to
a
> separate function.
> -The NonLinearProcessing function has been renamed to EchoSuppressor which I
> think is more appropriate.
> -Part of the code was replaced by a call to the TimeToFrequency function
(which
> was also suggested by an existing todo).
> -For consistency, a function FrequencyToTime doing the opposite of
> TimeToFrequency was added and part of the code was moved to that. 
> -The ScaleErrorSignal function was changed to no longer have the state as an
> input parameter. This entailed also changing the corresponding assembly
> optimized files accordingly.
> 
> Testing:
> -The changes have been tested for bitexactness on Linux using a fairly
extensive
> test.
> -All the unittests pass on linux.

I think most of the description you give in the message to reviewers should go
in the CL description.

[hlundin-webrtc, 2015-11-20 10:55:41]

A few naming nits, comment nits, and a request to copy more from your "message"
into the CL description. But, LGTM.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:181: float *xPow,
Rename xPow -> x_pow while you are at it.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:972: // Produce echo estimate.
... in frequency domain.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:978: // Extract the output signal
and compute the time-domain error
.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec/aec_core_mips.c (right):

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core_mips.c:705: float *xPow,
x_pow

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec/aec_core_neon.c (right):

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core_neon.c:128: float *xPow,
x_pow

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec/aec_core_sse2.c (right):

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core_sse2.c:80: float *xPow,
x_pow

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core_sse2.c:92: const __m128 xPowLocal =
_mm_loadu_ps(&xPow[i]);
x_pow_local

[ivoc, 2015-11-20 10:58:14]

Nice work, see minor comments below. LGTM.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:181: float *xPow,
I know the style guide doesn't cover C-code, but does it makes sense to use
const-ness to indicate input parameters when possible? This one could be float
const * const, I think. Also applies to a lot of other input arguments
elsewhere.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:847: time_data[1] =
freq_data[0][PART_LEN];
Is it me, or are freq_data[1][0] and freq_data[1][PART_LEN] not used at all? Or
is that because the time-domain signal is real-valued?

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:981: y[i] = fft[PART_LEN + i] *
scale;  // fft scaling
Would it make sense to integrate this into the FrequencyToTime function (with
another parameter for the scaling factor)?

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:985: e[i] = nearend_ptr[i] -
y[i];
Could be integrated with the previous loop, for efficiency.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:988: 
2 empty lines is a bit much here (in my opinion).

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:1014: 
2 empty lines is a bit much here (in my opinion).

[peah-webrtc, 2015-11-20 20:25:44]

Description was changed from

==========
Initial refactoring preparing for further AEC work (changes are bitexact).

BUG=webrtc:5201
==========

to

==========
First part of the preparatory work before the actual work for solving the
ducking problem starts.

This works aims to:
-More clearly separate the functionalities in the AEC.
-Make the inputs and outputs to functions more clear (currently the state struct
is often passed as a parameter to the functions and the functions use members of
the state both as inputs and outputs, which reduces the readability of the code
and makes it difficult to change/refactor.

What is done in this CL:
-Most of what belongs to the echo subtraction functionality has been moved to a
separate function.
-The NonLinearProcessing function has been renamed to EchoSuppressor which I
think is more appropriate.
-Part of the code was replaced by a call to the TimeToFrequency function (which
was also suggested by an existing todo).
-For consistency, a function FrequencyToTime doing the opposite of
TimeToFrequency was added and part of the code was moved to that. 
-The ScaleErrorSignal function was changed to no longer have the state as an
input parameter. This entailed also changing the corresponding assembly
optimized files accordingly.

Testing:
-The changes have been tested for bitexactness on Linux using a fairly extensive
test.
-All the unittests pass on linux.

BUG=webrtc:5201
==========

[peah-webrtc, 2015-11-23 21:39:07]

Thanks for the comments!

Here comes the revised code. I also found a name clash (introduced by me) in
aec_core_neon.c that I fixed.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:181: float *xPow,
On 2015/11/20 10:55:40, hlundin-webrtc wrote:
> Rename xPow -> x_pow while you are at it.

Done.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:181: float *xPow,
On 2015/11/20 10:58:13, ivoc wrote:
> I know the style guide doesn't cover C-code, but does it makes sense to use
> const-ness to indicate input parameters when possible? This one could be float
> const * const, I think. Also applies to a lot of other input arguments
> elsewhere.

I got the advice from kwiberg (I think) not to add const specifiers to arguments
that are passed by value as those clutter the function interface for the user of
the function, without adding any value to the user of the function.

For pointers it is different and I totally agree that it would make sense to add
const specifiers to this one, but in this case I prefer to leave the variable as
it is since it is used by optimized code for NEON/MIPS/SSE2 and I don't know how
it would affect that one.
But it could be that I'm too cautious with that.

What do you think, does this make sense?

What do you think, us

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:847: time_data[1] =
freq_data[0][PART_LEN];
On 2015/11/20 10:58:13, ivoc wrote:
> Is it me, or are freq_data[1][0] and freq_data[1][PART_LEN] not used at all?
Or
> is that because the time-domain signal is real-valued?

You are correct, and I think that that format is used in the aec to simplify the
code. It seems like the aec_rdft_inverse function uses the optimized storage
variant for real-valued fft data.
I think the wasted two floats in the array are acceptable if the code gets
easier to read, but long-term I think it could make sense to store the fft data
as an interlieved array so that we don't have to bother with multi-dimensional
arrays.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:972: // Produce echo estimate.
On 2015/11/20 10:55:40, hlundin-webrtc wrote:
> ... in frequency domain.

Done.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:978: // Extract the output signal
and compute the time-domain error
On 2015/11/20 10:55:40, hlundin-webrtc wrote:
> .

Done.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:981: y[i] = fft[PART_LEN + i] *
scale;  // fft scaling
On 2015/11/20 10:58:13, ivoc wrote:
> Would it make sense to integrate this into the FrequencyToTime function (with
> another parameter for the scaling factor)?

I don't think we should, as that loses the symmetry with the TimeToFrequency
function. Furthermore, in one of the upcoming CLs I have renamed these functions
to Ifft and Fft which is really what they are (or at least wrappers of
real-valued such). It could make sense to include the 1/PART_LEN2 term but I
think that would add complexity as it is not necessary to use in all places.

In my mind, this scaling is more related to the block scheme, which requires
this particular scaling a factor 2/PART_LEN2.

Does that make sense?

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:985: e[i] = nearend_ptr[i] -
y[i];
On 2015/11/20 10:58:13, ivoc wrote:
> Could be integrated with the previous loop, for efficiency.

Agree, will do.

Done.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:988: 
On 2015/11/20 10:58:14, ivoc wrote:
> 2 empty lines is a bit much here (in my opinion).

Done.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:1014: 
On 2015/11/20 10:58:13, ivoc wrote:
> 2 empty lines is a bit much here (in my opinion).

Done.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec/aec_core_mips.c (right):

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core_mips.c:705: float *xPow,
On 2015/11/20 10:55:40, hlundin-webrtc wrote:
> x_pow

Done.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec/aec_core_neon.c (right):

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core_neon.c:128: float *xPow,
On 2015/11/20 10:55:40, hlundin-webrtc wrote:
> x_pow

Done.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec/aec_core_sse2.c (right):

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core_sse2.c:80: float *xPow,
On 2015/11/20 10:55:40, hlundin-webrtc wrote:
> x_pow

Done.

https://codereview.webrtc.org/1455163006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core_sse2.c:92: const __m128 xPowLocal =
_mm_loadu_ps(&xPow[i]);
On 2015/11/20 10:55:41, hlundin-webrtc wrote:
> x_pow_local

Done.

[peah-webrtc, 2015-11-23 21:40:32]

Ah, I saw that I already had a LGTM conditional on the comments. Thanks for the
review! Will commit the code now.

[peah-webrtc, 2015-11-23 21:40:55]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2015-11-23 21:40:55]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org,
henrik.lundin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1455163006/#ps20001 (title:
"Changes in response to reviewer comments. Also fixed a name clash in the
optimized code that was cr…")

[commit-bot: I haz the power, 2015-11-23 21:41:11]

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

[commit-bot: I haz the power, 2015-11-23 21:45:15]

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

[commit-bot: I haz the power, 2015-11-23 21:45:16]

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

[peah-webrtc, 2015-11-24 06:10:35]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2015-11-24 06:10:35]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org,
henrik.lundin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1455163006/#ps40001 (title:
"Fixed problem with variable declared inside loop statement")

[commit-bot: I haz the power, 2015-11-24 06:10:38]

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

[commit-bot: I haz the power, 2015-11-24 07:05:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-24 07:05:56]

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