Refactoring (bitexact) of the EchoSuppressor in WebRTC AEC (#1)

Moved code into the lowest level of EchoSuppression
to simplify future refactoring and development.

In more detail:
1) Moved the updating of eBuf from the EchoSubtraction method
   to the EchoSuppression method as it is only used in the latter.
2) Moved the computation of efw and dfw from the SubbandCoherence method
   as those are actually the analysis filterbank computation that is not
   directly related to the coherence.
3) As a consequence of 2) 3 functions needed to be replaced by the
   generic function pointer scheme used in WebRTCAec as they have
   optimized versions for SSE2 and NEON (which before were local to each
   of the aec_core*.c files.

Motivation:
Apart from making sense from a logical point of view, the changes will
a) Allow eBuf stored in half the size on the state.
b) Allow simpler switching between using the the microphone signal
   and echo subtractor output in the echo suppressor.
c) Allow further refactoring that move all the changes to eBuf to one method
   (currently those are happening in at least 4 different methods.

Drawbacks:
i) dfw is moved to EchoSuppression which increases the stack usage for that
 method. This will, however, be improved once further refactoring can be done.

The changes have been tested for bitexactness on Linux using a quite extensive dataset.

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

[peah-webrtc, 2015-12-02 15:17:57]

Description was changed from

==========
Moved code from into lowest level of EchoSuppression
to simplify future refactoring and development.

In more detail:
1) Moved the updating of eBuf from the EchoSubtraction method
   to the EchoSuppression method as it is only used in the latter.
2) Moved the computation of efw and dfw from the SubbandCoherence method
   as those are actually the analysis filterbank computation that is not
   directly related to the coherence.
3) As a consequence of 2) 3 functions needed to be replaced by the
   generic function pointer scheme used in WebRTCAec as they have
   optimized versions for SSE2 and NEON (which before were local to each
   of the aec_core*.c files.

Motivation:
Apart from making sense from a logical point of view, the changes will
a) Allow eBuf stored in half the size on the state.
b) Allow simpler switching between using the the microphone signal
   and echo subtractor output in the echo suppressor.
c) Allow further refactoring that move all the changes to eBuf to one method
   (currently those are happening in at least 4 different methods.

Drawbacks:
i) dfw is moved to EchoSuppression which increases the stack usage for that
 method. This will, however, be improved once further refactoring can be done.


The changes have been tested for bitexactness on Linux




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

to

==========
Moved code into the lowest level of EchoSuppression
to simplify future refactoring and development.

In more detail:
1) Moved the updating of eBuf from the EchoSubtraction method
   to the EchoSuppression method as it is only used in the latter.
2) Moved the computation of efw and dfw from the SubbandCoherence method
   as those are actually the analysis filterbank computation that is not
   directly related to the coherence.
3) As a consequence of 2) 3 functions needed to be replaced by the
   generic function pointer scheme used in WebRTCAec as they have
   optimized versions for SSE2 and NEON (which before were local to each
   of the aec_core*.c files.

Motivation:
Apart from making sense from a logical point of view, the changes will
a) Allow eBuf stored in half the size on the state.
b) Allow simpler switching between using the the microphone signal
   and echo subtractor output in the echo suppressor.
c) Allow further refactoring that move all the changes to eBuf to one method
   (currently those are happening in at least 4 different methods.

Drawbacks:
i) dfw is moved to EchoSuppression which increases the stack usage for that
 method. This will, however, be improved once further refactoring can be done.


The changes have been tested for bitexactness on Linux using a quite extensive
dataset.


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

[peah-webrtc, 2015-12-02 15:17:57]

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

[peah-webrtc, 2015-12-03 10:23:38]

Description was changed from

==========
Moved code into the lowest level of EchoSuppression
to simplify future refactoring and development.

In more detail:
1) Moved the updating of eBuf from the EchoSubtraction method
   to the EchoSuppression method as it is only used in the latter.
2) Moved the computation of efw and dfw from the SubbandCoherence method
   as those are actually the analysis filterbank computation that is not
   directly related to the coherence.
3) As a consequence of 2) 3 functions needed to be replaced by the
   generic function pointer scheme used in WebRTCAec as they have
   optimized versions for SSE2 and NEON (which before were local to each
   of the aec_core*.c files.

Motivation:
Apart from making sense from a logical point of view, the changes will
a) Allow eBuf stored in half the size on the state.
b) Allow simpler switching between using the the microphone signal
   and echo subtractor output in the echo suppressor.
