Moving FFT on farend signal to where it is used in AEC (bit exact).

Moving FFT on farend signal to where it is used in AEC (bit exact).

Currently, FFT is performance when AEC buffers farend signal. This has some drawbacks
1. memory inefficiency: two ring buffers are needed;
2. computation inefficiency: if ringbuffer gets wrapped around, some FFT computation will be wasted;
3. accessibility: the main AEC function looses accessibility to the time-domain signal.

Therefore, this CL tries to buffer time domain data, which is buffered any way if a debugging macro is defined, and calculate the FFTs where they are actually used.

BUG=

Committed: https://crrev.com/92594a30ce02aed75f8a2a9f21e5b8c5c4e5f199
Cr-Commit-Position: refs/heads/master@{#11091}

[minyue-webrtc, 2015-12-08 12:22:08]

Description was changed from

==========
patch finished

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

to

==========
Moving FFT on farend signal to where it is used in AEC (bit exact).

Currently, FFT is performance when AEC buffers farend signal. This has some
drawbacks
1. memory inefficiency: two ring buffers are needed;
2. computation inefficiency: if ringbuffer gets wrapped around, some FFT
computation will be wasted;
3. accessibility: the main AEC function looses accessibility to the time-domain
signal.

Therefore, this CL tries to buffer time domain data, which is buffered any way
if a debugging macro is defined, and calculate the FFTs where they are actually
used.

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

[minyue-webrtc, 2015-12-08 12:22:09]

minyue@webrtc.org changed reviewers:
+ peah@webrtc.org

[minyue-webrtc, 2015-12-08 13:04:22]

Hi Per,

Would you help me reviewing this CL?

[minyue-webrtc, 2015-12-11 13:27:15]

On 2015/12/08 13:04:22, minyue-webrtc wrote:
> Hi Per,
> 
> Would you help me reviewing this CL?

Hi Per,

A kind reminder of review.

[peah-webrtc, 2015-12-11 14:10:51]

On 2015/12/11 13:27:15, minyue-webrtc wrote:
> On 2015/12/08 13:04:22, minyue-webrtc wrote:
> > Hi Per,
> > 
> > Would you help me reviewing this CL?
> 
> Hi Per,
> 
> A kind reminder of review.

Hi,
The CL definitely sounds motivated, and I think it is a good move. Regarding the
wrapping around, that should not happen though so that is not a motivation for
doing this, but the other two reasons are definitely valid.

The CL involves quite a lot of code that I don't know by heart, and that is
crucial for the AEC performance. Therefore I wonder the following, before I do
further review:
1) Could you please rebase on the latest master? As it is now, I get a merge
conflict that I'm not fully sure on how to properly resolve to test your code.
2) You state it is bitexact, and looking at your CL so far I believe you are
correct in that, but how did you test this?

[minyue-webrtc, 2015-12-11 14:23:54]

On 2015/12/11 14:10:51, peah-webrtc wrote:
> On 2015/12/11 13:27:15, minyue-webrtc wrote:
> > On 2015/12/08 13:04:22, minyue-webrtc wrote:
> > > Hi Per,
> > > 
> > > Would you help me reviewing this CL?
> > 
> > Hi Per,
> > 
> > A kind reminder of review.
> 
> Hi,
> The CL definitely sounds motivated, and I think it is a good move. Regarding
the
> wrapping around, that should not happen though so that is not a motivation for
> doing this, but the other two reasons are definitely valid.

OK. I agree that a good design should not allow wrapping happen, but it is a
ringbuffer and can, in principle, happen. For curiosity, how is long long
buffering time handled in current implementation?

> 
> The CL involves quite a lot of code that I don't know by heart, and that is
> crucial for the AEC performance. Therefore I wonder the following, before I do
> further review:
> 1) Could you please rebase on the latest master? As it is now, I get a merge
> conflict that I'm not fully sure on how to properly resolve to test your code.

I think there will be more rebasing once you landed your CLs. Good if we
coordinate. I do not mind if you land all yours first. You may now focus on the
methodology. After rebasing, I will create new patch that deals only with
rebasing.

> 2) You state it is bitexact, and looking at your CL so far I believe you are
> correct in that, but how did you test this?

