Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1424663003:
Lock scheme #8: Introduced the new locking scheme (Closed)
|
Created:
5 years, 1 month ago by peah-webrtc Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, hlundin-webrtc, kwiberg-webrtc, the sun, aluebs-webrtc, bjornv1, ivoc Base URL:
https://chromium.googlesource.com/external/webrtc.git@add_threadcheckers_CL Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionIntroduced the new locking scheme
BUG=webrtc:5099
Committed: https://crrev.com/df3efa8c079294857a8b8e0a02634d06a6d6b6d6
Cr-Commit-Position: refs/heads/master@{#10836}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Major general updates, completing the locking scheme, and increasing readability #
Total comments: 79
Patch Set 3 : Fixed build error caused by wrong usage of std::max #Patch Set 4 : Changes according to latest reviewer comments #Patch Set 5 : Fixed bug in one of the places where conditional locking is used #Patch Set 6 : Corrected if statement that caused the APM to run in a single threaded manner when the AEC is on #Patch Set 7 : Merge from master #Patch Set 8 : Fixed a bad merge error for the beamformer settings and updated with the latest merge from master #
Total comments: 40
Patch Set 9 : Changes in response to reviewer comments #
Total comments: 6
Patch Set 10 : Changed comment typos #Patch Set 11 : Merged changes from Lock scheme #9: WIP:Adding lock and thread annotations to the APM submodules in… #
Total comments: 13
Patch Set 12 : Moved struct forwards to AudioProcImpl, removed redundant forward, bundled the debug state variable… #
Total comments: 2
Patch Set 13 : Moved the debug state declaration to be a privately nested in AudioProcessingImpl #Patch Set 14 : Activated the brief APM lock unittest, removed a stray empty line #Patch Set 15 : Removed the call to num_reverse_channels in the lock test, changed to a mobile compatible AGC mode #Patch Set 16 : Created a threadsafe wrapper for the random generator in the locktest. Fixed an unprotected variabl… #Depends on Patchset: Messages
Total messages: 46 (13 generated)
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kwiberg@webrtc.org
Description was changed from ========== Introduced the new locking scheme BUG=webrtc:5099 ========== to ========== Introduced the new locking scheme BUG=webrtc:5099 ==========
A bunch of the classes don't have lock annotations on their variables. Why? https://codereview.webrtc.org/1424663003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:328: RTC_DCHECK(crit_render_.IsLocked()); Is this check useful? Doesn't it just check that *someone's* holding the mutex, not that *we* are? https://codereview.webrtc.org/1424663003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:475: // Run in a single-threaded manner when setting ehe extra options. the https://codereview.webrtc.org/1424663003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/gain_control_impl.h (right): https://codereview.webrtc.org/1424663003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/gain_control_impl.h:109: rtc::CriticalSection crit_queue_; What do you use crit_queue_ for?
On 2015/10/27 13:15:57, kwiberg-webrtc wrote: > A bunch of the classes don't have lock annotations on their variables. Why? > The reason is that it is only possible to use the lock annotations in the class/(compilation unit?) where the locks are located. For classes that use only references to these locks, lock annotation seem not to work (which is kind of expected and makes sense).
This CL is now again in the desired shape. Please review at will. https://codereview.webrtc.org/1424663003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:328: RTC_DCHECK(crit_render_.IsLocked()); On 2015/10/27 13:15:56, kwiberg-webrtc wrote: > Is this check useful? Doesn't it just check that *someone's* holding the mutex, > not that *we* are? I added the check as a threadchecker instead which is how it should be. The idea is that it is only allowed to call this function while holding the render mutex. Done. https://codereview.webrtc.org/1424663003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:475: // Run in a single-threaded manner when setting ehe extra options. On 2015/10/27 13:15:57, kwiberg-webrtc wrote: > the Done. https://codereview.webrtc.org/1424663003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/gain_control_impl.h (right): https://codereview.webrtc.org/1424663003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/gain_control_impl.h:109: rtc::CriticalSection crit_queue_; On 2015/10/27 13:15:57, kwiberg-webrtc wrote: > What do you use crit_queue_ for? Good find! That is a leftover from a fix for a problem in the usage of the message queue. Removed it. Done.
Good. But please see comments. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:242: rtc::CritScope cs_render(&crit_render_); Can you avoid taking the locks in the ctor? https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:283: rtc::CritScope cs_render(&crit_render_); There is no point in acquiring locks in the dtor. All other threads that could possibly access this object must already be stopped (or at least been made aware of that this object is no longer). https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:286: // Set submodule pointers to NULL to ensure proper behavior if the accessor This also makes no sense. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:579: // lock. Widow. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:1: No. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:44: struct ApmExternallyAccessibleSubModules; Suggest ApmPublicSubModules ... https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:45: struct ApmInternalSubModules; ... and ApmPrivateSubmodules. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:49: class FileWrapper; No need to forward-declare now that you have the "real thing" included. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:88: // Methods forcing APM to running in a single-threaded manner. Nit: running -> run https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:144: // Hence there is no need for locks in these End with . https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:154: // Methods passing pointers to APM submodules. passing -> returning It sounds as if the methods pass some kind of pointer into the submodules. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:158: // during APM creation) End with . https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:176: // that Widow https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:179: // render Widow https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:183: // Method for checking for the need of conversion. Accesses the formats Generally quite ugly line-breaking in this comment. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:191: // Are called with both the render and capture locks already Double space between 'with' and 'both'. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:205: // manner Widow https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:218: // manner Widow https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:255: rtc::scoped_ptr<ApmExternallyAccessibleSubModules> public_submodules_; Any reason these two are (scoped) pointers and not just plain members? https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:277: const struct ApmConstants { The struct is const, and _one_ of the members is const too. It seems to me that either no members or all members should be const. I don't know what the difference between const and non-const members in a const struct would be. That is, is const struct A { const int x; const int y; } a; any different from const struct B { int x; int y; } b; (Both structs of course having suitable constructors to set the members.) https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:289: // Only set through the constructor's Config parameter. Isn't this true for all members in this struct? https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:317: Remove blank line. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:122: bool success = render_signal_queue_->Insert(&render_queue_buffer_); You don't need success. if (!render_signal_queue_->Insert(&render_queue_buffer_)) { ... https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:338: if (crit_capture_->TryEnter()) { I don't understand this code, but I wonder if rtc::TryCritScope is what you want to use. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.h:15: #include "webrtc/base/thread_annotations.h" You're not using any of this, right? https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.h:55: const AudioBuffer* audio); // Called holding the render lock. +1 for adding the comments. -1 for the ugly line breaks. Please, add the comments to the line above each method and restore the original format. Here and in all places below. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:128: if (!success) { if (!render_signal_queue_->Insert(&render_queue_buffer_)) { https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.h (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.h:1: No https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.h:16: #include "webrtc/base/thread_annotations.h" Not used. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.h:57: int ProcessRenderAudio( Again, please restore the one-line definitions where possible, and put the comment on a separate line if needed. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:91: bool success = render_signal_queue_->Insert(&render_queue_buffer_); if (!render_signal_queue_->Insert(&render_queue_buffer_)( { https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.h (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.h:49: // Called with both locks held. Is this still true? https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.h:55: virtual ~GainControlImpl(); // Called with both locks held. Still true? https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.h:57: int ProcessRenderAudio( Same nagging about putting the comments on separate lines in order to keep the definitions on one line. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/high_pass_filter_impl.h (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/high_pass_filter_impl.h:14: #include "webrtc/base/thread_checker.h" Not used? https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/high_pass_filter_impl.h:21: class CriticalSectionWrapper; Wrong forward declaration. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/noise_suppression_impl.h (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/noise_suppression_impl.h:14: #include "webrtc/base/criticalsection.h" You include this here, but not in all other submodules' .h files. In other cases, you include it in the .cc file instead. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/noise_suppression_impl.h:22: class CriticalSectionWrapper; Wrong forward declaration. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/voice_detection_impl.h (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/voice_detection_impl.h:22: class CriticalSectionWrapper; Wrong forw-decl.
https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:242: rtc::CritScope cs_render(&crit_render_); On 2015/11/05 16:11:21, hlundin-webrtc wrote: > Can you avoid taking the locks in the ctor? Seems to work. I thought that the threadannotation would complain, but it does not :-) Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:283: rtc::CritScope cs_render(&crit_render_); On 2015/11/05 16:11:21, hlundin-webrtc wrote: > There is no point in acquiring locks in the dtor. All other threads that could > possibly access this object must already be stopped (or at least been made aware > of that this object is no longer). Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:286: // Set submodule pointers to NULL to ensure proper behavior if the accessor On 2015/11/05 16:11:21, hlundin-webrtc wrote: > This also makes no sense. Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:579: // lock. On 2015/11/05 16:11:21, hlundin-webrtc wrote: > Widow. Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:1: On 2015/11/05 16:11:21, hlundin-webrtc wrote: > No. Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:44: struct ApmExternallyAccessibleSubModules; On 2015/11/05 16:11:22, hlundin-webrtc wrote: > Suggest ApmPublicSubModules ... Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:45: struct ApmInternalSubModules; On 2015/11/05 16:11:21, hlundin-webrtc wrote: > ... and ApmPrivateSubmodules. Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:49: class FileWrapper; On 2015/11/05 16:11:22, hlundin-webrtc wrote: > No need to forward-declare now that you have the "real thing" included. Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:88: // Methods forcing APM to running in a single-threaded manner. On 2015/11/05 16:11:21, hlundin-webrtc wrote: > Nit: running -> run Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:144: // Hence there is no need for locks in these On 2015/11/05 16:11:21, hlundin-webrtc wrote: > End with . Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:154: // Methods passing pointers to APM submodules. On 2015/11/05 16:11:22, hlundin-webrtc wrote: > passing -> returning > It sounds as if the methods pass some kind of pointer into the submodules. Fully true! Good point! Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:158: // during APM creation) On 2015/11/05 16:11:21, hlundin-webrtc wrote: > End with . Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:176: // that On 2015/11/05 16:11:21, hlundin-webrtc wrote: > Widow Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:179: // render On 2015/11/05 16:11:21, hlundin-webrtc wrote: > Widow Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:183: // Method for checking for the need of conversion. Accesses the formats On 2015/11/05 16:11:22, hlundin-webrtc wrote: > Generally quite ugly line-breaking in this comment. Totally agree! I'll blame it on CL format, but it's for sure my bad that I did not properly check the output of CL format. Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:191: // Are called with both the render and capture locks already On 2015/11/05 16:11:22, hlundin-webrtc wrote: > Double space between 'with' and 'both'. Nice catch! Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:205: // manner On 2015/11/05 16:11:21, hlundin-webrtc wrote: > Widow Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:218: // manner On 2015/11/05 16:11:22, hlundin-webrtc wrote: > Widow Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:255: rtc::scoped_ptr<ApmExternallyAccessibleSubModules> public_submodules_; On 2015/11/05 16:11:21, hlundin-webrtc wrote: > Any reason these two are (scoped) pointers and not just plain members? The only reason is that I did not want to expose their types outside APM since they should not at all be used outside APM. If I add them as plain members they need to be instantiated and then their types need to be known inside the header file, but by using pointers their types can be specified as forwards. Is there any other way of achieving this? Or do you prefer that I include those types in this header file? https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:277: const struct ApmConstants { On 2015/11/05 16:11:21, hlundin-webrtc wrote: > The struct is const, and _one_ of the members is const too. It seems to me that > either no members or all members should be const. I don't know what the > difference between const and non-const members in a const struct would be. That > is, is > > const struct A { > const int x; > const int y; > } a; > > any different from > > const struct B { > int x; > int y; > } b; > > (Both structs of course having suitable constructors to set the members.) Good catch! I prefer the latter, and as you point out the const specifier any member of a const struct should be redundant (and misleading if any other members don't have that). https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:289: // Only set through the constructor's Config parameter. On 2015/11/05 16:11:22, hlundin-webrtc wrote: > Isn't this true for all members in this struct? True. That old comment is now redundant. Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:317: On 2015/11/05 16:11:21, hlundin-webrtc wrote: > Remove blank line. Will do, also will move the beamformer enabled flag to the constants struct. Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:122: bool success = render_signal_queue_->Insert(&render_queue_buffer_); On 2015/11/05 16:11:22, hlundin-webrtc wrote: > You don't need success. > if (!render_signal_queue_->Insert(&render_queue_buffer_)) { > ... Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:338: if (crit_capture_->TryEnter()) { On 2015/11/05 16:11:22, hlundin-webrtc wrote: > I don't understand this code, but I wonder if rtc::TryCritScope is what you want > to use. Sounds like that! Thanks! Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.h:15: #include "webrtc/base/thread_annotations.h" On 2015/11/05 16:11:22, hlundin-webrtc wrote: > You're not using any of this, right? Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.h:55: const AudioBuffer* audio); // Called holding the render lock. On 2015/11/05 16:11:22, hlundin-webrtc wrote: > +1 for adding the comments. > -1 for the ugly line breaks. Please, add the comments to the line above each > method and restore the original format. Here and in all places below. Sorry!!! Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:128: if (!success) { On 2015/11/05 16:11:22, hlundin-webrtc wrote: > if (!render_signal_queue_->Insert(&render_queue_buffer_)) { Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.h (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.h:1: On 2015/11/05 16:11:22, hlundin-webrtc wrote: > No Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.h:16: #include "webrtc/base/thread_annotations.h" On 2015/11/05 16:11:22, hlundin-webrtc wrote: > Not used. Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.h:57: int ProcessRenderAudio( On 2015/11/05 16:11:22, hlundin-webrtc wrote: > Again, please restore the one-line definitions where possible, and put the > comment on a separate line if needed. Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:91: bool success = render_signal_queue_->Insert(&render_queue_buffer_); On 2015/11/05 16:11:22, hlundin-webrtc wrote: > if (!render_signal_queue_->Insert(&render_queue_buffer_)( { Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.h (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.h:49: // Called with both locks held. On 2015/11/05 16:11:22, hlundin-webrtc wrote: > Is this still true? Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.h:55: virtual ~GainControlImpl(); // Called with both locks held. On 2015/11/05 16:11:23, hlundin-webrtc wrote: > Still true? Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.h:57: int ProcessRenderAudio( On 2015/11/05 16:11:22, hlundin-webrtc wrote: > Same nagging about putting the comments on separate lines in order to keep the > definitions on one line. Fully and totally well motivated! Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/high_pass_filter_impl.h (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/high_pass_filter_impl.h:14: #include "webrtc/base/thread_checker.h" On 2015/11/05 16:11:23, hlundin-webrtc wrote: > Not used? Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/high_pass_filter_impl.h:21: class CriticalSectionWrapper; On 2015/11/05 16:11:23, hlundin-webrtc wrote: > Wrong forward declaration. Great! Removed that in several other files as well. Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/noise_suppression_impl.h (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/noise_suppression_impl.h:14: #include "webrtc/base/criticalsection.h" On 2015/11/05 16:11:23, hlundin-webrtc wrote: > You include this here, but not in all other submodules' .h files. In other > cases, you include it in the .cc file instead. Good find! A thing that further messed things up is that thread_checker.h includes criticalsection.h which makes my includes quite inconsistent. Will fix. Done. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/noise_suppression_impl.h:22: class CriticalSectionWrapper; On 2015/11/05 16:11:23, hlundin-webrtc wrote: > Wrong forward declaration. Done.
I needed to do an update to fix an issue with one of the conditional locks that I had missed. The fix is tiny, but some code merge changes from the linked CLs slipped through as well.
peah@webrtc.org changed reviewers: + solenberg@webrtc.org
lgtm https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:242: rtc::CritScope cs_render(&crit_render_); On 2015/11/06 09:54:31, peah-webrtc wrote: > On 2015/11/05 16:11:21, hlundin-webrtc wrote: > > Can you avoid taking the locks in the ctor? > > Seems to work. I thought that the threadannotation would complain, but it does > not :-) > > Done. I think the it intentionally turns a blind eye to ctors and dtors. https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1424663003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:255: rtc::scoped_ptr<ApmExternallyAccessibleSubModules> public_submodules_; On 2015/11/06 09:54:32, peah-webrtc wrote: > On 2015/11/05 16:11:21, hlundin-webrtc wrote: > > Any reason these two are (scoped) pointers and not just plain members? > > The only reason is that I did not want to expose their types outside APM since > they should not at all be used outside APM. If I add them as plain members they > need to be instantiated and then their types need to be known inside the header > file, but by using pointers their types can be specified as forwards. > > Is there any other way of achieving this? Or do you prefer that I include those > types in this header file? This is probably fine.
I've added a few comments in echo_cancellation_impl.*; the patterns repeat in the other modules, so the comments apply there too. audio_processing_impl.* was rather tough on the digestive system. I'll need to chew that piecewise. https://codereview.chromium.org/1424663003/diff/140001/webrtc/modules/audio_p... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.chromium.org/1424663003/diff/140001/webrtc/modules/audio_p... webrtc/modules/audio_processing/echo_cancellation_impl.cc:83: render_queue_element_max_size_(0) {} crit_render and crit_capture must be set; that's part of this class' invariant. You should: RTC_DCHECK(crit_render_); RTC_DCHECK(crit_capture_); Here, and the other modules that also take these as arguments. https://codereview.chromium.org/1424663003/diff/140001/webrtc/modules/audio_p... webrtc/modules/audio_processing/echo_cancellation_impl.cc:256: rtc::CritScope cs(crit_capture_); It's great that you have added these. Thank you! https://codereview.chromium.org/1424663003/diff/140001/webrtc/modules/audio_p... webrtc/modules/audio_processing/echo_cancellation_impl.cc:336: rtc::TryCritScope cs(crit_capture_); I don't understand why you are using TryCritScope in this way. You are not sure after instantiating this object that you have taken the critical section, and below you do not verify, using TryCritSect::locked() that you have the lock, before operating on the data that, presumably, you set out to protect. For instance, on Windows, TryEnterCriticalSection() is used and here's documentation for that: https://msdn.microsoft.com/en-us/library/windows/desktop/ms686857(v=vs.85).aspx https://codereview.chromium.org/1424663003/diff/140001/webrtc/modules/audio_p... webrtc/modules/audio_processing/echo_cancellation_impl.cc:338: // Call the lock function (without this call TryCritScope will DCHECK). TryCritScope::locked() does *not* call any lock function. It returns a bool telling the client whether the lock was taken or not. The DCHECK fails so that you don't forget to check whether the lock was taken. Here it looks like you're bypassing that check by ignoring the return value. https://codereview.chromium.org/1424663003/diff/140001/webrtc/modules/audio_p... webrtc/modules/audio_processing/echo_cancellation_impl.cc:353: return Configure(); You may be setting delay_logging_enabled_ and calling Configure() without having a lock here. https://codereview.chromium.org/1424663003/diff/140001/webrtc/modules/audio_p... webrtc/modules/audio_processing/echo_cancellation_impl.cc:412: // Only called from within APM, hence no locking is needed. What do you mean with "called from within APM"? All of this is "within APM", right? :) The machine and compiler do not read these comments. Assert your preconditions instead. For instance, you can: 1. Call IsLocked() on your critical sections to verify they have been taken by an outer layer. 2. Use ThreadChecker. 3. Build your own logic to verify the context is what you intend. https://codereview.chromium.org/1424663003/diff/140001/webrtc/modules/audio_p... webrtc/modules/audio_processing/echo_cancellation_impl.cc:483: // Only called from within APM, hence no locking is needed. This function is called from ProcessingComponent::Configure(), which is triggered from EchoCancellationImpl::enable_delay_logging() and EchoCancellationImpl::enable_delay_logging(), where you are using TryCritScope in a way I don't understand and which looks like you are not actually taking the lock. Please, explain why this works and what I misunderstand. https://codereview.chromium.org/1424663003/diff/140001/webrtc/modules/audio_p... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.chromium.org/1424663003/diff/140001/webrtc/modules/audio_p... webrtc/modules/audio_processing/echo_cancellation_impl.h:34: // Called holding the render lock. These comments just add noise. Remove. Document your assumptions with RTC_DCHECKs. https://codereview.chromium.org/1424663003/diff/140001/webrtc/modules/audio_p... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.chromium.org/1424663003/diff/140001/webrtc/modules/audio_p... webrtc/modules/audio_processing/gain_control_impl.cc:257: rtc::CritScope cs(crit_capture_); Why do you sometimes use "cs_capture", and sometimes "cs"?
I have some specific comments below, but the main problem is that with the sort of spotty lock annotation coverage you're able to get, I won't be able to catch mistakes reliably---not in a chunk of code this large. I have some ideas on how to improve the coverage, but I'm not sure we actually want to go down the path of making it more palatable to share locks between objects like this... https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:87: voice_detection(NULL) {} nullptr https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:233: #endif You can shrink the ifdef region to one line, right? https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:575: // to ProcessStream(,,,); Are the locks reentrant or not? Above in AudioProcessingImpl::Initialize, you take locks and then call another Initialize that also takes the same locks. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:101: int StartDebugRecording(const char filename[kMaxFilenameSize]) override; Why not std::string? https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:107: // multi-threaded manner. Acquires the capture lock. Acquire. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:127: // multi-threaded manner. Acquires the render lock. Acquire. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:340: This doesn't look right. The effect here is to take the lock if no one else is holding it; this leaves the member access below possibly unprotected. It seems better to have two variants of this method: one that locks (unconditionally), and one that documents that the caller must already hold the lock. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:351: Same. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:454: Same. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:42: // Aquires the capture loc.k Swap the last two characters. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:137: #endif // WEBRTC_MODULES_AUDIO_PROCESSING_ECHO_CANCELLATION_IMPL_H_ It's a serious problem that you aren't able to use the lock annotations here. But I guess it's the best we'll get without a substantial refactoring (well, an even more substantial one).
https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:87: voice_detection(NULL) {} On 2015/11/23 22:15:10, kwiberg-webrtc wrote: > nullptr Agree, and in the spirit of a boyscout I did a search-replace on all NULL in this file. Done. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:233: #endif On 2015/11/23 22:15:10, kwiberg-webrtc wrote: > You can shrink the ifdef region to one line, right? Done. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:575: // to ProcessStream(,,,); On 2015/11/23 22:15:10, kwiberg-webrtc wrote: > Are the locks reentrant or not? Above in AudioProcessingImpl::Initialize, you > take locks and then call another Initialize that also takes the same locks. Great find! I think they are probably not reentrant and the reason is that this function is only used in process_test.cc and an APM mock. And in process_test.cc it was only used if run without an aecdump recording. That meant that this function has not been tested at all with the new locks scheme. I'll remove the locks (as they are not needed) but eventually this function should be removed (there is actually an old TODO about this). Done. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:101: int StartDebugRecording(const char filename[kMaxFilenameSize]) override; On 2015/11/23 22:15:11, kwiberg-webrtc wrote: > Why not std::string? Good point! Not sure. This is something that should be changed, but since it was there before this work I'd rather do that in the scope of another CL. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:107: // multi-threaded manner. Acquires the capture lock. On 2015/11/23 22:15:11, kwiberg-webrtc wrote: > Acquire. Done. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:127: // multi-threaded manner. Acquires the render lock. On 2015/11/23 22:15:10, kwiberg-webrtc wrote: > Acquire. Done. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:256: rtc::CritScope cs(crit_capture_); On 2015/11/23 21:36:05, the sun wrote: > It's great that you have added these. Thank you! You are welcome! :-) Acknowledged. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:336: rtc::TryCritScope cs(crit_capture_); Agree, something is wrong with this. I'll revise. Please check so that it makes sense now. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:338: // Call the lock function (without this call TryCritScope will DCHECK). On 2015/11/23 21:36:05, the sun wrote: > TryCritScope::locked() does *not* call any lock function. It returns a bool > telling the client whether the lock was taken or not. The DCHECK fails so that > you don't forget to check whether the lock was taken. Here it looks like you're > bypassing that check by ignoring the return value. Of course you are correct, I'll revise. Please re-review! https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:340: On 2015/11/23 22:15:11, kwiberg-webrtc wrote: > This doesn't look right. The effect here is to take the lock if no one else is > holding it; this leaves the member access below possibly unprotected. > > It seems better to have two variants of this method: one that locks > (unconditionally), and one that documents that the caller must already hold the > lock. I agree on both, it was not correct, but I revised it now so now it should be ok. I think it worked out fine anyway because these are most likely anyway called by the same thread but the code was definitely not correct as it was. I agree about the two variants of the method, but please look first to see whether the refactored code is fine. I'd rather avoid two variants of the method if possible, as this then would need to apply to several methods, those methods would need to be made public in the API, and therefore anyone outside the API could call both methods (a DCHECK will partially ensure proper behavior but it is not a 100% guarantee that this method will not be called in Release mode) . This would increase the public APM API in a messy way when what we really want is to go in the direction of decreasing the public APM API. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:351: On 2015/11/23 22:15:11, kwiberg-webrtc wrote: > Same. Agree, please look at the revised solution. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:353: return Configure(); On 2015/11/23 21:36:05, the sun wrote: > You may be setting delay_logging_enabled_ and calling Configure() without having > a lock here. Agree, that was a bad design, but the revised code works I think. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:412: // Only called from within APM, hence no locking is needed. On 2015/11/23 21:36:05, the sun wrote: > What do you mean with "called from within APM"? All of this is "within APM", > right? :) > The machine and compiler do not read these comments. Assert your preconditions > instead. > > For instance, you can: > 1. Call IsLocked() on your critical sections to verify they have been taken by > an outer layer. > 2. Use ThreadChecker. > 3. Build your own logic to verify the context is what you intend. No, it is actually not, although I think I see your point. What I mean is that this function is only called from within APM, which means that it is called within a locking scheme that is owned by APM (a bit fuzzily phrased, but please ask if it was not clear). For some of the functions, that is not the case as they may be called directly from functions that do not belong to APM (due to the scheme of APM allowing pointers to submodules being passed to other functions that then can use them to access the public APIs to the subfunctions). After today's discussion, I think it is best to address this in one more chained CL. Does that sound ok? https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:454: On 2015/11/23 22:15:11, kwiberg-webrtc wrote: > Same. Agree. Please see the revised version. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:483: // Only called from within APM, hence no locking is needed. On 2015/11/23 21:36:05, the sun wrote: > This function is called from ProcessingComponent::Configure(), which is > triggered from EchoCancellationImpl::enable_delay_logging() and > EchoCancellationImpl::enable_delay_logging(), where you are using TryCritScope > in a way I don't understand and which looks like you are not actually taking the > lock. > > Please, explain why this works and what I misunderstand. You are right in that the lock was not properly acquired. But with the current construction it should be fine. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:34: // Called holding the render lock. On 2015/11/23 21:36:05, the sun wrote: > These comments just add noise. Remove. Document your assumptions with > RTC_DCHECKs. After today's discussion, I think this work will merit one more CL chained to this one. So if ok with you, I will fix this in that CL instead. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:42: // Aquires the capture loc.k On 2015/11/23 22:15:11, kwiberg-webrtc wrote: > Swap the last two characters. Done. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:137: #endif // WEBRTC_MODULES_AUDIO_PROCESSING_ECHO_CANCELLATION_IMPL_H_ On 2015/11/23 22:15:11, kwiberg-webrtc wrote: > It's a serious problem that you aren't able to use the lock annotations here. > But I guess it's the best we'll get without a substantial refactoring (well, an > even more substantial one). Agree! I'm not sure how far the lock annotation reaches, but by experience it definitely does not reach as far as when pointers are passed to the locks. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:257: rtc::CritScope cs(crit_capture_); On 2015/11/23 21:36:05, the sun wrote: > Why do you sometimes use "cs_capture", and sometimes "cs"? The scheme is to only use cs, when there is only one critical section active (one lock acquired), and if two critical sections are active cs_render/cs_capture is used to distinguish between them. But there clearly was a miss here on that. Please let me know if you find any other place where that happened! Done.
The replacement of the trylock thingies with recursive use of the locks looks good. https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:575: // to ProcessStream(,,,); On 2015/11/24 21:42:23, peah-webrtc wrote: > On 2015/11/23 22:15:10, kwiberg-webrtc wrote: > > Are the locks reentrant or not? Above in AudioProcessingImpl::Initialize, you > > take locks and then call another Initialize that also takes the same locks. > > Great find! > > I think they are probably not reentrant and the reason is that this function is > only used in process_test.cc and an APM mock. And in process_test.cc it was only > used if run without an aecdump recording. That meant that this function has not > been tested at all with the new locks scheme. > > I'll remove the locks (as they are not needed) but eventually this function > should be removed (there is actually an old TODO about this). > > Done. (As we later found out, the locks are in fact reentrant.) https://codereview.webrtc.org/1424663003/diff/160001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/160001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:336: // reentrant. possibly https://codereview.webrtc.org/1424663003/diff/160001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:345: // reentrant. possibly https://codereview.webrtc.org/1424663003/diff/160001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:446: // reentrant. possibly
https://codereview.webrtc.org/1424663003/diff/160001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/160001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:336: // reentrant. On 2015/11/25 10:17:14, kwiberg-webrtc wrote: > possibly Done. https://codereview.webrtc.org/1424663003/diff/160001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:345: // reentrant. On 2015/11/25 10:17:14, kwiberg-webrtc wrote: > possibly Done. https://codereview.webrtc.org/1424663003/diff/160001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:446: // reentrant. On 2015/11/25 10:17:14, kwiberg-webrtc wrote: > possibly Done.
lgtm, provided that you also land "Lock scheme #9", which fixes complains I had about this CL.
https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:276: AudioProcessingImpl::~AudioProcessingImpl() { CS not needed here anymore? https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:442: constants_.array_geometry.size() || did git cl format do this? I think the old version was more readable... https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:42: struct ApmPublicSubmodules; You can forward declare these inside of AudioProcessingImpl (requires you to prefix AudioProcessingImpl:: when defining the structs in the .cc of course). There's no need for them to be widely visible. https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:59: class Event; no fwd decl required https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:247: #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP Can we move all the private fields dealing with debug dumps into a single #ifdef/#endif block?
https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:276: AudioProcessingImpl::~AudioProcessingImpl() { On 2015/11/27 12:25:52, the sun wrote: > CS not needed here anymore? The critical section was there in the old locking scheme. There was a discussion about this before, and the reasoning was that destruction cannot be concurrent, and if the APM is concurrently accessed during destruction there is a big problem that should be solved elsewhere, Therefore the mutex was removed. Also, I don't think it is possible to really protect against anything during the destruction using a lock. Does that make sense? https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:442: constants_.array_geometry.size() || On 2015/11/27 12:25:52, the sun wrote: > did git cl format do this? I think the old version was more readable... Yes, I retried, and got the same result. https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:42: struct ApmPublicSubmodules; On 2015/11/27 12:25:52, the sun wrote: > You can forward declare these inside of AudioProcessingImpl (requires you to > prefix AudioProcessingImpl:: when defining the structs in the .cc of course). > There's no need for them to be widely visible. Really nice! Does this look ok? Done. https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:59: class Event; On 2015/11/27 12:25:52, the sun wrote: > no fwd decl required Done. https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:247: #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP On 2015/11/27 12:25:52, the sun wrote: > Can we move all the private fields dealing with debug dumps into a single > #ifdef/#endif block? Done.
Added updates according to reviewer comments: -Moved struct forwards to AudioProcImpl -Removed redundant forward -Bundled the debug state variables -Reran git cl format and added some changes that I had missed.
LGTM, with disclaimer: It is impossible to reason about the validity of the locking scheme because of how the APM is designed (the ProcessingComponent structure and the mutable state seem like the biggest problems). I stamp this because the extensive tests that you've created catches problems with the old locking scheme that appear gone with the new scheme, and because you have gone to great length adding thread annotations and splitting up state into structs that belong to certain threads, or is explicitly shared. But really, I cannot tell from reading this code that it will actually work. The next course of action should therefore be to refactor the APM into something that is easier to reason about. https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:276: AudioProcessingImpl::~AudioProcessingImpl() { On 2015/11/27 12:57:51, peah-webrtc wrote: > On 2015/11/27 12:25:52, the sun wrote: > > CS not needed here anymore? > > The critical section was there in the old locking scheme. There was a discussion > about this before, and the reasoning was that destruction cannot be concurrent, > and if the APM is concurrently accessed during destruction there is a big > problem that should be solved elsewhere, > > Therefore the mutex was removed. > > Also, I don't think it is possible to really protect against anything during the > destruction using a lock. > > Does that make sense? > Yeah, sorry. https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:442: constants_.array_geometry.size() || On 2015/11/27 12:57:51, peah-webrtc wrote: > On 2015/11/27 12:25:52, the sun wrote: > > did git cl format do this? I think the old version was more readable... > > Yes, I retried, and got the same result. Acknowledged. https://codereview.webrtc.org/1424663003/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1424663003/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:55: // State for the debug dump. You could move these two also into the private section of the class, right?
On 2015/11/26 21:34:50, kwiberg-webrtc wrote: > lgtm, provided that you also land "Lock scheme #9", which fixes complains I had > about this CL. We decided to move the Lock scheme #9 CL into this CL, which means that all the changes in #9 are included in this commit. So that should be fine now.
On 2015/11/27 13:32:28, the sun wrote: > LGTM, with disclaimer: > > It is impossible to reason about the validity of the locking scheme because of > how the APM is designed (the ProcessingComponent structure and the mutable state > seem like the biggest problems). > > I stamp this because the extensive tests that you've created catches problems > with the old locking scheme that appear gone with the new scheme, and because > you have gone to great length adding thread annotations and splitting up state > into structs that belong to certain threads, or is explicitly shared. > > But really, I cannot tell from reading this code that it will actually work. The > next course of action should therefore be to refactor the APM into something > that is easier to reason about. > > https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/audio_processing_impl.cc:276: > AudioProcessingImpl::~AudioProcessingImpl() { > On 2015/11/27 12:57:51, peah-webrtc wrote: > > On 2015/11/27 12:25:52, the sun wrote: > > > CS not needed here anymore? > > > > The critical section was there in the old locking scheme. There was a > discussion > > about this before, and the reasoning was that destruction cannot be > concurrent, > > and if the APM is concurrently accessed during destruction there is a big > > problem that should be solved elsewhere, > > > > Therefore the mutex was removed. > > > > Also, I don't think it is possible to really protect against anything during > the > > destruction using a lock. > > > > Does that make sense? > > > > Yeah, sorry. > > https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/audio_processing_impl.cc:442: > constants_.array_geometry.size() || > On 2015/11/27 12:57:51, peah-webrtc wrote: > > On 2015/11/27 12:25:52, the sun wrote: > > > did git cl format do this? I think the old version was more readable... > > > > Yes, I retried, and got the same result. > > Acknowledged. > > https://codereview.webrtc.org/1424663003/diff/220001/webrtc/modules/audio_pro... > File webrtc/modules/audio_processing/audio_processing_impl.h (right): > > https://codereview.webrtc.org/1424663003/diff/220001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/audio_processing_impl.h:55: // State for the > debug dump. > You could move these two also into the private section of the class, right? Well formulated and truly motivated disclaimer!
https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1424663003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:276: AudioProcessingImpl::~AudioProcessingImpl() { On 2015/11/27 13:32:28, the sun wrote: > On 2015/11/27 12:57:51, peah-webrtc wrote: > > On 2015/11/27 12:25:52, the sun wrote: > > > CS not needed here anymore? > > > > The critical section was there in the old locking scheme. There was a > discussion > > about this before, and the reasoning was that destruction cannot be > concurrent, > > and if the APM is concurrently accessed during destruction there is a big > > problem that should be solved elsewhere, > > > > Therefore the mutex was removed. > > > > Also, I don't think it is possible to really protect against anything during > the > > destruction using a lock. > > > > Does that make sense? > > > > Yeah, sorry. Np :-) https://codereview.webrtc.org/1424663003/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1424663003/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:55: // State for the debug dump. On 2015/11/27 13:32:28, the sun wrote: > You could move these two also into the private section of the class, right? Great point as usual! Will do! Done.
Thanks everyone the an enormous effort you have put into the reviewing of this CL, and for the consistently great reviews! I will do some further testing of what is in here, but if I don't find anything concerning, I'll try to land it during the weekend.
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1424663003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1424663003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_tsan2 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/8315)
Updated the lock test that fails for the CL -Changed AGC mode as the previous fails on Android (as Android does not support it). -Removed the calls to num_reverse_channels() in APM as that causes the thread sanitizers to fail in the test. (This APM member function is never otherwise (apart from in unpack_aecdump and process_test.cc) called from outside APM) so it makes sense to remove it (and then remove the API call in the upcoming APM refactoring)
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1424663003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1424663003/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_tsan2 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/8325)
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1424663003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1424663003/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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, kwiberg@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1424663003/#ps300001 (title: "Created a threadsafe wrapper for the random generator in the locktest. Fixed an unprotected variabl…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1424663003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1424663003/300001
Message was sent while issue was closed.
Description was changed from ========== Introduced the new locking scheme BUG=webrtc:5099 ========== to ========== Introduced the new locking scheme BUG=webrtc:5099 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Introduced the new locking scheme BUG=webrtc:5099 ========== to ========== Introduced the new locking scheme BUG=webrtc:5099 Committed: https://crrev.com/df3efa8c079294857a8b8e0a02634d06a6d6b6d6 Cr-Commit-Position: refs/heads/master@{#10836} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/df3efa8c079294857a8b8e0a02634d06a6d6b6d6 Cr-Commit-Position: refs/heads/master@{#10836} |
