Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1639773002:
Moved buffering of farend into the EchoSubtraction method. (Closed)
|
Created:
4 years, 11 months ago by peah-webrtc Modified:
4 years, 10 months ago CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMoved buffering of farend into the EchoSubtraction method.
This makes sense since the buffered data is only used by
the echo subtraction method. Furthermore, it simplifies the
upcoming modifications to the echo subtraction method since
the way the buffering is done can then be specific for the
echo subtraction implementation used.
The change is bitexact and this was verified using a fairly
extensive bitexactness suite.
BUG=
Committed: https://crrev.com/1147b75b52c0d827281c7563e45dcfbf03907ed9
Cr-Commit-Position: refs/heads/master@{#11547}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Changed variable name according to reviewer comments #
Total comments: 2
Patch Set 3 : Merge from latest master #Messages
Total messages: 19 (8 generated)
Description was changed from ========== Moved buffering of farend into the EchoSubtraction method. This makes sense since this is only used by the echo subtraction method. Furthermore, it simplifies the upcoming modifications to the echo subtraction method since the way the buffering is done can then be specific for the echo subtraction implementation used. The change is bitexact and this was verified using a fairly extensive bitexactness suite. BUG= ========== to ========== Moved buffering of farend into the EchoSubtraction method. This makes sense since the buffered data is only used by the echo subtraction method. Furthermore, it simplifies the upcoming modifications to the echo subtraction method since the way the buffering is done can then be specific for the echo subtraction implementation used. The change is bitexact and this was verified using a fairly extensive bitexactness suite. BUG= ==========
peah@webrtc.org changed reviewers: + ivoc@webrtc.org, minyue@webrtc.org
https://codereview.webrtc.org/1639773002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1639773002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:904: int metrics_mode, I prefer the old line breaking. less lines and clearer. https://codereview.webrtc.org/1639773002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:908: float* xf, would it better to change xf to something more meaningful, e.g., farend_fft. https://codereview.webrtc.org/1639773002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:1371: aec->normal_error_threshold, &xf[0][0], &aec->xfBufBlockPos, if you like you can change xf here to something more meaningful as well.
https://codereview.webrtc.org/1639773002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1639773002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:904: int metrics_mode, On 2016/01/26 15:15:26, minyue-webrtc wrote: > I prefer the old line breaking. less lines and clearer. This breaking is according to (caused by) clang-format (CL format) which I think is the one we should use. It was automatically formatted like this. Is that ok? https://codereview.webrtc.org/1639773002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:908: float* xf, On 2016/01/26 15:15:26, minyue-webrtc wrote: > would it better to change xf to something more meaningful, e.g., farend_fft. That makes sense! I will change to x_fft, as that is in line with the rest of the current abbreviations for farend. https://codereview.webrtc.org/1639773002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:1371: aec->normal_error_threshold, &xf[0][0], &aec->xfBufBlockPos, On 2016/01/26 15:15:26, minyue-webrtc wrote: > if you like you can change xf here to something more meaningful as well. Done.
lgtm https://codereview.webrtc.org/1639773002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1639773002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:904: int metrics_mode, On 2016/01/28 13:06:07, peah-webrtc wrote: > On 2016/01/26 15:15:26, minyue-webrtc wrote: > > I prefer the old line breaking. less lines and clearer. > > This breaking is according to (caused by) clang-format (CL format) which I think > is the one we should use. It was automatically formatted like this. Is that ok? That is generally ok, although I prefer some manual work if the machine cannot make it as friendly as a human does. In this case, I'd leave it for you.
LGTM for the code, see my remark below, I'll leave it up to you to decide if you want to address it. https://codereview.webrtc.org/1639773002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1639773002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:917: float echo_subtractor_output[PART_LEN]) { Although I noticed that most functions in the file don't have this, I was thinking that a bit of documentation on what the input/output variables are and do would be useful. What do you think?
https://codereview.webrtc.org/1639773002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1639773002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:917: float echo_subtractor_output[PART_LEN]) { On 2016/02/05 09:05:04, ivoc wrote: > Although I noticed that most functions in the file don't have this, I was > thinking that a bit of documentation on what the input/output variables are and > do would be useful. What do you think? Great point! That would definitely make sense. I'm a bit hesitant to add it right now, though, as this is code that is going through, and for several months will continue to go through, a lot of changes. Therefore I'll not add any comments at this point but instead wait with the comments until a later stage.
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639773002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639773002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/11108) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/622) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/6857)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/1639773002/#ps40001 (title: "Merge from latest master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639773002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639773002/40001
Message was sent while issue was closed.
Description was changed from ========== Moved buffering of farend into the EchoSubtraction method. This makes sense since the buffered data is only used by the echo subtraction method. Furthermore, it simplifies the upcoming modifications to the echo subtraction method since the way the buffering is done can then be specific for the echo subtraction implementation used. The change is bitexact and this was verified using a fairly extensive bitexactness suite. BUG= ========== to ========== Moved buffering of farend into the EchoSubtraction method. This makes sense since the buffered data is only used by the echo subtraction method. Furthermore, it simplifies the upcoming modifications to the echo subtraction method since the way the buffering is done can then be specific for the echo subtraction implementation used. The change is bitexact and this was verified using a fairly extensive bitexactness suite. BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Moved buffering of farend into the EchoSubtraction method. This makes sense since the buffered data is only used by the echo subtraction method. Furthermore, it simplifies the upcoming modifications to the echo subtraction method since the way the buffering is done can then be specific for the echo subtraction implementation used. The change is bitexact and this was verified using a fairly extensive bitexactness suite. BUG= ========== to ========== Moved buffering of farend into the EchoSubtraction method. This makes sense since the buffered data is only used by the echo subtraction method. Furthermore, it simplifies the upcoming modifications to the echo subtraction method since the way the buffering is done can then be specific for the echo subtraction implementation used. The change is bitexact and this was verified using a fairly extensive bitexactness suite. BUG= Committed: https://crrev.com/1147b75b52c0d827281c7563e45dcfbf03907ed9 Cr-Commit-Position: refs/heads/master@{#11547} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1147b75b52c0d827281c7563e45dcfbf03907ed9 Cr-Commit-Position: refs/heads/master@{#11547} |