I actually have tested it by having the old frequency domain ring buffers
together with the new time domain ring buffer, and compared the new FFT results
and the stored ones, and they are bit exact.

[peah-webrtc, 2015-12-12 22:30:50]

On 2015/12/11 14:23:54, minyue-webrtc wrote:
> On 2015/12/11 14:10:51, peah-webrtc wrote:
> > On 2015/12/11 13:27:15, minyue-webrtc wrote:
> > > On 2015/12/08 13:04:22, minyue-webrtc wrote:
> > > > Hi Per,
> > > > 
> > > > Would you help me reviewing this CL?
> > > 
> > > Hi Per,
> > > 
> > > A kind reminder of review.
> > 
> > Hi,
> > The CL definitely sounds motivated, and I think it is a good move. Regarding
> the
> > wrapping around, that should not happen though so that is not a motivation
for
> > doing this, but the other two reasons are definitely valid.
> 
> OK. I agree that a good design should not allow wrapping happen, but it is a
> ringbuffer and can, in principle, happen. For curiosity, how is long long
> buffering time handled in current implementation?

1 second of 16 kHz data, which is too much, but at least should not wraparound.
If it does, the setup has far worse problems than echo effects due to
wraparounds. 

> 
> > 
> > The CL involves quite a lot of code that I don't know by heart, and that is
> > crucial for the AEC performance. Therefore I wonder the following, before I
do
> > further review:
> > 1) Could you please rebase on the latest master? As it is now, I get a merge
> > conflict that I'm not fully sure on how to properly resolve to test your
code.
> 
> I think there will be more rebasing once you landed your CLs. Good if we
> coordinate. I do not mind if you land all yours first. You may now focus on
the
> methodology. After rebasing, I will create new patch that deals only with
> rebasing.


When I review this change, I want to be able to run it in order to run my tests
on it. But I'll see if I can resolve the merge on Monday.

> 
> > 2) You state it is bitexact, and looking at your CL so far I believe you are
> > correct in that, but how did you test this?
> 
> I actually have tested it by having the old frequency domain ring buffers
> together with the new time domain ring buffer, and compared the new FFT
results
> and the stored ones, and they are bit exact.


I think we should test it more. Timing is critical and things have changed place
in the code. Therefore, in order to test it fully I would like to run my test
suite to verify that also the produced output is bitexact.

[minyue-webrtc, 2015-12-13 22:22:38]

On 2015/12/12 22:30:50, peah-webrtc wrote:
> On 2015/12/11 14:23:54, minyue-webrtc wrote:
> > On 2015/12/11 14:10:51, peah-webrtc wrote:
> > > On 2015/12/11 13:27:15, minyue-webrtc wrote:
> > > > On 2015/12/08 13:04:22, minyue-webrtc wrote:
> > > > > Hi Per,
> > > > > 
> > > > > Would you help me reviewing this CL?
> > > > 
> > > > Hi Per,
> > > > 
> > > > A kind reminder of review.
> > > 
> > > Hi,
> > > The CL definitely sounds motivated, and I think it is a good move.
Regarding
> > the
> > > wrapping around, that should not happen though so that is not a motivation
> for
> > > doing this, but the other two reasons are definitely valid.
> > 
> > OK. I agree that a good design should not allow wrapping happen, but it is a
> > ringbuffer and can, in principle, happen. For curiosity, how is long long
> > buffering time handled in current implementation?
> 
> 1 second of 16 kHz data, which is too much, but at least should not
wraparound.
> If it does, the setup has far worse problems than echo effects due to
> wraparounds. 
> 
> > 
> > > 
> > > The CL involves quite a lot of code that I don't know by heart, and that
is
> > > crucial for the AEC performance. Therefore I wonder the following, before
I
> do
> > > further review:
> > > 1) Could you please rebase on the latest master? As it is now, I get a
merge
> > > conflict that I'm not fully sure on how to properly resolve to test your
> code.
> > 
> > I think there will be more rebasing once you landed your CLs. Good if we
> > coordinate. I do not mind if you land all yours first. You may now focus on
> the
> > methodology. After rebasing, I will create new patch that deals only with
> > rebasing.
> 
> 
> When I review this change, I want to be able to run it in order to run my
tests
> on it. But I'll see if I can resolve the merge on Monday.
> 
> > 
> > > 2) You state it is bitexact, and looking at your CL so far I believe you
are
> > > correct in that, but how did you test this?
> > 
> > I actually have tested it by having the old frequency domain ring buffers
> > together with the new time domain ring buffer, and compared the new FFT
> results
> > and the stored ones, and they are bit exact.
> 
> 
> I think we should test it more. Timing is critical and things have changed
place
> in the code. Therefore, in order to test it fully I would like to run my test
> suite to verify that also the produced output is bitexact.

