Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2292863002:
Introduced new scheme for controlling the functionality inside the audio processing module (Closed)
|
Created:
4 years, 3 months ago by peah-webrtc Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionThe current scheme for setting parameters and specifying the behavior
of the audio processing module is quite complex and hard to implement
in a threadsafe and efficient manner. Therefore a new scheme for setting
the parameters in the audio processing module is introduced in this CL.
The idea is to roll this scheme out gradually and as a first functionality
in the audio processing module where this is applied the level controller
was chosen. This CL includes the replacement of the Config-based
level controller scheme with the new scheme.
BUG=webrtc:5298
Committed: https://crrev.com/c8bbe3fe9aad9e9a1189a42dcaa8f5d6c261ecc8
Cr-Commit-Position: refs/heads/master@{#14171}
Patch Set 1 #
Total comments: 50
Patch Set 2 : Changes in response to reviewer comments #
Total comments: 18
Patch Set 3 : Changes in response to reviewer comments #
Total comments: 36
Patch Set 4 : Changes in response to reviewer comments #
Total comments: 10
Patch Set 5 : Changes in response to reviewer comments #Patch Set 6 : Removed the validation method and added logging of the config settings and revertion to default val… #
Total comments: 26
Patch Set 7 : Changes in response to reviewer comments #Patch Set 8 : Corrected comment #
Total comments: 6
Patch Set 9 : Changes in response to reviewer comments #
Total comments: 4
Patch Set 10 : Changes in response to reviewer comments #Patch Set 11 : Rebase #Patch Set 12 : Fixed bad merge #Messages
Total messages: 46 (15 generated)
Patchset #1 (id:1) has been deleted
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, solenberg@webrtc.org
Description was changed from ========== Introduced new scheme for controlling the functionality inside the audio processing module The current scheme for setting parameters and specifying the behavior of the audio processing module is quite complex and hard to implement in a threadsafe and efficient manner. Therefore a new scheme for setting the parameters in the audio processing module is introduced in this CL. The idea is to roll this scheme out gradually and as a first functionality in the audio processing module where this is applied the level controller was chosen. This CL includes the replacement of the Config-based level controller scheme with the new scheme. BUG=webrtc:5298 ========== to ========== The current scheme for setting parameters and specifying the behavior of the audio processing module is quite complex and hard to implement in a threadsafe and efficient manner. Therefore a new scheme for setting the parameters in the audio processing module is introduced in this CL. The idea is to roll this scheme out gradually and as a first functionality in the audio processing module where this is applied the level controller was chosen. This CL includes the replacement of the Config-based level controller scheme with the new scheme. BUG=webrtc:5298 ==========
https://codereview.webrtc.org/2292863002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (left): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:876: config.Set<webrtc::LevelControl>(new webrtc::LevelControl(*level_control_)); oh, yeah! :) https://codereview.webrtc.org/2292863002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:878: voe_wrapper_->base()->audio_processing()->ApplySettings(apm_settings); Move line 883 up before this block instead. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:119: bool ApmSettings::LevelControllerSettings::IsOk() const { I don't think this is the right place for this knowledge (assuming there will be more conditions to enforce). Eventually, I think we'll have a LevelController::Config instead, with the LevelController exposed as a separate processing module. While we still have the monolithic APM, I'd suggest keeping the struct almost where you've put it: class AudioProcessing { ... struct Config { struct LevelController { ... } level_controller; ... }; ...but to keep the config structs as PODs. Instead, add a "static bool LevelController::ValidateConfig(const Config& config)" method so we keep as much knowledge of each module locally. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:49: int ApplySettings(const ApmSettings& settings) override; Call, AudioReceiveStream etc, use "Config" for these types of structs. We should stick to that. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (left): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:98: static const ConfigOptionID identifier = ConfigOptionID::kLevelControl; Don't forget to remove ConfigOptionID::kLevelControl https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:57: struct ApmSettings { name this Config and put it inside the AudioProcessing interface. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:286: virtual int ApplySettings(const ApmSettings& settings) = 0; - ApplyConfig() - Add TODO(peah) that this is temporary?
Nice. See comments inline. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:878: voe_wrapper_->base()->audio_processing()->ApplySettings(apm_settings); Verify the settings before applying. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:252: if (!settings_ok) { I think you can be more aggressive, and simply RTC_CHECK(settings.IsOk()); You do provide a method for the "user" to validate the settings before calling ApplySettings. Then this function cannot fail and should return void. If you do not like this, then at least make the function return bool. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:49: int ApplySettings(const ApmSettings& settings) override; On 2016/08/30 10:32:38, the sun wrote: > Call, AudioReceiveStream etc, use "Config" for these types of structs. We should > stick to that. Acknowledged. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:51: // The parameters and behavior of the audio processing module is speficied is -> are https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:51: // The parameters and behavior of the audio processing module is speficied speficied -> specified https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:52: // by changing the default values in the ApmSettings struct. It is possible The parameters and behavior are in fact specified by the struct regardless of whether the default values are kept or changed. Consider rephrasing. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:54: // substruct. The settings are applied by passing the struct to the What happens if I call ApmSettings::IsOk()? Will it aggregate the results of all substructs' IsOk functions? https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:55: // ApplySettings Line wrapping is odd. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:57: struct ApmSettings { On 2016/08/30 10:32:39, the sun wrote: > name this Config and put it inside the AudioProcessing interface. +1 https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:58: struct LevelControllerSettings { Should the substructs' configs be declared here or in their respective classes? In ACM, we have an AudioCodingModule::Config struct, which contains a NetEq::Config struct. This does of course mean that audio_coding_module.h must include neteq.h, which may not be desired in the APM. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:220: // RTC_DCHECK(settings.IsOk()); The "user" should always verify the settings, not only in debug mode. That is if (!settings.IsOk()) { // Failure. Don't use this config. } https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/level_controller_complexity_unittest.cc (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/level_controller_complexity_unittest.cc:212: apm->ApplySettings(apm_settings); ASSERT_TRUE(apm_settings.IsOk()); https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:449: RTC_DCHECK(apm_settings.IsOk()); This is test code. Use RTC_CHECK, to also catch the unlikely event that someone runs this in release mode. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:242: RTC_DCHECK(apm_settings.IsOk()); RTC_CHECK https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:147: RTC_DCHECK(apm_settings.IsOk()); RTC_CHECK https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/process_test.cc (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/process_test.cc:455: RTC_DCHECK(apm_settings.IsOk()); RTC_CHECK
Patchset #2 (id:40001) has been deleted
https://codereview.webrtc.org/2292863002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (left): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:876: config.Set<webrtc::LevelControl>(new webrtc::LevelControl(*level_control_)); On 2016/08/30 10:32:38, the sun wrote: > oh, yeah! :) :-) https://codereview.webrtc.org/2292863002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:878: voe_wrapper_->base()->audio_processing()->ApplySettings(apm_settings); On 2016/08/30 11:26:26, hlundin-webrtc wrote: > Verify the settings before applying. Done. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:878: voe_wrapper_->base()->audio_processing()->ApplySettings(apm_settings); On 2016/08/30 10:32:38, the sun wrote: > Move line 883 up before this block instead. Makes sense. What I did now is that I split out apm_settings from the local block, as that is how it will be used for the next parameter. PTAL. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:119: bool ApmSettings::LevelControllerSettings::IsOk() const { On 2016/08/30 10:32:38, the sun wrote: > I don't think this is the right place for this knowledge (assuming there will be > more conditions to enforce). Eventually, I think we'll have a > LevelController::Config instead, with the LevelController exposed as a separate > processing module. While we still have the monolithic APM, I'd suggest keeping > the struct almost where you've put it: > > class AudioProcessing { > ... > struct Config { > struct LevelController { > ... > } level_controller; > ... > }; > > ...but to keep the config structs as PODs. Instead, add a "static bool > LevelController::ValidateConfig(const Config& config)" method so we keep as much > knowledge of each module locally. That makes sense. I've rewritten this. PTAL. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:252: if (!settings_ok) { On 2016/08/30 11:26:26, hlundin-webrtc wrote: > I think you can be more aggressive, and simply RTC_CHECK(settings.IsOk()); > > You do provide a method for the "user" to validate the settings before calling > ApplySettings. > > Then this function cannot fail and should return void. If you do not like this, > then at least make the function return bool. I think it is not absolutely necessary to CHECK here, so I'll keep the DCHECK. I will add the bool return type. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:49: int ApplySettings(const ApmSettings& settings) override; On 2016/08/30 11:26:26, hlundin-webrtc wrote: > On 2016/08/30 10:32:38, the sun wrote: > > Call, AudioReceiveStream etc, use "Config" for these types of structs. We > should > > stick to that. > > Acknowledged. Acknowledged. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:49: int ApplySettings(const ApmSettings& settings) override; On 2016/08/30 10:32:38, the sun wrote: > Call, AudioReceiveStream etc, use "Config" for these types of structs. We should > stick to that. Done! https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (left): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:98: static const ConfigOptionID identifier = ConfigOptionID::kLevelControl; On 2016/08/30 10:32:39, the sun wrote: > Don't forget to remove ConfigOptionID::kLevelControl Can we do that? There is a comment in common.h that specifically states that those should never be removed, but rather deprecated. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:51: // The parameters and behavior of the audio processing module is speficied On 2016/08/30 11:26:26, hlundin-webrtc wrote: > is -> are Done. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:51: // The parameters and behavior of the audio processing module is speficied On 2016/08/30 11:26:26, hlundin-webrtc wrote: > speficied -> specified Done. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:52: // by changing the default values in the ApmSettings struct. It is possible On 2016/08/30 11:26:26, hlundin-webrtc wrote: > The parameters and behavior are in fact specified by the struct regardless of > whether the default values are kept or changed. Consider rephrasing. Done. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:54: // substruct. The settings are applied by passing the struct to the On 2016/08/30 11:26:26, hlundin-webrtc wrote: > What happens if I call ApmSettings::IsOk()? Will it aggregate the results of all > substructs' IsOk functions? Yes, I added a comment about that. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:55: // ApplySettings On 2016/08/30 11:26:26, hlundin-webrtc wrote: > Line wrapping is odd. Done. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:57: struct ApmSettings { On 2016/08/30 10:32:39, the sun wrote: > name this Config and put it inside the AudioProcessing interface. Done. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:57: struct ApmSettings { On 2016/08/30 11:26:26, hlundin-webrtc wrote: > On 2016/08/30 10:32:39, the sun wrote: > > name this Config and put it inside the AudioProcessing interface. > > +1 Done. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:58: struct LevelControllerSettings { On 2016/08/30 11:26:26, hlundin-webrtc wrote: > Should the substructs' configs be declared here or in their respective classes? > In ACM, we have an AudioCodingModule::Config struct, which contains a > NetEq::Config struct. This does of course mean that audio_coding_module.h must > include neteq.h, which may not be desired in the APM. I don't think that we should allow/require audio_processing.h to include the header files of the submodules. I'll try with another variant and see what you think of that. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:220: // RTC_DCHECK(settings.IsOk()); On 2016/08/30 11:26:26, hlundin-webrtc wrote: > The "user" should always verify the settings, not only in debug mode. That is > if (!settings.IsOk()) { > // Failure. Don't use this config. > } Good point! I edited it a bit. PTAL https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:286: virtual int ApplySettings(const ApmSettings& settings) = 0; On 2016/08/30 10:32:38, the sun wrote: > - ApplyConfig() > - Add TODO(peah) that this is temporary? Done. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/level_controller_complexity_unittest.cc (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/level_controller_complexity_unittest.cc:212: apm->ApplySettings(apm_settings); On 2016/08/30 11:26:26, hlundin-webrtc wrote: > ASSERT_TRUE(apm_settings.IsOk()); Done. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:449: RTC_DCHECK(apm_settings.IsOk()); On 2016/08/30 11:26:26, hlundin-webrtc wrote: > This is test code. Use RTC_CHECK, to also catch the unlikely event that someone > runs this in release mode. Done. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:242: RTC_DCHECK(apm_settings.IsOk()); On 2016/08/30 11:26:26, hlundin-webrtc wrote: > RTC_CHECK Done. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:147: RTC_DCHECK(apm_settings.IsOk()); On 2016/08/30 11:26:26, hlundin-webrtc wrote: > RTC_CHECK Done. https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/process_test.cc (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/process_test.cc:455: RTC_DCHECK(apm_settings.IsOk()); On 2016/08/30 11:26:27, hlundin-webrtc wrote: > RTC_CHECK Great point! Done!
https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:220: // RTC_DCHECK(settings.IsOk()); On 2016/08/30 11:26:26, hlundin-webrtc wrote: > The "user" should always verify the settings, not only in debug mode. That is > if (!settings.IsOk()) { > // Failure. Don't use this config. > } I disagree. We need to validate the config anyway, so why force that on the user? Make the API simple instead. https://codereview.webrtc.org/2292863002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2292863002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:888: voe_wrapper_->base()->audio_processing()->ApplyConfig(apm_config); It looks to me, reading the comment above, that this call should be within the above conditional. As a bonus, you can use the "audioproc" variable, rather than fetching the APM* again. https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (left): https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:431: config.Get<LevelControl>().enabled) { Note that the new code won't avoid initializing the LC if the enabled flag doesn't change. Was that intentional? https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:247: dd https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:257: if (capture_nonlocked_.level_controller_enabled) { You could drop the capture_nonlocked_.level_controller_enabled and just have the pointer to the LevelController. Destroy it when not needed (pointer is null), or create it when needed. https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:204: // if (config_ok { No. The client should not be burdened with validating the struct. Just validate internally in ApplyConfig() and return false if there's a problem (*before* acting on anything in the struct). https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:252: // by changing the default values in the ApmConfig struct. It is possible Update comment. https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:261: bool Validate() const; Again, the *knowledge* about a sane state for each config should live in the actual processing module/component. The monolithic APM should know as little detail as possible about each of the processors. https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:292: virtual bool ApplyConfig(const Config& config) = 0; nit: Does it really belong before Initialize() ? https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/level_controller.cc:75: bool AudioProcessing::Config::LevelController::Validate() const { How about a static function in LevelController instead: bool LevelController::Validate(const AudioProcessing::Config::LevelControllerConfig& config) { ? That will be easy to change into bool LevelController::Validate(const LevelController::Config& config) { once we get there. Adding the function definition here, of something declared in AudioProcessing, is confusing, and likely validates the style guide (or it should).
https://codereview.webrtc.org/2292863002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2292863002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:888: voe_wrapper_->base()->audio_processing()->ApplyConfig(apm_config); On 2016/08/30 18:05:35, the sun wrote: > It looks to me, reading the comment above, that this call should be within the > above conditional. As a bonus, you can use the "audioproc" variable, rather than > fetching the APM* again. Great point! Done. https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (left): https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:431: config.Get<LevelControl>().enabled) { On 2016/08/30 18:05:35, the sun wrote: > Note that the new code won't avoid initializing the LC if the enabled flag > doesn't change. Was that intentional? No, that was not intended at all. Great catch!!! Done. https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:247: On 2016/08/30 18:05:35, the sun wrote: > dd Done. https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:257: if (capture_nonlocked_.level_controller_enabled) { On 2016/08/30 18:05:35, the sun wrote: > You could drop the capture_nonlocked_.level_controller_enabled and just have the > pointer to the LevelController. Destroy it when not needed (pointer is null), or > create it when needed. I'm not sure I follow that. Do you mean that I should drop it only here, or in general? I think it would make sense to simplify this but note that the LevelController does not have any enabled property, i.e., it does not know whether it is enabled or not. That is tracked by capture_nonlocked_.level_controller_enabled. I think it would make sense to simplify this but I'd rather wait until the parameter scheme has settled. The level controller is not the only one behaving like this. The intelligibility enhancer and the beamformer does it similarly. https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:204: // if (config_ok { On 2016/08/30 18:05:35, the sun wrote: > No. The client should not be burdened with validating the struct. Just validate > internally in ApplyConfig() and return false if there's a problem (*before* > acting on anything in the struct). Done. https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:252: // by changing the default values in the ApmConfig struct. It is possible On 2016/08/30 18:05:35, the sun wrote: > Update comment. Done. https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:261: bool Validate() const; On 2016/08/30 18:05:35, the sun wrote: > Again, the *knowledge* about a sane state for each config should live in the > actual processing module/component. The monolithic APM should know as little > detail as possible about each of the processors. Done. https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:292: virtual bool ApplyConfig(const Config& config) = 0; On 2016/08/30 18:05:35, the sun wrote: > nit: Does it really belong before Initialize() ? Good point! I'll put it by SetExtraOptions Done. https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2292863002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/level_controller.cc:75: bool AudioProcessing::Config::LevelController::Validate() const { On 2016/08/30 18:05:35, the sun wrote: > How about a static function in LevelController instead: > > bool LevelController::Validate(const > AudioProcessing::Config::LevelControllerConfig& config) { > ? > > That will be easy to change into > bool LevelController::Validate(const LevelController::Config& config) { > once we get there. > > Adding the function definition here, of something declared in AudioProcessing, > is confusing, and likely validates the style guide (or it should). That sounds good! Done.
https://codereview.webrtc.org/2292863002/diff/80001/webrtc/media/engine/fakew... File webrtc/media/engine/fakewebrtcvoiceengine.h (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtcvoiceengine.h:45: WEBRTC_BOOL_STUB(ApplyConfig, (const AudioProcessing::Config& config)); Use same order as in audio_processing.h https://codereview.webrtc.org/2292863002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:885: RTC_DCHECK(apm_config.Validate()); ApplyConfig already returns a bool - use that instead: RTC_CHECK(audioproc->ApplyConfig(... https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:119: bool AudioProcessing::Config::Validate() const { Make .cc-local or bake into ApplyConfig(). https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:430: if (config.level_controller.enabled && != https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:436: capture_nonlocked_.level_controller_enabled = config.level_controller.enabled; Move inside the above conditional https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:202: // apm->ApplyConfig(config); So I guess it is best if the example includes handling the return code, perhaps with an RTC_CHECK(). https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:248: // by changing the default values in the ApmConfig struct. The config is ApmConfig -> AudioProcessing::Config https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:250: // processing interface. It is possible to verify the specified parameters by "audio processing interface" is unnecessary in this context. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:256: bool Validate() const; Please remove this method and keep it a plain struct. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/mock_audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/mock_audio_processing.h:182: MOCK_METHOD1(ApplyConfig, bool(const Config& config)); Use same order as in audio_processing.h https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/level_controller_complexity_unittest.cc (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/level_controller_complexity_unittest.cc:9: */ The name of this file is confusing. What is it? A unit test or a performance test? https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:449: RTC_DCHECK(apm_config.Validate()); dd https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:242: RTC_DCHECK(apm_config.Validate()); dd https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:148: RTC_DCHECK(apm_config.Validate()); dd https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/process_test.cc (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/process_test.cc:455: RTC_DCHECK(apm_config.Validate()); dd
https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:220: // RTC_DCHECK(settings.IsOk()); On 2016/08/30 18:05:35, the sun wrote: > On 2016/08/30 11:26:26, hlundin-webrtc wrote: > > The "user" should always verify the settings, not only in debug mode. That is > > if (!settings.IsOk()) { > > // Failure. Don't use this config. > > } > > I disagree. We need to validate the config anyway, so why force that on the > user? Make the API simple instead. The argument by the other reviewer is that there anyway needs to be a validation method once the config is used at the creation of apm in order to allow any other action that to crash for falsy parameters. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/media/engine/fakew... File webrtc/media/engine/fakewebrtcvoiceengine.h (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtcvoiceengine.h:45: WEBRTC_BOOL_STUB(ApplyConfig, (const AudioProcessing::Config& config)); On 2016/09/01 13:48:00, the sun wrote: > Use same order as in audio_processing.h Good find! Done. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:885: RTC_DCHECK(apm_config.Validate()); On 2016/09/01 13:48:00, the sun wrote: > ApplyConfig already returns a bool - use that instead: > RTC_CHECK(audioproc->ApplyConfig(... > > I don't think that this mandates a CHECK, so I do a DCHECK but please challenge that if it does not make sense to you! https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:119: bool AudioProcessing::Config::Validate() const { On 2016/09/01 13:48:00, the sun wrote: > Make .cc-local or bake into ApplyConfig(). Done. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:430: if (config.level_controller.enabled && On 2016/09/01 13:48:00, the sun wrote: > != Done. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:436: capture_nonlocked_.level_controller_enabled = config.level_controller.enabled; On 2016/09/01 13:48:00, the sun wrote: > Move inside the above conditional Done. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:202: // apm->ApplyConfig(config); On 2016/09/01 13:48:00, the sun wrote: > So I guess it is best if the example includes handling the return code, perhaps > with an RTC_CHECK(). I rewrote this again after feedback from the other reviewer. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:248: // by changing the default values in the ApmConfig struct. The config is On 2016/09/01 13:48:00, the sun wrote: > ApmConfig -> AudioProcessing::Config Done. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:250: // processing interface. It is possible to verify the specified parameters by On 2016/09/01 13:48:01, the sun wrote: > "audio processing interface" is unnecessary in this context. Done. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:256: bool Validate() const; On 2016/09/01 13:48:00, the sun wrote: > Please remove this method and keep it a plain struct. The argument by the other reviewer is that this is good to have as it allows the user to test a config without having to crash the application once we set these settings during construction time. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/mock_audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/mock_audio_processing.h:182: MOCK_METHOD1(ApplyConfig, bool(const Config& config)); On 2016/09/01 13:48:01, the sun wrote: > Use same order as in audio_processing.h Done. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/level_controller_complexity_unittest.cc (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/level_controller_complexity_unittest.cc:9: */ On 2016/09/01 13:48:01, the sun wrote: > The name of this file is confusing. What is it? A unit test or a performance > test? It is a performance test. Do you have a better suggestion for a name? Should I skip the _unittest ending? https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:449: RTC_DCHECK(apm_config.Validate()); On 2016/09/01 13:48:01, the sun wrote: > dd Done. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:242: RTC_DCHECK(apm_config.Validate()); On 2016/09/01 13:48:01, the sun wrote: > dd Done. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:148: RTC_DCHECK(apm_config.Validate()); On 2016/09/01 13:48:01, the sun wrote: > dd Done. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/process_test.cc (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/process_test.cc:455: RTC_DCHECK(apm_config.Validate()); On 2016/09/01 13:48:01, the sun wrote: > dd Done.
https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:202: // apm->ApplyConfig(config); On 2016/09/02 08:22:00, peah-webrtc wrote: > On 2016/09/01 13:48:00, the sun wrote: > > So I guess it is best if the example includes handling the return code, > perhaps > > with an RTC_CHECK(). > > I rewrote this again after feedback from the other reviewer. I don't see that feedback in this CL, unless you're talking about Henrik's first set of comments? I'm confused, because you already changed the implementation after that. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:256: bool Validate() const; On 2016/09/02 08:22:00, peah-webrtc wrote: > On 2016/09/01 13:48:00, the sun wrote: > > Please remove this method and keep it a plain struct. > > The argument by the other reviewer is that this is good to have as it allows the > user to test a config without having to crash the application once we set these > settings during construction time. Well, sure, but we could just as well handle that in the ctor then: either we DCHECK the config is valid and revert to some sane settings if it isn't, or we CHECK the config, if we're lazy or just don't believe we can recover. In the unlikely even that the client actually does DCHECK(Validate) on the config before giving it to the ctor, we still need to prepare for an invalid config. And besides, what is the client to do with the information? Additionally, none of the other places where we use construct-from-config semantics, employ a Validate() method. I know, consistency is the hobgoblin of little minds etc. :) But still, it makes sense to make the APIs work the same. There's a better way than all of this, of course, which is to use types for the config parameters that enforce these constraints; ranges etc. https://codereview.webrtc.org/2292863002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:203: // if (valid_config) { This doesn't make sense to me. Why would a client do this?
https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:220: // RTC_DCHECK(settings.IsOk()); On 2016/09/02 08:21:59, peah-webrtc wrote: > On 2016/08/30 18:05:35, the sun wrote: > > On 2016/08/30 11:26:26, hlundin-webrtc wrote: > > > The "user" should always verify the settings, not only in debug mode. That > is > > > if (!settings.IsOk()) { > > > // Failure. Don't use this config. > > > } > > > > I disagree. We need to validate the config anyway, so why force that on the > > user? Make the API simple instead. > > The argument by the other reviewer is that there anyway needs to be a validation > method once the config is used at the creation of apm in order to allow any > other action that to crash for falsy parameters. (Same comment as in later patch set, for completeness and increased confusion.) I agree that it isn't necessary to do that before calling ApplyConfig. That method could instead return an error, as it does now. My thinking was a bit ahead of the current implementation. I suppose we want to be able to provide a Config to the APM ctor in the future. And in that case, the user must validate the config before passing it, since the ctor itself has no option but to crash if the config is invalid. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:256: bool Validate() const; On 2016/09/02 09:37:05, the sun wrote: > On 2016/09/02 08:22:00, peah-webrtc wrote: > > On 2016/09/01 13:48:00, the sun wrote: > > > Please remove this method and keep it a plain struct. > > > > The argument by the other reviewer is that this is good to have as it allows > the > > user to test a config without having to crash the application once we set > these > > settings during construction time. > > Well, sure, but we could just as well handle that in the ctor then: either we > DCHECK the config is valid and revert to some sane settings if it isn't, or we > CHECK the config, if we're lazy or just don't believe we can recover. In the > unlikely even that the client actually does DCHECK(Validate) on the config > before giving it to the ctor, we still need to prepare for an invalid config. > And besides, what is the client to do with the information? > > Additionally, none of the other places where we use construct-from-config > semantics, employ a Validate() method. I know, consistency is the hobgoblin of > little minds etc. :) But still, it makes sense to make the APIs work the same. > > There's a better way than all of this, of course, which is to use types for the > config parameters that enforce these constraints; ranges etc. We use this pattern for the AudioEncoders. Each implementation of the AudioEncoder interface has a Config which can be passed to the ctor. The ctor will crash on invalid configurations (silently modifying it to a valid config is not tractable), and it is the responsibility of the caller to validate it. What is the caller to do with a failed validation? Well, unlike the ctor, it can probably flag an error, and maybe revert to a default config if that makes sense. https://codereview.webrtc.org/2292863002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2292863002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:886: RTC_DCHECK(audioproc->ApplyConfig(apm_config)); Using a DCHECK here means that ApplyConfig is only called in debug mode. Not your intention, I suppose. https://codereview.webrtc.org/2292863002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:203: // if (valid_config) { On 2016/09/02 09:37:05, the sun wrote: > This doesn't make sense to me. Why would a client do this? I agree that it isn't necessary to do that before calling ApplyConfig. That method could instead return an error, as it does now. My thinking was a bit ahead of the current implementation. I suppose we want to be able to provide a Config to the APM ctor in the future. And in that case, the user must validate the config before passing it, since the ctor itself has no option but to crash if the config is invalid. https://codereview.webrtc.org/2292863002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:251: // by changing the default values in the AudioProcessing::ApmConfig struct. Not called ApmConfig anymore. https://codereview.webrtc.org/2292863002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:253: // the Incomplete sentence.
https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:220: // RTC_DCHECK(settings.IsOk()); On 2016/09/06 08:56:03, hlundin-webrtc wrote: > On 2016/09/02 08:21:59, peah-webrtc wrote: > > On 2016/08/30 18:05:35, the sun wrote: > > > On 2016/08/30 11:26:26, hlundin-webrtc wrote: > > > > The "user" should always verify the settings, not only in debug mode. That > > is > > > > if (!settings.IsOk()) { > > > > // Failure. Don't use this config. > > > > } > > > > > > I disagree. We need to validate the config anyway, so why force that on the > > > user? Make the API simple instead. > > > > The argument by the other reviewer is that there anyway needs to be a > validation > > method once the config is used at the creation of apm in order to allow any > > other action that to crash for falsy parameters. > > (Same comment as in later patch set, for completeness and increased confusion.) > I agree that it isn't necessary to do that before calling ApplyConfig. That > method could instead return an error, as it does now. > > My thinking was a bit ahead of the current implementation. I suppose we want to > be able to provide a Config to the APM ctor in the future. And in that case, the > user must validate the config before passing it, since the ctor itself has no > option but to crash if the config is invalid. Acknowledged. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:202: // apm->ApplyConfig(config); On 2016/09/02 09:37:05, the sun wrote: > On 2016/09/02 08:22:00, peah-webrtc wrote: > > On 2016/09/01 13:48:00, the sun wrote: > > > So I guess it is best if the example includes handling the return code, > > perhaps > > > with an RTC_CHECK(). > > > > I rewrote this again after feedback from the other reviewer. > > I don't see that feedback in this CL, unless you're talking about Henrik's first > set of comments? I'm confused, because you already changed the implementation > after that. That feedback is now in the comments from Henrik. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:256: bool Validate() const; On 2016/09/06 08:56:03, hlundin-webrtc wrote: > On 2016/09/02 09:37:05, the sun wrote: > > On 2016/09/02 08:22:00, peah-webrtc wrote: > > > On 2016/09/01 13:48:00, the sun wrote: > > > > Please remove this method and keep it a plain struct. > > > > > > The argument by the other reviewer is that this is good to have as it allows > > the > > > user to test a config without having to crash the application once we set > > these > > > settings during construction time. > > > > Well, sure, but we could just as well handle that in the ctor then: either we > > DCHECK the config is valid and revert to some sane settings if it isn't, or we > > CHECK the config, if we're lazy or just don't believe we can recover. In the > > unlikely even that the client actually does DCHECK(Validate) on the config > > before giving it to the ctor, we still need to prepare for an invalid config. > > And besides, what is the client to do with the information? > > > > Additionally, none of the other places where we use construct-from-config > > semantics, employ a Validate() method. I know, consistency is the hobgoblin of > > little minds etc. :) But still, it makes sense to make the APIs work the same. > > > > There's a better way than all of this, of course, which is to use types for > the > > config parameters that enforce these constraints; ranges etc. > > We use this pattern for the AudioEncoders. Each implementation of the > AudioEncoder interface has a Config which can be passed to the ctor. The ctor > will crash on invalid configurations (silently modifying it to a valid config is > not tractable), and it is the responsibility of the caller to validate it. What > is the caller to do with a failed validation? Well, unlike the ctor, it can > probably flag an error, and maybe revert to a default config if that makes > sense. Acknowledged. https://codereview.webrtc.org/2292863002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:256: bool Validate() const; On 2016/09/02 09:37:05, the sun wrote: > On 2016/09/02 08:22:00, peah-webrtc wrote: > > On 2016/09/01 13:48:00, the sun wrote: > > > Please remove this method and keep it a plain struct. > > > > The argument by the other reviewer is that this is good to have as it allows > the > > user to test a config without having to crash the application once we set > these > > settings during construction time. > > Well, sure, but we could just as well handle that in the ctor then: either we > DCHECK the config is valid and revert to some sane settings if it isn't, or we > CHECK the config, if we're lazy or just don't believe we can recover. In the > unlikely even that the client actually does DCHECK(Validate) on the config > before giving it to the ctor, we still need to prepare for an invalid config. > And besides, what is the client to do with the information? > > Additionally, none of the other places where we use construct-from-config > semantics, employ a Validate() method. I know, consistency is the hobgoblin of > little minds etc. :) But still, it makes sense to make the APIs work the same. > > There's a better way than all of this, of course, which is to use types for the > config parameters that enforce these constraints; ranges etc. Types would be a good step on the way, but it is extremely hard, and becomes quite messy if generalized, to incorporate valid intra-parameter dependencies. An example of that (which can be eliminated in the long run using AEC modes, but for the sake of discussion lets assume it cannot), is the selection of the mobile aec and aec. As it is now, each ProcessStream call checks for whether these are activated at the same time and return an error. It would be much nicer if that error could instead be returned at the time of the creation of APM/at the time when the parameters in APM are set. Letting the ctor crash for such a setup is not the right thing to do and in my mind it would then be better to 1) Let the Validate() method flag that an improper setting is used. 2) Let the ctor default to the safest choice of only using the mobile aec in this case. https://codereview.webrtc.org/2292863002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2292863002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:886: RTC_DCHECK(audioproc->ApplyConfig(apm_config)); On 2016/09/06 08:56:03, hlundin-webrtc wrote: > Using a DCHECK here means that ApplyConfig is only called in debug mode. Not > your intention, I suppose. Ah, thanks! Done. https://codereview.webrtc.org/2292863002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:203: // if (valid_config) { On 2016/09/02 09:37:05, the sun wrote: > This doesn't make sense to me. Why would a client do this? See my comment below. https://codereview.webrtc.org/2292863002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:203: // if (valid_config) { On 2016/09/06 08:56:04, hlundin-webrtc wrote: > On 2016/09/02 09:37:05, the sun wrote: > > This doesn't make sense to me. Why would a client do this? > > I agree that it isn't necessary to do that before calling ApplyConfig. That > method could instead return an error, as it does now. > > My thinking was a bit ahead of the current implementation. I suppose we want to > be able to provide a Config to the APM ctor in the future. And in that case, the > user must validate the config before passing it, since the ctor itself has no > option but to crash if the config is invalid. I think it makes sense to do it like that. Eventually we want the parameters to be set at creation time, and then it makes sense for the user to be able to fail in a graceful manner for faulty parameters. Without the Validate method, the only way for the user to know that the parameters are faulty would be via a crash due to a check, or via some delayed error scheme. https://codereview.webrtc.org/2292863002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:251: // by changing the default values in the AudioProcessing::ApmConfig struct. On 2016/09/06 08:56:04, hlundin-webrtc wrote: > Not called ApmConfig anymore. Done. https://codereview.webrtc.org/2292863002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:253: // the On 2016/09/06 08:56:04, hlundin-webrtc wrote: > Incomplete sentence. Done.
I have added a new patchset according to what was discussed offline according to the config validation.
I have added a new patchset according to what was discussed offline according to the config validation.
https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:418: bool config_ok = LevelController::Validate(config_to_use.level_controller); I really like this; nice and clear. https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:428: rtc::CritScope cs_render(&crit_render_); Nice that you're making the locked section as small as possible! https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:244: // audio processing functionality. It is being introduced gradually and drop "functionality" and use all 80 chars of each line, pls. https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:245: // until it is fully introduced, it is prone to change. TODO(peah): Remove comment once new config scheme fully rolled out. https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:262: kMonoAndKeyboard, Not related but: if this is a logical AND, then there shouldn't be a comma in the above comment. (I'm equally confused by the below comment.) https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.cc:268: ss << "LevelController config: " Please use the json-ish way of string serialization that is used elsewhere for configs. See e.g. AudioReceiveStream::Config::ToString(). (I'm missing enclosing "{" "}". https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:277: DebugDumpGenerator generator(config, apm_config); Note: you could: DebugDumpGenerator generator(config, AudioProcessing::Config()); if you want to save a few lines.
Nice. A few more comments from me. https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:418: bool config_ok = LevelController::Validate(config_to_use.level_controller); On 2016/09/07 15:49:39, the sun wrote: > I really like this; nice and clear. Agree. But wouldn't it be nice if ApplyConfig returned a bool to signal if the config was valid or not? At least tests can be made to die a screaming death if they do not test what was intended. https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:424: config_to_use.level_controller = LevelController::DefaultConfig(); I don't think you need the DefaultConfig() function. Why can't you simply ask for a new level controller config struct? The ctor should give you the default config. https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:262: kMonoAndKeyboard, On 2016/09/07 15:49:39, the sun wrote: > Not related but: if this is a logical AND, then there shouldn't be a comma in > the above comment. (I'm equally confused by the below comment.) The below comment must be a cadence call. "Left, right! Keyboard mic! Left, right! Keyboard mic! ..." https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.cc:275: config.enabled = false; The default config should be the one you get from the constructor, imho. Thus, you should only need to return AudioProcessing::Config::LevelController(). But, as I alluded to previously, I don't think this method is needed.
https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:418: bool config_ok = LevelController::Validate(config_to_use.level_controller); On 2016/09/07 15:49:39, the sun wrote: > I really like this; nice and clear. Acknowledged. https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:418: bool config_ok = LevelController::Validate(config_to_use.level_controller); On 2016/09/07 21:13:05, hlundin-webrtc wrote: > On 2016/09/07 15:49:39, the sun wrote: > > I really like this; nice and clear. > > Agree. But wouldn't it be nice if ApplyConfig returned a bool to signal if the > config was valid or not? At least tests can be made to die a screaming death if > they do not test what was intended. I agree but my concern with that is that we would then use a scheme that we know we will not be using/do not want to use in the future once we can do this initialization in the constructor. I'm open to both variants though. WDYT? https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:424: config_to_use.level_controller = LevelController::DefaultConfig(); On 2016/09/07 21:13:05, hlundin-webrtc wrote: > I don't think you need the DefaultConfig() function. Why can't you simply ask > for a new level controller config struct? The ctor should give you the default > config. The Config is a struct that is defined inside audio_processing.h and I'm not convinced the constructor of that can be able to properly configure the submodules of APM. That would require logic in the submodules to be put into the Config functionality which I don't think we want to have. The best approach I could of think that does not require any settings logic oufside of the submodules is to ask the LevelController for what it thinks a properly configured default config should be. An option could be to send the faulty Config as an input to the DefaultConfig() method so that the submodule can do it's best to fix the parameters that are set. (Probably the method should then be renamed to FixConfig as you previously proposed). WDYT? How would we accomp https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:428: rtc::CritScope cs_render(&crit_render_); On 2016/09/07 15:49:39, the sun wrote: > Nice that you're making the locked section as small as possible! :-) https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:244: // audio processing functionality. It is being introduced gradually and On 2016/09/07 15:49:39, the sun wrote: > drop "functionality" and use all 80 chars of each line, pls. Done. https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:245: // until it is fully introduced, it is prone to change. On 2016/09/07 15:49:39, the sun wrote: > TODO(peah): Remove comment once new config scheme fully rolled out. Done. https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:262: kMonoAndKeyboard, On 2016/09/07 15:49:39, the sun wrote: > Not related but: if this is a logical AND, then there shouldn't be a comma in > the above comment. (I'm equally confused by the below comment.) Done. https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:262: kMonoAndKeyboard, On 2016/09/07 21:13:05, hlundin-webrtc wrote: > On 2016/09/07 15:49:39, the sun wrote: > > Not related but: if this is a logical AND, then there shouldn't be a comma in > > the above comment. (I'm equally confused by the below comment.) > > The below comment must be a cadence call. "Left, right! Keyboard mic! Left, > right! Keyboard mic! ..." Done. https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.cc:268: ss << "LevelController config: " On 2016/09/07 15:49:39, the sun wrote: > Please use the json-ish way of string serialization that is used elsewhere for > configs. See e.g. AudioReceiveStream::Config::ToString(). (I'm missing enclosing > "{" "}". Good suggestion! Done. https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.cc:275: config.enabled = false; On 2016/09/07 21:13:05, hlundin-webrtc wrote: > The default config should be the one you get from the constructor, imho. Thus, > you should only need to return AudioProcessing::Config::LevelController(). But, > as I alluded to previously, I don't think this method is needed. You are probably correct that it is not needed, and very likely this should be changed anyway to something along the FixConfig approach I mentioned in the other comment. . But I cannot see how to use the constructor in AudioProcessing::Config::LevelController() to produce default parameters without moving logic from within the level controller out to the audio_processing_impl.cc file. The only exception would be if the default value would be everything off. WDYT? Should I change this to AudioProcessing::Config::LevelController LevelController::FixConfig(AudioProcessing::Config::LevelController config), or should I let the constructor AudioProcessing::Config::LevelController() produce a default parameter set where everything is off? https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:277: DebugDumpGenerator generator(config, apm_config); On 2016/09/07 15:49:39, the sun wrote: > Note: you could: > DebugDumpGenerator generator(config, AudioProcessing::Config()); > if you want to save a few lines. Nice one! Done.
https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:424: config_to_use.level_controller = LevelController::DefaultConfig(); On 2016/09/08 09:13:48, peah-webrtc wrote: > On 2016/09/07 21:13:05, hlundin-webrtc wrote: > > I don't think you need the DefaultConfig() function. Why can't you simply ask > > for a new level controller config struct? The ctor should give you the default > > config. > > The Config is a struct that is defined inside audio_processing.h and I'm not > convinced the constructor of that can be able to properly configure the > submodules of APM. That would require logic in the submodules to be put into the > Config functionality which I don't think we want to have. > > The best approach I could of think that does not require any settings logic > oufside of the submodules is to ask the LevelController for what it thinks a > properly configured default config should be. > > An option could be to send the faulty Config as an input to the DefaultConfig() > method so that the submodule can do it's best to fix the parameters that are > set. (Probably the method should then be renamed to FixConfig as you previously > proposed). > > WDYT? > > > How would we accomp Going forward, I'd like to see the Config definitions move into their respective sub modules. Also, in this case we're talking about a single bool. Let's stick to just using the default ctor for now and introduce specialized methods to generate other default configs, when we actually need that. https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.cc:268: ss << "LevelController config: " On 2016/09/08 09:13:49, peah-webrtc wrote: > On 2016/09/07 15:49:39, the sun wrote: > > Please use the json-ish way of string serialization that is used elsewhere for > > configs. See e.g. AudioReceiveStream::Config::ToString(). (I'm missing > enclosing > > "{" "}". > > Good suggestion! > Done. Add a test for the function too. https://codereview.webrtc.org/2292863002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:201: // config.level_controller.enable = true; nit: enable -> enabled or change the name in the config struct. https://codereview.webrtc.org/2292863002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2292863002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.cc:268: ss << "{ LevelController config: " Sorry, I should've noticed: only name the individual fields and output their respective values (see audio_receive_stream.cc). The method calling this function should do e.g.: ss << "level_controller: " << level_controller.ToString(); So, no "LevelController config:" in this function. By following this approach, our log statements are more easily machine parseable. https://codereview.webrtc.org/2292863002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.cc:270: << "enabled=" << (config.enabled ? "true" : "false") << "}"; "enabled: "
https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:424: config_to_use.level_controller = LevelController::DefaultConfig(); On 2016/09/08 09:45:55, the sun wrote: > On 2016/09/08 09:13:48, peah-webrtc wrote: > > On 2016/09/07 21:13:05, hlundin-webrtc wrote: > > > I don't think you need the DefaultConfig() function. Why can't you simply > ask > > > for a new level controller config struct? The ctor should give you the > default > > > config. > > > > The Config is a struct that is defined inside audio_processing.h and I'm not > > convinced the constructor of that can be able to properly configure the > > submodules of APM. That would require logic in the submodules to be put into > the > > Config functionality which I don't think we want to have. > > > > The best approach I could of think that does not require any settings logic > > oufside of the submodules is to ask the LevelController for what it thinks a > > properly configured default config should be. > > > > An option could be to send the faulty Config as an input to the > DefaultConfig() > > method so that the submodule can do it's best to fix the parameters that are > > set. (Probably the method should then be renamed to FixConfig as you > previously > > proposed). > > > > WDYT? > > > > > > How would we accomp > > Going forward, I'd like to see the Config definitions move into their respective > sub modules. > > Also, in this case we're talking about a single bool. Let's stick to just using > the default ctor for now and introduce specialized methods to generate other > default configs, when we actually need that. That makes sense. Done. https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2292863002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.cc:268: ss << "LevelController config: " On 2016/09/08 09:45:55, the sun wrote: > On 2016/09/08 09:13:49, peah-webrtc wrote: > > On 2016/09/07 15:49:39, the sun wrote: > > > Please use the json-ish way of string serialization that is used elsewhere > for > > > configs. See e.g. AudioReceiveStream::Config::ToString(). (I'm missing > > enclosing > > > "{" "}". > > > > Good suggestion! > > Done. > > Add a test for the function too. Done. https://codereview.webrtc.org/2292863002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2292863002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:201: // config.level_controller.enable = true; On 2016/09/08 09:45:55, the sun wrote: > nit: enable -> enabled > or change the name in the config struct. Done. https://codereview.webrtc.org/2292863002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2292863002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.cc:268: ss << "{ LevelController config: " On 2016/09/08 09:45:55, the sun wrote: > Sorry, I should've noticed: only name the individual fields and output their > respective values (see audio_receive_stream.cc). The method calling this > function should do e.g.: > ss << "level_controller: " << level_controller.ToString(); > So, no "LevelController config:" in this function. > > By following this approach, our log statements are more easily machine > parseable. Done. https://codereview.webrtc.org/2292863002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.cc:270: << "enabled=" << (config.enabled ? "true" : "false") << "}"; On 2016/09/08 09:45:55, the sun wrote: > "enabled: " Done.
lgtm https://codereview.webrtc.org/2292863002/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2292863002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.cc:269: << "enabled:" << (config.enabled ? "true" : "false") << "}"; nit: space after the ":" https://codereview.webrtc.org/2292863002/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/2292863002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:341: AudioProcessing::Config apm_config; Why the inconsistency - compared to the above?
lgtm % solenberg's comments.
https://codereview.webrtc.org/2292863002/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2292863002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.cc:269: << "enabled:" << (config.enabled ? "true" : "false") << "}"; On 2016/09/08 20:32:27, the sun wrote: > nit: space after the ":" Done. https://codereview.webrtc.org/2292863002/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/2292863002/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:341: AudioProcessing::Config apm_config; On 2016/09/08 20:32:27, the sun wrote: > Why the inconsistency - compared to the above? Done.
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/2292863002/#ps240001 (title: "Rebase")
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: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/14213)
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/2292863002/#ps260001 (title: "Fixed bad merge")
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: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by peah@webrtc.org
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: android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by peah@webrtc.org
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 current scheme for setting parameters and specifying the behavior of the audio processing module is quite complex and hard to implement in a threadsafe and efficient manner. Therefore a new scheme for setting the parameters in the audio processing module is introduced in this CL. The idea is to roll this scheme out gradually and as a first functionality in the audio processing module where this is applied the level controller was chosen. This CL includes the replacement of the Config-based level controller scheme with the new scheme. BUG=webrtc:5298 ========== to ========== The current scheme for setting parameters and specifying the behavior of the audio processing module is quite complex and hard to implement in a threadsafe and efficient manner. Therefore a new scheme for setting the parameters in the audio processing module is introduced in this CL. The idea is to roll this scheme out gradually and as a first functionality in the audio processing module where this is applied the level controller was chosen. This CL includes the replacement of the Config-based level controller scheme with the new scheme. BUG=webrtc:5298 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== The current scheme for setting parameters and specifying the behavior of the audio processing module is quite complex and hard to implement in a threadsafe and efficient manner. Therefore a new scheme for setting the parameters in the audio processing module is introduced in this CL. The idea is to roll this scheme out gradually and as a first functionality in the audio processing module where this is applied the level controller was chosen. This CL includes the replacement of the Config-based level controller scheme with the new scheme. BUG=webrtc:5298 ========== to ========== The current scheme for setting parameters and specifying the behavior of the audio processing module is quite complex and hard to implement in a threadsafe and efficient manner. Therefore a new scheme for setting the parameters in the audio processing module is introduced in this CL. The idea is to roll this scheme out gradually and as a first functionality in the audio processing module where this is applied the level controller was chosen. This CL includes the replacement of the Config-based level controller scheme with the new scheme. BUG=webrtc:5298 Committed: https://crrev.com/c8bbe3fe9aad9e9a1189a42dcaa8f5d6c261ecc8 Cr-Commit-Position: refs/heads/master@{#14171} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/c8bbe3fe9aad9e9a1189a42dcaa8f5d6c261ecc8 Cr-Commit-Position: refs/heads/master@{#14171}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:260001) has been created in https://codereview.webrtc.org/2334583002/ by kjellander@webrtc.org. The reason for reverting is: Interface change in the mock breaks downstream code.. |
