Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1761813002:
Removed the inheritance from ProcessingComponent for EchoCancellerImpl. (Closed)
|
Created:
4 years, 9 months ago by peah-webrtc Modified:
4 years, 9 months ago Reviewers:
the sun 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. |
DescriptionRemoved the inheritance from ProcessingComponent for EchoCancellerImpl.
BUG=webrtc:5352
Committed: https://crrev.com/3af0a009f8a7f2dfb630a4f4730044cbbd95bee8
Cr-Commit-Position: refs/heads/master@{#11876}
Committed: https://crrev.com/b624d8c852fbe8eff8ff8888f673191b8a411421
Cr-Commit-Position: refs/heads/master@{#11881}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Changed CHECK to DCHECK and moved the checking for aec data processing to where the other checks ar… #Patch Set 3 : Changed erroneous DCHECK #Patch Set 4 : Manual merge from master in order to avoid bad automatic merge #Patch Set 5 : Corrected a bad merge #
Messages
Total messages: 49 (25 generated)
Description was changed from ========== Removed the inheritance from ProcessingComponent for EchoCancellerImpl. BUG= ========== to ========== Removed the inheritance from ProcessingComponent for EchoCancellerImpl. BUG=webrtc:5352 ==========
peah@webrtc.org changed reviewers: + solenberg@webrtc.org
https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.cc:74: Handle* state() { return state_; } So removing this (either folding functions into Canceller, or returning a proper type) will be in a follow-up? https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.h:79: const AudioProcessing* apm_; Ah, this is why there aren't more changes to the implementation. Yes, I guess it makes sense to remove the dependency on AudioProcessing in a follow-up CL. We certainly want to break to circular relationship. https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.h:84: bool enabled_ = false; No crit sect guard?
https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.cc:74: Handle* state() { return state_; } On 2016/03/03 10:00:53, the sun wrote: > So removing this (either folding functions into Canceller, or returning a proper > type) will be in a follow-up? Yes, that is the intention. I could do it in this CL, but I think it makes sense to minimize the changes. Wdyt? https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.h:79: const AudioProcessing* apm_; On 2016/03/03 10:00:53, the sun wrote: > Ah, this is why there aren't more changes to the implementation. Yes, I guess it > makes sense to remove the dependency on AudioProcessing in a follow-up CL. We > certainly want to break to circular relationship. Yes, that should definitely be broken. But in can be done in a separate CL and therefore we should do it in a separate CL I think. https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.h:84: bool enabled_ = false; On 2016/03/03 10:00:53, the sun wrote: > No crit sect guard? This is not possible, as this variable is used in such a manner that it is only changed when holding both the render and capture locks, but is read while holding either (or both) of them.
lgtm https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:1148: if (public_submodules_->echo_cancellation->is_enabled()) { I think you can put this in the first conditional in this function, with the hpf and ns equivalents. Or make things consistent by having a separate condition for each sub component. https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.cc:70: RTC_CHECK(state_); DCHECK not CHECK here https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.cc:74: Handle* state() { return state_; } On 2016/03/03 12:06:33, peah-webrtc wrote: > On 2016/03/03 10:00:53, the sun wrote: > > So removing this (either folding functions into Canceller, or returning a > proper > > type) will be in a follow-up? > > Yes, that is the intention. I could do it in this CL, but I think it makes sense > to minimize the changes. > Wdyt? I think that is good thinking! :) https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.h:79: const AudioProcessing* apm_; On 2016/03/03 12:06:34, peah-webrtc wrote: > On 2016/03/03 10:00:53, the sun wrote: > > Ah, this is why there aren't more changes to the implementation. Yes, I guess > it > > makes sense to remove the dependency on AudioProcessing in a follow-up CL. We > > certainly want to break to circular relationship. > > Yes, that should definitely be broken. But in can be done in a separate CL and > therefore we should do it in a separate CL I think. Good idea to do things piecewise! When I did the HPF etc there were a lot of changes because I did it all in one go. I don't think that would work well with something as large as the AEC. https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.h:84: bool enabled_ = false; On 2016/03/03 12:06:34, peah-webrtc wrote: > On 2016/03/03 10:00:53, the sun wrote: > > No crit sect guard? > > This is not possible, as this variable is used in such a manner that it is only > changed when holding both the render and capture locks, but is read while > holding either (or both) of them. Acknowledged.
https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:1148: if (public_submodules_->echo_cancellation->is_enabled()) { On 2016/03/03 13:13:05, the sun wrote: > I think you can put this in the first conditional in this function, with the hpf > and ns equivalents. Or make things consistent by having a separate condition for > each sub component. Done. https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.cc:70: RTC_CHECK(state_); On 2016/03/03 13:13:05, the sun wrote: > DCHECK not CHECK here Done. https://codereview.webrtc.org/1761813002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.cc:74: Handle* state() { return state_; } On 2016/03/03 13:13:05, the sun wrote: > On 2016/03/03 12:06:33, peah-webrtc wrote: > > On 2016/03/03 10:00:53, the sun wrote: > > > So removing this (either folding functions into Canceller, or returning a > > proper > > > type) will be in a follow-up? > > > > Yes, that is the intention. I could do it in this CL, but I think it makes > sense > > to minimize the changes. > > Wdyt? > > I think that is good thinking! :) Acknowledged.
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/1761813002/#ps20001 (title: "Changed CHECK to DCHECK and moved the checking for aec data processing to where the other checks ar…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1761813002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1761813002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/12916)
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/1761813002/#ps40001 (title: "Changed erroneous DCHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1761813002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1761813002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_ubsan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/232) linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/12929)
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1761813002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1761813002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/240) linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
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/1761813002/#ps60001 (title: "Manual merge from master in order to avoid bad automatic merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1761813002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1761813002/60001
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) android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on 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/patch-status/1761813002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1761813002/60001
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)
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/1761813002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1761813002/60001
Message was sent while issue was closed.
Description was changed from ========== Removed the inheritance from ProcessingComponent for EchoCancellerImpl. BUG=webrtc:5352 ========== to ========== Removed the inheritance from ProcessingComponent for EchoCancellerImpl. BUG=webrtc:5352 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Removed the inheritance from ProcessingComponent for EchoCancellerImpl. BUG=webrtc:5352 ========== to ========== Removed the inheritance from ProcessingComponent for EchoCancellerImpl. BUG=webrtc:5352 Committed: https://crrev.com/3af0a009f8a7f2dfb630a4f4730044cbbd95bee8 Cr-Commit-Position: refs/heads/master@{#11876} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3af0a009f8a7f2dfb630a4f4730044cbbd95bee8 Cr-Commit-Position: refs/heads/master@{#11876}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.webrtc.org/1764293002/ by solenberg@google.com. The reason for reverting is: Breaks all bots, e.g. https://uberchromegw.corp.google.com/i/client.webrtc/builders/Android32%20Bui... Looks like a missing rebase before landing. Resolve and try again..
Message was sent while issue was closed.
Description was changed from ========== Removed the inheritance from ProcessingComponent for EchoCancellerImpl. BUG=webrtc:5352 Committed: https://crrev.com/3af0a009f8a7f2dfb630a4f4730044cbbd95bee8 Cr-Commit-Position: refs/heads/master@{#11876} ========== to ========== Removed the inheritance from ProcessingComponent for EchoCancellerImpl. BUG=webrtc:5352 Committed: https://crrev.com/3af0a009f8a7f2dfb630a4f4730044cbbd95bee8 Cr-Commit-Position: refs/heads/master@{#11876} ==========
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/1761813002/#ps80001 (title: "Corrected a bad merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1761813002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1761813002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
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/1761813002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1761813002/80001
Message was sent while issue was closed.
Description was changed from ========== Removed the inheritance from ProcessingComponent for EchoCancellerImpl. BUG=webrtc:5352 Committed: https://crrev.com/3af0a009f8a7f2dfb630a4f4730044cbbd95bee8 Cr-Commit-Position: refs/heads/master@{#11876} ========== to ========== Removed the inheritance from ProcessingComponent for EchoCancellerImpl. BUG=webrtc:5352 Committed: https://crrev.com/3af0a009f8a7f2dfb630a4f4730044cbbd95bee8 Cr-Commit-Position: refs/heads/master@{#11876} ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Removed the inheritance from ProcessingComponent for EchoCancellerImpl. BUG=webrtc:5352 Committed: https://crrev.com/3af0a009f8a7f2dfb630a4f4730044cbbd95bee8 Cr-Commit-Position: refs/heads/master@{#11876} ========== to ========== Removed the inheritance from ProcessingComponent for EchoCancellerImpl. BUG=webrtc:5352 Committed: https://crrev.com/3af0a009f8a7f2dfb630a4f4730044cbbd95bee8 Cr-Commit-Position: refs/heads/master@{#11876} Committed: https://crrev.com/b624d8c852fbe8eff8ff8888f673191b8a411421 Cr-Commit-Position: refs/heads/master@{#11881} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b624d8c852fbe8eff8ff8888f673191b8a411421 Cr-Commit-Position: refs/heads/master@{#11881} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
