|
|
Chromium Code Reviews|
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. |
DescriptionThe 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 #
Messages
Total messages: 36 (22 generated)
Description was changed from ========== To be decided BUG= ========== to ========== To be decided BUG= ==========
Description was changed from ========== To be decided BUG= ========== to ========== 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 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 also be slightly cheaper. 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 ==========
Description was changed from ========== 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 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 also be slightly cheaper. 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 ========== to ========== 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 also be slightly cheaper. 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 ==========
Description was changed from ========== 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 also be slightly cheaper. 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 ========== to ========== 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 also have slightly lower computational complexity. 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 ==========
Description was changed from ========== 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 also have slightly lower computational complexity. 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 ========== to ========== 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 ==========
Patchset #1 (id:1) has been deleted
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
On 2016/09/07 12:11:34, peah-webrtc wrote: Commit message : "but it should significantly less computational complex", rephrase.
Nice improvement. See some minor comments inline. https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1709: void FormNearEndBlock(size_t nearend_start_index, Naming nit: either change to FormNearendBlock, or rename the variables from *_nearend_* to *_near_end_*. I prefer the former. https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1712: int num_samples_from_nearend_frame, Num samples should in general be a size_t. Any reason this is not? https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1713: float nearend_buffer[NUM_HIGH_BANDS_MAX + 1] const? https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1716: const int num_samples_from_buffer = PART_LEN - num_samples_from_nearend_frame; Can this be negative, or do we demand that PART_LEN >= num_samples...? Document using DCHECK. https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1736: int num_samples_to_buffer, size_t? https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1749: float output_block[NUM_HIGH_BANDS_MAX + 1][PART_LEN], const https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1750: int* output_buffer_size, size_t https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1762: int* output_buffer_size, size_t https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1772: RTC_DCHECK_GT(FRAME_LEN, (*output_buffer_size)); Combining this and the previous sanity check, it should hold that FRAME_LEN <= *output_buffer_size < 2*FRAME_LEN, correct? https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.h (right): https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.h:135: int output_buffer_size; size_t
https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... 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: > Naming nit: either change to FormNearendBlock, or rename the variables from > *_nearend_* to *_near_end_*. I prefer the former. Good find! Fully agree. Done. https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1712: int num_samples_from_nearend_frame, On 2016/09/09 09:21:19, hlundin-webrtc wrote: > Num samples should in general be a size_t. Any reason this is not? Done. https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1713: float nearend_buffer[NUM_HIGH_BANDS_MAX + 1] On 2016/09/09 09:21:19, hlundin-webrtc wrote: > const? Done. https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1716: const int num_samples_from_buffer = PART_LEN - num_samples_from_nearend_frame; On 2016/09/09 09:21:19, hlundin-webrtc wrote: > Can this be negative, or do we demand that PART_LEN >= num_samples...? Document > using DCHECK. Good find! It cannot be negative. DCHECK added. Done. https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1736: int num_samples_to_buffer, On 2016/09/09 09:21:19, hlundin-webrtc wrote: > size_t? Done. https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1749: float output_block[NUM_HIGH_BANDS_MAX + 1][PART_LEN], On 2016/09/09 09:21:19, hlundin-webrtc wrote: > const Done. https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1750: int* output_buffer_size, On 2016/09/09 09:21:19, hlundin-webrtc wrote: > size_t Done. https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1762: int* output_buffer_size, On 2016/09/09 09:21:19, hlundin-webrtc wrote: > size_t Done. https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1772: RTC_DCHECK_GT(FRAME_LEN, (*output_buffer_size)); On 2016/09/09 09:21:19, hlundin-webrtc wrote: > Combining this and the previous sanity check, it should hold that FRAME_LEN <= > *output_buffer_size < 2*FRAME_LEN, correct? Yes, if the code enters there, the DCHECKS would imply that. I see, however, that the DCHECK here is wrong so I corrected that. https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.h (right): https://codereview.webrtc.org/2321483002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.h:135: int output_buffer_size; On 2016/09/09 09:21:19, hlundin-webrtc wrote: > size_t Done.
lgtm with one nit. https://codereview.webrtc.org/2321483002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/2321483002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:1718: const int num_samples_from_buffer = PART_LEN - num_samples_from_nearend_frame; I would argue that this should be size_t too now.
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2321483002/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
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)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2321483002/#ps140001 (title: "New testvector for mac")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
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)
Patchset #5 (id:140001) has been deleted
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2321483002/#ps160001 (title: "New testvectors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8e56521143f2bd481710ea7508d5cdd3a8b6d59c Cr-Commit-Position: refs/heads/master@{#14184} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
