Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1768943002:
Removed the dependency in GainControlImpl on the ProcessingComponent class (Closed)
|
Created:
4 years, 9 months ago by peah-webrtc Modified:
4 years, 9 months ago Reviewers:
the sun 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@RemoveComponentFromAECM_CL Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemoved the dependency in GainControlImpl on the ProcessingComponent class
BUG=webrtc:5353
Committed: https://crrev.com/bfa971198d8f7047e6f74840307b3c1a17df6fb4
Cr-Commit-Position: refs/heads/master@{#11949}
Patch Set 1 : Removed the dependency in GainControlImpl on the ProcessComponent class #Patch Set 2 : Changes according to reviewer comments #
Total comments: 31
Patch Set 3 : Changes in response to reviewer comments #
Total comments: 10
Patch Set 4 : Changes in response to reviewer comments #
Total comments: 3
Patch Set 5 : Rebase with latest master #Patch Set 6 : Removed blank line and changed assert to DCHECK #
Dependent Patchsets: Messages
Total messages: 27 (11 generated)
Patchset #1 (id:1) has been deleted
peah@webrtc.org changed reviewers: + solenberg@webrtc.org
Looks like I'll have basically the same set of comments as on the EchoControlMobile CL. Please give it a once-over and fix DCHECK/CHECKs, etc, then ping me back.
Patchset #2 (id:40001) has been deleted
On 2016/03/07 14:54:20, the sun wrote: > Looks like I'll have basically the same set of comments as on the > EchoControlMobile CL. Please give it a once-over and fix DCHECK/CHECKs, etc, > then ping me back. Great! The corresponding changes should now be in place :-)
https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:84: : echo_cancellation(nullptr), echo_control_mobile(nullptr) {} Need a rebase? https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:13: #include <assert.h> Remove assert() while you're at it https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:15: #include "webrtc/base/constructormagic.h" Won't be needed. https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:54: RTC_DCHECK(state_); remove this DCHECK or add one in each of the other methods. https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:145: for (size_t i = 0; i < num_handles_required(); i++) { for (auto& gain_controller : gain_controllers_) { https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:163: RTC_DCHECK_EQ(audio->num_channels(), num_handles_required()); num_handles_required() -> gain_controllers_.size(), here, and below methods and loops. https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:173: audio->split_bands(i), Is there one split band per channel? Is the DCHECK_EQ above correct? https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:253: analog_capture_level_ += capture_levels_[i]; Make capture_level_ a member of GainController to avoid keeping the arrays in sync? https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:358: if (level > 31 || level < 0) { Verify the input parameter outside this scope, before taking the lock. https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:375: if (gain < 0 || gain > 90) { ditto https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:412: const size_t gain_controllers_old_size = gain_controllers_.size(); Remove const https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:478: error = AudioProcessing::kNoError; error = handle_error https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.h (right): https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.h:32: virtual ~GainControlImpl(); override instead of virtual https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.h:100: std::vector<std::unique_ptr<GainController>> gain_controllers_; Add a little constructormagic!
https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:84: : echo_cancellation(nullptr), echo_control_mobile(nullptr) {} On 2016/03/09 09:42:10, the sun wrote: > Need a rebase? Yes definitely, I delayed that though as that would introduce noise in the code review. https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:13: #include <assert.h> On 2016/03/09 09:42:11, the sun wrote: > Remove assert() while you're at it Done. https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:15: #include "webrtc/base/constructormagic.h" On 2016/03/09 09:42:11, the sun wrote: > Won't be needed. Done. https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:54: RTC_DCHECK(state_); On 2016/03/09 09:42:10, the sun wrote: > remove this DCHECK or add one in each of the other methods. Done. https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:145: for (size_t i = 0; i < num_handles_required(); i++) { On 2016/03/09 09:42:10, the sun wrote: > for (auto& gain_controller : gain_controllers_) { This does not work, as the number of gain controllers may be larger than num_handles_required. This behavior is weird, but according to how it was before. Lets refactor the whole subcomponent-channel behavior once this is done to remedy that. I'll keep it as it is for now. Wdyt? https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:163: RTC_DCHECK_EQ(audio->num_channels(), num_handles_required()); On 2016/03/09 09:42:11, the sun wrote: > num_handles_required() -> gain_controllers_.size(), here, and below methods and > loops. Done. https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:173: audio->split_bands(i), On 2016/03/09 09:42:10, the sun wrote: > Is there one split band per channel? Is the DCHECK_EQ above correct? In analog_agc.c, the in each channel bands are processed. Therefore, at this level, the bands are just passed to the AGC, and the AGC then received a ** (pointer to pointer). Therefore, I think the DCHECK is fine (and it also matches what was in place before this CL as well as before the locking scheme was introduced. https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:253: analog_capture_level_ += capture_levels_[i]; On 2016/03/09 09:42:10, the sun wrote: > Make capture_level_ a member of GainController to avoid keeping the arrays in > sync? Awesome idea!!! Done. https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:358: if (level > 31 || level < 0) { On 2016/03/09 09:42:11, the sun wrote: > Verify the input parameter outside this scope, before taking the lock. Done. https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:375: if (gain < 0 || gain > 90) { On 2016/03/09 09:42:11, the sun wrote: > ditto Done. https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:412: const size_t gain_controllers_old_size = gain_controllers_.size(); On 2016/03/09 09:42:10, the sun wrote: > Remove const Done. https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:478: error = AudioProcessing::kNoError; On 2016/03/09 09:42:11, the sun wrote: > error = handle_error Good catch! Done. https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.h (right): https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.h:32: virtual ~GainControlImpl(); On 2016/03/09 09:42:11, the sun wrote: > override instead of virtual Done. https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.h:100: std::vector<std::unique_ptr<GainController>> gain_controllers_; On 2016/03/09 09:42:11, the sun wrote: > Add a little constructormagic! Done.
https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:145: for (size_t i = 0; i < num_handles_required(); i++) { On 2016/03/09 12:23:39, peah-webrtc wrote: > On 2016/03/09 09:42:10, the sun wrote: > > for (auto& gain_controller : gain_controllers_) { > > This does not work, as the number of gain controllers may be larger than > num_handles_required. > > This behavior is weird, but according to how it was before. Lets refactor the > whole subcomponent-channel behavior once this is done to remedy that. > > I'll keep it as it is for now. Wdyt? That's ok, but it's quite a simple fix to resize the gain_controllers_ array even when the number of required "handles" decrease. https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:413: gain_controllers_.resize(num_handles_required()); Like the comment for EchoControlMobile - is there any reason to keep the unused objects around, if num_handles_required() decreases? https://codereview.webrtc.org/1768943002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1768943002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:51: ~GainController() { WebRtcAgc_Free(state_); } Add DCHECK here or remove in the other places (and rely on class invariant). https://codereview.webrtc.org/1768943002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:64: const int error = remove const https://codereview.webrtc.org/1768943002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:83: rtc::Optional<int> capture_level_; The Optional seems a bit overkill in this situation, but sure, it allows to have the DCHECK in get_capture_level(). Add a TODO that it should be removed once GainController::Initialize() is folded into the ctor. https://codereview.webrtc.org/1768943002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:367: if (level > 31 || level < 0) { Move out of scope; scope is there to control the CritScope lifetime. https://codereview.webrtc.org/1768943002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:384: if (gain < 0 || gain > 90) { ditto
https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1768943002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:145: for (size_t i = 0; i < num_handles_required(); i++) { On 2016/03/09 13:01:53, the sun wrote: > On 2016/03/09 12:23:39, peah-webrtc wrote: > > On 2016/03/09 09:42:10, the sun wrote: > > > for (auto& gain_controller : gain_controllers_) { > > > > This does not work, as the number of gain controllers may be larger than > > num_handles_required. > > > > This behavior is weird, but according to how it was before. Lets refactor the > > whole subcomponent-channel behavior once this is done to remedy that. > > > > I'll keep it as it is for now. Wdyt? > > That's ok, but it's quite a simple fix to resize the gain_controllers_ array > even when the number of required "handles" decrease. Simple it indeed is but this is as it was before this CL. I.e.: cancellers were allocated when needed but not destructed when no longer needed. There is a slight point in doing that since if they will again be needed later on they are already constructed and no overhead of canceller construction will be taken. Having said that, this scheme is a thing that should be refactored, and since I now see an opportunity to start that refactoring within the scope of this CL, I will boldly take that opportunity :-) https://codereview.webrtc.org/1768943002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1768943002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:51: ~GainController() { WebRtcAgc_Free(state_); } On 2016/03/09 13:01:53, the sun wrote: > Add DCHECK here or remove in the other places (and rely on class invariant). Done. https://codereview.webrtc.org/1768943002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:64: const int error = On 2016/03/09 13:01:53, the sun wrote: > remove const Done. https://codereview.webrtc.org/1768943002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:83: rtc::Optional<int> capture_level_; On 2016/03/09 13:01:53, the sun wrote: > The Optional seems a bit overkill in this situation, but sure, it allows to have > the DCHECK in get_capture_level(). Add a TODO that it should be removed once > GainController::Initialize() is folded into the ctor. Done. https://codereview.webrtc.org/1768943002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:367: if (level > 31 || level < 0) { On 2016/03/09 13:01:53, the sun wrote: > Move out of scope; scope is there to control the CritScope lifetime. Done. https://codereview.webrtc.org/1768943002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:384: if (gain < 0 || gain > 90) { On 2016/03/09 13:01:53, the sun wrote: > ditto Done.
lgtm https://codereview.webrtc.org/1768943002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1768943002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:31: assert(false); RTC_DCHECK https://codereview.webrtc.org/1768943002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:370: nit: delete blank line
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1768943002/#ps120001 (title: "Rebase with latest master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1768943002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1768943002/120001
The CQ bit was unchecked by peah@webrtc.org
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1768943002/#ps140001 (title: "Removed blank line and changed assert to DCHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1768943002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1768943002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
https://codereview.webrtc.org/1768943002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1768943002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:370: On 2016/03/09 21:51:24, the sun wrote: > nit: delete blank line Done.
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/1768943002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1768943002/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Removed the dependency in GainControlImpl on the ProcessingComponent class BUG=webrtc:5353 ========== to ========== Removed the dependency in GainControlImpl on the ProcessingComponent class BUG=webrtc:5353 Committed: https://crrev.com/bfa971198d8f7047e6f74840307b3c1a17df6fb4 Cr-Commit-Position: refs/heads/master@{#11949} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/bfa971198d8f7047e6f74840307b3c1a17df6fb4 Cr-Commit-Position: refs/heads/master@{#11949} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