c) Allow further refactoring that move all the changes to eBuf to one method
   (currently those are happening in at least 4 different methods.

Drawbacks:
i) dfw is moved to EchoSuppression which increases the stack usage for that
 method. This will, however, be improved once further refactoring can be done.


The changes have been tested for bitexactness on Linux using a quite extensive
dataset.


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

to

==========
Moved code into the lowest level of EchoSuppression
to simplify future refactoring and development.

In more detail:
1) Moved the updating of eBuf from the EchoSubtraction method
   to the EchoSuppression method as it is only used in the latter.
2) Moved the computation of efw and dfw from the SubbandCoherence method
   as those are actually the analysis filterbank computation that is not
   directly related to the coherence.
3) As a consequence of 2) 3 functions needed to be replaced by the
   generic function pointer scheme used in WebRTCAec as they have
   optimized versions for SSE2 and NEON (which before were local to each
   of the aec_core*.c files.

Motivation:
Apart from making sense from a logical point of view, the changes will
a) Allow eBuf stored in half the size on the state.
b) Allow simpler switching between using the the microphone signal
   and echo subtractor output in the echo suppressor.
c) Allow further refactoring that move all the changes to eBuf to one method
   (currently those are happening in at least 4 different methods.

Drawbacks:
i) dfw is moved to EchoSuppression which increases the stack usage for that
 method. This will, however, be improved once further refactoring can be done.


The changes have been tested for bitexactness on Linux using a quite extensive
dataset.


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

[minyue-webrtc, 2015-12-03 12:42:51]

Good, just one comment from me

https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec/aec_core.c (left):

https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec/aec_core.c:1402: memcpy(aec->eBuf +
PART_LEN,
This is ok, but it makes the treatment of eBuf different from dBuf (and other)

                Filling-in new data   |   Updating history
______________________________________________
eBuf    |       EchoSuppression       |  EchoSuppression
______________________________________________
dBuf    |       ProcessBlock          |  EchoSuppression

The left-down corner (ProcessBlock) is not movable. I personally think it would
make better sense to move all operations to EchoSuppression because
1. more consistent and clear,
2. the handling of eBuf/dBuf does not sound to belong to EchoSuppression,
3. We may likely use eBuf/dBuf in ProcessBlock, at some point.

[peah-webrtc, 2015-12-03 15:00:36]

https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec/aec_core.c (left):

https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec/aec_core.c:1402: memcpy(aec->eBuf +
PART_LEN,
On 2015/12/03 12:42:51, minyue-webrtc wrote:
> This is ok, but it makes the treatment of eBuf different from dBuf (and other)
> 
>                 Filling-in new data   |   Updating history
> ______________________________________________
> eBuf    |       EchoSuppression       |  EchoSuppression
> ______________________________________________
> dBuf    |       ProcessBlock          |  EchoSuppression
> 
> The left-down corner (ProcessBlock) is not movable. I personally think it
would
> make better sense to move all operations to EchoSuppression because
> 1. more consistent and clear,
> 2. the handling of eBuf/dBuf does not sound to belong to EchoSuppression,
> 3. We may likely use eBuf/dBuf in ProcessBlock, at some point.

I guess that what you are saying is that all operations should move from
EchoSuppression to ProcessBlock, right (and not the other way around).

1) For eBuf I think it is more clear to do the actual buffering close to where
it is used. We anyway cannot do the filling in of new data in the same place as
where that happens for dBuf so I don't think we can achieve any consistency gain
by moving it to ProcessBlock. 

2) For dBuf I agree that the updating of the history should be moved to
ProcessBlock.
But for eBuf, it is currently not used by anything other than EchoSuppression
and therefore I think it should belong there. 

3) If we need it for some future use we could refactor that then, but currently
I don't see any need for it and if you don't see any immediate place where to
use it, I don't think we should plan for that.

[minyue-webrtc, 2015-12-04 07:28:04]

