Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(8)

Issue 1456123003: Ducking fix #3: Removed the state as an input to the FilterAdaptation function (Closed)

Created:
3 years, 7 months ago by peah-webrtc
Modified:
3 years, 6 months ago
Reviewers:
ivoc, hlundin-webrtc
CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1
Base URL:
https://chromium.googlesource.com/external/webrtc.git@Aec_Code_Cleanup2_CL
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

-Removed the state as an input to the FilterAdaptation function. -Renamed the TimeToFrequency and FrequencyToTime functions. -Moved the windowing from the TimeToFrequency function. -Simplified the EchoSubtraction function. Note that the aec state is still an input to the EchoSubtraction function, and it currently needs to be that in order to support the output of the debug file. The longer-term goal is, however, to order the state into substates. This will simplify the parameter lists to the EchoCancellation function as well as replace the aec state as a parameter BUG=webrtc:5201 Committed: https://crrev.com/7e43138c0890bd99f627fa061b122c8d5716a99d Cr-Commit-Position: refs/heads/master@{#10830}

Patch Set 1 #

Patch Set 2 : Various refactoring, among other things simplification of the EchoSubtraction function #

Total comments: 30

Patch Set 3 : Changes in response to reviewer comments #

Total comments: 12

Patch Set 4 : Changes in response to reviewer comments #

Patch Set 5 : Merged the loops in the InverseFFT function #

Patch Set 6 : Merged with latest master #

Patch Set 7 : Moved for loop variable declaration to beginning of function to be compliant with the old C standar… #

Patch Set 8 : Removed gcc build-breaking method argument specifiers #

