Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1770823002:
Removed the AudioProcessing dependency in EchoCancellerImpl (Closed)
|
Created:
4 years, 9 months ago by peah-webrtc Modified:
4 years, 9 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@CleanUpAecImpl_CL Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemoved the AudioProcessing dependency in EchoCancellerImpl.
This CL removes the dependency of AudioProcessing in
EchoCancellerImpl. It is breaking the public APM API by
having a different error code behavior so please review it
carefully. I made a comment about the API breaking change
in the code section of this CL.
BUG=webrtc:5337
Committed: https://crrev.com/b58a158fce9a64ac372913ae99f462cf228aa9e2
Cr-Commit-Position: refs/heads/master@{#11998}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Changed the way the error checks were done #
Total comments: 2
Patch Set 3 : Added missing removal of now unused state variables #
Total comments: 20
Patch Set 4 : Changes in response to reviewer comments #Patch Set 5 : Corrected the error code returned when the stream delay had not been set #
Total comments: 9
Patch Set 6 : Changes in response to reviewer comments #Patch Set 7 : Rebase with latest master #Patch Set 8 : Rebase from latest master #
Messages
Total messages: 22 (8 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Removed the APM dependency in EchoCancellerImpl. BUG=webrtc:5337 ========== to ========== Removed the AudioProcessing dependency in EchoCancellerImpl. This CL removes the dependency of AudioProcessing in EchoCancellerImpl. It is breaking the public APM API by having a different error code behavior so please review it carefully. I made a comment about the API breaking change in the code section of this CL. BUG=webrtc:5337 ==========
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, solenberg@webrtc.org
https://codereview.webrtc.org/1770823002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (left): https://codereview.webrtc.org/1770823002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:249: if (enable && apm_->echo_control_mobile()->is_enabled()) { Please examine this change carefully as it is actually an api-breaking change. The check is moved from this function call to the ProcessStream() call in APM, which will then report an error message if both AEC and AECM are active at the same time. I saw this approach as the only way to remove the dependency of APM without first deprecating the AEC Enable call, which is a public API call in APM. Please let me know your concerns about this and whether you see any better solution for this!
LGTM with comments. https://codereview.webrtc.org/1770823002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1770823002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:722: RETURN_ON_ERR((public_submodules_->echo_cancellation->is_enabled() && You don't have to use RETURN_ON_ERR here, since you are essentially creating your return values in situ. if (...) return AudioProcessing::kNoError; https://codereview.webrtc.org/1770823002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:729: RETURN_ON_ERR((was_stream_delay_set() ? AudioProcessing::kNoError Same as above.
https://codereview.webrtc.org/1770823002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1770823002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:722: RETURN_ON_ERR((public_submodules_->echo_cancellation->is_enabled() && On 2016/03/07 16:03:08, hlundin-webrtc wrote: > You don't have to use RETURN_ON_ERR here, since you are essentially creating > your return values in situ. > > if (...) > return AudioProcessing::kNoError; Done. https://codereview.webrtc.org/1770823002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:722: RETURN_ON_ERR((public_submodules_->echo_cancellation->is_enabled() && On 2016/03/07 16:03:08, hlundin-webrtc wrote: > You don't have to use RETURN_ON_ERR here, since you are essentially creating > your return values in situ. > > if (...) > return AudioProcessing::kNoError; You are definitely right on that. It is just that I wanted to use the macro RETURN_ON_ERR for consistency, as it basically does exactly the if-statement if (public_submodules_->echo_cancellation->is_enabled() && public_submodules_->echo_control_mobile->is_enabled()) { return AudioProcessing::kBadParameterError; } The advantage of doing the full if-statement is that it is more explicit and the right thing to do as the macro is really mainly intended for function error return values, but the disadvantage is that it clutters the code and that it mixes up two ways of reporting errors. I'll change, but please let me know if you in light of my comments above would think it could make sense to do it the way it was. https://codereview.webrtc.org/1770823002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:729: RETURN_ON_ERR((was_stream_delay_set() ? AudioProcessing::kNoError On 2016/03/07 16:03:08, hlundin-webrtc wrote: > Same as above. Done.
Still LGTM. https://codereview.webrtc.org/1770823002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1770823002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:722: RETURN_ON_ERR((public_submodules_->echo_cancellation->is_enabled() && On 2016/03/08 06:05:08, peah-webrtc wrote: > On 2016/03/07 16:03:08, hlundin-webrtc wrote: > > You don't have to use RETURN_ON_ERR here, since you are essentially creating > > your return values in situ. > > > > if (...) > > return AudioProcessing::kNoError; > > Done. I think the code is nicer without the macro.
https://codereview.webrtc.org/1770823002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1770823002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:722: if (public_submodules_->echo_cancellation->is_enabled() && Make this a DCHECK and move it to the beginning of the method. https://codereview.webrtc.org/1770823002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:729: if (!was_stream_delay_set()) { Likewise, this is a precondition, so move to beginning of method. It's also a logic error (and one which we should make impossible by setting property at construction) so make it a DCHECK.
https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:61: public: remove; default for struct https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:71: int sample_rate_hz; default init these values or add StreamProperties() = delete; https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:130: RTC_DCHECK(!!stream_properties_); One ! too many https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:135: audio->num_channels()); Funky formatting https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:183: RTC_DCHECK(!!stream_properties_); One ! too many https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:214: RTC_DCHECK(!!stream_properties_); .. https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:257: int EchoCancellationImpl::Enable(bool enable) { nit: Could you make a separate CL to change the order of method definitions to match the declaration? https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:267: RTC_DCHECK(!!stream_properties_); .. https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:556: RTC_DCHECK(!!stream_properties_); .. https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.h:41: void Initialize(); Not used?
https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:61: public: On 2016/03/11 10:26:01, the sun wrote: > remove; default for struct Done. https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:71: int sample_rate_hz; On 2016/03/11 10:26:01, the sun wrote: > default init these values or add > StreamProperties() = delete; Done. https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:130: RTC_DCHECK(!!stream_properties_); On 2016/03/11 10:26:01, the sun wrote: > One ! too many Actually two !! too many. I thought the double !! was needed for the unique_ptr (it was for the rtc:: variant, right?) https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:135: audio->num_channels()); On 2016/03/11 10:26:01, the sun wrote: > Funky formatting Done. https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:183: RTC_DCHECK(!!stream_properties_); On 2016/03/11 10:26:01, the sun wrote: > One ! too many Done. https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:214: RTC_DCHECK(!!stream_properties_); On 2016/03/11 10:26:01, the sun wrote: > .. Done. https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:257: int EchoCancellationImpl::Enable(bool enable) { On 2016/03/11 10:26:01, the sun wrote: > nit: Could you make a separate CL to change the order of method definitions to > match the declaration? Acknowledged. https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:267: RTC_DCHECK(!!stream_properties_); On 2016/03/11 10:26:01, the sun wrote: > .. Done. https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:556: RTC_DCHECK(!!stream_properties_); On 2016/03/11 10:26:01, the sun wrote: > .. Done. https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1770823002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.h:41: void Initialize(); On 2016/03/11 10:26:02, the sun wrote: > Not used? Done.
Corrected an incorrect error code that was returned, which broke tests that I had not yet run. Sorry for the late code change, but at least it is very minor.
https://codereview.webrtc.org/1770823002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1770823002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:722: if (public_submodules_->echo_cancellation->is_enabled() && Make this a DCHECK and move it to the beginning of the method. https://codereview.webrtc.org/1770823002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:730: !was_stream_delay_set()) { Likewise, this is a precondition, so move to beginning of method. It's also a logic error (and one which we should make impossible by setting the property at construction) so make it a DCHECK. https://codereview.webrtc.org/1770823002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1770823002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:71: int sample_rate_hz; const declare these or make it a class with accessors https://codereview.webrtc.org/1770823002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1770823002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:84: nit: delete one blank line
https://codereview.webrtc.org/1770823002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1770823002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:722: if (public_submodules_->echo_cancellation->is_enabled() && On 2016/03/15 09:38:55, the sun wrote: > Make this a DCHECK and move it to the beginning of the method. Done. https://codereview.webrtc.org/1770823002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:730: !was_stream_delay_set()) { On 2016/03/15 09:38:55, the sun wrote: > Likewise, this is a precondition, so move to beginning of method. It's also a > logic error (and one which we should make impossible by setting the property at > construction) so make it a DCHECK. What you say makes sense! I think this case is a bit different though compared to the above one as that one is actually a necessary change that also breaks the API behavior. In this case, there is actually a unittest that test for this error code though. Possibly there may be external code that relies on this error message as well. I definitely agree that this should be refactored but I think it makes sense to postpone that to upcoming CLs as the current code construct maintains the APM behavior that was before this CL. Removing the unittest for this error code is trivial, and I'd be happy to do that, but before doing that I just wanted to raise the above concerns. whyt? https://codereview.webrtc.org/1770823002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1770823002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:71: int sample_rate_hz; On 2016/03/15 09:38:55, the sun wrote: > const declare these or make it a class with accessors Great point! Done. https://codereview.webrtc.org/1770823002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1770823002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:84: On 2016/03/15 09:38:55, the sun wrote: > nit: delete one blank line Done.
lgtm https://codereview.webrtc.org/1770823002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1770823002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:730: !was_stream_delay_set()) { On 2016/03/15 10:32:10, peah-webrtc wrote: > On 2016/03/15 09:38:55, the sun wrote: > > Likewise, this is a precondition, so move to beginning of method. It's also a > > logic error (and one which we should make impossible by setting the property > at > > construction) so make it a DCHECK. > > What you say makes sense! I think this case is a bit different though compared > to the above one as that one is actually a necessary change that also breaks the > API behavior. > > In this case, there is actually a unittest that test for this error code though. > Possibly there may be external code that relies on this error message as well. > > I definitely agree that this should be refactored but I think it makes sense to > postpone that to upcoming CLs as the current code construct maintains the APM > behavior that was before this CL. > > Removing the unittest for this error code is trivial, and I'd be happy to do > that, but before doing that I just wanted to raise the above concerns. > > whyt? Ok, let's leave as is.
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, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1770823002/#ps180001 (title: "Rebase from latest master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1770823002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1770823002/180001
Message was sent while issue was closed.
Description was changed from ========== Removed the AudioProcessing dependency in EchoCancellerImpl. This CL removes the dependency of AudioProcessing in EchoCancellerImpl. It is breaking the public APM API by having a different error code behavior so please review it carefully. I made a comment about the API breaking change in the code section of this CL. BUG=webrtc:5337 ========== to ========== Removed the AudioProcessing dependency in EchoCancellerImpl. This CL removes the dependency of AudioProcessing in EchoCancellerImpl. It is breaking the public APM API by having a different error code behavior so please review it carefully. I made a comment about the API breaking change in the code section of this CL. BUG=webrtc:5337 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Removed the AudioProcessing dependency in EchoCancellerImpl. This CL removes the dependency of AudioProcessing in EchoCancellerImpl. It is breaking the public APM API by having a different error code behavior so please review it carefully. I made a comment about the API breaking change in the code section of this CL. BUG=webrtc:5337 ========== to ========== Removed the AudioProcessing dependency in EchoCancellerImpl. This CL removes the dependency of AudioProcessing in EchoCancellerImpl. It is breaking the public APM API by having a different error code behavior so please review it carefully. I made a comment about the API breaking change in the code section of this CL. BUG=webrtc:5337 Committed: https://crrev.com/b58a158fce9a64ac372913ae99f462cf228aa9e2 Cr-Commit-Position: refs/heads/master@{#11998} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b58a158fce9a64ac372913ae99f462cf228aa9e2 Cr-Commit-Position: refs/heads/master@{#11998} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