On 2015/12/03 15:00:36, peah-webrtc wrote:
>
https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
> File webrtc/modules/audio_processing/aec/aec_core.c (left):
> 
>
https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/aec/aec_core.c:1402: memcpy(aec->eBuf +
> PART_LEN,
> On 2015/12/03 12:42:51, minyue-webrtc wrote:
> > This is ok, but it makes the treatment of eBuf different from dBuf (and
other)
> > 
> >                 Filling-in new data   |   Updating history
> > ______________________________________________
> > eBuf    |       EchoSuppression       |  EchoSuppression
> > ______________________________________________
> > dBuf    |       ProcessBlock          |  EchoSuppression
> > 
> > The left-down corner (ProcessBlock) is not movable. I personally think it
> would
> > make better sense to move all operations to EchoSuppression because
> > 1. more consistent and clear,
> > 2. the handling of eBuf/dBuf does not sound to belong to EchoSuppression,
> > 3. We may likely use eBuf/dBuf in ProcessBlock, at some point.
> 
> I guess that what you are saying is that all operations should move from
> EchoSuppression to ProcessBlock, right (and not the other way around).
> 
> 1) For eBuf I think it is more clear to do the actual buffering close to where
> it is used. We anyway cannot do the filling in of new data in the same place
as
> where that happens for dBuf so I don't think we can achieve any consistency
gain
> by moving it to ProcessBlock. 
> 
> 2) For dBuf I agree that the updating of the history should be moved to
> ProcessBlock.
> But for eBuf, it is currently not used by anything other than EchoSuppression
> and therefore I think it should belong there. 
> 
> 3) If we need it for some future use we could refactor that then, but
currently
> I don't see any need for it and if you don't see any immediate place where to
> use it, I don't think we should plan for that.

Sorry, I indeed mistyped ProcessBlock by EchoSuppression. Thanking for thinking
of making the operations to dBuf in ProcessBlock. It will make it clear.

For 3), I actually plan to use eBuf to update linoutlevel, as now using dBuf to
update nearlevel.
Currently, linoutlevel is updated from e_fft, which does not have buffered data.
But dBuf uses buffered data.

Nevertheless, I think what you proposed is fine. linoutlevel can also be updated
in EchoSuppression (but there is still an awkwardness, i.e., we cannot put
UpdateLevel together)

[peah-webrtc, 2015-12-04 09:34:46]

On 2015/12/04 07:28:04, minyue-webrtc wrote:
> On 2015/12/03 15:00:36, peah-webrtc wrote:
> >
>
https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
> > File webrtc/modules/audio_processing/aec/aec_core.c (left):
> > 
> >
>
https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
> > webrtc/modules/audio_processing/aec/aec_core.c:1402: memcpy(aec->eBuf +
> > PART_LEN,
> > On 2015/12/03 12:42:51, minyue-webrtc wrote:
> > > This is ok, but it makes the treatment of eBuf different from dBuf (and
> other)
> > > 
> > >                 Filling-in new data   |   Updating history
> > > ______________________________________________
> > > eBuf    |       EchoSuppression       |  EchoSuppression
> > > ______________________________________________
> > > dBuf    |       ProcessBlock          |  EchoSuppression
> > > 
> > > The left-down corner (ProcessBlock) is not movable. I personally think it
> > would
> > > make better sense to move all operations to EchoSuppression because
> > > 1. more consistent and clear,
> > > 2. the handling of eBuf/dBuf does not sound to belong to EchoSuppression,
> > > 3. We may likely use eBuf/dBuf in ProcessBlock, at some point.
> > 
> > I guess that what you are saying is that all operations should move from
> > EchoSuppression to ProcessBlock, right (and not the other way around).
> > 
> > 1) For eBuf I think it is more clear to do the actual buffering close to
where
> > it is used. We anyway cannot do the filling in of new data in the same place
> as
> > where that happens for dBuf so I don't think we can achieve any consistency
> gain
> > by moving it to ProcessBlock. 
> > 
> > 2) For dBuf I agree that the updating of the history should be moved to
> > ProcessBlock.
> > But for eBuf, it is currently not used by anything other than
EchoSuppression
> > and therefore I think it should belong there. 
> > 
> > 3) If we need it for some future use we could refactor that then, but
> currently
> > I don't see any need for it and if you don't see any immediate place where
to
> > use it, I don't think we should plan for that.
> 
> Sorry, I indeed mistyped ProcessBlock by EchoSuppression. Thanking for
thinking
> of making the operations to dBuf in ProcessBlock. It will make it clear.
> 
> For 3), I actually plan to use eBuf to update linoutlevel, as now using dBuf
to
> update nearlevel.
> Currently, linoutlevel is updated from e_fft, which does not have buffered
data.
> But dBuf uses buffered data.
> 
> Nevertheless, I think what you proposed is fine. linoutlevel can also be
updated
> in EchoSuppression (but there is still an awkwardness, i.e., we cannot put
> UpdateLevel together)

