Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1772453002:
Removing dependency of the EchoControlMobileImpl class on ProcessingComponent. (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. |
DescriptionRemoving dependency of the EchoControlMobileImpl class on ProcessingComponent.
BUG=webrtc:5351
Committed: https://crrev.com/bb9edbd04852fd0932642369b3938995c7e705fb
Cr-Commit-Position: refs/heads/master@{#11945}
Patch Set 1 #Patch Set 2 : Fixed a couple of implementation errors #
Total comments: 31
Patch Set 3 : Changes in response to reviewer comments #
Total comments: 11
Patch Set 4 : Changes in response to reviewer comments #
Total comments: 8
Patch Set 5 : Changes in response to reviewer comments #
Total comments: 8
Patch Set 6 : Changes in response to reviewer comments" #
Total comments: 2
Patch Set 7 : Changed DCHECK format #Patch Set 8 : Merge from master #Patch Set 9 : Fixed counter update issue that was triggered for multiple streams and fixed error reporting behavi… #Patch Set 10 : Corrected error check to not be done when AECM is not enabled #Patch Set 11 : Rebase with latest master #
Messages
Total messages: 53 (24 generated)
peah@webrtc.org changed reviewers: + solenberg@webrtc.org
Good stuff! https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1257: void AudioProcessingImpl::InitializeEchoControlMobile() { In a future CL, consider removing these methods since they are only called in a single place. https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:74: RTC_DCHECK(state_); RTC_CHECK https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:78: RTC_CHECK(state_); RTC_DCHECK, or just don't check - as per the ctor it's part of the Canceller invariant that state_ != nullptr https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:88: RTC_DCHECK_EQ(AudioProcessing::kNoError, error); DCHECK or CHECK? Can this ever fail? To we reallocate internal buffers or anything? https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:92: RTC_DCHECK_EQ(AudioProcessing::kNoError, error); Likewise - DCHECK or CHECK? https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:97: Handle* state_; RTC_DISALLOW_COPY_AND_ASSIGN(EchoControlMobileImpl::Canceller); https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:139: Handle* my_handle = cancellers_[handle_index++]->state(); fyi: while neat, doing multiple things in the same line actually makes it harder to read and to step through in a debugger https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:331: const int32_t err = I don't think you should const declare this local. You'll have a LOT of work to do if you're to do that consistently (and you already missed one in this method). https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:349: RTC_DCHECK_GE(AudioProcessing::kSampleRate16kHz, What is the DCHECK for? You're already logging. Besides, looks to me like the DCHECK condition is wrong and will always trigger if we get inside this conditional, so perhaps there's a test case missing too? https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:364: for (size_t i = 0; i < cancellers_.size(); ++i) { nit: for (auto canceller : cancellers_) { ... https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:408: for (size_t i = 0; i < cancellers_.size(); i++) { ditto
https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:89: EchoControlMobileImpl* echo_control_mobile; Sir, I think we have a leak.
https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:89: EchoControlMobileImpl* echo_control_mobile; On 2016/03/07 14:57:43, the sun wrote: > Sir, I think we have a leak. Good find! :-) Done. https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1257: void AudioProcessingImpl::InitializeEchoControlMobile() { On 2016/03/07 14:43:12, the sun wrote: > In a future CL, consider removing these methods since they are only called in a > single place. Good point! I've had that in mind! Will do :-) https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:74: RTC_DCHECK(state_); On 2016/03/07 14:43:12, the sun wrote: > RTC_CHECK Done. https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:78: RTC_CHECK(state_); On 2016/03/07 14:43:12, the sun wrote: > RTC_DCHECK, or just don't check - as per the ctor it's part of the Canceller > invariant that state_ != nullptr Done. https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:88: RTC_DCHECK_EQ(AudioProcessing::kNoError, error); Afaics, there is only buffer creation inside the WebRtcAecm_Create method so I think a DCHECK should be used. However, there are lots of checks being done so the method can fail (but not by failing to allocate memory). https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:92: RTC_DCHECK_EQ(AudioProcessing::kNoError, error); On 2016/03/07 14:43:12, the sun wrote: > Likewise - DCHECK or CHECK? I'd say DCHECK, since no memory allocation takes place. https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:97: Handle* state_; On 2016/03/07 14:43:13, the sun wrote: > RTC_DISALLOW_COPY_AND_ASSIGN(EchoControlMobileImpl::Canceller); Done. https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:139: Handle* my_handle = cancellers_[handle_index++]->state(); On 2016/03/07 14:43:13, the sun wrote: > fyi: while neat, doing multiple things in the same line actually makes it harder > to read and to step through in a debugger True, but I think it is kind of an edge case since this counter is only used for the purpose of indexing the cancellers_ vector. Would it be better to do the increment of handle_index inside the for (size_t j = 0; j < audio->num_channels(); j++) increment statement? https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:331: const int32_t err = On 2016/03/07 14:43:13, the sun wrote: > I don't think you should const declare this local. You'll have a LOT of work to > do if you're to do that consistently (and you already missed one in this > method). Not really sure what you mean here. Even though not done consistently in the code, I think that whenever possible it makes sense to const declare (Effective C++ Item #3). Not sure what variable you are referring to that has been missed to const declare in this method? Do you mean size_bytes? https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:349: RTC_DCHECK_GE(AudioProcessing::kSampleRate16kHz, On 2016/03/07 14:43:13, the sun wrote: > What is the DCHECK for? You're already logging. Besides, looks to me like the > DCHECK condition is wrong and will always trigger if we get inside this > conditional, so perhaps there's a test case missing too? You are correct in that the condition is wrong. I'll remove the DCHECK. https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:364: for (size_t i = 0; i < cancellers_.size(); ++i) { On 2016/03/07 14:43:12, the sun wrote: > nit: > for (auto canceller : cancellers_) { > ... Done. https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:408: for (size_t i = 0; i < cancellers_.size(); i++) { On 2016/03/07 14:43:13, the sun wrote: > ditto Done.
https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:92: RTC_DCHECK_EQ(AudioProcessing::kNoError, error); On 2016/03/08 07:53:32, peah-webrtc wrote: > On 2016/03/07 14:43:12, the sun wrote: > > Likewise - DCHECK or CHECK? > > I'd say DCHECK, since no memory allocation takes place. Same as above, runtime or logic? If the arguments we supply should never ever cause an error to be returned, go with DCHECK. https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:139: Handle* my_handle = cancellers_[handle_index++]->state(); On 2016/03/08 07:53:32, peah-webrtc wrote: > On 2016/03/07 14:43:13, the sun wrote: > > fyi: while neat, doing multiple things in the same line actually makes it > harder > > to read and to step through in a debugger > > True, but I think it is kind of an edge case since this counter is only used for > the purpose of indexing the cancellers_ vector. > > Would it be better to do the increment of handle_index inside the > for (size_t j = 0; j < audio->num_channels(); j++) increment statement? Never mind. https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:331: const int32_t err = On 2016/03/08 07:53:32, peah-webrtc wrote: > On 2016/03/07 14:43:13, the sun wrote: > > I don't think you should const declare this local. You'll have a LOT of work > to > > do if you're to do that consistently (and you already missed one in this > > method). > > Not really sure what you mean here. Even though not done consistently in the > code, I think that whenever possible it makes sense to const declare (Effective > C++ Item #3). > > Not sure what variable you are referring to that has been missed to const > declare in this method? Do you mean size_bytes? Discussed offline. https://codereview.webrtc.org/1772453002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1772453002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:16: #include "webrtc/base/constructormagic.h" Not needed. https://codereview.webrtc.org/1772453002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:39: assert(false); If you're replacing assert() with RTC_DCHECK(), here's one more! (and you can include the assert.h include) https://codereview.webrtc.org/1772453002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:91: error = WebRtcAecm_InitEchoPath(state_, external_echo_path, nit: since you're DCHECKing in the dtor, you should DCHECK here as well, for consistency. Or don't DCHECK in the dtor. state_!=0 is part of the class invariant according to ctor. https://codereview.webrtc.org/1772453002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:410: const int handle_error = WebRtcAecm_set_config(my_handle, config); There's that const again https://codereview.webrtc.org/1772453002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:412: error = AudioProcessing::kNoError; Not nit: = handle_error ?
https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:92: RTC_DCHECK_EQ(AudioProcessing::kNoError, error); On 2016/03/09 09:23:14, the sun wrote: > On 2016/03/08 07:53:32, peah-webrtc wrote: > > On 2016/03/07 14:43:12, the sun wrote: > > > Likewise - DCHECK or CHECK? > > > > I'd say DCHECK, since no memory allocation takes place. > > Same as above, runtime or logic? If the arguments we supply should never ever > cause an error to be returned, go with DCHECK. Acknowledged. https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:139: Handle* my_handle = cancellers_[handle_index++]->state(); On 2016/03/09 09:23:14, the sun wrote: > On 2016/03/08 07:53:32, peah-webrtc wrote: > > On 2016/03/07 14:43:13, the sun wrote: > > > fyi: while neat, doing multiple things in the same line actually makes it > > harder > > > to read and to step through in a debugger > > > > True, but I think it is kind of an edge case since this counter is only used > for > > the purpose of indexing the cancellers_ vector. > > > > Would it be better to do the increment of handle_index inside the > > for (size_t j = 0; j < audio->num_channels(); j++) increment statement? > > Never mind. :-) https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:139: Handle* my_handle = cancellers_[handle_index++]->state(); On 2016/03/09 09:23:14, the sun wrote: > On 2016/03/08 07:53:32, peah-webrtc wrote: > > On 2016/03/07 14:43:13, the sun wrote: > > > fyi: while neat, doing multiple things in the same line actually makes it > > harder > > > to read and to step through in a debugger > > > > True, but I think it is kind of an edge case since this counter is only used > for > > the purpose of indexing the cancellers_ vector. > > > > Would it be better to do the increment of handle_index inside the > > for (size_t j = 0; j < audio->num_channels(); j++) increment statement? > > Never mind. Acknowledged. https://codereview.webrtc.org/1772453002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:331: const int32_t err = On 2016/03/09 09:23:14, the sun wrote: > On 2016/03/08 07:53:32, peah-webrtc wrote: > > On 2016/03/07 14:43:13, the sun wrote: > > > I don't think you should const declare this local. You'll have a LOT of work > > to > > > do if you're to do that consistently (and you already missed one in this > > > method). > > > > Not really sure what you mean here. Even though not done consistently in the > > code, I think that whenever possible it makes sense to const declare > (Effective > > C++ Item #3). > > > > Not sure what variable you are referring to that has been missed to const > > declare in this method? Do you mean size_bytes? > > Discussed offline. Acknowledged. https://codereview.webrtc.org/1772453002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1772453002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:16: #include "webrtc/base/constructormagic.h" On 2016/03/09 09:23:14, the sun wrote: > Not needed. Done. https://codereview.webrtc.org/1772453002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:91: error = WebRtcAecm_InitEchoPath(state_, external_echo_path, On 2016/03/09 09:23:14, the sun wrote: > nit: since you're DCHECKing in the dtor, you should DCHECK here as well, for > consistency. Or don't DCHECK in the dtor. state_!=0 is part of the class > invariant according to ctor. This makes a lot of sense. Since the class is so small, I'll add DCHECKS for all the methods apart from the ctor (which includes a CHECK as memory is allocated) and dtor. Done. https://codereview.webrtc.org/1772453002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:410: const int handle_error = WebRtcAecm_set_config(my_handle, config); On 2016/03/09 09:23:14, the sun wrote: > There's that const again Done. https://codereview.webrtc.org/1772453002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:412: error = AudioProcessing::kNoError; On 2016/03/09 09:23:14, the sun wrote: > Not nit: > = handle_error ? Excellent find! Done.
https://codereview.webrtc.org/1772453002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1772453002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:91: error = WebRtcAecm_InitEchoPath(state_, external_echo_path, On 2016/03/09 12:34:53, peah-webrtc wrote: > On 2016/03/09 09:23:14, the sun wrote: > > nit: since you're DCHECKing in the dtor, you should DCHECK here as well, for > > consistency. Or don't DCHECK in the dtor. state_!=0 is part of the class > > invariant according to ctor. > > This makes a lot of sense. Since the class is so small, I'll add DCHECKS for all > the methods apart from the ctor (which includes a CHECK as memory is allocated) > and dtor. > > Done. I don't understand. Why did you remove the DCHECK in the dtor? I think you should be consistently DCHECKing whenever state_ is referenced, or rely on the class invariant (which is what I would do). https://codereview.webrtc.org/1772453002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1772453002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:333: const int32_t err = nit: I thought we agreed to remove these. https://codereview.webrtc.org/1772453002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:353: const int sample_rate_hz = apm_->proc_sample_rate_hz(); and here https://codereview.webrtc.org/1772453002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:356: const size_t cancellers_old_size = cancellers_.size(); and here https://codereview.webrtc.org/1772453002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:357: cancellers_.resize(num_handles_required()); If we shrink the number of required Cancellers, is there any reason to keep the unused ones around? Can we always resize the array instead?
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
https://codereview.webrtc.org/1772453002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1772453002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:91: error = WebRtcAecm_InitEchoPath(state_, external_echo_path, On 2016/03/09 12:46:11, the sun wrote: > On 2016/03/09 12:34:53, peah-webrtc wrote: > > On 2016/03/09 09:23:14, the sun wrote: > > > nit: since you're DCHECKing in the dtor, you should DCHECK here as well, for > > > consistency. Or don't DCHECK in the dtor. state_!=0 is part of the class > > > invariant according to ctor. > > > > This makes a lot of sense. Since the class is so small, I'll add DCHECKS for > all > > the methods apart from the ctor (which includes a CHECK as memory is > allocated) > > and dtor. > > > > Done. > > I don't understand. Why did you remove the DCHECK in the dtor? I think you > should be consistently DCHECKing whenever state_ is referenced, or rely on the > class invariant (which is what I would do). Sorry, I misunderstood you. Now I get it :-) Done. https://codereview.webrtc.org/1772453002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1772453002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:333: const int32_t err = On 2016/03/09 12:46:11, the sun wrote: > nit: I thought we agreed to remove these. I'm on it! :-) Done. https://codereview.webrtc.org/1772453002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:353: const int sample_rate_hz = apm_->proc_sample_rate_hz(); On 2016/03/09 12:46:11, the sun wrote: > and here Done. https://codereview.webrtc.org/1772453002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:356: const size_t cancellers_old_size = cancellers_.size(); On 2016/03/09 12:46:11, the sun wrote: > and here Done. https://codereview.webrtc.org/1772453002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:357: cancellers_.resize(num_handles_required()); On 2016/03/09 12:46:11, the sun wrote: > If we shrink the number of required Cancellers, is there any reason to keep the > unused ones around? Can we always resize the array instead? It 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. Done. 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 :-)
LGTM, but please change back ProcessCaptureAudio() to be more readable. https://codereview.webrtc.org/1772453002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1772453002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:36: assert(false); Please change to DCHECK, like you did for gaincontrol https://codereview.webrtc.org/1772453002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:185: for (auto& canceller : cancellers_) { Nice! https://codereview.webrtc.org/1772453002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:213: size_t render_channel = apm_->num_reverse_channels() - 1; Uhm. I think the old code actually was easier to read. Can you please change back and add a DCHECK that cancellers_ is large enough? https://codereview.webrtc.org/1772453002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:358: if (!canceller) { Ah, cute! :)
https://codereview.webrtc.org/1772453002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1772453002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:36: assert(false); On 2016/03/09 15:12:40, the sun wrote: > Please change to DCHECK, like you did for gaincontrol Done. https://codereview.webrtc.org/1772453002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:185: for (auto& canceller : cancellers_) { On 2016/03/09 15:12:40, the sun wrote: > Nice! :-) Acknowledged. https://codereview.webrtc.org/1772453002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:213: size_t render_channel = apm_->num_reverse_channels() - 1; On 2016/03/09 15:12:40, the sun wrote: > Uhm. I think the old code actually was easier to read. Can you please change > back and add a DCHECK that cancellers_ is large enough? This is extremely strange code, which is accentuated by the foreach loop scheme. I change back but the channel scheme is in desperate need of refactoring (a fact that was not fixed but rather emphasized by the foreach loop). Done. https://codereview.webrtc.org/1772453002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:358: if (!canceller) { On 2016/03/09 15:12:40, the sun wrote: > Ah, cute! :) :-) Acknowledged.
still lgtm https://codereview.webrtc.org/1772453002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1772453002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:206: RTC_DCHECK_LE(audio->num_channels() * apm_->num_reverse_channels(), nit: same but opposite condition on line 132
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/1772453002/#ps180001 (title: "Rebase from latest master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772453002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772453002/180001
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/386) linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
https://codereview.webrtc.org/1772453002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1772453002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:206: RTC_DCHECK_LE(audio->num_channels() * apm_->num_reverse_channels(), On 2016/03/09 21:46:52, the sun wrote: > nit: same but opposite condition on line 132 Good catch, I'll make them similar. Done.
Patchset #8 (id:180001) has been deleted
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/1772453002/#ps200001 (title: "Merge from master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772453002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772453002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/13395) win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/13075)
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/1772453002/#ps220001 (title: "Fixed counter update issue that was triggered for multiple streams and fixed error reporting behavi…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772453002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772453002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/13402)
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/1772453002/#ps240001 (title: "Corrected error check to not be done when AECM is not enabled")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772453002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772453002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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/1772453002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772453002/240001
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/1772453002/#ps260001 (title: "Rebase with latest master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772453002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772453002/260001
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)
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/1772453002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772453002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
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/1772453002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772453002/260001
Message was sent while issue was closed.
Committed patchset #11 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Removing dependency of the EchoControlMobileImpl class on ProcessingComponent. BUG=webrtc:5351 ========== to ========== Removing dependency of the EchoControlMobileImpl class on ProcessingComponent. BUG=webrtc:5351 Committed: https://crrev.com/bb9edbd04852fd0932642369b3938995c7e705fb Cr-Commit-Position: refs/heads/master@{#11945} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/bb9edbd04852fd0932642369b3938995c7e705fb Cr-Commit-Position: refs/heads/master@{#11945} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