That would be nice, thank you. I can help with merging if there is any conflict.

[minyue-webrtc, 2015-12-14 10:42:21]

Hi Per,

Now it is rebased. Would you try your bit-exactness test on this?

[minyue-webrtc, 2015-12-14 10:42:22]

Hi Per,

Now it is rebased. Would you try your bit-exactness test on this?

[peah-webrtc, 2015-12-16 12:52:22]

Nice CL! I tested the code for bitexactness and it is bitexact on the test suite
I use.

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

https://codereview.webrtc.org/1512573003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec/aec_core.c:844: static int
MoveFarReadPtrWithoutSystemDelayUpdate(AecCore* self, int elements) {
I think that since this is now a one-liner, we should remove the function and
instead call WebRtc_MoveReadPtr directly. This will increase code readability.

[minyue-webrtc, 2015-12-16 14:47:17]

Thanks! It is now updated.

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

https://codereview.webrtc.org/1512573003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec/aec_core.c:844: static int
MoveFarReadPtrWithoutSystemDelayUpdate(AecCore* self, int elements) {
On 2015/12/16 12:52:22, peah-webrtc wrote:
> I think that since this is now a one-liner, we should remove the function and
> instead call WebRtc_MoveReadPtr directly. This will increase code readability.

yes, certainly.

[minyue-webrtc, 2015-12-17 08:51:53]

On 2015/12/16 14:47:17, minyue-webrtc wrote:
> Thanks! It is now updated.
> 
>
https://codereview.webrtc.org/1512573003/diff/20001/webrtc/modules/audio_proc...
> File webrtc/modules/audio_processing/aec/aec_core.c (right):
> 
>
https://codereview.webrtc.org/1512573003/diff/20001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/aec/aec_core.c:844: static int
> MoveFarReadPtrWithoutSystemDelayUpdate(AecCore* self, int elements) {
> On 2015/12/16 12:52:22, peah-webrtc wrote:
> > I think that since this is now a one-liner, we should remove the function
and
> > instead call WebRtc_MoveReadPtr directly. This will increase code
readability.
> 
> yes, certainly.

hi, any further comment?

[peah-webrtc, 2015-12-18 13:13:19]

On 2015/12/17 08:51:53, minyue-webrtc wrote:
> On 2015/12/16 14:47:17, minyue-webrtc wrote:
> > Thanks! It is now updated.
> > 
> >
>
https://codereview.webrtc.org/1512573003/diff/20001/webrtc/modules/audio_proc...
> > File webrtc/modules/audio_processing/aec/aec_core.c (right):
> > 
> >
>
https://codereview.webrtc.org/1512573003/diff/20001/webrtc/modules/audio_proc...
> > webrtc/modules/audio_processing/aec/aec_core.c:844: static int
> > MoveFarReadPtrWithoutSystemDelayUpdate(AecCore* self, int elements) {
> > On 2015/12/16 12:52:22, peah-webrtc wrote:
> > > I think that since this is now a one-liner, we should remove the function
> and
> > > instead call WebRtc_MoveReadPtr directly. This will increase code
> readability.
> > 
> > yes, certainly.
> 
> hi, any further comment?

Looks great!
lgtm

[minyue-webrtc, 2015-12-18 13:18:27]

The CQ bit was checked by minyue@webrtc.org

[commit-bot: I haz the power, 2015-12-18 13:18:37]

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

[commit-bot: I haz the power, 2015-12-18 13:19:19]

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

[commit-bot: I haz the power, 2015-12-18 13:19:20]

Try jobs failed on following builders:
  ios64_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/4111)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/6429)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/10254)
  linux_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/11466)
  mac_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/11439)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_compile_x64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_x64_dbg/bui...)
  mac_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/11689)
  mac_x64_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_dbg/builds/6081)
  mac_x64_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_rel/builds/6036)