Would it be an option to update linoutlevel inside the EchoSuppression and have
it as an output parameter from the echo suppressor, similarly to how linoutlevel
is currently computed inside EchoSubtraction and returned as an output
parameter?

[minyue-webrtc, 2015-12-04 09:35:34]

On 2015/12/04 09:34:46, peah-webrtc wrote:
> On 2015/12/04 07:28:04, minyue-webrtc wrote:
> > On 2015/12/03 15:00:36, peah-webrtc wrote:
> > >
> >
>
https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
> > > File webrtc/modules/audio_processing/aec/aec_core.c (left):
> > > 
> > >
> >
>
https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
> > > webrtc/modules/audio_processing/aec/aec_core.c:1402: memcpy(aec->eBuf +
> > > PART_LEN,
> > > On 2015/12/03 12:42:51, minyue-webrtc wrote:
> > > > This is ok, but it makes the treatment of eBuf different from dBuf (and
> > other)
> > > > 
> > > >                 Filling-in new data   |   Updating history
> > > > ______________________________________________
> > > > eBuf    |       EchoSuppression       |  EchoSuppression
> > > > ______________________________________________
> > > > dBuf    |       ProcessBlock          |  EchoSuppression
> > > > 
> > > > The left-down corner (ProcessBlock) is not movable. I personally think
it
> > > would
> > > > make better sense to move all operations to EchoSuppression because
> > > > 1. more consistent and clear,
> > > > 2. the handling of eBuf/dBuf does not sound to belong to
EchoSuppression,
> > > > 3. We may likely use eBuf/dBuf in ProcessBlock, at some point.
> > > 
> > > I guess that what you are saying is that all operations should move from
> > > EchoSuppression to ProcessBlock, right (and not the other way around).
> > > 
> > > 1) For eBuf I think it is more clear to do the actual buffering close to
> where
> > > it is used. We anyway cannot do the filling in of new data in the same
place
> > as
> > > where that happens for dBuf so I don't think we can achieve any
consistency
> > gain
> > > by moving it to ProcessBlock. 
> > > 
> > > 2) For dBuf I agree that the updating of the history should be moved to
> > > ProcessBlock.
> > > But for eBuf, it is currently not used by anything other than
> EchoSuppression
> > > and therefore I think it should belong there. 
> > > 
> > > 3) If we need it for some future use we could refactor that then, but
> > currently
> > > I don't see any need for it and if you don't see any immediate place where
> to
> > > use it, I don't think we should plan for that.
> > 
> > Sorry, I indeed mistyped ProcessBlock by EchoSuppression. Thanking for
> thinking
> > of making the operations to dBuf in ProcessBlock. It will make it clear.
> > 
> > For 3), I actually plan to use eBuf to update linoutlevel, as now using dBuf
> to
> > update nearlevel.
> > Currently, linoutlevel is updated from e_fft, which does not have buffered
> data.
> > But dBuf uses buffered data.
> > 
> > Nevertheless, I think what you proposed is fine. linoutlevel can also be
> updated
> > in EchoSuppression (but there is still an awkwardness, i.e., we cannot put
> > UpdateLevel together)
> 
> Would it be an option to update linoutlevel inside the EchoSuppression and
have
> it as an output parameter from the echo suppressor, similarly to how
linoutlevel
> is currently computed inside EchoSubtraction and returned as an output
> parameter?

sure I think that is good.

[hlundin-webrtc, 2015-12-04 09:56:58]

LG, but I have two questions.

https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec/aec_core.c:1031: // Update eBuf with echo
subtractor output.
Is there any reason you moved this block earlier than it was before? The natural
position seems to me to be just before calling WebRtcAec_SubbandCoherence below.

https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec/aec_core.c:1040:
WebRtcAec_StoreAsComplex(fft, dfw);
Why did this function change to WebRtcAec_*, but not the one below?

[peah-webrtc, 2015-12-04 14:14:49]

https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec/aec_core.c:1031: // Update eBuf with echo
subtractor output.
On 2015/12/04 09:56:58, hlundin-webrtc wrote:
> Is there any reason you moved this block earlier than it was before? The
natural
> position seems to me to be just before calling WebRtcAec_SubbandCoherence
below.

