Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1963493003:
Moved the AEC echo suppression gain computation code to a separate method (Closed)
|
Created:
4 years, 7 months ago by peah-webrtc Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, 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 the AEC echo suppression gain computation code to
a separate method.
This CL will be followed by other CLs that simplify this method and break out the state specific to this computation
into a separate substate.
The changes are bitexact.
BUG=webrtc:5201, webrtc:5298
Committed: https://crrev.com/9bbf89bca1d722cf5d1c4b3e98517c7c3f71d3e3
Cr-Commit-Position: refs/heads/master@{#12712}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase #Messages
Total messages: 18 (7 generated)
Description was changed from ========== Moved the echo suppression gain computation code to a separate method BUG= ========== to ========== Moved the AEC echo suppression gain computation code to a separate method. This CL will be followed by other CLs that simplify this method and break out the state specific to this computation into a separate substate. The changes are bitexact. BUG=webrtc:5201, webrtc:5298 ==========
peah@webrtc.org changed reviewers: + ivoc@webrtc.org
Nice! LGTM with one question. https://codereview.webrtc.org/1963493003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1963493003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:1031: memcpy(hNl, cohde, sizeof(hNl[0]) * PART_LEN1); Is this better than sizeof(hNl), or does that not work with the new structure?
On 2016/05/12 10:42:43, ivoc wrote: > Nice! LGTM with one question. > > https://codereview.webrtc.org/1963493003/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/aec/aec_core.cc (right): > > https://codereview.webrtc.org/1963493003/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/aec/aec_core.cc:1031: memcpy(hNl, cohde, > sizeof(hNl[0]) * PART_LEN1); > Is this better than sizeof(hNl), or does that not work with the new structure? Great! Thanks for the review!!!
https://codereview.webrtc.org/1963493003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1963493003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:1031: memcpy(hNl, cohde, sizeof(hNl[0]) * PART_LEN1); On 2016/05/12 10:42:43, ivoc wrote: > Is this better than sizeof(hNl), or does that not work with the new structure? No, I would have preferred the previous way to do it, but the compiler complained so I needed to change the memcpy call to be like this. I think the difference may be that the compiler only has access to a pointer to the matrix in this case, while it in the former case had full access to the actual variable declaration.
https://codereview.webrtc.org/1963493003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1963493003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:1031: memcpy(hNl, cohde, sizeof(hNl[0]) * PART_LEN1); On 2016/05/12 11:07:50, peah-webrtc wrote: > On 2016/05/12 10:42:43, ivoc wrote: > > Is this better than sizeof(hNl), or does that not work with the new structure? > > No, I would have preferred the previous way to do it, but the compiler > complained so I needed to change the memcpy call to be like this. I think the > difference may be that the compiler only has access to a pointer to the matrix > in this case, while it in the former case had full access to the actual variable > declaration. I see, that makes sense, thanks.
https://codereview.webrtc.org/1963493003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1963493003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:1031: memcpy(hNl, cohde, sizeof(hNl[0]) * PART_LEN1); On 2016/05/12 11:19:39, ivoc wrote: > On 2016/05/12 11:07:50, peah-webrtc wrote: > > On 2016/05/12 10:42:43, ivoc wrote: > > > Is this better than sizeof(hNl), or does that not work with the new > structure? > > > > No, I would have preferred the previous way to do it, but the compiler > > complained so I needed to change the memcpy call to be like this. I think the > > difference may be that the compiler only has access to a pointer to the matrix > > in this case, while it in the former case had full access to the actual > variable > > declaration. > > I see, that makes sense, thanks. It'll work if you pass a reference to the array as an argument. Passing just the array like you do here is equivalent to passing a pointer, for historical reasons. The syntax for passing a reference to an array is a wee bit awkward because you need extra parentheses: T (&array)[N]. Side note: Since this code is C++ now, you can use std::copy instead of memcpy. Once they are arrays, you ought to be able to write e.g. std::copy(std::begin(cohde), std::end(cohde), std::begin(hNl));
https://codereview.webrtc.org/1963493003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1963493003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:1031: memcpy(hNl, cohde, sizeof(hNl[0]) * PART_LEN1); On 2016/05/12 11:29:58, kwiberg-webrtc wrote: > On 2016/05/12 11:19:39, ivoc wrote: > > On 2016/05/12 11:07:50, peah-webrtc wrote: > > > On 2016/05/12 10:42:43, ivoc wrote: > > > > Is this better than sizeof(hNl), or does that not work with the new > > structure? > > > > > > No, I would have preferred the previous way to do it, but the compiler > > > complained so I needed to change the memcpy call to be like this. I think > the > > > difference may be that the compiler only has access to a pointer to the > matrix > > > in this case, while it in the former case had full access to the actual > > variable > > > declaration. > > > > I see, that makes sense, thanks. > > It'll work if you pass a reference to the array as an argument. Passing just the > array like you do here is equivalent to passing a pointer, for historical > reasons. > > The syntax for passing a reference to an array is a wee bit awkward because you > need extra parentheses: T (&array)[N]. > > Side note: Since this code is C++ now, you can use std::copy instead of memcpy. > Once they are arrays, you ought to be able to write e.g. > > std::copy(std::begin(cohde), std::end(cohde), std::begin(hNl)); Thanks! The ::copy approach is definitely much nicer than the memcpy. Will adopt that asap :-)
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 Link to the patchset: https://codereview.webrtc.org/1963493003/#ps20001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1963493003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1963493003/20001
Message was sent while issue was closed.
Description was changed from ========== Moved the AEC echo suppression gain computation code to a separate method. This CL will be followed by other CLs that simplify this method and break out the state specific to this computation into a separate substate. The changes are bitexact. BUG=webrtc:5201, webrtc:5298 ========== to ========== Moved the AEC echo suppression gain computation code to a separate method. This CL will be followed by other CLs that simplify this method and break out the state specific to this computation into a separate substate. The changes are bitexact. BUG=webrtc:5201, webrtc:5298 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Moved the AEC echo suppression gain computation code to a separate method. This CL will be followed by other CLs that simplify this method and break out the state specific to this computation into a separate substate. The changes are bitexact. BUG=webrtc:5201, webrtc:5298 ========== to ========== Moved the AEC echo suppression gain computation code to a separate method. This CL will be followed by other CLs that simplify this method and break out the state specific to this computation into a separate substate. The changes are bitexact. BUG=webrtc:5201, webrtc:5298 Committed: https://crrev.com/9bbf89bca1d722cf5d1c4b3e98517c7c3f71d3e3 Cr-Commit-Position: refs/heads/master@{#12712} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9bbf89bca1d722cf5d1c4b3e98517c7c3f71d3e3 Cr-Commit-Position: refs/heads/master@{#12712}
Message was sent while issue was closed.
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/1963493003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1963493003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:1031: memcpy(hNl, cohde, sizeof(hNl[0]) * PART_LEN1); On 2016/05/13 04:05:47, peah-webrtc wrote: > On 2016/05/12 11:29:58, kwiberg-webrtc wrote: > > On 2016/05/12 11:19:39, ivoc wrote: > > > On 2016/05/12 11:07:50, peah-webrtc wrote: > > > > On 2016/05/12 10:42:43, ivoc wrote: > > > > > Is this better than sizeof(hNl), or does that not work with the new > > > structure? > > > > > > > > No, I would have preferred the previous way to do it, but the compiler > > > > complained so I needed to change the memcpy call to be like this. I think > > the > > > > difference may be that the compiler only has access to a pointer to the > > matrix > > > > in this case, while it in the former case had full access to the actual > > > variable > > > > declaration. > > > > > > I see, that makes sense, thanks. > > > > It'll work if you pass a reference to the array as an argument. Passing just > the > > array like you do here is equivalent to passing a pointer, for historical > > reasons. > > > > The syntax for passing a reference to an array is a wee bit awkward because > you > > need extra parentheses: T (&array)[N]. > > > > Side note: Since this code is C++ now, you can use std::copy instead of > memcpy. > > Once they are arrays, you ought to be able to write e.g. > > > > std::copy(std::begin(cohde), std::end(cohde), std::begin(hNl)); > > Thanks! The ::copy approach is definitely much nicer than the memcpy. Will adopt > that asap :-) Do note that while it's specialized for trivial types, IIRC it uses memmove rather than memcpy since it can't assume nonoverlapping source and target arrays. So in really really performance-critical situations, memcpy is still a valid choice. In this case the ease-of-use seems like a good trade-off, though. |
