Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2405403003:
Add empty residual echo detector. (Closed)
|
Created:
4 years, 2 months ago by ivoc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd empty residual echo detector.
This CL does not contain the actual algorithm, but only creates an empty processing component and connects the right signals to it. The algorithm will be added in a follow-up CL.
BUG=webrtc:6525
Committed: https://crrev.com/9f4a4a096bf11e26ce6446620dcd3dd825807d3e
Cr-Commit-Position: refs/heads/master@{#14820}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Removed forward declaration. #
Total comments: 33
Patch Set 3 : Addressed reviewer comments. #
Total comments: 9
Patch Set 4 : Moved responsibility for proper locking to APM. #
Total comments: 9
Patch Set 5 : Addressed review comments. #Patch Set 6 : Refactored this CL to use the new locking/buffering scheme. #Patch Set 7 : Removed echo_detector/echo_detector.{h,cc} #Patch Set 8 : Small update to comments. #
Dependent Patchsets: Messages
Total messages: 33 (6 generated)
Description was changed from ========== Add empty residual echo detector. This CL does not contain the actual algorithm, but only creates an empty processing component and connects the right signals to it. The algorithm will be added in a follow-up CL. BUG=webrtc:6525 ========== to ========== Add empty residual echo detector. This CL does not contain the actual algorithm, but only creates an empty processing component and connects the right signals to it. The algorithm will be added in a follow-up CL. BUG=webrtc:6525 ==========
ivoc@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, peah@webrtc.org
Hi guys, Please have a look at this initial CL to add an empty residual echo detector. Thanks! https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (left): https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_detector/echo_detector.cc:12: #if !defined(RTC_DISABLE_VP9) Same here. https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_detector/echo_detector.h (left): https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_detector/echo_detector.h:12: #include <stdint.h> It seems like the diff has chosen another random file as a base here. Please ignore this, this is a newly created file.
On 2016/10/12 15:40:14, ivoc wrote: > Hi guys, > > Please have a look at this initial CL to add an empty residual echo detector. > Thanks! > > https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (left): > > https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/echo_detector/echo_detector.cc:12: #if > !defined(RTC_DISABLE_VP9) > Same here. > > https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/echo_detector/echo_detector.h (left): > > https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/echo_detector/echo_detector.h:12: #include > <stdint.h> > It seems like the diff has chosen another random file as a base here. Please > ignore this, this is a newly created file. Great! lgtm with some nits
On 2016/10/12 15:40:14, ivoc wrote: > Hi guys, > > Please have a look at this initial CL to add an empty residual echo detector. > Thanks! > > https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (left): > > https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/echo_detector/echo_detector.cc:12: #if > !defined(RTC_DISABLE_VP9) > Same here. > > https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/echo_detector/echo_detector.h (left): > > https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/echo_detector/echo_detector.h:12: #include > <stdint.h> > It seems like the diff has chosen another random file as a base here. Please > ignore this, this is a newly created file. Great! lgtm with some nits
https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:228: std::unique_ptr<ResidualEchoDetector> residual_echo_detector; It would be better to place this among the private submodules. The partitioning public and private is based on the fact that the api of the public submodules is accessible outside of the APM, while the private submodules can only be accessed from inside APM. In the long run, we would like all submodules to be private. https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_detector/echo_detector.h:18: class EchoDetector { What is the difference between the EchoDetector and ResidualEchoDetector classes? https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/include/audio_processing.h:41: class ResidualEchoDetector; This forward can be removed.
Thanks for the quick review! https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:228: std::unique_ptr<ResidualEchoDetector> residual_echo_detector; On 2016/10/13 10:11:17, peah-webrtc wrote: > It would be better to place this among the private submodules. > > The partitioning public and private is based on the fact that the api of the > public submodules is accessible outside of the APM, while the private submodules > can only be accessed from inside APM. In the long run, we would like all > submodules to be private. I did try placing it in the private submodules, but I ran into the issue that private_submodules_ is guarded by the capture lock, which means that we would need to hold the capture lock on the render side while accessing the residual echo detector. I thought that sounded like a bad idea, so I placed it here instead. What do you think is the best solution? https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_detector/echo_detector.h:18: class EchoDetector { On 2016/10/13 10:11:17, peah-webrtc wrote: > What is the difference between the EchoDetector and ResidualEchoDetector > classes? I was planning to put the actual algorithmic code in here, and use the ResidualEchoDetector as a wrapper to integrate it into APM. Of course it's possible to do both of these from a single class, do you think that would be better? https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/include/audio_processing.h:41: class ResidualEchoDetector; On 2016/10/13 10:11:17, peah-webrtc wrote: > This forward can be removed. Done.
https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:228: std::unique_ptr<ResidualEchoDetector> residual_echo_detector; On 2016/10/13 10:31:25, ivoc wrote: > On 2016/10/13 10:11:17, peah-webrtc wrote: > > It would be better to place this among the private submodules. > > > > The partitioning public and private is based on the fact that the api of the > > public submodules is accessible outside of the APM, while the private > submodules > > can only be accessed from inside APM. In the long run, we would like all > > submodules to be private. > > I did try placing it in the private submodules, but I ran into the issue that > private_submodules_ is guarded by the capture lock, which means that we would > need to hold the capture lock on the render side while accessing the residual > echo detector. I thought that sounded like a bad idea, so I placed it here > instead. What do you think is the best solution? I think you should remove the guard for that for the private submodules struct. It is basically only there because it was possible to add it when the locking scheme was introduced. I have actually also removed it in another of the CLs I'm working on, but it is fully fine to do it now. https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_detector/echo_detector.h:18: class EchoDetector { On 2016/10/13 10:31:25, ivoc wrote: > On 2016/10/13 10:11:17, peah-webrtc wrote: > > What is the difference between the EchoDetector and ResidualEchoDetector > > classes? > > I was planning to put the actual algorithmic code in here, and use the > ResidualEchoDetector as a wrapper to integrate it into APM. Of course it's > possible to do both of these from a single class, do you think that would be > better? That sounds good! It is very hard to tell at this stage. You know the better than me as you know what follows.
LG, but please, see comments. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.cc:17: // TODO(ivoc): Add implementation. If you want to ensure that this is not invoked, you can add FATAL() or RTC_NOTREACHED(). https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.h:14: #include <stddef.h> Do you need stddef.h here? https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.h:23: void BufferFarend(const float* farend, size_t nrOfSamples); nr_of_samples, or num_samples. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.h:25: void Process(const float* nearend, size_t nrOfSamples); again https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/residual_echo_detector.cc (right): https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.cc:1: /* The order of the methods in this file does not match the .h file. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.cc:45: RTC_DCHECK_EQ(render_signal_queue_->Insert(&render_queue_buffer_), true); The argument of DCHECK is only evaluated in debug mode. You must change this to store the result and then DCHECK on that. Also, no need to DCHECK_EQ(x, true) -- simply DCHECK(x) suffices. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.cc:58: while (render_signal_queue_->Remove(&capture_queue_buffer_)) { Is capture_queue_buffer_ only a container that conveys the audio from render_signal_queue_ to detector_? If so, does it have to be a class member? Perhaps performance considerations says it does. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/residual_echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.h:31: ResidualEchoDetector(rtc::CriticalSection* crit_render, I don't think you need the shared locks with the rest of APM. Create local locks, or even better, use thread checkers instead. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.h:36: void AnalyzeCaptureAudio(AudioBuffer* audio); Not const? https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.h:41: // Called holding the capture lock. Document this locking assumption with EXCLUSIVE_LOCKS_REQUIRED. But, as mentioned above, rather get rid of the locks if possible. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.h:44: float get_echo_likelihood(); const method.
https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.cc:17: // TODO(ivoc): Add implementation. On 2016/10/13 11:45:20, hlundin-webrtc wrote: > If you want to ensure that this is not invoked, you can add FATAL() or > RTC_NOTREACHED(). In this current version this function is actually called from residual_echo_detector.cc. My idea was to set everything up so I can just drop in the algorithm itself in the next CL. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.h:14: #include <stddef.h> On 2016/10/13 11:45:20, hlundin-webrtc wrote: > Do you need stddef.h here? I got an error about size_t being undefined, that's why I added it. Apparently it is defined in several different headers, I didn't have a strong reason for picking this one but it works. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.h:23: void BufferFarend(const float* farend, size_t nrOfSamples); On 2016/10/13 11:45:20, hlundin-webrtc wrote: > nr_of_samples, or num_samples. Done. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.h:25: void Process(const float* nearend, size_t nrOfSamples); On 2016/10/13 11:45:20, hlundin-webrtc wrote: > again Done. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/residual_echo_detector.cc (right): https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.cc:1: /* On 2016/10/13 11:45:20, hlundin-webrtc wrote: > The order of the methods in this file does not match the .h file. Fixed now. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.cc:45: RTC_DCHECK_EQ(render_signal_queue_->Insert(&render_queue_buffer_), true); On 2016/10/13 11:45:20, hlundin-webrtc wrote: > The argument of DCHECK is only evaluated in debug mode. You must change this to > store the result and then DCHECK on that. > > Also, no need to DCHECK_EQ(x, true) -- simply DCHECK(x) suffices. Good point, since I copied this from the AEC, it should probably be fixed there as well (I can make a follow-up CL). https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.cc:58: while (render_signal_queue_->Remove(&capture_queue_buffer_)) { On 2016/10/13 11:45:20, hlundin-webrtc wrote: > Is capture_queue_buffer_ only a container that conveys the audio from > render_signal_queue_ to detector_? If so, does it have to be a class member? > Perhaps performance considerations says it does. I think this is indeed done for performance reasons (copied from AEC again). https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/residual_echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.h:31: ResidualEchoDetector(rtc::CriticalSection* crit_render, On 2016/10/13 11:45:20, hlundin-webrtc wrote: > I don't think you need the shared locks with the rest of APM. Create local > locks, or even better, use thread checkers instead. I'm not that familiar with the locking scheme in the APM, so I copied this approach from the AEC/AECM. I'm not sure if thread checkers are a good idea here. For example ReadQueuedRenderData() needs to hold both the render and capture locks and is called from a place which only holds the render lock. I'm not sure how to get the same behavior with thread checkers. I can add local locks in this module if you think that's better, but it is a different approach than the other modules are currently taking. WDYT? https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.h:36: void AnalyzeCaptureAudio(AudioBuffer* audio); On 2016/10/13 11:45:20, hlundin-webrtc wrote: > Not const? Done. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.h:41: // Called holding the capture lock. On 2016/10/13 11:45:20, hlundin-webrtc wrote: > Document this locking assumption with EXCLUSIVE_LOCKS_REQUIRED. But, as > mentioned above, rather get rid of the locks if possible. Adding EXCLUSIVE_LOCKS_REQUIRED here does not work because this function is called directly from APM in a few places. Since the lock is a private member of ResidualEchoDetector, it cannot be taken from inside APM. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.h:44: float get_echo_likelihood(); On 2016/10/13 11:45:20, hlundin-webrtc wrote: > const method. Done.
I'd like peah to weigh in on the locking structure. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.cc:17: // TODO(ivoc): Add implementation. On 2016/10/13 13:46:15, ivoc wrote: > On 2016/10/13 11:45:20, hlundin-webrtc wrote: > > If you want to ensure that this is not invoked, you can add FATAL() or > > RTC_NOTREACHED(). > > In this current version this function is actually called from > residual_echo_detector.cc. My idea was to set everything up so I can just drop > in the algorithm itself in the next CL. But the functionality is still disabled, right? https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.h:14: #include <stddef.h> On 2016/10/13 13:46:15, ivoc wrote: > On 2016/10/13 11:45:20, hlundin-webrtc wrote: > > Do you need stddef.h here? > > I got an error about size_t being undefined, that's why I added it. Apparently > it is defined in several different headers, I didn't have a strong reason for > picking this one but it works. Ah, yes, size_t... https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/residual_echo_detector.cc (right): https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.cc:45: RTC_DCHECK_EQ(render_signal_queue_->Insert(&render_queue_buffer_), true); On 2016/10/13 13:46:16, ivoc wrote: > On 2016/10/13 11:45:20, hlundin-webrtc wrote: > > The argument of DCHECK is only evaluated in debug mode. You must change this > to > > store the result and then DCHECK on that. > > > > Also, no need to DCHECK_EQ(x, true) -- simply DCHECK(x) suffices. > > Good point, since I copied this from the AEC, it should probably be fixed there > as well (I can make a follow-up CL). Oh. Yes, please do! https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.cc:58: while (render_signal_queue_->Remove(&capture_queue_buffer_)) { On 2016/10/13 13:46:16, ivoc wrote: > On 2016/10/13 11:45:20, hlundin-webrtc wrote: > > Is capture_queue_buffer_ only a container that conveys the audio from > > render_signal_queue_ to detector_? If so, does it have to be a class member? > > Perhaps performance considerations says it does. > > I think this is indeed done for performance reasons (copied from AEC again). Acknowledged. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/residual_echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.h:18: #include "webrtc/base/criticalsection.h" You are still using critsects below. The way the code is written now, you could get away with forward declaring rtc::CriticalSection, but if you go for local locks, I think they should be plain member variables, not pointers. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.h:31: ResidualEchoDetector(rtc::CriticalSection* crit_render, On 2016/10/13 13:46:16, ivoc wrote: > On 2016/10/13 11:45:20, hlundin-webrtc wrote: > > I don't think you need the shared locks with the rest of APM. Create local > > locks, or even better, use thread checkers instead. > > I'm not that familiar with the locking scheme in the APM, so I copied this > approach from the AEC/AECM. I'm not sure if thread checkers are a good idea > here. For example ReadQueuedRenderData() needs to hold both the render and > capture locks and is called from a place which only holds the render lock. I'm > not sure how to get the same behavior with thread checkers. > > I can add local locks in this module if you think that's better, but it is a > different approach than the other modules are currently taking. WDYT? I think local locks is preferred, since they only protect local assets. But peah would know the APM locking structure much better than I do. He will have to make the decision. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.h:41: // Called holding the capture lock. On 2016/10/13 13:46:16, ivoc wrote: > On 2016/10/13 11:45:20, hlundin-webrtc wrote: > > Document this locking assumption with EXCLUSIVE_LOCKS_REQUIRED. But, as > > mentioned above, rather get rid of the locks if possible. > > Adding EXCLUSIVE_LOCKS_REQUIRED here does not work because this function is > called directly from APM in a few places. Since the lock is a private member of > ResidualEchoDetector, it cannot be taken from inside APM. Acknowledged. https://codereview.webrtc.org/2405403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/residual_echo_detector.cc (right): https://codereview.webrtc.org/2405403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.cc:71: // Read chunks of data that were received and queued on the render side from You have two similar, but not identical, descriptions for this method. One here and one in the .h file. Consider consolidating in the .h file, unless you are documenting gritty implementation details which should go here. https://codereview.webrtc.org/2405403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/residual_echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.h:19: #include "webrtc/base/thread_checker.h" You didn't add any thread checker, did you?
https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/residual_echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.h:31: ResidualEchoDetector(rtc::CriticalSection* crit_render, On 2016/10/13 13:46:16, ivoc wrote: > On 2016/10/13 11:45:20, hlundin-webrtc wrote: > > I don't think you need the shared locks with the rest of APM. Create local > > locks, or even better, use thread checkers instead. > > I'm not that familiar with the locking scheme in the APM, so I copied this > approach from the AEC/AECM. I'm not sure if thread checkers are a good idea > here. For example ReadQueuedRenderData() needs to hold both the render and > capture locks and is called from a place which only holds the render lock. I'm > not sure how to get the same behavior with thread checkers. > > I can add local locks in this module if you think that's better, but it is a > different approach than the other modules are currently taking. WDYT? I agree with hlundin@ that no locks should be required. That was something I also should have pointed out in my review. It is better to rewrite it without locks from the beginning. The locks in the AEC/AECM are only needed because of their APIs being part of the public AudioProcessing API which means that the locks are needed to ensure proper behavior. Since you are not adding this submodule to the public API, you can instead explicitly ensure that the submodule is called properly from APM and does not need to use any locks to ensure that. Afaics, the threadcheckers cannot really be used to solve all the issues with threading here, since some the ReadQueuedRenderData may be called from different threads. One way could, however, be to have two variants of ReadQueuedRenderData, one for each thread.
https://codereview.webrtc.org/2405403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.h:23: void BufferFarend(const float* farend, size_t num_samples); I think you should use rtc::ArrayView<const float> instead of "const float* farend, size_t num_samples". https://codereview.webrtc.org/2405403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.h:25: void Process(const float* nearend, size_t num_samples); And here.
https://codereview.webrtc.org/2405403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.h:23: void BufferFarend(const float* farend, size_t num_samples); On 2016/10/14 07:15:52, hlundin-webrtc wrote: > I think you should use rtc::ArrayView<const float> instead of "const float* > farend, size_t num_samples". +1. ArrayView should be used instead of separate pointer+size pretty much everywhere.
Thanks for the comments. I have now removed the locking from the ResidualEchoDetector class, which moves the responsibility for proper locking to the APM. This has simplified the ResidualEchoDetector class considerably, so it may make more sense now to put the algorithm itself in this class as well, what do you guys think? https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_detector/echo_detector.cc (right): https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.cc:17: // TODO(ivoc): Add implementation. On 2016/10/14 06:59:17, hlundin-webrtc wrote: > On 2016/10/13 13:46:15, ivoc wrote: > > On 2016/10/13 11:45:20, hlundin-webrtc wrote: > > > If you want to ensure that this is not invoked, you can add FATAL() or > > > RTC_NOTREACHED(). > > > > In this current version this function is actually called from > > residual_echo_detector.cc. My idea was to set everything up so I can just drop > > in the algorithm itself in the next CL. > > But the functionality is still disabled, right? Right, this function only gets called when the enable flag in the AudioProcessing::Config struct is set to true, which it currently never is. So the functionality is indeed disabled. I guess it does make sense to add RTC_NOTREACHED() here then. https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/residual_echo_detector.cc (right): https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.cc:45: RTC_DCHECK_EQ(render_signal_queue_->Insert(&render_queue_buffer_), true); On 2016/10/14 06:59:18, hlundin-webrtc wrote: > On 2016/10/13 13:46:16, ivoc wrote: > > On 2016/10/13 11:45:20, hlundin-webrtc wrote: > > > The argument of DCHECK is only evaluated in debug mode. You must change this > > to > > > store the result and then DCHECK on that. > > > > > > Also, no need to DCHECK_EQ(x, true) -- simply DCHECK(x) suffices. > > > > Good point, since I copied this from the AEC, it should probably be fixed > there > > as well (I can make a follow-up CL). > > Oh. Yes, please do! Seems like Per beat me to it :) https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/residual_echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.h:31: ResidualEchoDetector(rtc::CriticalSection* crit_render, On 2016/10/14 07:12:29, peah-webrtc wrote: > On 2016/10/13 13:46:16, ivoc wrote: > > On 2016/10/13 11:45:20, hlundin-webrtc wrote: > > > I don't think you need the shared locks with the rest of APM. Create local > > > locks, or even better, use thread checkers instead. > > > > I'm not that familiar with the locking scheme in the APM, so I copied this > > approach from the AEC/AECM. I'm not sure if thread checkers are a good idea > > here. For example ReadQueuedRenderData() needs to hold both the render and > > capture locks and is called from a place which only holds the render lock. I'm > > not sure how to get the same behavior with thread checkers. > > > > I can add local locks in this module if you think that's better, but it is a > > different approach than the other modules are currently taking. WDYT? > > I agree with hlundin@ that no locks should be required. That was something I > also should have pointed out in my review. > It is better to rewrite it without locks from the beginning. > > The locks in the AEC/AECM are only needed because of their APIs being part of > the public AudioProcessing API which means that the locks are needed to ensure > proper behavior. > > Since you are not adding this submodule to the public API, you can instead > explicitly ensure that the submodule is called properly from APM and does not > need to use any locks to ensure that. > > Afaics, the threadcheckers cannot really be used to solve all the issues with > threading here, since some the ReadQueuedRenderData may be called from different > threads. One way could, however, be to have two variants of > ReadQueuedRenderData, one for each thread. > > > I have now removed all locks from this class and moved the responsibility for locking to the APM. https://codereview.webrtc.org/2405403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.h:23: void BufferFarend(const float* farend, size_t num_samples); On 2016/10/14 07:28:48, kwiberg-webrtc wrote: > On 2016/10/14 07:15:52, hlundin-webrtc wrote: > > I think you should use rtc::ArrayView<const float> instead of "const float* > > farend, size_t num_samples". > > +1. ArrayView should be used instead of separate pointer+size pretty much > everywhere. Done. https://codereview.webrtc.org/2405403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.h:25: void Process(const float* nearend, size_t num_samples); On 2016/10/14 07:15:53, hlundin-webrtc wrote: > And here. Done. https://codereview.webrtc.org/2405403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/residual_echo_detector.cc (right): https://codereview.webrtc.org/2405403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.cc:71: // Read chunks of data that were received and queued on the render side from On 2016/10/14 06:59:18, hlundin-webrtc wrote: > You have two similar, but not identical, descriptions for this method. One here > and one in the .h file. Consider consolidating in the .h file, unless you are > documenting gritty implementation details which should go here. Ok, I removed this one. https://codereview.webrtc.org/2405403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/residual_echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.h:19: #include "webrtc/base/thread_checker.h" On 2016/10/14 06:59:18, hlundin-webrtc wrote: > You didn't add any thread checker, did you? Good point.
https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.h:24: void BufferFarend(const rtc::ArrayView<const float>& farend); The typical pattern is to pass the ArrayView by value, not by reference. That is: void BufferFarend(rtc::ArrayView<const float> farend); This does not mean that all of the data is passed by value, though. (kwiberg@, keep me honest here...) https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.h:26: void Process(const rtc::ArrayView<const float>& nearend); Same here. https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/residual_echo_detector.cc (right): https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.cc:21: int ResidualEchoDetector::AnalyzeRenderAudio(const AudioBuffer* audio) const { This is clearly not a const method. Remove const, and also remove the mutable qualifiers on the two members. https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/residual_echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.h:35: int AnalyzeRenderAudio(const AudioBuffer* audio) const; Make this return true or false instead.
https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.h:24: void BufferFarend(const rtc::ArrayView<const float>& farend); On 2016/10/14 12:36:21, hlundin-webrtc wrote: > The typical pattern is to pass the ArrayView by value, not by reference. That > is: > void BufferFarend(rtc::ArrayView<const float> farend); > > This does not mean that all of the data is passed by value, though. > (kwiberg@, keep me honest here...) That's right---the ArrayView itself just points to the data, and it's extremely lightweight (it's literally just a data pointer and a length, with a bunch of useful methods), so the rule of thumb is to just pass it by value always.
https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/residual_echo_detector.cc (right): https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.cc:21: int ResidualEchoDetector::AnalyzeRenderAudio(const AudioBuffer* audio) const { On 2016/10/14 12:36:21, hlundin-webrtc wrote: > This is clearly not a const method. Remove const, and also remove the mutable > qualifiers on the two members I proposed this offline. The underlying idea is the following: The AnalyzeRenderAudio method belongs to the render thread and should not touch anything inside this class that has to do with the capture methods. The sole exception of this is the swapqueue object. In order to (as far as possible) enforce proper access to the data inside this class I see three ways: 1) Locks and lock annotation. 2) Put the render processing and capture processing parts into two objects, and inject the swapqueue into these. 3) Let the render processing method be a const with the swapqueue declared mutable as an exception to the constness. I proposed 3) as 1) It allows simpler code 2) It does not require any extra locks. The problem I see with that is that the method is not const which is confusing. What do you propose? I think we should as far as possible write the code in order to avoid illegal access to the capture-side data.
On 2016/10/14 13:34:48, peah-webrtc wrote: > https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/residual_echo_detector.cc (right): > > https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/residual_echo_detector.cc:21: int > ResidualEchoDetector::AnalyzeRenderAudio(const AudioBuffer* audio) const { > On 2016/10/14 12:36:21, hlundin-webrtc wrote: > > This is clearly not a const method. Remove const, and also remove the mutable > > qualifiers on the two members > > I proposed this offline. The underlying idea is the following: > The AnalyzeRenderAudio method belongs to the render thread and should not touch > anything inside this class that has to do with the capture methods. > The sole exception of this is the swapqueue object. > > In order to (as far as possible) enforce proper access to the data inside this > class I see three ways: > 1) Locks and lock annotation. > 2) Put the render processing and capture processing parts into two objects, and > inject the swapqueue into these. > 3) Let the render processing method be a const with the swapqueue declared > mutable as an exception to the constness. > > I proposed 3) as > 1) It allows simpler code > 2) It does not require any extra locks. > > The problem I see with that is that the method is not const which is confusing. > > What do you propose? I think we should as far as possible write the code in > order to avoid illegal access to the capture-side data. Per offline discussion, I'll let you figure out how to best solve the multi-threading issues in this class. Also, my comments on the const and mutable stuff may or may not be void depending on the outcome.
I'm leaving the locking issue for monday so we have a bit more time to consider the alternatives. https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_detector/echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.h:24: void BufferFarend(const rtc::ArrayView<const float>& farend); On 2016/10/14 12:36:21, hlundin-webrtc wrote: > The typical pattern is to pass the ArrayView by value, not by reference. That > is: > void BufferFarend(rtc::ArrayView<const float> farend); > > This does not mean that all of the data is passed by value, though. > (kwiberg@, keep me honest here...) Ok, done. https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_detector/echo_detector.h:26: void Process(const rtc::ArrayView<const float>& nearend); On 2016/10/14 12:36:21, hlundin-webrtc wrote: > Same here. Done. https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/residual_echo_detector.h (right): https://codereview.webrtc.org/2405403003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/residual_echo_detector.h:35: int AnalyzeRenderAudio(const AudioBuffer* audio) const; On 2016/10/14 12:36:21, hlundin-webrtc wrote: > Make this return true or false instead. Good idea, done.
I give you carte blanche regarding the locking issue. The rest of the CL LGTM.
I've made some significant changes to the CL to use the new locking/buffering scheme. Per, could you have another look at this, please? Due to the refactoring, the ResidualEchoDetector class is now nearly empty (I made it mainly to handle the buffering/locking), so I think it should be merged with the EchoDetector class, do you guys agree? Thanks!
On 2016/10/27 15:46:04, ivoc wrote: > I've made some significant changes to the CL to use the new locking/buffering > scheme. Per, could you have another look at this, please? > Due to the refactoring, the ResidualEchoDetector class is now nearly empty (I > made it mainly to handle the buffering/locking), so I think it should be merged > with the EchoDetector class, do you guys agree? > Thanks! I agree. I think you should remove the residual_echo_detector class as it is a very thin wrapper layer now. It would also be great to change AnalyzeCaptureAudio call to take a rtc::arrayview array instead of an audiobuffer, as the echodetector don't really use the audiobuffer functionality apart from extracting the data.
On 2016/10/28 06:24:27, peah-webrtc wrote: > On 2016/10/27 15:46:04, ivoc wrote: > > I've made some significant changes to the CL to use the new locking/buffering > > scheme. Per, could you have another look at this, please? > > Due to the refactoring, the ResidualEchoDetector class is now nearly empty (I > > made it mainly to handle the buffering/locking), so I think it should be > merged > > with the EchoDetector class, do you guys agree? > > Thanks! > > I agree. I think you should remove the residual_echo_detector class as it is a > very thin wrapper layer now. > > It would also be great to change AnalyzeCaptureAudio call to take a > rtc::arrayview array instead of an audiobuffer, as the echodetector don't really > use the audiobuffer functionality apart from extracting the data. I did the opposite, and removed the EchoDetector class (I kept the ResidualEchoDetector class), but it doesn't really matter since both classes are being merged into one. I also changed AnalyzeCaptureAudio to take an rtc::arrayview as input, like you suggested.
On 2016/10/28 11:07:00, ivoc wrote: > On 2016/10/28 06:24:27, peah-webrtc wrote: > > On 2016/10/27 15:46:04, ivoc wrote: > > > I've made some significant changes to the CL to use the new > locking/buffering > > > scheme. Per, could you have another look at this, please? > > > Due to the refactoring, the ResidualEchoDetector class is now nearly empty > (I > > > made it mainly to handle the buffering/locking), so I think it should be > > merged > > > with the EchoDetector class, do you guys agree? > > > Thanks! > > > > I agree. I think you should remove the residual_echo_detector class as it is a > > very thin wrapper layer now. > > > > It would also be great to change AnalyzeCaptureAudio call to take a > > rtc::arrayview array instead of an audiobuffer, as the echodetector don't > really > > use the audiobuffer functionality apart from extracting the data. > > I did the opposite, and removed the EchoDetector class (I kept the > ResidualEchoDetector class), but it doesn't really matter since both classes are > being merged into one. I also changed AnalyzeCaptureAudio to take an > rtc::arrayview as input, like you suggested. Awesome! lgtm again
On 2016/10/28 11:07:00, ivoc wrote: > On 2016/10/28 06:24:27, peah-webrtc wrote: > > On 2016/10/27 15:46:04, ivoc wrote: > > > I've made some significant changes to the CL to use the new > locking/buffering > > > scheme. Per, could you have another look at this, please? > > > Due to the refactoring, the ResidualEchoDetector class is now nearly empty > (I > > > made it mainly to handle the buffering/locking), so I think it should be > > merged > > > with the EchoDetector class, do you guys agree? > > > Thanks! > > > > I agree. I think you should remove the residual_echo_detector class as it is a > > very thin wrapper layer now. > > > > It would also be great to change AnalyzeCaptureAudio call to take a > > rtc::arrayview array instead of an audiobuffer, as the echodetector don't > really > > use the audiobuffer functionality apart from extracting the data. > > I did the opposite, and removed the EchoDetector class (I kept the > ResidualEchoDetector class), but it doesn't really matter since both classes are > being merged into one. I also changed AnalyzeCaptureAudio to take an > rtc::arrayview as input, like you suggested. Awesome! lgtm again
The CQ bit was checked by ivoc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2405403003/#ps140001 (title: "Small update to comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Add empty residual echo detector. This CL does not contain the actual algorithm, but only creates an empty processing component and connects the right signals to it. The algorithm will be added in a follow-up CL. BUG=webrtc:6525 ========== to ========== Add empty residual echo detector. This CL does not contain the actual algorithm, but only creates an empty processing component and connects the right signals to it. The algorithm will be added in a follow-up CL. BUG=webrtc:6525 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add empty residual echo detector. This CL does not contain the actual algorithm, but only creates an empty processing component and connects the right signals to it. The algorithm will be added in a follow-up CL. BUG=webrtc:6525 ========== to ========== Add empty residual echo detector. This CL does not contain the actual algorithm, but only creates an empty processing component and connects the right signals to it. The algorithm will be added in a follow-up CL. BUG=webrtc:6525 Committed: https://crrev.com/9f4a4a096bf11e26ce6446620dcd3dd825807d3e Cr-Commit-Position: refs/heads/master@{#14820} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9f4a4a096bf11e26ce6446620dcd3dd825807d3e Cr-Commit-Position: refs/heads/master@{#14820} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