I'm a bit confused, afaics it has been moved later. It was before called before
EchoSuppression(), right? And since it is only used in echosuppression() I think
it should be updated there only.

https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec/aec_core.c:1040:
WebRtcAec_StoreAsComplex(fft, dfw);
On 2015/12/04 09:56:58, hlundin-webrtc wrote:
> Why did this function change to WebRtcAec_*, but not the one below?

Good find! Initially neither had the WebRtcAec_ prefix (which means it has an
optimized counterpart). I assume there was a reason for that (maybe the
optimized counterpart is broken) so I'll change both to use the local version
(as I cannot verify the correctness of the optimized code).

Does that sound ok?

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

https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec/aec_core.c:1031: // Update eBuf with echo
subtractor output.
On 2015/12/04 14:14:49, peah-webrtc wrote:
> On 2015/12/04 09:56:58, hlundin-webrtc wrote:
> > Is there any reason you moved this block earlier than it was before? The
> natural
> > position seems to me to be just before calling WebRtcAec_SubbandCoherence
> below.
> 
> I'm a bit confused, afaics it has been moved later. It was before called
before
> EchoSuppression(), right? And since it is only used in echosuppression() I
think
> it should be updated there only.

What I meant was this: What used to be one block of code (lines 417-433 in
SubbandCoherence) is now split into two blocks; this block and the one at line
1067 below. The order of these two subblocks of code has changed compared with
the original.

https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec/aec_core.c:1040:
WebRtcAec_StoreAsComplex(fft, dfw);
On 2015/12/04 14:14:49, peah-webrtc wrote:
> On 2015/12/04 09:56:58, hlundin-webrtc wrote:
> > Why did this function change to WebRtcAec_*, but not the one below?
> 
> Good find! Initially neither had the WebRtcAec_ prefix (which means it has an
> optimized counterpart). I assume there was a reason for that (maybe the
> optimized counterpart is broken) so I'll change both to use the local version
> (as I cannot verify the correctness of the optimized code).
> 
> Does that sound ok?

Acknowledged.

[peah-webrtc, 2015-12-04 22:37:52]

https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec/aec_core.c:1031: // Update eBuf with echo
subtractor output.
On 2015/12/04 15:04:04, hlundin-webrtc wrote:
> On 2015/12/04 14:14:49, peah-webrtc wrote:
> > On 2015/12/04 09:56:58, hlundin-webrtc wrote:
> > > Is there any reason you moved this block earlier than it was before? The
> > natural
> > > position seems to me to be just before calling WebRtcAec_SubbandCoherence
> > below.
> > 
> > I'm a bit confused, afaics it has been moved later. It was before called
> before
> > EchoSuppression(), right? And since it is only used in echosuppression() I
> think
> > it should be updated there only.
> 
> What I meant was this: What used to be one block of code (lines 417-433 in
> SubbandCoherence) is now split into two blocks; this block and the one at line
> 1067 below. The order of these two subblocks of code has changed compared with
> the original.

Ah, then I see :-) Sorry.


Firstly, note that the initialization of the comfort noise will be moved in an
upcoming CL as it is placed far away from where it is actually used. When moving
that, what remains before the call to SubbandCoherence all are preparations of
the inputs to that function. Also, the lines 1047-1050 will be bundled with the
1067-1068. And the updates of the buffers eBuf and xfwBuf will be moved as well.

I like to think of the echo suppressor as something that operates fully in the
filterbank domain, and then it feels natural for me to as far as possible have
it as an analysis filterbank followed by processing and then a synthesis
filterbank. This is one of the main operations in the EchoSuppressor and since
it can be arbitrarily placed among the input preparations to the
SubbandCoherence function I think it makes sense to put it in the beginning.

Does it make sense?

[hlundin-webrtc, 2015-12-08 11:48:52]

lgtm

