Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(239)

Issue 2321483002: Refactoring of the buffering of the output signal done inside the AEC (Closed)

Created:
4 years, 3 months ago by peah-webrtc
Modified:
4 years, 3 months ago
Reviewers:
hlundin-webrtc
CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

The output signal of the AEC needs to be buffered as the internal block size of the AEC differ from the frame size in the AEC output. Before this CL, this buffering was done using ringbuffers as well as secondary internal AEC buffers that were stored on the state. The internal buffers were redundant, and the ringbuffers were so short that the benefit of using ringbuffers were lost. This CL addresses the above issues by replacing the ringbuffers by linear buffers. This has the main advantage of cleaner code but it should significantly less computational complex. Furthermore, as the complexity of the function where the conversion to external and internal AEC frame sizes is done increased significantly with the changes in this CL, the CL also include refactoring the near-end buffer handling to increase readability and reduce code repetition. After the changes in this CL it is very clear that the former buffering of the output was incorrectly done for the first frames. This CL corrects that but in doing that it breaks the bitexactness with the former code. The bitexactness is, however, only broken for the first 1000 samples and it has been verified that for a test suite the CL maintains bitexactness in the AEC output after the first 1000 samples. This CL is chained to the CL https://codereview.webrtc.org/2311833002/ and will be followed by more CLs that refactor the other buffers inside the AEC. BUG=webrtc:5298, webrtc:6018 Committed: https://crrev.com/8e56521143f2bd481710ea7508d5cdd3a8b6d59c Cr-Commit-Position: refs/heads/master@{#14184}

Patch Set 1 #

Total comments: 20

Patch Set 2 : Changes in response to reviewer comments #

Patch Set 3 : Updated the binary test vectors #

Patch Set 4 : Rebase #

Patch Set 5 : New testvectors #

Unified diffs Side-by-side diffs Delta from patch set Stats (+133 lines, -106 lines) Patch
M data/audio_processing/output_data_mac.pb View 1 2 3 4 Binary file 0 comments Download
M webrtc/modules/audio_processing/aec/aec_core.h View 1 2 3 1 chunk +3 lines, -4 lines 0 comments Download
M webrtc/modules/audio_processing/aec/aec_core.cc View 1 2 3 12 chunks +130 lines, -102 lines 0 comments Download

Messages

Total messages: 36 (22 generated)
peah-webrtc
4 years, 3 months ago (2016-09-07 12:11:34 UTC) #8
hlundin-webrtc
On 2016/09/07 12:11:34, peah-webrtc wrote: Commit message : "but it should significantly less computational complex", ...
4 years, 3 months ago (2016-09-09 08:47:52 UTC) #9
hlundin-webrtc
Nice improvement. See some minor comments inline. https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_processing/aec/aec_core.cc File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_processing/aec/aec_core.cc#newcode1709 webrtc/modules/audio_processing/aec/aec_core.cc:1709: void FormNearEndBlock(size_t ...
4 years, 3 months ago (2016-09-09 09:21:19 UTC) #10
peah-webrtc
https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_processing/aec/aec_core.cc File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_processing/aec/aec_core.cc#newcode1709 webrtc/modules/audio_processing/aec/aec_core.cc:1709: void FormNearEndBlock(size_t nearend_start_index, On 2016/09/09 09:21:19, hlundin-webrtc wrote: > ...
4 years, 3 months ago (2016-09-09 14:05:56 UTC) #11
hlundin-webrtc
lgtm with one nit. https://codereview.webrtc.org/2321483002/diff/80001/webrtc/modules/audio_processing/aec/aec_core.cc File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/2321483002/diff/80001/webrtc/modules/audio_processing/aec/aec_core.cc#newcode1718 webrtc/modules/audio_processing/aec/aec_core.cc:1718: const int num_samples_from_buffer = PART_LEN ...
4 years, 3 months ago (2016-09-09 14:22:48 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2321483002/120001
4 years, 3 months ago (2016-09-11 17:16:24 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/18127)
4 years, 3 months ago (2016-09-11 17:34:15 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2321483002/140001
4 years, 3 months ago (2016-09-11 18:09:01 UTC) #22
commit-bot: I haz the power
Try jobs failed on following builders: mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/18129)
4 years, 3 months ago (2016-09-11 18:25:10 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2321483002/160001
4 years, 3 months ago (2016-09-12 08:03:31 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
4 years, 3 months ago (2016-09-12 10:04:07 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2321483002/160001
4 years, 3 months ago (2016-09-12 11:27:32 UTC) #32
commit-bot: I haz the power
Committed patchset #5 (id:160001)
4 years, 3 months ago (2016-09-12 11:49:51 UTC) #34
commit-bot: I haz the power
4 years, 3 months ago (2016-09-12 11:49:55 UTC) #36
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/8e56521143f2bd481710ea7508d5cdd3a8b6d59c
Cr-Commit-Position: refs/heads/master@{#14184}

Powered by Google App Engine
This is Rietveld 408576698