Unified diffs Side-by-side diffs Delta from patch set Stats (+299 lines, -277 lines) Patch
M webrtc/modules/audio_processing/aec/aec_core.c View 1 2 3 4 5 6 7 10 chunks +137 lines, -130 lines 0 comments Download
M webrtc/modules/audio_processing/aec/aec_core_internal.h View 1 2 3 4 5 6 7 1 chunk +10 lines, -7 lines 0 comments Download
M webrtc/modules/audio_processing/aec/aec_core_mips.c View 1 2 3 4 5 6 7 4 chunks +30 lines, -26 lines 0 comments Download
M webrtc/modules/audio_processing/aec/aec_core_neon.c View 1 2 3 4 5 6 7 5 chunks +61 lines, -57 lines 0 comments Download
M webrtc/modules/audio_processing/aec/aec_core_sse2.c View 1 2 3 4 5 6 7 6 chunks +61 lines, -57 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 40 (17 generated)
peah-webrtc
3 years, 7 months ago (2015-11-19 11:18:48 UTC) #3
hlundin-webrtc
LG. I have the same two comments repeating: 1. When you add/modify function parameters, make ...
3 years, 7 months ago (2015-11-20 11:55:20 UTC) #5
ivoc
A few comments, See below. https://codereview.webrtc.org/1456123003/diff/20001/webrtc/modules/audio_processing/aec/aec_core.c File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1456123003/diff/20001/webrtc/modules/audio_processing/aec/aec_core.c#newcode841 webrtc/modules/audio_processing/aec/aec_core.c:841: static void Fft(float time_data[PART_LEN2], ...
3 years, 7 months ago (2015-11-20 12:58:02 UTC) #6
peah-webrtc
https://codereview.webrtc.org/1456123003/diff/20001/webrtc/modules/audio_processing/aec/aec_core.c File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1456123003/diff/20001/webrtc/modules/audio_processing/aec/aec_core.c#newcode213 webrtc/modules/audio_processing/aec/aec_core.c:213: int xfBufBlockPos, On 2015/11/20 11:55:20, hlundin-webrtc wrote: > Rename ...
3 years, 6 months ago (2015-11-24 13:03:02 UTC) #7
hlundin-webrtc
LG, but I have a few nits and a concern. https://codereview.webrtc.org/1456123003/diff/40001/webrtc/modules/audio_processing/aec/aec_core.c File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1456123003/diff/40001/webrtc/modules/audio_processing/aec/aec_core.c#newcode851 ...
3 years, 6 months ago (2015-11-24 13:51:44 UTC) #8
peah-webrtc
https://codereview.webrtc.org/1456123003/diff/40001/webrtc/modules/audio_processing/aec/aec_core.c File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1456123003/diff/40001/webrtc/modules/audio_processing/aec/aec_core.c#newcode851 webrtc/modules/audio_processing/aec/aec_core.c:851: memcpy(time_data_copy, time_data, sizeof(float) * PART_LEN2); On 2015/11/24 13:51:44, hlundin-webrtc ...
3 years, 6 months ago (2015-11-26 05:55:17 UTC) #9
hlundin-webrtc
lgtm https://codereview.webrtc.org/1456123003/diff/40001/webrtc/modules/audio_processing/aec/aec_core.c File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1456123003/diff/40001/webrtc/modules/audio_processing/aec/aec_core.c#newcode851 webrtc/modules/audio_processing/aec/aec_core.c:851: memcpy(time_data_copy, time_data, sizeof(float) * PART_LEN2); On 2015/11/26 05:55:17, ...
3 years, 6 months ago (2015-11-26 08:41:45 UTC) #10
peah-webrtc
On 2015/11/26 08:41:45, hlundin-webrtc wrote: > lgtm > > https://codereview.webrtc.org/1456123003/diff/40001/webrtc/modules/audio_processing/aec/aec_core.c > File webrtc/modules/audio_processing/aec/aec_core.c (right): > ...
3 years, 6 months ago (2015-11-26 08:43:16 UTC) #11
ivoc
Just one comment for me. https://codereview.webrtc.org/1456123003/diff/40001/webrtc/modules/audio_processing/aec/aec_core.c File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1456123003/diff/40001/webrtc/modules/audio_processing/aec/aec_core.c#newcode842 webrtc/modules/audio_processing/aec/aec_core.c:842: time_data[i] *= scale; I ...
3 years, 6 months ago (2015-11-26 12:46:44 UTC) #12
peah-webrtc
https://codereview.webrtc.org/1456123003/diff/40001/webrtc/modules/audio_processing/aec/aec_core.c File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1456123003/diff/40001/webrtc/modules/audio_processing/aec/aec_core.c#newcode842 webrtc/modules/audio_processing/aec/aec_core.c:842: time_data[i] *= scale; On 2015/11/26 12:46:44, ivoc wrote: > ...
3 years, 6 months ago (2015-11-27 04:58:36 UTC) #13
ivoc
LGTM!
3 years, 6 months ago (2015-11-27 08:12:02 UTC) #14
peah-webrtc
Thanks for a good review!
3 years, 6 months ago (2015-11-27 08:13:36 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1456123003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1456123003/80001
3 years, 6 months ago (2015-11-27 08:14:02 UTC) #18
commit-bot: I haz the power
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/5842) ios_rel on tryserver.webrtc (JOB_FAILED, ...
3 years, 6 months ago (2015-11-27 08:15:25 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1456123003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1456123003/100001
3 years, 6 months ago (2015-11-27 10:10:19 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/9372) ios_dbg on tryserver.webrtc (JOB_FAILED, ...
3 years, 6 months ago (2015-11-27 10:12:38 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1456123003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1456123003/140001
3 years, 6 months ago (2015-11-27 13:56:23 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: linux_msan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/6267)
3 years, 6 months ago (2015-11-27 14:30:53 UTC) #30
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1456123003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1456123003/140001
3 years, 6 months ago (2015-11-27 19:46:24 UTC) #32
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
3 years, 6 months ago (2015-11-27 21:46:34 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1456123003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1456123003/140001
3 years, 6 months ago (2015-11-27 22:58:48 UTC) #36
commit-bot: I haz the power
Committed patchset #8 (id:140001)
3 years, 6 months ago (2015-11-27 23:24:32 UTC) #38
commit-bot: I haz the power
3 years, 6 months ago (2015-11-27 23:24:40 UTC) #40
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/7e43138c0890bd99f627fa061b122c8d5716a99d
Cr-Commit-Position: refs/heads/master@{#10830}

Powered by Google App Engine
This is Rietveld 408576698