https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec/aec_core.c:1031: // Update eBuf with echo
subtractor output.
On 2015/12/04 22:37:52, peah-webrtc wrote:
> On 2015/12/04 15:04:04, hlundin-webrtc wrote:
> > On 2015/12/04 14:14:49, peah-webrtc wrote:
> > > On 2015/12/04 09:56:58, hlundin-webrtc wrote:
> > > > Is there any reason you moved this block earlier than it was before? The
> > > natural
> > > > position seems to me to be just before calling
WebRtcAec_SubbandCoherence
> > > below.
> > > 
> > > I'm a bit confused, afaics it has been moved later. It was before called
> > before
> > > EchoSuppression(), right? And since it is only used in echosuppression() I
> > think
> > > it should be updated there only.
> > 
> > What I meant was this: What used to be one block of code (lines 417-433 in
> > SubbandCoherence) is now split into two blocks; this block and the one at
line
> > 1067 below. The order of these two subblocks of code has changed compared
with
> > the original.
> 
> Ah, then I see :-) Sorry.
> 
> 
> Firstly, note that the initialization of the comfort noise will be moved in an
> upcoming CL as it is placed far away from where it is actually used. When
moving
> that, what remains before the call to SubbandCoherence all are preparations of
> the inputs to that function. Also, the lines 1047-1050 will be bundled with
the
> 1067-1068. And the updates of the buffers eBuf and xfwBuf will be moved as
well.
> 
> I like to think of the echo suppressor as something that operates fully in the
> filterbank domain, and then it feels natural for me to as far as possible have
> it as an analysis filterbank followed by processing and then a synthesis
> filterbank. This is one of the main operations in the EchoSuppressor and since
> it can be arbitrarily placed among the input preparations to the
> SubbandCoherence function I think it makes sense to put it in the beginning.
> 
> Does it make sense?

That makes sense. Thanks!

[minyue-webrtc, 2015-12-08 12:27:26]

On 2015/12/04 07:28:04, minyue-webrtc wrote:
> On 2015/12/03 15:00:36, peah-webrtc wrote:
> >
>
https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
> > File webrtc/modules/audio_processing/aec/aec_core.c (left):
> > 
> >
>
https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
> > webrtc/modules/audio_processing/aec/aec_core.c:1402: memcpy(aec->eBuf +
> > PART_LEN,
> > On 2015/12/03 12:42:51, minyue-webrtc wrote:
> > > This is ok, but it makes the treatment of eBuf different from dBuf (and
> other)
> > > 
> > >                 Filling-in new data   |   Updating history
> > > ______________________________________________
> > > eBuf    |       EchoSuppression       |  EchoSuppression
> > > ______________________________________________
> > > dBuf    |       ProcessBlock          |  EchoSuppression
> > > 
> > > The left-down corner (ProcessBlock) is not movable. I personally think it
> > would
> > > make better sense to move all operations to EchoSuppression because
> > > 1. more consistent and clear,
> > > 2. the handling of eBuf/dBuf does not sound to belong to EchoSuppression,
> > > 3. We may likely use eBuf/dBuf in ProcessBlock, at some point.
> > 
> > I guess that what you are saying is that all operations should move from
> > EchoSuppression to ProcessBlock, right (and not the other way around).
> > 
> > 1) For eBuf I think it is more clear to do the actual buffering close to
where
> > it is used. We anyway cannot do the filling in of new data in the same place
> as
> > where that happens for dBuf so I don't think we can achieve any consistency
> gain
> > by moving it to ProcessBlock. 
> > 
> > 2) For dBuf I agree that the updating of the history should be moved to
> > ProcessBlock.

Do you want to move update on dBuf to ProcessBlock() in this CL?

> > But for eBuf, it is currently not used by anything other than
EchoSuppression
> > and therefore I think it should belong there. 
> > 
> > 3) If we need it for some future use we could refactor that then, but
> currently
> > I don't see any need for it and if you don't see any immediate place where
to
> > use it, I don't think we should plan for that.
> 
> Sorry, I indeed mistyped ProcessBlock by EchoSuppression. Thanking for
thinking
> of making the operations to dBuf in ProcessBlock. It will make it clear.
> 
> For 3), I actually plan to use eBuf to update linoutlevel, as now using dBuf
to
> update nearlevel.
> Currently, linoutlevel is updated from e_fft, which does not have buffered
data.
> But dBuf uses buffered data.
> 
> Nevertheless, I think what you proposed is fine. linoutlevel can also be
updated
> in EchoSuppression (but there is still an awkwardness, i.e., we cannot put
> UpdateLevel together)

[minyue-webrtc, 2015-12-08 13:24:18]

