Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1422013002:
Preparational work for an upcoming addition of a threadchecking scheme for APM (Closed)
|
Created:
5 years, 1 month ago by peah-webrtc Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, tterriberry_mozilla.com, hlundin-webrtc, kwiberg-webrtc, the sun, aluebs-webrtc, bjornv1, ivoc Base URL:
https://chromium.googlesource.com/external/webrtc.git@bundling_of_state_CL Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded a threadchecking scheme to APM that checks that the APM API calls are called from the correct threads. The actual threadcheckers were, however, removed and will be reintroduced in another upcoming CL.
BUG=webrtc:5099
Committed: https://crrev.com/81b9bfe6856bb4f9fd0ba79899f72b8385c58979
Cr-Commit-Position: refs/heads/master@{#10817}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Changes in response to user comments #
Total comments: 22
Patch Set 3 : Changes in response to latest reviewer comments #
Total comments: 5
Patch Set 4 : Re-revised the thread_checking scheme #Patch Set 5 : Added threadcheckers also to the private functions (when possible) and added a signal threadchecker #Patch Set 6 : Fixed the final threadchecker refactoring issues (and merged from latest master) #
Total comments: 6
Patch Set 7 : Added DCHECKS with threadchecker combinations #Patch Set 8 : Fixed the conditional thread-checking scheme" #
Total comments: 2
Patch Set 9 : Modified and shortened comments #Patch Set 10 : Merge with latest master and changed some comments #Patch Set 11 : Removed the thread checkers (which will be applied later in a separate CL) #Patch Set 12 : Merge with master #
Dependent Patchsets: Messages
Total messages: 72 (34 generated)
Description was changed from ========== Format changes Reverted accidental change in the aec unittest Added thread checkers BUG= ========== to ========== Format changes BUG=webrtc:5099 ==========
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kwiberg@webrtc.org
https://codereview.webrtc.org/1422013002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:254: capture_thread_.DetachFromThread(); Move these as high up as possible, so that you never expose the half-initialized state to your underlings? That way, it'll fail if one of them calls CalledOnValidThread in its constructor. https://codereview.webrtc.org/1422013002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1422013002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.h:159: rtc::ThreadChecker capture_thread_; *_thread_checker_ ?
This CL is now again in the desired shape. Please review at will. https://codereview.webrtc.org/1422013002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:254: capture_thread_.DetachFromThread(); On 2015/10/26 13:57:56, kwiberg-webrtc wrote: > Move these as high up as possible, so that you never expose the half-initialized > state to your underlings? That way, it'll fail if one of them calls > CalledOnValidThread in its constructor. I'm not convinced this is the right thing to do. If the work in the construction would only be to initialize the state, it may be fine. But if the constructors do more than just that and also calls other methods that have threadcheckers in them, these calls will reinitialize the thread checkers to again point to the initialization threads, which will cause things to start failing once the threads are called using the render and capture threads. Does that make sense? However, since it seems to work, I moved them according to your recommendation. Done. https://codereview.webrtc.org/1422013002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1422013002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.h:159: rtc::ThreadChecker capture_thread_; On 2015/10/26 13:57:56, kwiberg-webrtc wrote: > *_thread_checker_ ? Done.
LG, just a few nits. https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:446: C-k https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.h:48: rtc::ThreadChecker* render_thread_checker); const rtc::ThreadChecker* https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.h (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.h:48: rtc::ThreadChecker* render_thread_checker); const rtc::ThreadChecker* https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.h (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.h:50: rtc::ThreadChecker* render_thread_checker, const rtc::ThreadChecker* https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.h:51: rtc::ThreadChecker* capture_thread_checker); const rtc::ThreadChecker* https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/noise_suppression_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/noise_suppression_impl.cc:51: : ProcessingComponent(), apm_(apm), crit_(crit), level_(kModerate) {} Leave this file out of the CL. https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/noise_suppression_impl.h (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/noise_suppression_impl.h:14: #include "webrtc/base/thread_checker.h" Why is this needed? https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/voice_detection_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/voice_detection_impl.cc:42: : ProcessingComponent(), Not in this CL. https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/voice_detection_impl.h (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/voice_detection_impl.h:14: #include "webrtc/base/thread_checker.h" Why?
https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.cc:446: On 2015/11/05 13:08:55, hlundin-webrtc wrote: > C-k Done. https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.h:48: rtc::ThreadChecker* render_thread_checker); On 2015/11/05 13:08:55, hlundin-webrtc wrote: > const rtc::ThreadChecker* Done. https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.h (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.h:48: rtc::ThreadChecker* render_thread_checker); On 2015/11/05 13:08:55, hlundin-webrtc wrote: > const rtc::ThreadChecker* Done. https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.h (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.h:50: rtc::ThreadChecker* render_thread_checker, On 2015/11/05 13:08:55, hlundin-webrtc wrote: > const rtc::ThreadChecker* Done. https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.h:51: rtc::ThreadChecker* capture_thread_checker); On 2015/11/05 13:08:55, hlundin-webrtc wrote: > const rtc::ThreadChecker* Done. https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/noise_suppression_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/noise_suppression_impl.cc:51: : ProcessingComponent(), apm_(apm), crit_(crit), level_(kModerate) {} On 2015/11/05 13:08:55, hlundin-webrtc wrote: > Leave this file out of the CL. It was initially included, but changes along the way eliminated the difference from the original. Not sure on how to remove it from the CL now. I'll revert the change to be identical to the original though. Please let me know if there is any other way to remove this file from the CL! https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/noise_suppression_impl.h (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/noise_suppression_impl.h:14: #include "webrtc/base/thread_checker.h" On 2015/11/05 13:08:55, hlundin-webrtc wrote: > Why is this needed? Not needed it turned out. Initially all public methods had thread checkers, but unfortunately the libjingle unit tests caused them to fail, most likely due to another thread usage there than I expected. Therefore lots of threadcheckers had to be removed to comply with the test. This include is a leftover from that. Done. https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/voice_detection_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/voice_detection_impl.cc:42: : ProcessingComponent(), On 2015/11/05 13:08:55, hlundin-webrtc wrote: > Not in this CL. It was initially included, but changes along the way eliminated the difference from the original. Not sure on how to remove it from the CL now. I'll revert the change to be identical to the original though. Please let me know if there is any other way to remove this file from the CL! https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/voice_detection_impl.h (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/voice_detection_impl.h:14: #include "webrtc/base/thread_checker.h" On 2015/11/05 13:08:55, hlundin-webrtc wrote: > Why? Not needed it turned out. Initially all public methods had thread checkers, but unfortunately the libjingle unit tests caused them to fail, most likely due to another thread usage there than I expected. Therefore lots of threadcheckers had to be removed to comply with the test. This include is a leftover from that. Done.
lgtm https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/noise_suppression_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/noise_suppression_impl.cc:51: : ProcessingComponent(), apm_(apm), crit_(crit), level_(kModerate) {} On 2015/11/06 07:31:14, peah-webrtc wrote: > On 2015/11/05 13:08:55, hlundin-webrtc wrote: > > Leave this file out of the CL. > > It was initially included, but changes along the way eliminated the difference > from the original. Not sure on how to remove it from the CL now. > > I'll revert the change to be identical to the original though. Please let me > know if there is any other way to remove this file from the CL! That is the way to do it. https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/noise_suppression_impl.h (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/noise_suppression_impl.h:14: #include "webrtc/base/thread_checker.h" On 2015/11/06 07:31:14, peah-webrtc wrote: > On 2015/11/05 13:08:55, hlundin-webrtc wrote: > > Why is this needed? > > Not needed it turned out. Initially all public methods had thread checkers, but > unfortunately the libjingle unit tests caused them to fail, most likely due to > another thread usage there than I expected. Therefore lots of threadcheckers had > to be removed to comply with the test. > > This include is a leftover from that. > > Done. Acknowledged. https://codereview.webrtc.org/1422013002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:349: const int n = num_handles(); (Effects of rebase.)
https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/noise_suppression_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/noise_suppression_impl.cc:51: : ProcessingComponent(), apm_(apm), crit_(crit), level_(kModerate) {} On 2015/11/06 07:45:07, hlundin-webrtc wrote: > On 2015/11/06 07:31:14, peah-webrtc wrote: > > On 2015/11/05 13:08:55, hlundin-webrtc wrote: > > > Leave this file out of the CL. > > > > It was initially included, but changes along the way eliminated the difference > > from the original. Not sure on how to remove it from the CL now. > > > > I'll revert the change to be identical to the original though. Please let me > > know if there is any other way to remove this file from the CL! > > That is the way to do it. Acknowledged. https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/noise_suppression_impl.h (right): https://codereview.webrtc.org/1422013002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/noise_suppression_impl.h:14: #include "webrtc/base/thread_checker.h" On 2015/11/06 07:45:07, hlundin-webrtc wrote: > On 2015/11/06 07:31:14, peah-webrtc wrote: > > On 2015/11/05 13:08:55, hlundin-webrtc wrote: > > > Why is this needed? > > > > Not needed it turned out. Initially all public methods had thread checkers, > but > > unfortunately the libjingle unit tests caused them to fail, most likely due to > > another thread usage there than I expected. Therefore lots of threadcheckers > had > > to be removed to comply with the test. > > > > This include is a leftover from that. > > > > Done. > > Acknowledged. Acknowledged. https://codereview.webrtc.org/1422013002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:349: const int n = num_handles(); On 2015/11/06 07:45:07, hlundin-webrtc wrote: > (Effects of rebase.) Yep. That is the cause
lgtm https://codereview.webrtc.org/1422013002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:254: capture_thread_.DetachFromThread(); On 2015/11/05 11:47:33, peah-webrtc wrote: > On 2015/10/26 13:57:56, kwiberg-webrtc wrote: > > Move these as high up as possible, so that you never expose the > half-initialized > > state to your underlings? That way, it'll fail if one of them calls > > CalledOnValidThread in its constructor. > > > I'm not convinced this is the right thing to do. If the work in the construction > would only be to initialize the state, it may be fine. But if the constructors > do more than just that and also calls other methods that have threadcheckers in > them, these calls will reinitialize the thread checkers to again point to the > initialization threads, which will cause things to start failing once the > threads are called using the render and capture threads. Does that make sense? > > However, since it seems to work, I moved them according to your recommendation. > > Done. Yes, that's exactly the sort of behavior you protect against by moving it up. Having it very early is a proof that none of the potential issues you mentioned are in fact happening.
peah@webrtc.org changed reviewers: + solenberg@webrtc.org
https://codereview.webrtc.org/1422013002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1422013002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:158: rtc::ThreadChecker render_thread_checker_; Why no rtc::ThreadChecker signal_thread_checker_; i.e., one for all the other accesses?
https://codereview.webrtc.org/1422013002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1422013002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:158: rtc::ThreadChecker render_thread_checker_; On 2015/11/11 10:50:47, the sun wrote: > Why no rtc::ThreadChecker signal_thread_checker_; i.e., one for all the other > accesses? I agree that these names are not that good as it is not clear what belongs to what. Since we are mainly accessed from two threads during a call, and one is the one handling the render data and the other is the one handling the capture data I chose this naming scheme, but for the capture thread it is not intuitively clear that some of the api calls should belong to that one. It may, however, be important for the lock design that that is the case and from that perspective I think it make sense to call the thread checker something with "capture". I'm not fully sure what you mean with the proposal, that the capture_thread_checker or that the render_thread_checker should be renamed to signal_thread_checker? I think any name change for the thread checkers should be reflected in a name change for the locks in the upcoming CLs
https://codereview.webrtc.org/1422013002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1422013002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:158: rtc::ThreadChecker render_thread_checker_; On 2015/11/17 16:03:46, peah-webrtc wrote: > On 2015/11/11 10:50:47, the sun wrote: > > Why no rtc::ThreadChecker signal_thread_checker_; i.e., one for all the other > > accesses? > > I agree that these names are not that good as it is not clear what belongs to > what. Since we are mainly accessed from two threads during a call, and one is > the one handling the render data and the other is the one handling the capture > data I chose this naming scheme, but for the capture thread it is not > intuitively clear that some of the api calls should belong to that one. It may, > however, be important for the lock design that that is the case and from that > perspective I think it make sense to call the thread checker something with > "capture". > > I'm not fully sure what you mean with the proposal, that the > capture_thread_checker or that the render_thread_checker should be renamed to > signal_thread_checker? > > I think any name change for the thread checkers should be reflected in a name > change for the locks in the upcoming CLs render_... and capture_... or good names, don't worry about those. Perhaps I'm wrong, but I though the APM was created on neither of those threads, and that some initialization, and method calls, would also happen on non-audio threads. If that is the case, we should have a thread checker for those as well, and I suggested the name signal_thread_checker, because in some sense it is signaling.
I again revised the thread-checking scheme, but this time with the difference that I did only try to add it to the functions in audio_processing_impl. This simplified the process of testing out various combinations of thread checkers in order to find the most extensive thread checking scheme. The thread checking scheme in audio_processing_impl have thereby been extended a bit further. According to discussions with Fredrik, I did not go any further into extending the thread-checking scheme in the submodules as the access to those will anyway be refactored to go via audio_processing_impl. I could not find any case where a third thread checker would help in audio_processing_impl. There are for sure more than 2 threads calling APM during runtime but I think that the other threads are accessing the submodules directly and are therefore not seen by the thread_checkers in APM.
Description was changed from ========== Format changes BUG=webrtc:5099 ========== to ========== BUG=webrtc:5099 ==========
Sorry again, need to do some minor further changes in the scheme due to a dcheck that fired when I tried the performance test on the lock scheme. Stay tuned for the update.
Description was changed from ========== BUG=webrtc:5099 ========== to ========== BUG=webrtc:5099 ==========
Now it is ready :-). Please go ahead and review whenever you want!
Thanks for doing this! A few more questions. https://codereview.webrtc.org/1422013002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:261: CriticalSectionScoped crit_scoped(crit_); RTC_DCHECK(signal_thread_checker_.CalledOnValidThread()); https://codereview.webrtc.org/1422013002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:323: // Called from both threads. Thread check is therefore not possible. Uhm, yes it is possible: RTC_DCHECK(render_thread_checker_.CalledOnValidThread() || capture_thread_checker_.CalledOnValidThread()); https://codereview.webrtc.org/1422013002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:158: int EchoCancellationImpl::ProcessCaptureAudio(AudioBuffer* audio) { Why no check on the capture_thread_checker here, like you do in gain control?
-Added thread checker for the destructor. -Removed the detach of the thread checker in the constructor. -Added DCHECKS on combinations of thread checkers when possible. https://codereview.webrtc.org/1422013002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:261: CriticalSectionScoped crit_scoped(crit_); On 2015/11/23 12:46:21, the sun wrote: > RTC_DCHECK(signal_thread_checker_.CalledOnValidThread()); Done. https://codereview.webrtc.org/1422013002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:323: // Called from both threads. Thread check is therefore not possible. On 2015/11/23 12:46:21, the sun wrote: > Uhm, yes it is possible: > RTC_DCHECK(render_thread_checker_.CalledOnValidThread() || > capture_thread_checker_.CalledOnValidThread()); Of course! Great! Added that! Done. https://codereview.webrtc.org/1422013002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:158: int EchoCancellationImpl::ProcessCaptureAudio(AudioBuffer* audio) { On 2015/11/23 12:46:21, the sun wrote: > Why no check on the capture_thread_checker here, like you do in gain control? You are definitely correct in that. The history behind it is that with the initial thread-checker scheme, it was not possible. But due to the changes done in response to the very good feedback on that scheme, it would now be possible. As the access to these submodules will at some point in the not-so-far future be redirected via the APM API, it was, together with the the reviewer, decided not to add the threadcheckers in the submodules in the scope of this work.
lgtm
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 Link to the patchset: https://codereview.webrtc.org/1422013002/#ps120001 (title: "Added DCHECKS with threadchecker combinations")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422013002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422013002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1916)
still lgtm, but see comment https://codereview.webrtc.org/1422013002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:404: // This is called from the initialization functionality which is shared I'm not a fan of repeated, verbose comments. Can you add a TODO(peah): instead, that should be checked using the clang annotations, going forward?
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/1422013002/#ps170001 (title: "Merge with current master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422013002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422013002/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1962)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, henrik.lundin@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1422013002/#ps190001 (title: "New merge with latest master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422013002/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422013002/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1964)
Patchset #11 (id:190001) has been deleted
Patchset #10 (id:170001) has been deleted
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422013002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422013002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1966)
Description was changed from ========== BUG=webrtc:5099 ========== to ========== Added a threadchecking scheme to APM that checks that the APM API calls are called from the correct threads. BUG=webrtc:5099 ==========
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422013002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422013002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/9295)
Realized I had not sent the latest comment reply. https://codereview.webrtc.org/1422013002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1422013002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:404: // This is called from the initialization functionality which is shared On 2015/11/25 08:53:56, the sun wrote: > I'm not a fan of repeated, verbose comments. Can you add a TODO(peah): instead, > that should be checked using the clang annotations, going forward? Could not be happier to oblige! :-) Done.
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422013002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422013002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/9341)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, henrik.lundin@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1422013002/#ps210001 (title: "Merge with latest master and changed some comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422013002/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422013002/210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Added a threadchecking scheme to APM that checks that the APM API calls are called from the correct threads. BUG=webrtc:5099 ========== to ========== Added a threadchecking scheme to APM that checks that the APM API calls are called from the correct threads. The actual threadcheckers were, however, removed and will be reintroduced in another upcoming CL. BUG=webrtc:5099 ==========
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, henrik.lundin@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1422013002/#ps230001 (title: "Removed the thread checkers (which will be applied later in a separate CL)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422013002/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422013002/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/9365)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, henrik.lundin@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1422013002/#ps250001 (title: "Merge with master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422013002/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422013002/250001
Message was sent while issue was closed.
Description was changed from ========== Added a threadchecking scheme to APM that checks that the APM API calls are called from the correct threads. The actual threadcheckers were, however, removed and will be reintroduced in another upcoming CL. BUG=webrtc:5099 ========== to ========== Added a threadchecking scheme to APM that checks that the APM API calls are called from the correct threads. The actual threadcheckers were, however, removed and will be reintroduced in another upcoming CL. BUG=webrtc:5099 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:250001)
Message was sent while issue was closed.
Description was changed from ========== Added a threadchecking scheme to APM that checks that the APM API calls are called from the correct threads. The actual threadcheckers were, however, removed and will be reintroduced in another upcoming CL. BUG=webrtc:5099 ========== to ========== Added a threadchecking scheme to APM that checks that the APM API calls are called from the correct threads. The actual threadcheckers were, however, removed and will be reintroduced in another upcoming CL. BUG=webrtc:5099 Committed: https://crrev.com/81b9bfe6856bb4f9fd0ba79899f72b8385c58979 Cr-Commit-Position: refs/heads/master@{#10817} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/81b9bfe6856bb4f9fd0ba79899f72b8385c58979 Cr-Commit-Position: refs/heads/master@{#10817} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