[minyue-webrtc, 2015-12-18 15:16:24]

The CQ bit was checked by minyue@webrtc.org

[minyue-webrtc, 2015-12-18 15:16:25]

The patchset sent to the CQ was uploaded after l-g-t-m from peah@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1512573003/#ps60001 (title:
"rebasing")

[commit-bot: I haz the power, 2015-12-18 15:16:31]

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

[commit-bot: I haz the power, 2015-12-18 17:16:55]

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

[commit-bot: I haz the power, 2015-12-18 17:16:56]

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

[minyue-webrtc, 2015-12-18 17:30:58]

The CQ bit was checked by minyue@webrtc.org

[commit-bot: I haz the power, 2015-12-18 17:31:05]

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

[commit-bot: I haz the power, 2015-12-18 19:31:28]

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

[commit-bot: I haz the power, 2015-12-18 19:31:29]

Try jobs failed on following builders:
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/8074)

[minyue-webrtc, 2015-12-18 23:30:00]

The CQ bit was checked by minyue@webrtc.org

[commit-bot: I haz the power, 2015-12-18 23:30:19]

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

[commit-bot: I haz the power, 2015-12-18 23:31:17]

Description was changed from

==========
Moving FFT on farend signal to where it is used in AEC (bit exact).

Currently, FFT is performance when AEC buffers farend signal. This has some
drawbacks
1. memory inefficiency: two ring buffers are needed;
2. computation inefficiency: if ringbuffer gets wrapped around, some FFT
computation will be wasted;
3. accessibility: the main AEC function looses accessibility to the time-domain
signal.

Therefore, this CL tries to buffer time domain data, which is buffered any way
if a debugging macro is defined, and calculate the FFTs where they are actually
used.

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

to

==========
Moving FFT on farend signal to where it is used in AEC (bit exact).

Currently, FFT is performance when AEC buffers farend signal. This has some
drawbacks
1. memory inefficiency: two ring buffers are needed;
2. computation inefficiency: if ringbuffer gets wrapped around, some FFT
computation will be wasted;
3. accessibility: the main AEC function looses accessibility to the time-domain
signal.

Therefore, this CL tries to buffer time domain data, which is buffered any way
if a debugging macro is defined, and calculate the FFTs where they are actually
used.

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

[commit-bot: I haz the power, 2015-12-18 23:31:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-18 23:31:27]

Description was changed from

==========
Moving FFT on farend signal to where it is used in AEC (bit exact).

Currently, FFT is performance when AEC buffers farend signal. This has some
drawbacks
1. memory inefficiency: two ring buffers are needed;
2. computation inefficiency: if ringbuffer gets wrapped around, some FFT
computation will be wasted;
3. accessibility: the main AEC function looses accessibility to the time-domain
signal.

Therefore, this CL tries to buffer time domain data, which is buffered any way
if a debugging macro is defined, and calculate the FFTs where they are actually
used.

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

to

==========
Moving FFT on farend signal to where it is used in AEC (bit exact).

Currently, FFT is performance when AEC buffers farend signal. This has some
drawbacks
1. memory inefficiency: two ring buffers are needed;
2. computation inefficiency: if ringbuffer gets wrapped around, some FFT
computation will be wasted;
3. accessibility: the main AEC function looses accessibility to the time-domain
signal.

Therefore, this CL tries to buffer time domain data, which is buffered any way
if a debugging macro is defined, and calculate the FFTs where they are actually
used.

BUG=

Committed: https://crrev.com/92594a30ce02aed75f8a2a9f21e5b8c5c4e5f199
Cr-Commit-Position: refs/heads/master@{#11091}
==========

[commit-bot: I haz the power, 2015-12-18 23:31:27]

Patchset 4 (id:??) landed as
https://crrev.com/92594a30ce02aed75f8a2a9f21e5b8c5c4e5f199
Cr-Commit-Position: refs/heads/master@{#11091}