On 2015/12/08 12:27:26, minyue-webrtc wrote:
> On 2015/12/04 07:28:04, minyue-webrtc wrote:
> > On 2015/12/03 15:00:36, peah-webrtc wrote:
> > >
> >
>
https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
> > > File webrtc/modules/audio_processing/aec/aec_core.c (left):
> > > 
> > >
> >
>
https://codereview.webrtc.org/1494563002/diff/20001/webrtc/modules/audio_proc...
> > > webrtc/modules/audio_processing/aec/aec_core.c:1402: memcpy(aec->eBuf +
> > > PART_LEN,
> > > On 2015/12/03 12:42:51, minyue-webrtc wrote:
> > > > This is ok, but it makes the treatment of eBuf different from dBuf (and
> > other)
> > > > 
> > > >                 Filling-in new data   |   Updating history
> > > > ______________________________________________
> > > > eBuf    |       EchoSuppression       |  EchoSuppression
> > > > ______________________________________________
> > > > dBuf    |       ProcessBlock          |  EchoSuppression
> > > > 
> > > > The left-down corner (ProcessBlock) is not movable. I personally think
it
> > > would
> > > > make better sense to move all operations to EchoSuppression because
> > > > 1. more consistent and clear,
> > > > 2. the handling of eBuf/dBuf does not sound to belong to
EchoSuppression,
> > > > 3. We may likely use eBuf/dBuf in ProcessBlock, at some point.
> > > 
> > > I guess that what you are saying is that all operations should move from
> > > EchoSuppression to ProcessBlock, right (and not the other way around).
> > > 
> > > 1) For eBuf I think it is more clear to do the actual buffering close to
> where
> > > it is used. We anyway cannot do the filling in of new data in the same
place
> > as
> > > where that happens for dBuf so I don't think we can achieve any
consistency
> > gain
> > > by moving it to ProcessBlock. 
> > > 
> > > 2) For dBuf I agree that the updating of the history should be moved to
> > > ProcessBlock.
> 
> Do you want to move update on dBuf to ProcessBlock() in this CL?
> 
> > > But for eBuf, it is currently not used by anything other than
> EchoSuppression
> > > and therefore I think it should belong there. 
> > > 
> > > 3) If we need it for some future use we could refactor that then, but
> > currently
> > > I don't see any need for it and if you don't see any immediate place where
> to
> > > use it, I don't think we should plan for that.
> > 
> > Sorry, I indeed mistyped ProcessBlock by EchoSuppression. Thanking for
> thinking
> > of making the operations to dBuf in ProcessBlock. It will make it clear.
> > 
> > For 3), I actually plan to use eBuf to update linoutlevel, as now using dBuf
> to
> > update nearlevel.
> > Currently, linoutlevel is updated from e_fft, which does not have buffered
> data.
> > But dBuf uses buffered data.
> > 
> > Nevertheless, I think what you proposed is fine. linoutlevel can also be
> updated
> > in EchoSuppression (but there is still an awkwardness, i.e., we cannot put
> > UpdateLevel together)

I just see that you made this change in #6, so this CL gets my LGTM

[peah-webrtc, 2015-12-08 16:17:53]

The CQ bit was checked by peah@webrtc.org

[commit-bot: I haz the power, 2015-12-08 16:18:05]

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

[commit-bot: I haz the power, 2015-12-08 16:20:04]

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

[commit-bot: I haz the power, 2015-12-08 16:20:05]

Try jobs failed on following builders:
  ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/6087)
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/11322)

[peah-webrtc, 2015-12-09 10:04:43]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2015-12-09 10:04:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1494563002/#ps60001 (title:
"Merge with master")

[commit-bot: I haz the power, 2015-12-09 10:04:56]

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

[commit-bot: I haz the power, 2015-12-09 10:06:36]

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

[commit-bot: I haz the power, 2015-12-09 10:06:37]

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

[peah-webrtc, 2015-12-09 14:53:51]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2015-12-09 14:53:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1494563002/#ps80001 (title:
"Fixed error in the function header")

[commit-bot: I haz the power, 2015-12-09 14:53:57]

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

[commit-bot: I haz the power, 2015-12-09 16:50:29]

Description was changed from

==========
Moved code into the lowest level of EchoSuppression
to simplify future refactoring and development.

In more detail:
1) Moved the updating of eBuf from the EchoSubtraction method
   to the EchoSuppression method as it is only used in the latter.
2) Moved the computation of efw and dfw from the SubbandCoherence method
   as those are actually the analysis filterbank computation that is not
   directly related to the coherence.
