Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1410833002:
Lock scheme #4: Introduced the render sample queue for the aec and aecm (Closed)
|
Created:
5 years, 2 months ago by peah-webrtc Modified:
5 years, 1 month 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@aec_error_report_CL Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionIntroduced the render sample queue for the aec and aecm.
BUG=webrtc:5099
Committed: https://crrev.com/fa6228e221d818af55e3d8343c792f2c1ecc7252
Cr-Commit-Position: refs/heads/master@{#10662}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Corrected the sample format when passing the render samples for AECM #Patch Set 3 : Changed the render side sample queueing scheme to be atomic over all channels for each frame #Patch Set 4 : Changed the scheme for verifying the vector capacity. And renamed constants #Patch Set 5 : Fixed the checks of frame lengths #Patch Set 6 : Merge #
Total comments: 20
Patch Set 7 : Changed scheme for putting samples into the queue, changed the verifier, together with minor other … #Patch Set 8 : Added conditional render queue recreation #Patch Set 9 : Added code to ensure that a zero sized queue is never created #Patch Set 10 : Merge from other CLs in the list #
Total comments: 26
Patch Set 11 : Changes in response to latest reviewer comments #
Total comments: 4
Patch Set 12 : Created a generic template for using to verify the invariance of the render queue #Patch Set 13 : Fixed swapqueue usage that changed in the merge #Patch Set 14 : Backported proper handling of full render queue #Patch Set 15 : Merge with latest master #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 32 (8 generated)
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kwiberg@webrtc.org, solenberg@google.com
https://codereview.webrtc.org/1410833002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1410833002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.cc:113: sizeof(*audio->split_bands_const_f(j)[kBand0To8kHz]))); Maybe I just haven't stared long enough, but where do you ensure that you don't fill render_queue_buffer_ with more data than it can hold? std::vector::clear followed by std::vector::insert (the one that takes an start and an end iterator) is a good (and safe!) way to copy an array of PODs into a vector. But you'll probably also want to DCHECK that the capacity is sufficient so that no reallocation will take place. https://codereview.webrtc.org/1410833002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.cc:114: render_queue_buffer_.resize(audio->num_frames_per_band()); Oh, here. This is illegal if it ends up increasing the size, since that means the previous statement wrote past the end of the vector. https://codereview.webrtc.org/1410833002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.cc:118: // started. Yeah, I guess that has to be done before this CL can land? Otherwise we risk dropping data. https://codereview.webrtc.org/1410833002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.cc:433: (v.size() == kMaxNumSamplesPerFrameToBuffer)); From the names of these constants, I would've expected any value in [min, max] to be allowed. Also, I don't see how verifying this property will help ensure there are no run-time reallocations. What if the vector to be filled has the smaller size, but we want to fill it with the larger number of elements?
A patch fixing a sample format. Will address the feedback in the next patch.
solenberg@webrtc.org changed reviewers: - solenberg@google.com
Description was changed from ========== Introduced the render sample queue for the aec and aecm. ========== to ========== Introduced the render sample queue for the aec and aecm. BUG=webrtc:5099 ==========
https://codereview.webrtc.org/1410833002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1410833002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.cc:113: sizeof(*audio->split_bands_const_f(j)[kBand0To8kHz]))); On 2015/10/17 07:50:07, kwiberg-webrtc wrote: > Maybe I just haven't stared long enough, but where do you ensure that you don't > fill render_queue_buffer_ with more data than it can hold? > > std::vector::clear followed by std::vector::insert (the one that takes an start > and an end iterator) is a good (and safe!) way to copy an array of PODs into a > vector. But you'll probably also want to DCHECK that the capacity is sufficient > so that no reallocation will take place. I rewrote this quite a lot. Could you please let me know what you think of the new code! https://codereview.webrtc.org/1410833002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.cc:114: render_queue_buffer_.resize(audio->num_frames_per_band()); On 2015/10/17 07:50:07, kwiberg-webrtc wrote: > Oh, here. This is illegal if it ends up increasing the size, since that means > the previous statement wrote past the end of the vector. Agree. It is now rewritten. Could you please recheck! https://codereview.webrtc.org/1410833002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.cc:118: // started. On 2015/10/17 07:50:07, kwiberg-webrtc wrote: > Yeah, I guess that has to be done before this CL can land? Otherwise we risk > dropping data. This is also now redesigned. It should be fine now. https://codereview.webrtc.org/1410833002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.cc:433: (v.size() == kMaxNumSamplesPerFrameToBuffer)); On 2015/10/17 07:50:07, kwiberg-webrtc wrote: > From the names of these constants, I would've expected any value in [min, max] > to be allowed. > > Also, I don't see how verifying this property will help ensure there are no > run-time reallocations. What if the vector to be filled has the smaller size, > but we want to fill it with the larger number of elements? I totally agree. Regarding the names I renamed them. Regarding the run-time reallocations avoidance, with the new functor verifier this is now properly verified. Could you please recheck!
https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:116: buffer_index += audio->num_frames_per_band(); Can't you use the nurmal features of std::vector to keep track of this? In other words, use the vector's size to keep track of how many elements there are in it, the vectors insert function to insert elements, etc. Ask me for details if you're unsure how. https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:123: render_signal_queue_->Insert(&render_queue_buffer_); Doesn't Insert return a status bool that you'd want to check? https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:136: while (samples_read) { Just while (render_signal_queue_->Remove(&capture_queue_buffer_)) ? That'll replace lines 135, 136, and 153. https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:398: (max_frame_size * num_handles_required()); Unnecessary parentheses. https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:401: (min_frame_size * num_handles_required()); Unnecessary parentheses. https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:32: (v.capacity() >= std::max(allowed_size_1_, allowed_size_2_))); Why test size in addition to capacity? The latter is sufficient to ensure there will be no reallocations. https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:121: render_signal_queue_; As of C++11, you can write >> instead of > >. Also, why make an indirection with a scoped_ptr instead of holding the SwapQueue directly? https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_control_mobile_impl.h (right): https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.h:99: rtc::scoped_ptr<SwapQueue<std::vector<int16_t>, AecmRenderQueueItemVerifier> > >>
https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:116: buffer_index += audio->num_frames_per_band(); On 2015/10/27 10:43:07, kwiberg-webrtc wrote: > Can't you use the nurmal features of std::vector to keep track of this? In other > words, use the vector's size to keep track of how many elements there are in it, > the vectors insert function to insert elements, etc. Ask me for details if > you're unsure how. Done. https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:123: render_signal_queue_->Insert(&render_queue_buffer_); On 2015/10/27 10:43:07, kwiberg-webrtc wrote: > Doesn't Insert return a status bool that you'd want to check? Done. https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:136: while (samples_read) { On 2015/10/27 10:43:07, kwiberg-webrtc wrote: > Just > > while (render_signal_queue_->Remove(&capture_queue_buffer_)) > > ? That'll replace lines 135, 136, and 153. Done. https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:398: (max_frame_size * num_handles_required()); On 2015/10/27 10:43:07, kwiberg-webrtc wrote: > Unnecessary parentheses. Done. https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:401: (min_frame_size * num_handles_required()); On 2015/10/27 10:43:07, kwiberg-webrtc wrote: > Unnecessary parentheses. Done. https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:32: (v.capacity() >= std::max(allowed_size_1_, allowed_size_2_))); On 2015/10/27 10:43:07, kwiberg-webrtc wrote: > Why test size in addition to capacity? The latter is sufficient to ensure there > will be no reallocations. Done. https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:121: render_signal_queue_; On 2015/10/27 10:43:07, kwiberg-webrtc wrote: > As of C++11, you can write >> instead of > >. > > Also, why make an indirection with a scoped_ptr instead of holding the SwapQueue > directly? The thinking was that since the queue needs to be reconstructed during runtime, it is better to do it this way as it is cheaper to new it up dynamically, than to create it and assign to a queue state variable. Would that make sense, or is there another way to do this? https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_control_mobile_impl.h (right): https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.h:99: rtc::scoped_ptr<SwapQueue<std::vector<int16_t>, AecmRenderQueueItemVerifier> > On 2015/10/27 10:43:07, kwiberg-webrtc wrote: > >> Done.
https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:121: render_signal_queue_; On 2015/10/29 11:36:56, peah-webrtc wrote: > On 2015/10/27 10:43:07, kwiberg-webrtc wrote: > > > Also, why make an indirection with a scoped_ptr instead of holding > > the SwapQueue directly? > > The thinking was that since the queue needs to be reconstructed > during runtime, it is better to do it this way as it is cheaper to > new it up dynamically, than to create it and assign to a queue state > variable. > > Would that make sense, or is there another way to do this? Well... Why do you need to recreate it? Isn't this what the Clean method is for?
https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:121: render_signal_queue_; On 2015/10/29 12:30:22, kwiberg-webrtc wrote: > On 2015/10/29 11:36:56, peah-webrtc wrote: > > On 2015/10/27 10:43:07, kwiberg-webrtc wrote: > > > > > Also, why make an indirection with a scoped_ptr instead of holding > > > the SwapQueue directly? > > > > The thinking was that since the queue needs to be reconstructed > > during runtime, it is better to do it this way as it is cheaper to > > new it up dynamically, than to create it and assign to a queue state > > variable. > > > > Would that make sense, or is there another way to do this? > > Well... Why do you need to recreate it? Isn't this what the Clean > method is for? I need to recreate it because the maximum item size has changed, and there is currently no functionality to enforce a new container size on the queue. This cannot be known in advance since the number of channels are only known during runtime and can change during reinitialization. Therefore queue reallocation may be needed. I realized that the reinitialization should be conditional though, so I added that in the latest update.
https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:121: render_signal_queue_; On 2015/10/29 13:07:36, peah-webrtc wrote: > On 2015/10/29 12:30:22, kwiberg-webrtc wrote: > > On 2015/10/29 11:36:56, peah-webrtc wrote: > > > On 2015/10/27 10:43:07, kwiberg-webrtc wrote: > > > > > > > Also, why make an indirection with a scoped_ptr instead of holding > > > > the SwapQueue directly? > > > > > > The thinking was that since the queue needs to be reconstructed > > > during runtime, it is better to do it this way as it is cheaper to > > > new it up dynamically, than to create it and assign to a queue state > > > variable. > > > > > > Would that make sense, or is there another way to do this? > > > > Well... Why do you need to recreate it? Isn't this what the Clean > > method is for? > > I need to recreate it because the maximum item size has changed, and there is > currently no functionality to enforce a new container size on the queue. > > This cannot be known in advance since the number of channels are only known > during runtime and can change during reinitialization. Therefore queue > reallocation may be needed. > > I realized that the reinitialization should be conditional though, so I added > that in the latest update. OK. So it isn't just a performance thing; you actually *need* the indirection, since SwapQueue doesn't support assignment. Then I'd argue that SwapQueue should support assignment. Move assignment should even be more efficient than the indirection you have now.
https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1410833002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:121: render_signal_queue_; On 2015/10/29 15:04:50, kwiberg-webrtc wrote: > On 2015/10/29 13:07:36, peah-webrtc wrote: > > On 2015/10/29 12:30:22, kwiberg-webrtc wrote: > > > On 2015/10/29 11:36:56, peah-webrtc wrote: > > > > On 2015/10/27 10:43:07, kwiberg-webrtc wrote: > > > > > > > > > Also, why make an indirection with a scoped_ptr instead of holding > > > > > the SwapQueue directly? > > > > > > > > The thinking was that since the queue needs to be reconstructed > > > > during runtime, it is better to do it this way as it is cheaper to > > > > new it up dynamically, than to create it and assign to a queue state > > > > variable. > > > > > > > > Would that make sense, or is there another way to do this? > > > > > > Well... Why do you need to recreate it? Isn't this what the Clean > > > method is for? > > > > I need to recreate it because the maximum item size has changed, and there is > > currently no functionality to enforce a new container size on the queue. > > > > This cannot be known in advance since the number of channels are only known > > during runtime and can change during reinitialization. Therefore queue > > reallocation may be needed. > > > > I realized that the reinitialization should be conditional though, so I added > > that in the latest update. > > OK. So it isn't just a performance thing; you actually *need* the indirection, > since SwapQueue doesn't support assignment. > > Then I'd argue that SwapQueue should support assignment. Move assignment should > even be more efficient than the indirection you have now. According to discussions in another CL, I found the conclusion to be that until move assignment is allowed, the current implementation simplifies the use of the SwapQueue while having the same complexity as if a copy assignment/construction is implemented. Therefore I keep the current implementation. Please say if you don't agree!
lgtm https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:93: render_queue_buffer_.resize(0); a.k.a. .clear() https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:99: render_queue_buffer_.resize(0); .clear()
LGTM with nits/questions. https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (left): https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:371: Why? https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:117: (void)render_signal_queue_->Insert(&render_queue_buffer_); Drop (void). https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:138: (void)WebRtcAec_BufferFarend(my_handle, Drop (void). https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:388: std::max(1UL, max_frame_size * num_handles_required()); Is 1UL always the same size as size_t, for the platforms we support? Otherwise, consider static_cast<size_t>(1). Or, if it is only the signedness that must be fixed, simply 1u may do the trick. https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:460: Why? https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:122: (void)render_signal_queue_->Insert(&render_queue_buffer_); Drop (void). https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:304: std::max(1UL, max_frame_size * num_handles_required()); Same question again about 1UL. https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:368: Why?
Hang on... I changed my mind. Not LGTM. https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:21: namespace { "Do not use unnamed namespaces in .h files." https://google.github.io/styleguide/cppguide.html#Namespaces https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_control_mobile_impl.h (right): https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.h:21: namespace { "Do not use unnamed namespaces in .h files." https://google.github.io/styleguide/cppguide.html#Namespaces
https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (left): https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:371: On 2015/11/05 12:45:26, hlundin-webrtc wrote: > Why? Done. https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:93: render_queue_buffer_.resize(0); On 2015/11/05 12:20:02, kwiberg-webrtc wrote: > a.k.a. .clear() Done. https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:117: (void)render_signal_queue_->Insert(&render_queue_buffer_); On 2015/11/05 12:45:26, hlundin-webrtc wrote: > Drop (void). I think it is needed, as the Insert is annotated with WARN_UNUSED_RESULT. Or is there another way to work around that? https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:138: (void)WebRtcAec_BufferFarend(my_handle, On 2015/11/05 12:45:26, hlundin-webrtc wrote: > Drop (void). Done. https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:388: std::max(1UL, max_frame_size * num_handles_required()); On 2015/11/05 12:45:26, hlundin-webrtc wrote: > Is 1UL always the same size as size_t, for the platforms we support? Otherwise, > consider static_cast<size_t>(1). Or, if it is only the signedness that must be > fixed, simply 1u may do the trick. Great! I got a trybot fail for this, so I'll also change to use the template version of max. Done. https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:460: On 2015/11/05 12:45:25, hlundin-webrtc wrote: > Why? Done. https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:21: namespace { On 2015/11/05 12:57:02, hlundin-webrtc wrote: > "Do not use unnamed namespaces in .h files." > https://google.github.io/styleguide/cppguide.html#Namespaces Done. https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:99: render_queue_buffer_.resize(0); On 2015/11/05 12:20:03, kwiberg-webrtc wrote: > .clear() Done. https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:122: (void)render_signal_queue_->Insert(&render_queue_buffer_); On 2015/11/05 12:45:26, hlundin-webrtc wrote: > Drop (void). I think it is needed, as the Insert is annotated with WARN_UNUSED_RESULT. Or is there another way to work around that? https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:304: std::max(1UL, max_frame_size * num_handles_required()); On 2015/11/05 12:45:26, hlundin-webrtc wrote: > Same question again about 1UL. Done. https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:368: On 2015/11/05 12:45:26, hlundin-webrtc wrote: > Why? Done. https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_control_mobile_impl.h (right): https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.h:21: namespace { On 2015/11/05 12:57:02, hlundin-webrtc wrote: > "Do not use unnamed namespaces in .h files." > https://google.github.io/styleguide/cppguide.html#Namespaces Done.
LGTM. Consider my comments about nested classes optional for now. https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.cc:117: (void)render_signal_queue_->Insert(&render_queue_buffer_); On 2015/11/06 06:55:04, peah-webrtc wrote: > On 2015/11/05 12:45:26, hlundin-webrtc wrote: > > Drop (void). > > I think it is needed, as the Insert is annotated with WARN_UNUSED_RESULT. Or is > there another way to work around that? Acknowledged. I didn't know you had annotated Insert with WARN_UNUSED_RESULT. https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1410833002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:122: (void)render_signal_queue_->Insert(&render_queue_buffer_); On 2015/11/06 06:55:04, peah-webrtc wrote: > On 2015/11/05 12:45:26, hlundin-webrtc wrote: > > Drop (void). > > I think it is needed, as the Insert is annotated with WARN_UNUSED_RESULT. Or is > there another way to work around that? Acknowledged. https://codereview.webrtc.org/1410833002/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1410833002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:24: // Functor to use when supplying a verifier function for the queue item I'm not sure, but isn't there a risk you'll get multiple definitions of this now? I recall this being duplicated in several .h files, and if they are included in the same place, what happens? If the idea with the unnamed namespace was to hide AecRenderQueueItemVerifier from the outside world, you may want to move that inside EchoCancellationImpl as a nested private class. On the other hand, maybe it is best added as a protected nested class under ProcessingComponent for all to use? Just a thought, if it doesn't work out I'm fine with it. https://codereview.webrtc.org/1410833002/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_control_mobile_impl.h (right): https://codereview.webrtc.org/1410833002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.h:26: class AecmRenderQueueItemVerifier { Oh, they have different names. Then I guess you'll be fine, but you may want to consider moving _one_ into ProcessingComponent for all to use.
https://codereview.webrtc.org/1410833002/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1410833002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_cancellation_impl.h:24: // Functor to use when supplying a verifier function for the queue item On 2015/11/06 07:05:13, hlundin-webrtc wrote: > I'm not sure, but isn't there a risk you'll get multiple definitions of this > now? I recall this being duplicated in several .h files, and if they are > included in the same place, what happens? > > If the idea with the unnamed namespace was to hide AecRenderQueueItemVerifier > from the outside world, you may want to move that inside EchoCancellationImpl as > a nested private class. On the other hand, maybe it is best added as a protected > nested class under ProcessingComponent for all to use? Just a thought, if it > doesn't work out I'm fine with it. Good point! It works as it is, but I instead created a template for this in the ProcessingComponent. Please verify that what I did makes sense. I did not put it as nested beneath processingcomponent though as that would cause that class to be a template. Or is there a better way to do it? https://codereview.webrtc.org/1410833002/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/echo_control_mobile_impl.h (right): https://codereview.webrtc.org/1410833002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/echo_control_mobile_impl.h:26: class AecmRenderQueueItemVerifier { On 2015/11/06 07:05:13, hlundin-webrtc wrote: > Oh, they have different names. Then I guess you'll be fine, but you may want to > consider moving _one_ into ProcessingComponent for all to use. Good point! It works as it is, but I instead created a template for this in the ProcessingComponent. Please verify that what I did makes sense.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1410833002/#ps240001 (title: "Fixed swapqueue usage that changed in the merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410833002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410833002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...) android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/8918)
In order to properly handle a full queue, I needed to backport the proper handling of that from a CL further up in the CL chain for this work. The change is small but it has not yet been reviewed in the upcoming CL (the source of the backported code), and it would be awesome if you could have a quick look to see that it makes sense.
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/1410833002/#ps280001 (title: "Merge with latest master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410833002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410833002/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/fa6228e221d818af55e3d8343c792f2c1ecc7252 Cr-Commit-Position: refs/heads/master@{#10662} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
