Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2304123002:
Cleaned up and revised the handling of resampling and bandsplitting in APM and (Closed)
|
Created:
4 years, 3 months ago by peah-webrtc Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, kwiberg-webrtc, minyue-webrtc, aluebs-webrtc, bjornv1, henrika_webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionThe audio processing module (APM) relies on two for
functionalities doing sample-rate conversions:
-The implicit resampling done in the AudioBuffer CopyTo,
CopyFrom, InterleaveTo and DeinterleaveFrom methods.
-The multi-band splitting scheme.
The selection of rates in these have been difficult and
complicated, partly due to that the APM API which allows
for activating the APM submodules without notifying
the APM.
This CL adds functionality that for each capture frame
polls all submodules for whether they are active or not
and compares this against a cached result.
Furthermore, new functionality is added that based on the
results of the comparison do a reinitialization of the APM.
This has several advantages
-The code deciding on whether to analysis and synthesis is
needed for the bandsplitting can be much simplified and
centralized.
-The selection of the processing rate can be done such as
to avoid the implicit resampling that was in some cases
unnecessarily done.
-The optimization for whether an output copy is needed
that was done to improve performance due to the implicit
resampling is no longer needed, which simplifies the
code and makes it less error-prone in the sense that
is no longer neccessary to keep track of whether any
module has changed the signal.
Finally, it should be noted that the polling of the state
for all the submodules was done previously as well, but in
a less obvious and distributed manner.
BUG=webrtc:6181, webrtc:6220, webrtc:5298, webrtc:6296, webrtc:6298, webrtc:6297
Committed: https://crrev.com/2ace3f94064e4d1980483d9ea3b0da8537aee540
Cr-Commit-Position: refs/heads/master@{#14175}
Patch Set 1 #Patch Set 2 : Typo correction #
Total comments: 20
Patch Set 3 : Changes in response to reviewer comments #
Total comments: 21
Patch Set 4 : Changes in response to reviewer comments #Patch Set 5 : Re-added conditional copy to the output buffer as not doing it caused implicit downmixing #Patch Set 6 : Created new test reference binaries for the unit tests in audio_processing_unittest.cc #Patch Set 7 : Avoided compiler warning by adding unreachable return statement #Patch Set 8 : Updated the test that required that no initialization should be done if APM is called with the defa… #Messages
Total messages: 44 (19 generated)
Description was changed from ========== Cleaned up the handling of resampling and bandsplitting in APM Cleaned up the handling of resampling and bandsplitting in APM BUG= ========== to ========== The audio processing module (APM) relies on two for unctionalities doing sample-rate conversions: -The implicit resampling done in the AudioBuffer CopyTo, CopyFrom, InterleaveTo and DeinterleaveFrom methods. -The multi-band splitting scheme. The selection of rates in these have been difficult and complicated, partly due to that the APM API which allows for activating the APM submodules without notifying the APM. This CL adds functionality that for each capture frame polls all submodules for whether they are active or not and compares this against a cached result. Furthermore, new functionality is added that based on the results of the comparison do a reinitialization of the APM. This has several advantages -The code deciding on whether to analysis and synthesis is needed for the bandsplitting can be much simplified and centralized. -The selection of the processing rate can be done such as to avoid the implicit resampling that was in some cases unnecessarily done. -The optimization for whether an output copy is needed that was done to improve performance due to the implicit resampling is no longer needed, which simplifies the code and makes it less error-prone in the sense that is no longer neccessary to keep track of whether any module has changed the signal. Finally, it should be noted that the polling of the state for all the submodules was done previously as well, but in a less obvious and distributed manner. BUG=webrtc:6181, webrtc:6220, webrtc:5298, webrtc:6296, webrtc:6298, webrtc:6297 ==========
Description was changed from ========== The audio processing module (APM) relies on two for unctionalities doing sample-rate conversions: -The implicit resampling done in the AudioBuffer CopyTo, CopyFrom, InterleaveTo and DeinterleaveFrom methods. -The multi-band splitting scheme. The selection of rates in these have been difficult and complicated, partly due to that the APM API which allows for activating the APM submodules without notifying the APM. This CL adds functionality that for each capture frame polls all submodules for whether they are active or not and compares this against a cached result. Furthermore, new functionality is added that based on the results of the comparison do a reinitialization of the APM. This has several advantages -The code deciding on whether to analysis and synthesis is needed for the bandsplitting can be much simplified and centralized. -The selection of the processing rate can be done such as to avoid the implicit resampling that was in some cases unnecessarily done. -The optimization for whether an output copy is needed that was done to improve performance due to the implicit resampling is no longer needed, which simplifies the code and makes it less error-prone in the sense that is no longer neccessary to keep track of whether any module has changed the signal. Finally, it should be noted that the polling of the state for all the submodules was done previously as well, but in a less obvious and distributed manner. BUG=webrtc:6181, webrtc:6220, webrtc:5298, webrtc:6296, webrtc:6298, webrtc:6297 ========== to ========== The audio processing module (APM) relies on two for unctionalities doing sample-rate conversions: -The implicit resampling done in the AudioBuffer CopyTo, CopyFrom, InterleaveTo and DeinterleaveFrom methods. -The multi-band splitting scheme. The selection of rates in these have been difficult and complicated, partly due to that the APM API which allows for activating the APM submodules without notifying the APM. This CL adds functionality that for each capture frame polls all submodules for whether they are active or not and compares this against a cached result. Furthermore, new functionality is added that based on the results of the comparison do a reinitialization of the APM. This has several advantages -The code deciding on whether to analysis and synthesis is needed for the bandsplitting can be much simplified and centralized. -The selection of the processing rate can be done such as to avoid the implicit resampling that was in some cases unnecessarily done. -The optimization for whether an output copy is needed that was done to improve performance due to the implicit resampling is no longer needed, which simplifies the code and makes it less error-prone in the sense that is no longer neccessary to keep track of whether any module has changed the signal. Finally, it should be noted that the polling of the state for all the submodules was done previously as well, but in a less obvious and distributed manner. BUG=webrtc:6181, webrtc:6220, webrtc:5298, webrtc:6296, webrtc:6298, webrtc:6297 ==========
Description was changed from ========== The audio processing module (APM) relies on two for unctionalities doing sample-rate conversions: -The implicit resampling done in the AudioBuffer CopyTo, CopyFrom, InterleaveTo and DeinterleaveFrom methods. -The multi-band splitting scheme. The selection of rates in these have been difficult and complicated, partly due to that the APM API which allows for activating the APM submodules without notifying the APM. This CL adds functionality that for each capture frame polls all submodules for whether they are active or not and compares this against a cached result. Furthermore, new functionality is added that based on the results of the comparison do a reinitialization of the APM. This has several advantages -The code deciding on whether to analysis and synthesis is needed for the bandsplitting can be much simplified and centralized. -The selection of the processing rate can be done such as to avoid the implicit resampling that was in some cases unnecessarily done. -The optimization for whether an output copy is needed that was done to improve performance due to the implicit resampling is no longer needed, which simplifies the code and makes it less error-prone in the sense that is no longer neccessary to keep track of whether any module has changed the signal. Finally, it should be noted that the polling of the state for all the submodules was done previously as well, but in a less obvious and distributed manner. BUG=webrtc:6181, webrtc:6220, webrtc:5298, webrtc:6296, webrtc:6298, webrtc:6297 ========== to ========== The audio processing module (APM) relies on two for functionalities doing sample-rate conversions: -The implicit resampling done in the AudioBuffer CopyTo, CopyFrom, InterleaveTo and DeinterleaveFrom methods. -The multi-band splitting scheme. The selection of rates in these have been difficult and complicated, partly due to that the APM API which allows for activating the APM submodules without notifying the APM. This CL adds functionality that for each capture frame polls all submodules for whether they are active or not and compares this against a cached result. Furthermore, new functionality is added that based on the results of the comparison do a reinitialization of the APM. This has several advantages -The code deciding on whether to analysis and synthesis is needed for the bandsplitting can be much simplified and centralized. -The selection of the processing rate can be done such as to avoid the implicit resampling that was in some cases unnecessarily done. -The optimization for whether an output copy is needed that was done to improve performance due to the implicit resampling is no longer needed, which simplifies the code and makes it less error-prone in the sense that is no longer neccessary to keep track of whether any module has changed the signal. Finally, it should be noted that the polling of the state for all the submodules was done previously as well, but in a less obvious and distributed manner. BUG=webrtc:6181, webrtc:6220, webrtc:5298, webrtc:6296, webrtc:6298, webrtc:6297 ==========
Description was changed from ========== The audio processing module (APM) relies on two for functionalities doing sample-rate conversions: -The implicit resampling done in the AudioBuffer CopyTo, CopyFrom, InterleaveTo and DeinterleaveFrom methods. -The multi-band splitting scheme. The selection of rates in these have been difficult and complicated, partly due to that the APM API which allows for activating the APM submodules without notifying the APM. This CL adds functionality that for each capture frame polls all submodules for whether they are active or not and compares this against a cached result. Furthermore, new functionality is added that based on the results of the comparison do a reinitialization of the APM. This has several advantages -The code deciding on whether to analysis and synthesis is needed for the bandsplitting can be much simplified and centralized. -The selection of the processing rate can be done such as to avoid the implicit resampling that was in some cases unnecessarily done. -The optimization for whether an output copy is needed that was done to improve performance due to the implicit resampling is no longer needed, which simplifies the code and makes it less error-prone in the sense that is no longer neccessary to keep track of whether any module has changed the signal. Finally, it should be noted that the polling of the state for all the submodules was done previously as well, but in a less obvious and distributed manner. BUG=webrtc:6181, webrtc:6220, webrtc:5298, webrtc:6296, webrtc:6298, webrtc:6297 ========== to ========== The audio processing module (APM) relies on two for functionalities doing sample-rate conversions: -The implicit resampling done in the AudioBuffer CopyTo, CopyFrom, InterleaveTo and DeinterleaveFrom methods. -The multi-band splitting scheme. The selection of rates in these have been difficult and complicated, partly due to that the APM API which allows for activating the APM submodules without notifying the APM. This CL adds functionality that for each capture frame polls all submodules for whether they are active or not and compares this against a cached result. Furthermore, new functionality is added that based on the results of the comparison do a reinitialization of the APM. This has several advantages -The code deciding on whether to analysis and synthesis is needed for the bandsplitting can be much simplified and centralized. -The selection of the processing rate can be done such as to avoid the implicit resampling that was in some cases unnecessarily done. -The optimization for whether an output copy is needed that was done to improve performance due to the implicit resampling is no longer needed, which simplifies the code and makes it less error-prone in the sense that is no longer neccessary to keep track of whether any module has changed the signal. Finally, it should be noted that the polling of the state for all the submodules was done previously as well, but in a less obvious and distributed manner. BUG=webrtc:6181, webrtc:6220, webrtc:5298, webrtc:6296, webrtc:6298, webrtc:6297 ==========
Description was changed from ========== The audio processing module (APM) relies on two for functionalities doing sample-rate conversions: -The implicit resampling done in the AudioBuffer CopyTo, CopyFrom, InterleaveTo and DeinterleaveFrom methods. -The multi-band splitting scheme. The selection of rates in these have been difficult and complicated, partly due to that the APM API which allows for activating the APM submodules without notifying the APM. This CL adds functionality that for each capture frame polls all submodules for whether they are active or not and compares this against a cached result. Furthermore, new functionality is added that based on the results of the comparison do a reinitialization of the APM. This has several advantages -The code deciding on whether to analysis and synthesis is needed for the bandsplitting can be much simplified and centralized. -The selection of the processing rate can be done such as to avoid the implicit resampling that was in some cases unnecessarily done. -The optimization for whether an output copy is needed that was done to improve performance due to the implicit resampling is no longer needed, which simplifies the code and makes it less error-prone in the sense that is no longer neccessary to keep track of whether any module has changed the signal. Finally, it should be noted that the polling of the state for all the submodules was done previously as well, but in a less obvious and distributed manner. BUG=webrtc:6181, webrtc:6220, webrtc:5298, webrtc:6296, webrtc:6298, webrtc:6297 ========== to ========== The audio processing module (APM) relies on two for functionalities doing sample-rate conversions: -The implicit resampling done in the AudioBuffer CopyTo, CopyFrom, InterleaveTo and DeinterleaveFrom methods. -The multi-band splitting scheme. The selection of rates in these have been difficult and complicated, partly due to that the APM API which allows for activating the APM submodules without notifying the APM. This CL adds functionality that for each capture frame polls all submodules for whether they are active or not and compares this against a cached result. Furthermore, new functionality is added that based on the results of the comparison do a reinitialization of the APM. This has several advantages -The code deciding on whether to analysis and synthesis is needed for the bandsplitting can be much simplified and centralized. -The selection of the processing rate can be done such as to avoid the implicit resampling that was in some cases unnecessarily done. -The optimization for whether an output copy is needed that was done to improve performance due to the implicit resampling is no longer needed, which simplifies the code and makes it less error-prone in the sense that is no longer neccessary to keep track of whether any module has changed the signal. Finally, it should be noted that the polling of the state for all the submodules was done previously as well, but in a less obvious and distributed manner. BUG=webrtc:6181, webrtc:6220, webrtc:5298, webrtc:6296, webrtc:6298, webrtc:6297 ==========
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, solenberg@webrtc.org
This CL needs more testing before landing, but I'm sending out to you so that you can give your opinion about it.
https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:143: bool ts_enabled) { What's "ts"? (I could look it up, but I pretend to be a casual reader of the code.) Generally the style guide advises against heavy use of abbreviations. Agree we have a few that are common in this field, but looking at this list I think we've crossed a border in terms of obscurity. :/
On 2016/09/02 20:11:34, the sun wrote: > https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:143: bool ts_enabled) { > What's "ts"? (I could look it up, but I pretend to be a casual reader of the > code.) > > Generally the style guide advises against heavy use of abbreviations. Agree we > have a few that are common in this field, but looking at this list I think we've > crossed a border in terms of obscurity. :/ (I promise to take a closer look later, btw.)
On 2016/09/02 20:12:15, the sun wrote: > On 2016/09/02 20:11:34, the sun wrote: > > > https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... > > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > > > > https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/audio_processing_impl.cc:143: bool ts_enabled) > { > > What's "ts"? (I could look it up, but I pretend to be a casual reader of the > > code.) > > > > Generally the style guide advises against heavy use of abbreviations. Agree we > > have a few that are common in this field, but looking at this list I think > we've > > crossed a border in terms of obscurity. :/ > > (I promise to take a closer look later, btw.) You may want to proof read the commit message one more time...
Nice. See comments inline. https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:110: RTC_DCHECK_LT(kMaxSplittingNativeProcessRateIndex, You should be able to use a static_assert instead. https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:115: for (size_t k = 0; k <= uppermost_native_rate_index; ++k) { Consider this alternative implementation of the function, using the frequencies directly instead of the indices. Not saying that it is very much better, but think about it... #ifdef WEBRTC_ARCH_ARM_FAMILY const int kMaxSplittingNativeProcessRate = AudioProcessing::kSampleRate32kHz; #else const int kMaxSplittingNativeProcessRate = AudioProcessing::kSampleRate48kHz; #endif static_assert(kMaxSplittingNativeProcessRate <= AudioProcessing::kMaxNativeSampleRateHz, "Stupid error string that will no longer be needed in C++17"); const int uppermost_native_rate = band_splitting_required ? kMaxSplittingNativeProcessRate : AudioProcessing::kSampleRate48kHz; for (auto rate : AudioProcessing::kNativeSampleRatesHz) { if (rate >= uppermost_native_rate) { return uppermost_native_rate; } if (rate >= minimum_rate) { return rate; } } RTC_NOTREACHED(); https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:180: return (hpf_enabled_ || aec_enabled_ || aecm_enabled_ || ns_enabled_ || You can drop the () here too. https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:650: bool reeinitialization_required; Typo: reeinit... -> reinit... https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:743: bool reeinitialization_required; Same typo. https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:130: class ApmSubmoduleStates; I'm not fond of forward-declaring private subclasses. Make a proper declaration here instead, and don't use an std::unique_ptr for the member variable below. https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:250: std::unique_ptr<ApmSubmoduleStates> submodule_states_; As said above, don't use std::unique_ptr. Let it be a plain member instead, since you are not using any features of the unique_ptr anyway (it is populated only once, in the ctor, and is never deleted before the dtor).
Patchset #3 (id:40001) has been deleted
https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:110: RTC_DCHECK_LT(kMaxSplittingNativeProcessRateIndex, On 2016/09/06 10:41:32, hlundin-webrtc wrote: > You should be able to use a static_assert instead. Great suggestion but I ran into some problems. See below. https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:115: for (size_t k = 0; k <= uppermost_native_rate_index; ++k) { On 2016/09/06 10:41:32, hlundin-webrtc wrote: > Consider this alternative implementation of the function, using the frequencies > directly instead of the indices. Not saying that it is very much better, but > think about it... > > #ifdef WEBRTC_ARCH_ARM_FAMILY > const int kMaxSplittingNativeProcessRate = AudioProcessing::kSampleRate32kHz; > #else > const int kMaxSplittingNativeProcessRate = AudioProcessing::kSampleRate48kHz; > #endif > static_assert(kMaxSplittingNativeProcessRate <= > AudioProcessing::kMaxNativeSampleRateHz, "Stupid error string that will no > longer be needed in C++17"); > > const int uppermost_native_rate = band_splitting_required ? > kMaxSplittingNativeProcessRate : AudioProcessing::kSampleRate48kHz; > > for (auto rate : AudioProcessing::kNativeSampleRatesHz) { > if (rate >= uppermost_native_rate) { > return uppermost_native_rate; > } > if (rate >= minimum_rate) { > return rate; > } > } > RTC_NOTREACHED(); This is a much nicer variant! Thanks!!! I could not, however, get the static assertion to work and get the error "static_assert expression is not an integral constant expression". Somehow it is caused by AudioProcessing::kMaxNativeSampleRateHz. The initialization of that is a bit intricate. I tried to avoid the issue using constexpr but did not manage that. Is it ok to keep iot as a DCHECK? https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:143: bool ts_enabled) { On 2016/09/02 20:11:34, the sun wrote: > What's "ts"? (I could look it up, but I pretend to be a casual reader of the > code.) > > Generally the style guide advises against heavy use of abbreviations. Agree we > have a few that are common in this field, but looking at this list I think we've > crossed a border in terms of obscurity. :/ ts is the transient suppressor. The abbreviations point is great! I'll remove/reduce those as much as possible. Done. https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:180: return (hpf_enabled_ || aec_enabled_ || aecm_enabled_ || ns_enabled_ || On 2016/09/06 10:41:32, hlundin-webrtc wrote: > You can drop the () here too. Done. https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:650: bool reeinitialization_required; On 2016/09/06 10:41:32, hlundin-webrtc wrote: > Typo: reeinit... -> reinit... Done. https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:743: bool reeinitialization_required; On 2016/09/06 10:41:32, hlundin-webrtc wrote: > Same typo. Done. https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:130: class ApmSubmoduleStates; On 2016/09/06 10:41:32, hlundin-webrtc wrote: > I'm not fond of forward-declaring private subclasses. Make a proper declaration > here instead, and don't use an std::unique_ptr for the member variable below. Done. https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:250: std::unique_ptr<ApmSubmoduleStates> submodule_states_; On 2016/09/06 10:41:32, hlundin-webrtc wrote: > As said above, don't use std::unique_ptr. Let it be a plain member instead, > since you are not using any features of the unique_ptr anyway (it is populated > only once, in the ctor, and is never deleted before the dtor). Sounds good! Done.
lgtm https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:110: RTC_DCHECK_LT(kMaxSplittingNativeProcessRateIndex, On 2016/09/07 06:28:11, peah-webrtc wrote: > On 2016/09/06 10:41:32, hlundin-webrtc wrote: > > You should be able to use a static_assert instead. > > Great suggestion but I ran into some problems. See below. Weird. I thought it'd work if you used constexpr in enough places. Never mind that; keep the DCHECK.
I think this is an improvement, but did you consider changing the API instead to not allow poking at the individual module states (setting everything up via AudioProcessing::Config)? https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (left): https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1215: return capture_nonlocked_.intelligibility_enabled; This was previously if-deffed but now is not - intentional? https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:104: int NativeProcessRateToUse(int minimum_rate, bool band_splitting_required) { FindNativeProcessRateToUse() also, this is the type of function which it would be very easy to write a test for... https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:181: bool AudioProcessingImpl::ApmSubmoduleStates::CaptureMultiBandModulesActive() Is this distinction between "modules" and "effects" widely known and agreed upon terminology? Feels a bit ad hoc to me. I guess the difference is that an "effect" will change the signal? Can we make it clearer? At least add comments in the .h. https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:481: int fwd_proc_rate = NativeProcessRateToUse( I find it confusing that "fwd" and "rev" are used sometimes, and "Capture" and "Render" in other places. Can we stick to a single nomenclature? https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:648: bool reinitialization_required; Please, give me a default value, in case someone later moves code around... https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:741: bool reinitialization_required; default init pls https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:209: // Returns a bool indicating whether the state had changed. nit: had -> has
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:115: for (size_t k = 0; k <= uppermost_native_rate_index; ++k) { On 2016/09/07 06:28:11, peah-webrtc wrote: > On 2016/09/06 10:41:32, hlundin-webrtc wrote: > > Consider this alternative implementation of the function, using the > frequencies > > directly instead of the indices. Not saying that it is very much better, but > > think about it... > > > > #ifdef WEBRTC_ARCH_ARM_FAMILY > > const int kMaxSplittingNativeProcessRate = AudioProcessing::kSampleRate32kHz; > > #else > > const int kMaxSplittingNativeProcessRate = AudioProcessing::kSampleRate48kHz; > > #endif > > static_assert(kMaxSplittingNativeProcessRate <= > > AudioProcessing::kMaxNativeSampleRateHz, "Stupid error string that will no > > longer be needed in C++17"); > > > > const int uppermost_native_rate = band_splitting_required ? > > kMaxSplittingNativeProcessRate : AudioProcessing::kSampleRate48kHz; > > > > for (auto rate : AudioProcessing::kNativeSampleRatesHz) { > > if (rate >= uppermost_native_rate) { > > return uppermost_native_rate; > > } > > if (rate >= minimum_rate) { > > return rate; > > } > > } > > RTC_NOTREACHED(); > > This is a much nicer variant! Thanks!!! > > I could not, however, get the static assertion to work and get the error > "static_assert expression is not an integral constant expression". > > Somehow it is caused by AudioProcessing::kMaxNativeSampleRateHz. The > initialization of that is a bit intricate. I tried to avoid the issue using > constexpr but did not manage that. Is it ok to keep iot as a DCHECK? What problems did you encounter? It feels like this ought to work if you change enough "const"s to "constexpr"s.
On 2016/09/08 08:17:27, kwiberg-webrtc wrote: > https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:115: for (size_t k = 0; > k <= uppermost_native_rate_index; ++k) { > On 2016/09/07 06:28:11, peah-webrtc wrote: > > On 2016/09/06 10:41:32, hlundin-webrtc wrote: > > > Consider this alternative implementation of the function, using the > > frequencies > > > directly instead of the indices. Not saying that it is very much better, but > > > think about it... > > > > > > #ifdef WEBRTC_ARCH_ARM_FAMILY > > > const int kMaxSplittingNativeProcessRate = > AudioProcessing::kSampleRate32kHz; > > > #else > > > const int kMaxSplittingNativeProcessRate = > AudioProcessing::kSampleRate48kHz; > > > #endif > > > static_assert(kMaxSplittingNativeProcessRate <= > > > AudioProcessing::kMaxNativeSampleRateHz, "Stupid error string that will no > > > longer be needed in C++17"); > > > > > > const int uppermost_native_rate = band_splitting_required ? > > > kMaxSplittingNativeProcessRate : AudioProcessing::kSampleRate48kHz; > > > > > > for (auto rate : AudioProcessing::kNativeSampleRatesHz) { > > > if (rate >= uppermost_native_rate) { > > > return uppermost_native_rate; > > > } > > > if (rate >= minimum_rate) { > > > return rate; > > > } > > > } > > > RTC_NOTREACHED(); > > > > This is a much nicer variant! Thanks!!! > > > > I could not, however, get the static assertion to work and get the error > > "static_assert expression is not an integral constant expression". > > > > Somehow it is caused by AudioProcessing::kMaxNativeSampleRateHz. The > > initialization of that is a bit intricate. I tried to avoid the issue using > > constexpr but did not manage that. Is it ok to keep iot as a DCHECK? > > What problems did you encounter? It feels like this ought to work if you change > enough "const"s to "constexpr"s. There are not that many constexpr's to add. I tried adding that for const int AudioProcessing::kMaxNativeSampleRateHz = AudioProcessing:: | kNativeSampleRatesHz[AudioProcessing::kNumNativeSampleRates - 1];
On 2016/09/08 08:35:06, peah-webrtc wrote: > On 2016/09/08 08:17:27, kwiberg-webrtc wrote: > > > https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... > > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > > > > https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/audio_processing_impl.cc:115: for (size_t k = > 0; > > k <= uppermost_native_rate_index; ++k) { > > On 2016/09/07 06:28:11, peah-webrtc wrote: > > > On 2016/09/06 10:41:32, hlundin-webrtc wrote: > > > > Consider this alternative implementation of the function, using the > > > frequencies > > > > directly instead of the indices. Not saying that it is very much better, > but > > > > think about it... > > > > > > > > #ifdef WEBRTC_ARCH_ARM_FAMILY > > > > const int kMaxSplittingNativeProcessRate = > > AudioProcessing::kSampleRate32kHz; > > > > #else > > > > const int kMaxSplittingNativeProcessRate = > > AudioProcessing::kSampleRate48kHz; > > > > #endif > > > > static_assert(kMaxSplittingNativeProcessRate <= > > > > AudioProcessing::kMaxNativeSampleRateHz, "Stupid error string that will no > > > > longer be needed in C++17"); > > > > > > > > const int uppermost_native_rate = band_splitting_required ? > > > > kMaxSplittingNativeProcessRate : AudioProcessing::kSampleRate48kHz; > > > > > > > > for (auto rate : AudioProcessing::kNativeSampleRatesHz) { > > > > if (rate >= uppermost_native_rate) { > > > > return uppermost_native_rate; > > > > } > > > > if (rate >= minimum_rate) { > > > > return rate; > > > > } > > > > } > > > > RTC_NOTREACHED(); > > > > > > This is a much nicer variant! Thanks!!! > > > > > > I could not, however, get the static assertion to work and get the error > > > "static_assert expression is not an integral constant expression". > > > > > > Somehow it is caused by AudioProcessing::kMaxNativeSampleRateHz. The > > > initialization of that is a bit intricate. I tried to avoid the issue using > > > constexpr but did not manage that. Is it ok to keep iot as a DCHECK? > > > > What problems did you encounter? It feels like this ought to work if you > change > > enough "const"s to "constexpr"s. > > There are not that many constexpr's to add. I tried adding that for > const int AudioProcessing::kMaxNativeSampleRateHz = AudioProcessing:: > | > kNativeSampleRatesHz[AudioProcessing::kNumNativeSampleRates - 1]; There are not that many constexpr's to add. I tried adding that for const int AudioProcessing::kMaxNativeSampleRateHz = AudioProcessing::kNativeSampleRatesHz[AudioProcessing::kNumNativeSampleRates - 1]; as well as for const size_t AudioProcessing::kNumNativeSampleRates = arraysize(AudioProcessing::kNativeSampleRatesHz); const int kMaxSplittingNativeProcessRate = AudioProcessing::kSampleRate48kHz; But it did not work. My guess was that the arraysize was an issue.
On 2016/09/08 08:10:15, the sun wrote: > I think this is an improvement, but did you consider changing the API instead to > not allow poking at the individual module states (setting everything up via > AudioProcessing::Config)? > > https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_processing_impl.cc (left): > > https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:1215: return > capture_nonlocked_.intelligibility_enabled; > This was previously if-deffed but now is not - intentional? > > https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:104: int > NativeProcessRateToUse(int minimum_rate, bool band_splitting_required) { > FindNativeProcessRateToUse() > > also, this is the type of function which it would be very easy to write a test > for... > > https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:181: bool > AudioProcessingImpl::ApmSubmoduleStates::CaptureMultiBandModulesActive() > Is this distinction between "modules" and "effects" widely known and agreed upon > terminology? Feels a bit ad hoc to me. I guess the difference is that an > "effect" will change the signal? Can we make it clearer? At least add comments > in the .h. > > https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:481: int fwd_proc_rate > = NativeProcessRateToUse( > I find it confusing that "fwd" and "rev" are used sometimes, and "Capture" and > "Render" in other places. Can we stick to a single nomenclature? > > https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:648: bool > reinitialization_required; > Please, give me a default value, in case someone later moves code around... > > https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:741: bool > reinitialization_required; > default init pls > > https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_processing_impl.h (right): > > https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.h:209: // Returns a bool > indicating whether the state had changed. > nit: had -> has In the long run, we should definitely use AudioProcessing::Config. But the problem with that is that for that to work all submodules must be controlled via that, and I expect introducing that for all of those will take a significant amount of time due to upstream dependencies, etc. This will solve this issue right away, allowing for two bugs to be solved very quickly.
https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:110: RTC_DCHECK_LT(kMaxSplittingNativeProcessRateIndex, On 2016/09/07 21:36:05, hlundin-webrtc wrote: > On 2016/09/07 06:28:11, peah-webrtc wrote: > > On 2016/09/06 10:41:32, hlundin-webrtc wrote: > > > You should be able to use a static_assert instead. > > > > Great suggestion but I ran into some problems. See below. > > Weird. I thought it'd work if you used constexpr in enough places. Never mind > that; keep the DCHECK. Acknowledged. https://codereview.webrtc.org/2304123002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:115: for (size_t k = 0; k <= uppermost_native_rate_index; ++k) { On 2016/09/08 08:17:27, kwiberg-webrtc wrote: > On 2016/09/07 06:28:11, peah-webrtc wrote: > > On 2016/09/06 10:41:32, hlundin-webrtc wrote: > > > Consider this alternative implementation of the function, using the > > frequencies > > > directly instead of the indices. Not saying that it is very much better, but > > > think about it... > > > > > > #ifdef WEBRTC_ARCH_ARM_FAMILY > > > const int kMaxSplittingNativeProcessRate = > AudioProcessing::kSampleRate32kHz; > > > #else > > > const int kMaxSplittingNativeProcessRate = > AudioProcessing::kSampleRate48kHz; > > > #endif > > > static_assert(kMaxSplittingNativeProcessRate <= > > > AudioProcessing::kMaxNativeSampleRateHz, "Stupid error string that will no > > > longer be needed in C++17"); > > > > > > const int uppermost_native_rate = band_splitting_required ? > > > kMaxSplittingNativeProcessRate : AudioProcessing::kSampleRate48kHz; > > > > > > for (auto rate : AudioProcessing::kNativeSampleRatesHz) { > > > if (rate >= uppermost_native_rate) { > > > return uppermost_native_rate; > > > } > > > if (rate >= minimum_rate) { > > > return rate; > > > } > > > } > > > RTC_NOTREACHED(); > > > > This is a much nicer variant! Thanks!!! > > > > I could not, however, get the static assertion to work and get the error > > "static_assert expression is not an integral constant expression". > > > > Somehow it is caused by AudioProcessing::kMaxNativeSampleRateHz. The > > initialization of that is a bit intricate. I tried to avoid the issue using > > constexpr but did not manage that. Is it ok to keep iot as a DCHECK? > > What problems did you encounter? It feels like this ought to work if you change > enough "const"s to "constexpr"s. There are not that many constexpr's to add. I tried adding that for const int AudioProcessing::kMaxNativeSampleRateHz = AudioProcessing::kNativeSampleRatesHz[AudioProcessing::kNumNativeSampleRates - 1]; as well as for const size_t AudioProcessing::kNumNativeSampleRates = arraysize(AudioProcessing::kNativeSampleRatesHz); const int kMaxSplittingNativeProcessRate = AudioProcessing::kSampleRate48kHz; But it did not work. My guess was that the arraysize was an issue. https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (left): https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1215: return capture_nonlocked_.intelligibility_enabled; On 2016/09/08 08:10:15, the sun wrote: > This was previously if-deffed but now is not - intentional? Good point! Done. https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:104: int NativeProcessRateToUse(int minimum_rate, bool band_splitting_required) { On 2016/09/08 08:10:15, the sun wrote: > FindNativeProcessRateToUse() > > also, this is the type of function which it would be very easy to write a test > for... Changed the name. It is indeed a good function to write a test for. But then I would need to expose that outside audio_processing_impl.cc, right? Now it is a local method in the local namespace. Is it worth doing that just for the purpose of adding a test? https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:181: bool AudioProcessingImpl::ApmSubmoduleStates::CaptureMultiBandModulesActive() On 2016/09/08 08:10:15, the sun wrote: > Is this distinction between "modules" and "effects" widely known and agreed upon > terminology? Feels a bit ad hoc to me. I guess the difference is that an > "effect" will change the signal? Can we make it clearer? At least add comments > in the .h. Good points! It is definitely not an accepted terminology. And it should actually be SubModules instead of modules to follow the rest of the code so I'll change that. I'll change the name Effects to Processing, which follows the terminology currently inside APM. PTAL! https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:481: int fwd_proc_rate = NativeProcessRateToUse( On 2016/09/08 08:10:15, the sun wrote: > I find it confusing that "fwd" and "rev" are used sometimes, and "Capture" and > "Render" in other places. Can we stick to a single nomenclature? I agree, and that is something that has been present for quite a while (>2 years). The changes in this CL makes the changes towards the "render" and "capture" nomenclature. Personally, I like the "render" and "capture" naming. But the AudioProcessing API uses "reverse" and "forward" so I think that is the terminology we should use. Would it be ok to keep the current CL as it is, and follow up with a renaming CL which fixes this for all other places inside audio_processing_impl.*? https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:648: bool reinitialization_required; On 2016/09/08 08:10:15, the sun wrote: > Please, give me a default value, in case someone later moves code around... Done. https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:741: bool reinitialization_required; On 2016/09/08 08:10:15, the sun wrote: > default init pls Done. https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:209: // Returns a bool indicating whether the state had changed. On 2016/09/08 08:10:15, the sun wrote: > nit: had -> has Done.
https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:104: int NativeProcessRateToUse(int minimum_rate, bool band_splitting_required) { On 2016/09/08 08:44:03, peah-webrtc wrote: > On 2016/09/08 08:10:15, the sun wrote: > > FindNativeProcessRateToUse() > > > > also, this is the type of function which it would be very easy to write a test > > for... > > Changed the name. > > It is indeed a good function to write a test for. But then I would need to > expose that outside audio_processing_impl.cc, right? Now it is a local method in > the local namespace. Is it worth doing that just for the purpose of adding a > test? I think so. Unless the functionality is covered by the other tests? But I guess to test that proper you'd need to expose the other state, and then through audio_processing.h, which seems worse. https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:481: int fwd_proc_rate = NativeProcessRateToUse( On 2016/09/08 08:44:03, peah-webrtc wrote: > On 2016/09/08 08:10:15, the sun wrote: > > I find it confusing that "fwd" and "rev" are used sometimes, and "Capture" and > > "Render" in other places. Can we stick to a single nomenclature? > > I agree, and that is something that has been present for quite a while (>2 > years). The changes in this CL makes the changes towards the "render" and > "capture" nomenclature. > > Personally, I like the "render" and "capture" naming. But the AudioProcessing > API uses "reverse" and "forward" so I think that is the terminology we should > use. > > Would it be ok to keep the current CL as it is, and follow up with a renaming CL > which fixes this for all other places inside audio_processing_impl.*? Personally I think Render and Capture makes much more sense so I'd rather stick to that internally and in the long term change the API.
https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:104: int NativeProcessRateToUse(int minimum_rate, bool band_splitting_required) { On 2016/09/08 09:20:48, the sun wrote: > On 2016/09/08 08:44:03, peah-webrtc wrote: > > On 2016/09/08 08:10:15, the sun wrote: > > > FindNativeProcessRateToUse() > > > > > > also, this is the type of function which it would be very easy to write a > test > > > for... > > > > Changed the name. > > > > It is indeed a good function to write a test for. But then I would need to > > expose that outside audio_processing_impl.cc, right? Now it is a local method > in > > the local namespace. Is it worth doing that just for the purpose of adding a > > test? > > I think so. Unless the functionality is covered by the other tests? But I guess > to test that proper you'd need to expose the other state, and then through > audio_processing.h, which seems worse. > This is indirectly tested by the tests that operate on the APM API, for instance the tests in audio_processing_unittest.cc and the tests in audio_processing_performance_unittest.cc. Is it ok to keep this as it is like this and consider that testing sufficient? https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:481: int fwd_proc_rate = NativeProcessRateToUse( On 2016/09/08 09:20:48, the sun wrote: > On 2016/09/08 08:44:03, peah-webrtc wrote: > > On 2016/09/08 08:10:15, the sun wrote: > > > I find it confusing that "fwd" and "rev" are used sometimes, and "Capture" > and > > > "Render" in other places. Can we stick to a single nomenclature? > > > > I agree, and that is something that has been present for quite a while (>2 > > years). The changes in this CL makes the changes towards the "render" and > > "capture" nomenclature. > > > > Personally, I like the "render" and "capture" naming. But the AudioProcessing > > API uses "reverse" and "forward" so I think that is the terminology we should > > use. > > > > Would it be ok to keep the current CL as it is, and follow up with a renaming > CL > > which fixes this for all other places inside audio_processing_impl.*? > > Personally I think Render and Capture makes much more sense so I'd rather stick > to that internally and in the long term change the API. That sounds great to me. Note that I named the variable fwd_proc_rate in order to harmonize with the rev_proc_rate that was there previously. I'd be delighted to do the name changes, but I'd prefer to keep those for a separate CL, as this one is already big and introduce non-bitexact changes (in the new reinitialization). WDYT?
lgtm https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:104: int NativeProcessRateToUse(int minimum_rate, bool band_splitting_required) { On 2016/09/08 18:57:50, peah-webrtc wrote: > On 2016/09/08 09:20:48, the sun wrote: > > On 2016/09/08 08:44:03, peah-webrtc wrote: > > > On 2016/09/08 08:10:15, the sun wrote: > > > > FindNativeProcessRateToUse() > > > > > > > > also, this is the type of function which it would be very easy to write a > > test > > > > for... > > > > > > Changed the name. > > > > > > It is indeed a good function to write a test for. But then I would need to > > > expose that outside audio_processing_impl.cc, right? Now it is a local > method > > in > > > the local namespace. Is it worth doing that just for the purpose of adding a > > > test? > > > > I think so. Unless the functionality is covered by the other tests? But I > guess > > to test that proper you'd need to expose the other state, and then through > > audio_processing.h, which seems worse. > > > > This is indirectly tested by the tests that operate on the APM API, for instance > the tests in audio_processing_unittest.cc and the tests in > audio_processing_performance_unittest.cc. > > Is it ok to keep this as it is like this and consider that testing sufficient? Very well. I assume the non-bitexact changes cannot possibly be caused by this then... https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:481: int fwd_proc_rate = NativeProcessRateToUse( On 2016/09/08 18:57:50, peah-webrtc wrote: > On 2016/09/08 09:20:48, the sun wrote: > > On 2016/09/08 08:44:03, peah-webrtc wrote: > > > On 2016/09/08 08:10:15, the sun wrote: > > > > I find it confusing that "fwd" and "rev" are used sometimes, and "Capture" > > and > > > > "Render" in other places. Can we stick to a single nomenclature? > > > > > > I agree, and that is something that has been present for quite a while (>2 > > > years). The changes in this CL makes the changes towards the "render" and > > > "capture" nomenclature. > > > > > > Personally, I like the "render" and "capture" naming. But the > AudioProcessing > > > API uses "reverse" and "forward" so I think that is the terminology we > should > > > use. > > > > > > Would it be ok to keep the current CL as it is, and follow up with a > renaming > > CL > > > which fixes this for all other places inside audio_processing_impl.*? > > > > Personally I think Render and Capture makes much more sense so I'd rather > stick > > to that internally and in the long term change the API. > > That sounds great to me. Note that I named the variable fwd_proc_rate in order > to harmonize with the rev_proc_rate that was there previously. > > I'd be delighted to do the name changes, but I'd prefer to keep those for a > separate CL, as this one is already big and introduce non-bitexact changes (in > the new reinitialization). > WDYT? sgtm
https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2304123002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:104: int NativeProcessRateToUse(int minimum_rate, bool band_splitting_required) { On 2016/09/08 20:36:38, the sun wrote: > On 2016/09/08 18:57:50, peah-webrtc wrote: > > On 2016/09/08 09:20:48, the sun wrote: > > > On 2016/09/08 08:44:03, peah-webrtc wrote: > > > > On 2016/09/08 08:10:15, the sun wrote: > > > > > FindNativeProcessRateToUse() > > > > > > > > > > also, this is the type of function which it would be very easy to write > a > > > test > > > > > for... > > > > > > > > Changed the name. > > > > > > > > It is indeed a good function to write a test for. But then I would need to > > > > expose that outside audio_processing_impl.cc, right? Now it is a local > > method > > > in > > > > the local namespace. Is it worth doing that just for the purpose of adding > a > > > > test? > > > > > > I think so. Unless the functionality is covered by the other tests? But I > > guess > > > to test that proper you'd need to expose the other state, and then through > > > audio_processing.h, which seems worse. > > > > > > > This is indirectly tested by the tests that operate on the APM API, for > instance > > the tests in audio_processing_unittest.cc and the tests in > > audio_processing_performance_unittest.cc. > > > > Is it ok to keep this as it is like this and consider that testing sufficient? > > > Very well. I assume the non-bitexact changes cannot possibly be caused by this > then... Of course, they can. But we have at least coverage to detect when something happens that should not happen.
peah@webrtc.org changed reviewers: - kwiberg@webrtc.org
On 2016/09/10 07:53:50, peah-webrtc wrote: > mailto:peah@webrtc.org changed reviewers: > - mailto:kwiberg@webrtc.org Removed kwiberg@ as a reviewer as that it was agreed during offline discussions that he would follow-up on the static_assert issue once this CL has landed.
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, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2304123002/#ps120001 (title: "Created new test reference binaries for the unit tests in audio_processing_unittest.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/1152)
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 Link to the patchset: https://codereview.webrtc.org/2304123002/#ps140001 (title: "Avoided compiler warning by adding unreachable return statement")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/17974)
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 Link to the patchset: https://codereview.webrtc.org/2304123002/#ps160001 (title: "Updated the test that required that no initialization should be done if APM is called with the defa…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== The audio processing module (APM) relies on two for functionalities doing sample-rate conversions: -The implicit resampling done in the AudioBuffer CopyTo, CopyFrom, InterleaveTo and DeinterleaveFrom methods. -The multi-band splitting scheme. The selection of rates in these have been difficult and complicated, partly due to that the APM API which allows for activating the APM submodules without notifying the APM. This CL adds functionality that for each capture frame polls all submodules for whether they are active or not and compares this against a cached result. Furthermore, new functionality is added that based on the results of the comparison do a reinitialization of the APM. This has several advantages -The code deciding on whether to analysis and synthesis is needed for the bandsplitting can be much simplified and centralized. -The selection of the processing rate can be done such as to avoid the implicit resampling that was in some cases unnecessarily done. -The optimization for whether an output copy is needed that was done to improve performance due to the implicit resampling is no longer needed, which simplifies the code and makes it less error-prone in the sense that is no longer neccessary to keep track of whether any module has changed the signal. Finally, it should be noted that the polling of the state for all the submodules was done previously as well, but in a less obvious and distributed manner. BUG=webrtc:6181, webrtc:6220, webrtc:5298, webrtc:6296, webrtc:6298, webrtc:6297 ========== to ========== The audio processing module (APM) relies on two for functionalities doing sample-rate conversions: -The implicit resampling done in the AudioBuffer CopyTo, CopyFrom, InterleaveTo and DeinterleaveFrom methods. -The multi-band splitting scheme. The selection of rates in these have been difficult and complicated, partly due to that the APM API which allows for activating the APM submodules without notifying the APM. This CL adds functionality that for each capture frame polls all submodules for whether they are active or not and compares this against a cached result. Furthermore, new functionality is added that based on the results of the comparison do a reinitialization of the APM. This has several advantages -The code deciding on whether to analysis and synthesis is needed for the bandsplitting can be much simplified and centralized. -The selection of the processing rate can be done such as to avoid the implicit resampling that was in some cases unnecessarily done. -The optimization for whether an output copy is needed that was done to improve performance due to the implicit resampling is no longer needed, which simplifies the code and makes it less error-prone in the sense that is no longer neccessary to keep track of whether any module has changed the signal. Finally, it should be noted that the polling of the state for all the submodules was done previously as well, but in a less obvious and distributed manner. BUG=webrtc:6181, webrtc:6220, webrtc:5298, webrtc:6296, webrtc:6298, webrtc:6297 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== The audio processing module (APM) relies on two for functionalities doing sample-rate conversions: -The implicit resampling done in the AudioBuffer CopyTo, CopyFrom, InterleaveTo and DeinterleaveFrom methods. -The multi-band splitting scheme. The selection of rates in these have been difficult and complicated, partly due to that the APM API which allows for activating the APM submodules without notifying the APM. This CL adds functionality that for each capture frame polls all submodules for whether they are active or not and compares this against a cached result. Furthermore, new functionality is added that based on the results of the comparison do a reinitialization of the APM. This has several advantages -The code deciding on whether to analysis and synthesis is needed for the bandsplitting can be much simplified and centralized. -The selection of the processing rate can be done such as to avoid the implicit resampling that was in some cases unnecessarily done. -The optimization for whether an output copy is needed that was done to improve performance due to the implicit resampling is no longer needed, which simplifies the code and makes it less error-prone in the sense that is no longer neccessary to keep track of whether any module has changed the signal. Finally, it should be noted that the polling of the state for all the submodules was done previously as well, but in a less obvious and distributed manner. BUG=webrtc:6181, webrtc:6220, webrtc:5298, webrtc:6296, webrtc:6298, webrtc:6297 ========== to ========== The audio processing module (APM) relies on two for functionalities doing sample-rate conversions: -The implicit resampling done in the AudioBuffer CopyTo, CopyFrom, InterleaveTo and DeinterleaveFrom methods. -The multi-band splitting scheme. The selection of rates in these have been difficult and complicated, partly due to that the APM API which allows for activating the APM submodules without notifying the APM. This CL adds functionality that for each capture frame polls all submodules for whether they are active or not and compares this against a cached result. Furthermore, new functionality is added that based on the results of the comparison do a reinitialization of the APM. This has several advantages -The code deciding on whether to analysis and synthesis is needed for the bandsplitting can be much simplified and centralized. -The selection of the processing rate can be done such as to avoid the implicit resampling that was in some cases unnecessarily done. -The optimization for whether an output copy is needed that was done to improve performance due to the implicit resampling is no longer needed, which simplifies the code and makes it less error-prone in the sense that is no longer neccessary to keep track of whether any module has changed the signal. Finally, it should be noted that the polling of the state for all the submodules was done previously as well, but in a less obvious and distributed manner. BUG=webrtc:6181, webrtc:6220, webrtc:5298, webrtc:6296, webrtc:6298, webrtc:6297 Committed: https://crrev.com/2ace3f94064e4d1980483d9ea3b0da8537aee540 Cr-Commit-Position: refs/heads/master@{#14175} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/2ace3f94064e4d1980483d9ea3b0da8537aee540 Cr-Commit-Position: refs/heads/master@{#14175} |