3) As a consequence of 2) 3 functions needed to be replaced by the
   generic function pointer scheme used in WebRTCAec as they have
   optimized versions for SSE2 and NEON (which before were local to each
   of the aec_core*.c files.

Motivation:
Apart from making sense from a logical point of view, the changes will
a) Allow eBuf stored in half the size on the state.
b) Allow simpler switching between using the the microphone signal
   and echo subtractor output in the echo suppressor.
c) Allow further refactoring that move all the changes to eBuf to one method
   (currently those are happening in at least 4 different methods.

Drawbacks:
i) dfw is moved to EchoSuppression which increases the stack usage for that
 method. This will, however, be improved once further refactoring can be done.


The changes have been tested for bitexactness on Linux using a quite extensive
dataset.


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

to

==========
Moved code into the lowest level of EchoSuppression
to simplify future refactoring and development.

In more detail:
1) Moved the updating of eBuf from the EchoSubtraction method
   to the EchoSuppression method as it is only used in the latter.
2) Moved the computation of efw and dfw from the SubbandCoherence method
   as those are actually the analysis filterbank computation that is not
   directly related to the coherence.
3) As a consequence of 2) 3 functions needed to be replaced by the
   generic function pointer scheme used in WebRTCAec as they have
   optimized versions for SSE2 and NEON (which before were local to each
   of the aec_core*.c files.

Motivation:
Apart from making sense from a logical point of view, the changes will
a) Allow eBuf stored in half the size on the state.
b) Allow simpler switching between using the the microphone signal
   and echo subtractor output in the echo suppressor.
c) Allow further refactoring that move all the changes to eBuf to one method
   (currently those are happening in at least 4 different methods.

Drawbacks:
i) dfw is moved to EchoSuppression which increases the stack usage for that
 method. This will, however, be improved once further refactoring can be done.


The changes have been tested for bitexactness on Linux using a quite extensive
dataset.


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

[commit-bot: I haz the power, 2015-12-09 16:50:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-09 16:50:34]

Description was changed from

==========
Moved code into the lowest level of EchoSuppression
to simplify future refactoring and development.

In more detail:
1) Moved the updating of eBuf from the EchoSubtraction method
   to the EchoSuppression method as it is only used in the latter.
2) Moved the computation of efw and dfw from the SubbandCoherence method
   as those are actually the analysis filterbank computation that is not
   directly related to the coherence.
3) As a consequence of 2) 3 functions needed to be replaced by the
   generic function pointer scheme used in WebRTCAec as they have
   optimized versions for SSE2 and NEON (which before were local to each
   of the aec_core*.c files.

Motivation:
Apart from making sense from a logical point of view, the changes will
a) Allow eBuf stored in half the size on the state.
b) Allow simpler switching between using the the microphone signal
   and echo subtractor output in the echo suppressor.
c) Allow further refactoring that move all the changes to eBuf to one method
   (currently those are happening in at least 4 different methods.

Drawbacks:
i) dfw is moved to EchoSuppression which increases the stack usage for that
 method. This will, however, be improved once further refactoring can be done.


The changes have been tested for bitexactness on Linux using a quite extensive
dataset.


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

to

==========
Moved code into the lowest level of EchoSuppression
to simplify future refactoring and development.

In more detail:
1) Moved the updating of eBuf from the EchoSubtraction method
   to the EchoSuppression method as it is only used in the latter.
2) Moved the computation of efw and dfw from the SubbandCoherence method
   as those are actually the analysis filterbank computation that is not
   directly related to the coherence.
3) As a consequence of 2) 3 functions needed to be replaced by the
   generic function pointer scheme used in WebRTCAec as they have
   optimized versions for SSE2 and NEON (which before were local to each
   of the aec_core*.c files.

Motivation:
Apart from making sense from a logical point of view, the changes will
a) Allow eBuf stored in half the size on the state.
b) Allow simpler switching between using the the microphone signal
   and echo subtractor output in the echo suppressor.
c) Allow further refactoring that move all the changes to eBuf to one method
   (currently those are happening in at least 4 different methods.

Drawbacks:
i) dfw is moved to EchoSuppression which increases the stack usage for that
 method. This will, however, be improved once further refactoring can be done.

The changes have been tested for bitexactness on Linux using a quite extensive
dataset.

BUG=webrtc:5201

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

[commit-bot: I haz the power, 2015-12-09 16:50:35]

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