Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1416583003:
Lock scheme #5: Applied the render queueing to the agc (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 Base URL:
https://chromium.googlesource.com/external/webrtc.git@introduce_queue_CL Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionApplied the render queueing to the agc.
BUG=webrtc:5099
Committed: https://crrev.com/4d291f7d5e088e3e9f4680dabbe431e62b827f64
Cr-Commit-Position: refs/heads/master@{#10667}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Corrected the sample format when storing the render samples" #Patch Set 3 : Corrected the expected number of samples for the AGC render side sample queue. #Patch Set 4 : Changed the render side sample queueing scheme to be atomic over all channels for each frame #Patch Set 5 : Merge #
Total comments: 4
Patch Set 6 : Replicated what was done for the usage of the render queue in the aec and aecm #Patch Set 7 : Merge from other Cls in the list #
Total comments: 20
Patch Set 8 : Changes in response to latest reviewer comments #
Total comments: 2
Patch Set 9 : Replaced the AGC specific render queue verifier with the generic template #Patch Set 10 : Backported proper handling of full render queue and fixed merge build error #Patch Set 11 : Merge from latest master #
Total comments: 12
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 28 (8 generated)
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org
https://codereview.webrtc.org/1416583003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1416583003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/gain_control_impl.cc:81: // once the APM properly checks the sample type passed the AGC. // TODO: Remove copying. https://codereview.webrtc.org/1416583003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/gain_control_impl.h (right): https://codereview.webrtc.org/1416583003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/gain_control_impl.h:96: rtc::scoped_ptr<SwapQueue<std::vector<int16_t>, &RenderQueueItemVerifier>> Don't need the scoped_ptr.
andrew@webrtc.org changed reviewers: + andrew@webrtc.org
Drive-by comment. https://codereview.webrtc.org/1416583003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/agc/legacy/analog_agc.c (right): https://codereview.webrtc.org/1416583003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/agc/legacy/analog_agc.c:264: int WebRtcAgc_GetAddFarendError(void *state, size_t samples) { Why bother with this? I'd resist changing this highly legacy code as much as possible :)
Fixing the sample format when storing the render side samples. Will address the feedback in the next patch.
Description was changed from ========== Applied the render queueing to the agc. BUG= ========== to ========== Applied the render queueing to the agc. BUG= ==========
Description was changed from ========== Applied the render queueing to the agc. BUG= ========== to ========== Applied the render queueing to the agc. BUG=webrtc:5099 ==========
https://codereview.webrtc.org/1416583003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/agc/legacy/analog_agc.c (right): https://codereview.webrtc.org/1416583003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/agc/legacy/analog_agc.c:264: int WebRtcAgc_GetAddFarendError(void *state, size_t samples) { On 2015/10/19 16:45:38, Andrew MacDonald wrote: > Why bother with this? I'd resist changing this highly legacy code as much as > possible :) The reason is that we report the error message returned from the function, and if we don't change this, the error message will be returned on the near-end instead of on the farend. Since I don't fully understand the error message scheme yet, and don't want to refactor it now, I changed this in order to fully behave as it did before the introduction of the render sample queue. https://codereview.webrtc.org/1416583003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1416583003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/gain_control_impl.cc:81: // once the APM properly checks the sample type passed the AGC. On 2015/10/19 14:47:04, the sun wrote: > // TODO: Remove copying. This has now been refactored. Please recheck! https://codereview.webrtc.org/1416583003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/gain_control_impl.h (right): https://codereview.webrtc.org/1416583003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/gain_control_impl.h:96: rtc::scoped_ptr<SwapQueue<std::vector<int16_t>, &RenderQueueItemVerifier>> On 2015/10/19 14:47:04, the sun wrote: > Don't need the scoped_ptr. That I don't see. We need to recreate the queue in runtime, and the fastest way should be to create a new queue. How should we otherwise recreate the queue?
https://codereview.webrtc.org/1416583003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/agc/legacy/analog_agc.c (right): https://codereview.webrtc.org/1416583003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/agc/legacy/analog_agc.c:264: int WebRtcAgc_GetAddFarendError(void *state, size_t samples) { On 2015/10/26 09:09:56, peah-webrtc wrote: > On 2015/10/19 16:45:38, Andrew MacDonald wrote: > > Why bother with this? I'd resist changing this highly legacy code as much as > > possible :) > > The reason is that we report the error message returned from the function, and > if we don't change this, the error message will be returned on the near-end > instead of on the farend. Since I don't fully understand the error message > scheme yet, and don't want to refactor it now, I changed this in order to fully > behave as it did before the introduction of the render sample queue. Ah OK, thanks.
https://codereview.webrtc.org/1416583003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1416583003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:96: void GainControlImpl::ReadQueuedRenderData() { Why do you need this extra function? Why not pop the queue directly in ProcessRenderAudio(), like before? https://codereview.webrtc.org/1416583003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.h (right): https://codereview.webrtc.org/1416583003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.h:33: return (((v.size() == allowed_size_1_) || (v.size() == allowed_size_2_)) && Like said in another review: you only need to assert the capacity, not the size. Besides, do you really need the two different sizes? They don't mean much unless matched to the sample rate. You could just verify each vector has capacity for the larger buffers.
https://codereview.webrtc.org/1416583003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/agc/legacy/analog_agc.c (right): https://codereview.webrtc.org/1416583003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/agc/legacy/analog_agc.c:264: int WebRtcAgc_GetAddFarendError(void *state, size_t samples) { On 2015/10/27 00:21:42, Andrew MacDonald wrote: > On 2015/10/26 09:09:56, peah-webrtc wrote: > > On 2015/10/19 16:45:38, Andrew MacDonald wrote: > > > Why bother with this? I'd resist changing this highly legacy code as much as > > > possible :) > > > > The reason is that we report the error message returned from the function, and > > if we don't change this, the error message will be returned on the near-end > > instead of on the farend. Since I don't fully understand the error message > > scheme yet, and don't want to refactor it now, I changed this in order to > fully > > behave as it did before the introduction of the render sample queue. > > Ah OK, thanks. Acknowledged. https://codereview.webrtc.org/1416583003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1416583003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.cc:96: void GainControlImpl::ReadQueuedRenderData() { On 2015/10/27 16:42:49, the sun wrote: > Why do you need this extra function? Why not pop the queue directly in > ProcessRenderAudio(), like before? Not fully sure what you mean? Do you mean that it should be done on AnalyzeCaptureAudio? In that case, it could be done on at the beginning of the call to AnalyzeCaptureAudio but the problem then is that potentially something else in APM has been done that relies on the agc render audio. Therefore, until we fully know the scheme for how APM operates I figured it was safest to empty the queue as early as possible on a capture-thread call, as this is the closest to how APM operated before this. https://codereview.webrtc.org/1416583003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/gain_control_impl.h (right): https://codereview.webrtc.org/1416583003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/gain_control_impl.h:33: return (((v.size() == allowed_size_1_) || (v.size() == allowed_size_2_)) && On 2015/10/27 16:42:49, the sun wrote: > Like said in another review: you only need to assert the capacity, not the size. > > Besides, do you really need the two different sizes? They don't mean much unless > matched to the sample rate. You could just verify each vector has capacity for > the larger buffers. Good find! This has now been refactored. Done.
henrik.lundin@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
See comments. https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc/legacy/analog_agc.c (right): https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc/legacy/analog_agc.c:264: int WebRtcAgc_GetAddFarendError(void *state, size_t samples) { Take a LegacyAgc* as input argument instead, and skip the casting below. https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc/legacy/gain_control.h (right): https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc/legacy/gain_control.h:57: * - agcInst : AGC instance. End both lines with our without . for consistency. https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc/legacy/gain_control.h:61: * : 0 - Normal operation. Again, consistency. https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:85: (void)render_signal_queue_->Insert(&render_queue_buffer_); Drop (void). https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:103: (void)WebRtcAgc_AddFarend(my_handle, &capture_queue_buffer_[buffer_index], Drop (void). https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:216: // TODO(peah): Verify that this is really needed to do the reading. Extra . at end of line. https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:350: std::max(1UL, (max_frame_size * num_handles())); 1UL... https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/gain_control_impl.h (right): https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.h:23: namespace { "Do not use unnamed namespaces in .h files." https://google.github.io/styleguide/cppguide.html#Namespaces
https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc/legacy/analog_agc.c (right): https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc/legacy/analog_agc.c:264: int WebRtcAgc_GetAddFarendError(void *state, size_t samples) { On 2015/11/05 13:01:09, hlundin-webrtc wrote: > Take a LegacyAgc* as input argument instead, and skip the casting below. That would be better, but since LegacyAgc is not available in gain_control.h, it is not possible as this function is declared in gain_control.h. This is not really needed though, since we don't really need to hide the state, but I'd prefer to keep it as it is as it is in line with the rest of the code in this file. Is that ok? https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc/legacy/gain_control.h (right): https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc/legacy/gain_control.h:57: * - agcInst : AGC instance. On 2015/11/05 13:01:09, hlundin-webrtc wrote: > End both lines with our without . for consistency. Done. https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc/legacy/gain_control.h:61: * : 0 - Normal operation. On 2015/11/05 13:01:09, hlundin-webrtc wrote: > Again, consistency. Done. https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:85: (void)render_signal_queue_->Insert(&render_queue_buffer_); On 2015/11/05 13:01:09, hlundin-webrtc wrote: > Drop (void). I think is needed due to the WARN_UNUSED_RESULT annotation in the Insert method. I added the comment to motivate this and it will be fixed in a later CL in the CL chain for this work. https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:103: (void)WebRtcAgc_AddFarend(my_handle, &capture_queue_buffer_[buffer_index], On 2015/11/05 13:01:09, hlundin-webrtc wrote: > Drop (void). Done. https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:216: // TODO(peah): Verify that this is really needed to do the reading. On 2015/11/05 13:01:09, hlundin-webrtc wrote: > Extra . at end of line. Done. https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:350: std::max(1UL, (max_frame_size * num_handles())); On 2015/11/05 13:01:09, hlundin-webrtc wrote: > 1UL... Done. https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/gain_control_impl.h (right): https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.h:23: namespace { On 2015/11/05 13:01:09, hlundin-webrtc wrote: > "Do not use unnamed namespaces in .h files." > https://google.github.io/styleguide/cppguide.html#Namespaces Done.
lgtm https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc/legacy/analog_agc.c (right): https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc/legacy/analog_agc.c:264: int WebRtcAgc_GetAddFarendError(void *state, size_t samples) { On 2015/11/06 07:10:58, peah-webrtc wrote: > On 2015/11/05 13:01:09, hlundin-webrtc wrote: > > Take a LegacyAgc* as input argument instead, and skip the casting below. > > That would be better, but since LegacyAgc is not available in gain_control.h, it > is not possible as this > function is declared in gain_control.h. > > This is not really needed though, since we don't really need to hide the state, > but I'd prefer to keep it as it is as it is in line with the rest of the code in > this file. > > Is that ok? Acknowledged. https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:85: (void)render_signal_queue_->Insert(&render_queue_buffer_); On 2015/11/06 07:10:58, peah-webrtc wrote: > On 2015/11/05 13:01:09, hlundin-webrtc wrote: > > Drop (void). > > I think is needed due to the WARN_UNUSED_RESULT annotation in the Insert method. > I added the comment to motivate this and it will be fixed in a later CL in the > CL chain for this work. Acknowledged. https://codereview.webrtc.org/1416583003/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1416583003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:341: const int n = num_handles(); (Due to rebase.)
https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc/legacy/analog_agc.c (right): https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc/legacy/analog_agc.c:264: int WebRtcAgc_GetAddFarendError(void *state, size_t samples) { On 2015/11/06 07:52:33, hlundin-webrtc wrote: > On 2015/11/06 07:10:58, peah-webrtc wrote: > > On 2015/11/05 13:01:09, hlundin-webrtc wrote: > > > Take a LegacyAgc* as input argument instead, and skip the casting below. > > > > That would be better, but since LegacyAgc is not available in gain_control.h, > it > > is not possible as this > > function is declared in gain_control.h. > > > > This is not really needed though, since we don't really need to hide the > state, > > but I'd prefer to keep it as it is as it is in line with the rest of the code > in > > this file. > > > > Is that ok? > > Acknowledged. Acknowledged. https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1416583003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:85: (void)render_signal_queue_->Insert(&render_queue_buffer_); On 2015/11/06 07:52:33, hlundin-webrtc wrote: > On 2015/11/06 07:10:58, peah-webrtc wrote: > > On 2015/11/05 13:01:09, hlundin-webrtc wrote: > > > Drop (void). > > > > I think is needed due to the WARN_UNUSED_RESULT annotation in the Insert > method. > > I added the comment to motivate this and it will be fixed in a later CL in the > > CL chain for this work. > > Acknowledged. Acknowledged. https://codereview.webrtc.org/1416583003/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1416583003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:341: const int n = num_handles(); On 2015/11/06 07:52:33, hlundin-webrtc wrote: > (Due to rebase.) Yep, that the cause.
Changed the agc specific render queue verifier to instead use the generic template created in the CL preceeding this in the CL chain.
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. I have a previous lgtm from Henrik, but even though the change is small, it has not yet been reviewed in the upcoming CL (the source of the backported code), so it would be awesome if also Henrik could have a quick look to see that it makes sense.
Still 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 Link to the patchset: https://codereview.webrtc.org/1416583003/#ps200001 (title: "Merge from latest master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1416583003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1416583003/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/4d291f7d5e088e3e9f4680dabbe431e62b827f64 Cr-Commit-Position: refs/heads/master@{#10667}
Message was sent while issue was closed.
solenberg@google.com changed reviewers: + solenberg@google.com
Message was sent while issue was closed.
https://codereview.webrtc.org/1416583003/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1416583003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:38: const size_t GainControlImpl::kAllowedValuesOfSamplesPerFrame1; Why did you need to declare these in the .cc but not kMaxNumFramesToBuffer ? https://codereview.webrtc.org/1416583003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:56: AllocateRenderQueue(); Isn't Initialize() always called for processing components? Why do you specifically need to allocate the queue here? https://codereview.webrtc.org/1416583003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:224: ReadQueuedRenderData(); You may want to add a comment here as to why you're doing this. It seems to me like a very strange side effect of a function that looks like a simple setter. https://codereview.webrtc.org/1416583003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:352: const size_t max_frame_size = std::max<size_t>( Template type should be deduced by the compiler. No need to write <size_t> https://codereview.webrtc.org/1416583003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:355: const size_t new_render_queue_element_max_size = std::max<size_t>( is this the queue size, or size of an element in the queue? https://codereview.webrtc.org/1416583003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:359: std::vector<int16_t> template_queue_element(render_queue_element_max_size_); you're using what looks like the old item size, and never set that to the new calculated size. so you will always go this branch?
Message was sent while issue was closed.
Thanks for the comments!!!! I'll upload a fix for the problems in a separate CL. Sorry for misunderstanding that caused me to commit the CL before this review!!!! https://codereview.webrtc.org/1416583003/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/gain_control_impl.cc (right): https://codereview.webrtc.org/1416583003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:38: const size_t GainControlImpl::kAllowedValuesOfSamplesPerFrame1; On 2015/11/17 09:34:13, solenberg wrote: > Why did you need to declare these in the .cc but not kMaxNumFramesToBuffer ? (I think) This is needed because the constants are used in a call to a function where the inputs are reference declared, therefore these needs to be declared in the .cc. That is not the case for the kMaxNumFramesToBuffer constant. I order to avoid this inconsistency, I now instead static_cast the constants before applying them which apparently is a workaround for this issue: const size_t max_frame_size = std::max<size_t>( static_cast<size_t>(kAllowedValuesOfSamplesPerFrame1), static_cast<size_t>(kAllowedValuesOfSamplesPerFrame2)); Then I can remove these declarations. Please let me know if you think it is ok! Done. https://codereview.webrtc.org/1416583003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:56: AllocateRenderQueue(); On 2015/11/17 09:34:13, solenberg wrote: > Isn't Initialize() always called for processing components? Why do you > specifically need to allocate the queue here? Good point! Done. https://codereview.webrtc.org/1416583003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:224: ReadQueuedRenderData(); On 2015/11/17 09:34:13, solenberg wrote: > You may want to add a comment here as to why you're doing this. It seems to me > like a very strange side effect of a function that looks like a simple setter. Removed it. Done. https://codereview.webrtc.org/1416583003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:352: const size_t max_frame_size = std::max<size_t>( On 2015/11/17 09:34:13, solenberg wrote: > Template type should be deduced by the compiler. No need to write <size_t> Done. https://codereview.webrtc.org/1416583003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:355: const size_t new_render_queue_element_max_size = std::max<size_t>( On 2015/11/17 09:34:12, solenberg wrote: > is this the queue size, or size of an element in the queue? This is the size of an element in the queue. The queue size is specified by kMaxNumFramesToBuffer https://codereview.webrtc.org/1416583003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/gain_control_impl.cc:359: std::vector<int16_t> template_queue_element(render_queue_element_max_size_); On 2015/11/17 09:34:13, solenberg wrote: > you're using what looks like the old item size, and never set that to the new > calculated size. so you will always go this branch? Fully true, and it worked since the verification broke due to this bug, which caused all items to be reallocated until the queue item size was achieved. Good find! Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
