Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2337083002:
Reland of added functionality for specifying the initial signal level to use for the gain estimation (Closed)
|
Created:
4 years, 3 months ago by peah-webrtc Modified:
4 years, 2 months ago CC:
webrtc-reviews_webrtc.org, 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. |
DescriptionThis CL adds functionality in the level controller to
receive a signal level to use initially, instead of the
default initial signal level.
The initial form of the CL
(https://codereview.webrtc.org/2254973003/) was reverted
due to down-stream dependencies. These have been resolved,
but the CL needed to be revised according to the new scheme
for passing parameters to the audio processing module.
Therefore, please review this CL as if it is new.
TBR=aleloi@webrtc.org
BUG=webrtc:6386
Committed: https://crrev.com/c19f312f54b9674621c0482d7c067043643308c1
Cr-Commit-Position: refs/heads/master@{#14579}
Patch Set 1 #
Total comments: 31
Patch Set 2 : Changes in response to reviewer comments #
Total comments: 7
Patch Set 3 : Changes in response to reviewer comments #Patch Set 4 : Changed parameter name from initial_level to the more correct initial_peak_level #
Total comments: 36
Patch Set 5 : Changes in response to reviewer comments #
Total comments: 4
Patch Set 6 : Changes in response to reviewer comments #
Total comments: 2
Patch Set 7 : Changed name of unit test #
Total comments: 32
Patch Set 8 : Changes in response to reviewer comments #
Total comments: 4
Patch Set 9 : Changes in response to reviewer comments #
Total comments: 10
Patch Set 10 : Rebase and changes in response to reviewer comments #Patch Set 11 : Fixed type conversion error #Messages
Total messages: 43 (13 generated)
Description was changed from ========== Reland Changing the functionality to use the new parameter setting scheme Changing the functionality to use the new parameter setting scheme BUG= ========== to ========== This CL adds functionality in the level controller to receive a signal level to use initially, instead of the default initial signal level. The initial form of the CL (https://codereview.webrtc.org/2254973003/) was reverted due to down-stream dependencies. These have been resolved, but the CL needed to be revised according to the new scheme for passing parameters to the audio processing module. Therefore, please review this CL as if it is new. BUG= ==========
peah@webrtc.org changed reviewers: + aleloi@webrtc.org, henrik.lundin@webrtc.org, solenberg@webrtc.org
Description was changed from ========== This CL adds functionality in the level controller to receive a signal level to use initially, instead of the default initial signal level. The initial form of the CL (https://codereview.webrtc.org/2254973003/) was reverted due to down-stream dependencies. These have been resolved, but the CL needed to be revised according to the new scheme for passing parameters to the audio processing module. Therefore, please review this CL as if it is new. BUG= ========== to ========== This CL adds functionality in the level controller to receive a signal level to use initially, instead of the default initial signal level. The initial form of the CL (https://codereview.webrtc.org/2254973003/) was reverted due to down-stream dependencies. These have been resolved, but the CL needed to be revised according to the new scheme for passing parameters to the audio processing module. Therefore, please review this CL as if it is new. BUG= ==========
https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing.gypi (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing.gypi:63: 'include/audio_processing.cc', Stick to alphabetical order https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:548: } Why change the previous code? You could just have put the call to: private_submodules_->level_controller->ApplyConfig(config_to_use.level_controller); beneath the CritScope:s. Also, it looks like that code needs to go after the below initialization of the LC, otherwise the initial level may be ignored? Note further, that we tripped up the logic below in the previous CL to introduce the Config scheme: you are ignoring whatever LC::Validate() has to say about the config and instead using the enabled flag from the config we've been given as argument (even though it might be set differently in the default state of LC::Config. The below code should use "config_to_use", not "config". https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/include/audio_processing.cc (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/include/audio_processing.cc:39: AudioProcessing::Config::LevelController::LevelController() {} Note that if you wish, you can be fancy and write this as: AudioProcessing::Config::LevelController::LevelController() = default; https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/include/audio_processing.h:256: ~LevelController(); is the dtor really necessary? https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/include/audio_processing.h:262: rtc::Optional<float> initial_level; Why don't we always have an initial level? https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.cc:300: (*config.initial_level < 0.1f && *config.initial_level > -100.1f); Should you be comparing with an epsilon? https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/level_controller/level_controller.h (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.h:42: // over the parameters in the audio processing module and is likely to change. I don't understand this comment. How do we take control over the APMs parameters? We only see the LCs parameters? We just apply those, like the name says, latch them in, if you're EE. Would be better to explain why it is a temporary solution; it should be done in ctor instead. https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.h:52: void SetInitialLevel(float level); Where is this used? The idea of setting up from a Config struct is to avoid individual setters. https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc:36: level_controller.SetInitialLevel(*initial_level); Why not use ApplyConfig?
https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:548: } On 2016/09/14 10:00:59, the sun wrote: > Why change the previous code? You could just have put the call to: > private_submodules_->level_controller->ApplyConfig(config_to_use.level_controller); > beneath the CritScope:s. > > Also, it looks like that code needs to go after the below initialization of the > LC, otherwise the initial level may be ignored? > > Note further, that we tripped up the logic below in the previous CL to introduce > the Config scheme: you are ignoring whatever LC::Validate() has to say about the > config and instead using the enabled flag from the config we've been given as > argument (even though it might be set differently in the default state of > LC::Config. The below code should use "config_to_use", not "config". If I'm not mistaken about that, it seems there's one or two test cases missing...
No more comments to add. https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:548: } On 2016/09/14 17:02:15, the sun wrote: > On 2016/09/14 10:00:59, the sun wrote: > > Why change the previous code? You could just have put the call to: > > > private_submodules_->level_controller->ApplyConfig(config_to_use.level_controller); > > beneath the CritScope:s. > > > > Also, it looks like that code needs to go after the below initialization of > the > > LC, otherwise the initial level may be ignored? > > > > Note further, that we tripped up the logic below in the previous CL to > introduce > > the Config scheme: you are ignoring whatever LC::Validate() has to say about > the > > config and instead using the enabled flag from the config we've been given as > > argument (even though it might be set differently in the default state of > > LC::Config. The below code should use "config_to_use", not "config". > > If I'm not mistaken about that, it seems there's one or two test cases > missing... +1 on all of the above.
Actually, one more nit. https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:21: const float kMinLevel = 30.f; constexpr
https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing.gypi (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing.gypi:63: 'include/audio_processing.cc', On 2016/09/14 10:00:59, the sun wrote: > Stick to alphabetical order Oups! Thanks!!! Done. https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:548: } On 2016/09/14 17:02:15, the sun wrote: > On 2016/09/14 10:00:59, the sun wrote: > > Why change the previous code? You could just have put the call to: > > > private_submodules_->level_controller->ApplyConfig(config_to_use.level_controller); > > beneath the CritScope:s. > > > > Also, it looks like that code needs to go after the below initialization of > the > > LC, otherwise the initial level may be ignored? > > > > Note further, that we tripped up the logic below in the previous CL to > introduce > > the Config scheme: you are ignoring whatever LC::Validate() has to say about > the > > config and instead using the enabled flag from the config we've been given as > > argument (even though it might be set differently in the default state of > > LC::Config. The below code should use "config_to_use", not "config". > > If I'm not mistaken about that, it seems there's one or two test cases > missing... Very, very likely. What I guess you mean is to check whether falsy config input gives nonproper APM initialization behavior? Right? https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:548: } On 2016/09/15 07:50:17, hlundin-webrtc wrote: > On 2016/09/14 17:02:15, the sun wrote: > > On 2016/09/14 10:00:59, the sun wrote: > > > Why change the previous code? You could just have put the call to: > > > > > > private_submodules_->level_controller->ApplyConfig(config_to_use.level_controller); > > > beneath the CritScope:s. > > > > > > Also, it looks like that code needs to go after the below initialization of > > the > > > LC, otherwise the initial level may be ignored? > > > > > > Note further, that we tripped up the logic below in the previous CL to > > introduce > > > the Config scheme: you are ignoring whatever LC::Validate() has to say about > > the > > > config and instead using the enabled flag from the config we've been given > as > > > argument (even though it might be set differently in the default state of > > > LC::Config. The below code should use "config_to_use", not "config". > > > > If I'm not mistaken about that, it seems there's one or two test cases > > missing... > > +1 on all of the above. Acknowledged. https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:548: } On 2016/09/14 10:00:59, the sun wrote: > Why change the previous code? You could just have put the call to: > private_submodules_->level_controller->ApplyConfig(config_to_use.level_controller); > beneath the CritScope:s. > > Also, it looks like that code needs to go after the below initialization of the > LC, otherwise the initial level may be ignored? > > Note further, that we tripped up the logic below in the previous CL to introduce > the Config scheme: you are ignoring whatever LC::Validate() has to say about the > config and instead using the enabled flag from the config we've been given as > argument (even though it might be set differently in the default state of > LC::Config. The below code should use "config_to_use", not "config". The reason for moving the critscopes is that 1) private_submodules_ can only be accessed while holding crit_capture_ 2) crit_render_ must always be aquired before crit_capture_. 3) Since the AudioProcessing::Config is no longer a POD, and I don't want to add a copy/assignment constructor if not strictly neccessary, I cannot simply copy the config. Regarding the enable flag, good find. Will fix that once we decide on the copy/assignment constructor issue. I have a feeling that we need to add a copy/assignment constructor for the ::Config struct since in order to 1) simplify this method which will grow in complexity as we add more methods. 2) allow caching a copy of the config in the APM state. WDYT? https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/include/audio_processing.cc (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/include/audio_processing.cc:39: AudioProcessing::Config::LevelController::LevelController() {} On 2016/09/14 10:00:59, the sun wrote: > Note that if you wish, you can be fancy and write this as: > > AudioProcessing::Config::LevelController::LevelController() = default; Done. https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/include/audio_processing.h:256: ~LevelController(); On 2016/09/14 10:00:59, the sun wrote: > is the dtor really necessary? clang complains if I remove it. So I guess it is needed. https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/include/audio_processing.h:262: rtc::Optional<float> initial_level; On 2016/09/14 10:00:59, the sun wrote: > Why don't we always have an initial level? We do. One way would be to hardcode the default initial value as a default parameter, but I thought it was better to hardcode the default initial value as a constant that is local to the level controller. I thought it made sense to gather the code that controls the LevelController behavior in one place. WDYT? https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.cc:300: (*config.initial_level < 0.1f && *config.initial_level > -100.1f); On 2016/09/14 10:00:59, the sun wrote: > Should you be comparing with an epsilon? I added a variant of that. Please take another look to see whether it looks ok! Done. https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/level_controller/level_controller.h (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.h:42: // over the parameters in the audio processing module and is likely to change. On 2016/09/14 10:00:59, the sun wrote: > I don't understand this comment. How do we take control over the APMs > parameters? We only see the LCs parameters? We just apply those, like the name > says, latch them in, if you're EE. > > Would be better to explain why it is a temporary solution; it should be done in > ctor instead. Totally agree. Done. https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller.h:52: void SetInitialLevel(float level); On 2016/09/14 10:00:59, the sun wrote: > Where is this used? The idea of setting up from a Config struct is to avoid > individual setters. This is a leftover from the former CL. Sorry about that! Removed it. Done. https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc:36: level_controller.SetInitialLevel(*initial_level); On 2016/09/14 10:00:59, the sun wrote: > Why not use ApplyConfig? Done. https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:21: const float kMinLevel = 30.f; On 2016/09/15 07:54:07, hlundin-webrtc wrote: > constexpr Done.
https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:548: } On 2016/09/16 07:11:06, peah-webrtc wrote: > On 2016/09/14 17:02:15, the sun wrote: > > On 2016/09/14 10:00:59, the sun wrote: > > > Why change the previous code? You could just have put the call to: > > > > > > private_submodules_->level_controller->ApplyConfig(config_to_use.level_controller); > > > beneath the CritScope:s. > > > > > > Also, it looks like that code needs to go after the below initialization of > > the > > > LC, otherwise the initial level may be ignored? > > > > > > Note further, that we tripped up the logic below in the previous CL to > > introduce > > > the Config scheme: you are ignoring whatever LC::Validate() has to say about > > the > > > config and instead using the enabled flag from the config we've been given > as > > > argument (even though it might be set differently in the default state of > > > LC::Config. The below code should use "config_to_use", not "config". > > > > If I'm not mistaken about that, it seems there's one or two test cases > > missing... > > Very, very likely. What I guess you mean is to check whether falsy config input > gives nonproper APM initialization behavior? Right? Yes, something like that - figure out a test to trigger the error I spotted, with using the wrong config object when deciding whether to enable the LC. https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:548: } On 2016/09/16 07:11:06, peah-webrtc wrote: > On 2016/09/14 10:00:59, the sun wrote: > > Why change the previous code? You could just have put the call to: > > > private_submodules_->level_controller->ApplyConfig(config_to_use.level_controller); > > beneath the CritScope:s. > > > > Also, it looks like that code needs to go after the below initialization of > the > > LC, otherwise the initial level may be ignored? > > > > Note further, that we tripped up the logic below in the previous CL to > introduce > > the Config scheme: you are ignoring whatever LC::Validate() has to say about > the > > config and instead using the enabled flag from the config we've been given as > > argument (even though it might be set differently in the default state of > > LC::Config. The below code should use "config_to_use", not "config". > > The reason for moving the critscopes is that > 1) private_submodules_ can only be accessed while holding crit_capture_ > 2) crit_render_ must always be aquired before crit_capture_. Yes, so figure out the config state before taking the locks, and only call ApplyConfig later on (i.e. after enable/disable of the module). > 3) Since the AudioProcessing::Config is no longer a POD, and I don't want to add > a copy/assignment constructor if not strictly neccessary, I cannot simply copy > the config. Right - so if we can instead set the initial level as a default in the config, we wouldn't need the Optional which forces us have explicit copy/assignment/ctor/dtor. Plus, the code wouldn't have to deal with the (tiny bit of) extra ambiguity. Sounds like a win to me. :) > Regarding the enable flag, good find. Will fix that once we decide on the > copy/assignment constructor issue. > > I have a feeling that we need to add a copy/assignment constructor for the > ::Config struct since in order to > 1) simplify this method which will grow in complexity as we add more methods. > 2) allow caching a copy of the config in the APM state. > > WDYT? > I think the longer we can stick to only primitive types in the config, the better. You may be right that eventually we'll need to add copy/assignment, but let's pretend the world is all fluff and rainbows and cross that bridge once we need to. https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/include/audio_processing.h:262: rtc::Optional<float> initial_level; On 2016/09/16 07:11:06, peah-webrtc wrote: > On 2016/09/14 10:00:59, the sun wrote: > > Why don't we always have an initial level? > > We do. One way would be to hardcode the default initial value as a default > parameter, but I thought it was better to hardcode the default initial value as > a constant that is local to the level controller. I thought it made sense to > gather the code that controls the LevelController behavior in one place. > > WDYT? If I picture myself as someone who'd consider using the LC, I'd actually prefer having the default value right here in the Config. For one thing, if the default is changed down the line, it'd be easier for me to spot that, since it would be visible as an interface change. https://codereview.webrtc.org/2337083002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2337083002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/level_controller.cc:295: (*config.initial_level < std::numeric_limits<float>::epsilon() && In the future, consider making a utility for this (if there isn't one in the std lib) - e.g. InRange(min, x, max) https://codereview.webrtc.org/2337083002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc:35: if (initial_level) { Note, since you currently have an optional in the struct, you could just assign to that, no need to check the argument here is set.
https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:548: } On 2016/09/16 08:00:40, the sun wrote: > On 2016/09/16 07:11:06, peah-webrtc wrote: > > On 2016/09/14 10:00:59, the sun wrote: > > > Why change the previous code? You could just have put the call to: > > > > > > private_submodules_->level_controller->ApplyConfig(config_to_use.level_controller); > > > beneath the CritScope:s. > > > > > > Also, it looks like that code needs to go after the below initialization of > > the > > > LC, otherwise the initial level may be ignored? > > > > > > Note further, that we tripped up the logic below in the previous CL to > > introduce > > > the Config scheme: you are ignoring whatever LC::Validate() has to say about > > the > > > config and instead using the enabled flag from the config we've been given > as > > > argument (even though it might be set differently in the default state of > > > LC::Config. The below code should use "config_to_use", not "config". > > > > The reason for moving the critscopes is that > > 1) private_submodules_ can only be accessed while holding crit_capture_ > > 2) crit_render_ must always be aquired before crit_capture_. > > Yes, so figure out the config state before taking the locks, and only call > ApplyConfig later on (i.e. after enable/disable of the module). > > > 3) Since the AudioProcessing::Config is no longer a POD, and I don't want to > add > > a copy/assignment constructor if not strictly neccessary, I cannot simply copy > > the config. > > Right - so if we can instead set the initial level as a default in the config, > we wouldn't need the Optional which forces us have explicit > copy/assignment/ctor/dtor. Plus, the code wouldn't have to deal with the (tiny > bit of) extra ambiguity. Sounds like a win to me. :) > > > Regarding the enable flag, good find. Will fix that once we decide on the > > copy/assignment constructor issue. > > > > I have a feeling that we need to add a copy/assignment constructor for the > > ::Config struct since in order to > > 1) simplify this method which will grow in complexity as we add more methods. > > 2) allow caching a copy of the config in the APM state. > > > > WDYT? > > > > I think the longer we can stick to only primitive types in the config, the > better. You may be right that eventually we'll need to add copy/assignment, but > let's pretend the world is all fluff and rainbows and cross that bridge once we > need to. Acknowledged.
https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:548: } On 2016/09/16 08:00:40, the sun wrote: > On 2016/09/16 07:11:06, peah-webrtc wrote: > > On 2016/09/14 10:00:59, the sun wrote: > > > Why change the previous code? You could just have put the call to: > > > > > > private_submodules_->level_controller->ApplyConfig(config_to_use.level_controller); > > > beneath the CritScope:s. > > > > > > Also, it looks like that code needs to go after the below initialization of > > the > > > LC, otherwise the initial level may be ignored? > > > > > > Note further, that we tripped up the logic below in the previous CL to > > introduce > > > the Config scheme: you are ignoring whatever LC::Validate() has to say about > > the > > > config and instead using the enabled flag from the config we've been given > as > > > argument (even though it might be set differently in the default state of > > > LC::Config. The below code should use "config_to_use", not "config". > > > > The reason for moving the critscopes is that > > 1) private_submodules_ can only be accessed while holding crit_capture_ > > 2) crit_render_ must always be aquired before crit_capture_. > > Yes, so figure out the config state before taking the locks, and only call > ApplyConfig later on (i.e. after enable/disable of the module). > > > 3) Since the AudioProcessing::Config is no longer a POD, and I don't want to > add > > a copy/assignment constructor if not strictly neccessary, I cannot simply copy > > the config. > > Right - so if we can instead set the initial level as a default in the config, > we wouldn't need the Optional which forces us have explicit > copy/assignment/ctor/dtor. Plus, the code wouldn't have to deal with the (tiny > bit of) extra ambiguity. Sounds like a win to me. :) > > > Regarding the enable flag, good find. Will fix that once we decide on the > > copy/assignment constructor issue. > > > > I have a feeling that we need to add a copy/assignment constructor for the > > ::Config struct since in order to > > 1) simplify this method which will grow in complexity as we add more methods. > > 2) allow caching a copy of the config in the APM state. > > > > WDYT? > > > > I think the longer we can stick to only primitive types in the config, the > better. You may be right that eventually we'll need to add copy/assignment, but > let's pretend the world is all fluff and rainbows and cross that bridge once we > need to. I really like rainbows and fluffy clouds! I'll include that level into the state. https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:548: } On 2016/09/16 08:17:16, hlundin-webrtc wrote: > On 2016/09/16 08:00:40, the sun wrote: > > On 2016/09/16 07:11:06, peah-webrtc wrote: > > > On 2016/09/14 10:00:59, the sun wrote: > > > > Why change the previous code? You could just have put the call to: > > > > > > > > > > private_submodules_->level_controller->ApplyConfig(config_to_use.level_controller); > > > > beneath the CritScope:s. > > > > > > > > Also, it looks like that code needs to go after the below initialization > of > > > the > > > > LC, otherwise the initial level may be ignored? > > > > > > > > Note further, that we tripped up the logic below in the previous CL to > > > introduce > > > > the Config scheme: you are ignoring whatever LC::Validate() has to say > about > > > the > > > > config and instead using the enabled flag from the config we've been given > > as > > > > argument (even though it might be set differently in the default state of > > > > LC::Config. The below code should use "config_to_use", not "config". > > > > > > The reason for moving the critscopes is that > > > 1) private_submodules_ can only be accessed while holding crit_capture_ > > > 2) crit_render_ must always be aquired before crit_capture_. > > > > Yes, so figure out the config state before taking the locks, and only call > > ApplyConfig later on (i.e. after enable/disable of the module). > > > > > 3) Since the AudioProcessing::Config is no longer a POD, and I don't want to > > add > > > a copy/assignment constructor if not strictly neccessary, I cannot simply > copy > > > the config. > > > > Right - so if we can instead set the initial level as a default in the config, > > we wouldn't need the Optional which forces us have explicit > > copy/assignment/ctor/dtor. Plus, the code wouldn't have to deal with the (tiny > > bit of) extra ambiguity. Sounds like a win to me. :) > > > > > Regarding the enable flag, good find. Will fix that once we decide on the > > > copy/assignment constructor issue. > > > > > > I have a feeling that we need to add a copy/assignment constructor for the > > > ::Config struct since in order to > > > 1) simplify this method which will grow in complexity as we add more > methods. > > > 2) allow caching a copy of the config in the APM state. > > > > > > WDYT? > > > > > > > I think the longer we can stick to only primitive types in the config, the > > better. You may be right that eventually we'll need to add copy/assignment, > but > > let's pretend the world is all fluff and rainbows and cross that bridge once > we > > need to. > > Acknowledged. Acknowledged. https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2337083002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/include/audio_processing.h:262: rtc::Optional<float> initial_level; On 2016/09/16 08:00:40, the sun wrote: > On 2016/09/16 07:11:06, peah-webrtc wrote: > > On 2016/09/14 10:00:59, the sun wrote: > > > Why don't we always have an initial level? > > > > We do. One way would be to hardcode the default initial value as a default > > parameter, but I thought it was better to hardcode the default initial value > as > > a constant that is local to the level controller. I thought it made sense to > > gather the code that controls the LevelController behavior in one place. > > > > WDYT? > > If I picture myself as someone who'd consider using the LC, I'd actually prefer > having the default value right here in the Config. For one thing, if the default > is changed down the line, it'd be easier for me to spot that, since it would be > visible as an interface change. That is true. I agree. https://codereview.webrtc.org/2337083002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.cc (left): https://codereview.webrtc.org/2337083002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.cc:36: return -1; I have now removed the ctor and dtor for the levelcontroller config. However, I figured it would still make sense to move this file to the same folder as audio_processing.h. WDYT? https://codereview.webrtc.org/2337083002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2337083002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/level_controller.cc:295: (*config.initial_level < std::numeric_limits<float>::epsilon() && On 2016/09/16 08:00:40, the sun wrote: > In the future, consider making a utility for this (if there isn't one in the std > lib) - e.g. InRange(min, x, max) Acknowledged. https://codereview.webrtc.org/2337083002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc:35: if (initial_level) { On 2016/09/16 08:00:40, the sun wrote: > Note, since you currently have an optional in the struct, you could just assign > to that, no need to check the argument here is set. True! This is, however, now changed and therefore done differently.
https://codereview.webrtc.org/2337083002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.cc (left): https://codereview.webrtc.org/2337083002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.cc:36: return -1; On 2016/09/16 11:36:05, peah-webrtc wrote: > I have now removed the ctor and dtor for the levelcontroller config. > > However, I figured it would still make sense to move this file to the same > folder as audio_processing.h. > > WDYT? I'm ok with that. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:539: config_.level_controller = default_config; nit: Do this in one line. No need to name temporary variables, if not required. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:551: config.level_controller.enabled; config_ where is the test case? nit: it is nice if the conditional and the assignment is ordered the same way, for these types of constructs. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:558: } Did you forget lc->ApplyConfig()? https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_unittest.cc:2789: // that is created only when the that is specified in the config. "the that" https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_unittest.cc:2810: apm.reset(new AudioProcessingImpl(webrtc::Config())); Split up into several test cases instead - it shouldn't be too much work/code since you're already adding lines to reset the state. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:23: #include "webrtc/base/optional.h" remove https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:260: float initial_peak_level = -6.0206; -6.0206f adding _db to the parameter name would make code using it easier to understand. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc:82: TEST(LevelControllerConfig, ToString) { Thank you, for not using fixtures! https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:44: initial_peak_level_ = std::max(linear_level, kMinLevel); Why is this variable necessary? Why not just an argument to Initialize()? IIUC the user needs to first SetInitialPeakLevel(), then call Initialize() for it to have any effect. That's a strange API to me. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.h (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.h:36: float initial_peak_level_ = kTargetLcPeakLevel; Is this default value really needed? Won't it be set up from the LC default config and ApplyConfig when the LC is initialized?
https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_unittest.cc:2781: // Verify that the level controller is default off, it can be activate using activate -> activated https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_unittest.cc:2783: // config has been appied. appied -> applied https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_unittest.cc:2810: apm.reset(new AudioProcessingImpl(webrtc::Config())); On 2016/09/16 12:40:24, the sun wrote: > Split up into several test cases instead - it shouldn't be too much work/code > since you're already adding lines to reset the state. Acknowledged. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:260: float initial_peak_level = -6.0206; On 2016/09/16 12:40:24, the sun wrote: > -6.0206f > > adding _db to the parameter name would make code using it easier to understand. Acknowledged. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc:21: #include "gtest/gtest.h" Why this special case for Android?
There should probably be a monorail issue for this and the remaining work to initialize the initial level controller state from peer connection constraints. There are three methods in LevelController that set the initial level in PeekLevelEstimator. I think that makes it a bit difficult to see how initialization happens. Granted, the methods are very different (constructor, initialize, apply config), and I haven't followed the discussion very closely, but they are all called in the default case. I couldn't think of any simplifications besides merging |LevelController::Initialize()| and |ApplyOptions(config)|, which I think is not ideal, since it adds conditional logic to Initialize. WDYT? Otherwise, I have no comments. https://codereview.webrtc.org/2337083002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_unittest.cc:2794: EXPECT_NEAR(-6.0206f, apm->config_.level_controller.initial_peak_level_dbfs, Maybe we should compare with the value in lc_constants.h instead? This comment applies to all comparisons with the default value. https://codereview.webrtc.org/2337083002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_unittest.cc:2800: // that is created only when the that is specified in the config. 'the that' - remove 'the'
On 2016/09/19 15:06:06, aleloi wrote: > There should probably be a monorail issue for this and the remaining work to > initialize the initial level controller state from peer connection constraints. > > There are three methods in LevelController that set the initial level in > PeekLevelEstimator. I think that makes it a bit difficult to see how > initialization happens. Granted, the methods are very different (constructor, > initialize, apply config), and I haven't followed the discussion very closely, > but they are all called in the default case. I couldn't think of any > simplifications besides merging |LevelController::Initialize()| and > |ApplyOptions(config)|, which I think is not ideal, since it adds conditional > logic to Initialize. > > WDYT? > > Otherwise, I have no comments. > > https://codereview.webrtc.org/2337083002/diff/80001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): > > https://codereview.webrtc.org/2337083002/diff/80001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_unittest.cc:2794: > EXPECT_NEAR(-6.0206f, apm->config_.level_controller.initial_peak_level_dbfs, > Maybe we should compare with the value in lc_constants.h instead? This comment > applies to all comparisons with the default value. > > https://codereview.webrtc.org/2337083002/diff/80001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_unittest.cc:2800: // that is > created only when the that is specified in the config. > 'the that' - remove 'the' That is a great point! I will create an issue and add it to the CL. You definitely have a great point that it is a bit confusing with how the level is set, and I think that it all boils down to the fact that the level controller allows reinitialization, which means that the initial level needs to be cached. Furthermore, as the config (initial peak level) is not set in the constructor a default value is needed to be set for the initial peak level there as well. (I'm not sure about the three methods though, as the the constructor and the setter belongs to the internal PeakLevelEstimator class and the ApplyConfig belongs to the LevelController "external" API. I think that the changes that were made in the latest patch makes things better, and it will be even more better when we start recreating the level controller upon initialization. That is, however, not something that should happen in this CL though, as this CL is already a bit too big. Please take a look at the new scheme and let me know what you think!
Description was changed from ========== This CL adds functionality in the level controller to receive a signal level to use initially, instead of the default initial signal level. The initial form of the CL (https://codereview.webrtc.org/2254973003/) was reverted due to down-stream dependencies. These have been resolved, but the CL needed to be revised according to the new scheme for passing parameters to the audio processing module. Therefore, please review this CL as if it is new. BUG= ========== to ========== This CL adds functionality in the level controller to receive a signal level to use initially, instead of the default initial signal level. The initial form of the CL (https://codereview.webrtc.org/2254973003/) was reverted due to down-stream dependencies. These have been resolved, but the CL needed to be revised according to the new scheme for passing parameters to the audio processing module. Therefore, please review this CL as if it is new. BUG=webrtc:6386 ==========
https://codereview.webrtc.org/2337083002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.cc (left): https://codereview.webrtc.org/2337083002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.cc:36: return -1; On 2016/09/16 12:40:24, the sun wrote: > On 2016/09/16 11:36:05, peah-webrtc wrote: > > I have now removed the ctor and dtor for the levelcontroller config. > > > > However, I figured it would still make sense to move this file to the same > > folder as audio_processing.h. > > > > WDYT? > > I'm ok with that. Acknowledged. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:539: config_.level_controller = default_config; On 2016/09/16 12:40:24, the sun wrote: > nit: Do this in one line. No need to name temporary variables, if not required. Done. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:551: config.level_controller.enabled; On 2016/09/16 12:40:24, the sun wrote: > config_ > where is the test case? > > nit: it is nice if the conditional and the assignment is ordered the same way, > for these types of constructs. The test case is in audio_processing_unittests.cc. But maybe some variant of the test is missing there? Please let me know then! Changed the ordering and changed to using config_ instead of config. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:558: } On 2016/09/16 12:40:24, the sun wrote: > Did you forget lc->ApplyConfig()? Oh no! Great find! I for sure did miss that. I'm not sure if we should add a unittest to avoid that though. It becomes quite messy in the long run with all the friend-class statements. The best thing would be to add tests that verify the effect of the level controller. WDYT, is it ok to leave it without this without any further testing to avoid this? https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_unittest.cc:2781: // Verify that the level controller is default off, it can be activate using On 2016/09/19 12:03:03, hlundin-webrtc wrote: > activate -> activated Done. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_unittest.cc:2783: // config has been appied. On 2016/09/19 12:03:03, hlundin-webrtc wrote: > appied -> applied Done. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_unittest.cc:2789: // that is created only when the that is specified in the config. On 2016/09/16 12:40:24, the sun wrote: > "the that" Done. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_unittest.cc:2810: apm.reset(new AudioProcessingImpl(webrtc::Config())); On 2016/09/16 12:40:24, the sun wrote: > Split up into several test cases instead - it shouldn't be too much work/code > since you're already adding lines to reset the state. I'm all in for that, but in order to limit the number of FRIEND_TEST_ALL_PREFIXES lines in the AudioProcessingImpl class when the other submodules are also transitioned into the new parameter scheme I'll group the tests into 4 categories that suit all submodules. Please let me know what you think of that! Done. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_unittest.cc:2810: apm.reset(new AudioProcessingImpl(webrtc::Config())); On 2016/09/19 12:03:03, hlundin-webrtc wrote: > On 2016/09/16 12:40:24, the sun wrote: > > Split up into several test cases instead - it shouldn't be too much work/code > > since you're already adding lines to reset the state. > > Acknowledged. Acknowledged. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:23: #include "webrtc/base/optional.h" On 2016/09/16 12:40:24, the sun wrote: > remove Done. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:260: float initial_peak_level = -6.0206; On 2016/09/16 12:40:24, the sun wrote: > -6.0206f > > adding _db to the parameter name would make code using it easier to understand. Done. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:260: float initial_peak_level = -6.0206; On 2016/09/19 12:03:03, hlundin-webrtc wrote: > On 2016/09/16 12:40:24, the sun wrote: > > -6.0206f > > > > adding _db to the parameter name would make code using it easier to > understand. > > Acknowledged. Acknowledged. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc:21: #include "gtest/gtest.h" On 2016/09/19 12:03:03, hlundin-webrtc wrote: > Why this special case for Android? I have no idea. Sorry, but I got that from audio_processing_unittests.cc. I'll change so that it is always the same for both places. Done. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc:82: TEST(LevelControllerConfig, ToString) { On 2016/09/16 12:40:25, the sun wrote: > Thank you, for not using fixtures! My sincere pleasure :-) https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:44: initial_peak_level_ = std::max(linear_level, kMinLevel); On 2016/09/16 12:40:25, the sun wrote: > Why is this variable necessary? Why not just an argument to Initialize()? IIUC > the user needs to first SetInitialPeakLevel(), then call Initialize() for it to > have any effect. That's a strange API to me. Firstly, note that this is not an formal API but rather an internal API for the level controller subfunctionality. Therefore, the user in this case is the level controller. The current code construct does not mean that the user of the level controller needs to do what you describe. I believe this construct is needed in order to avoid having the levelcontroller cache the value that is set. Basically, this constitutes a way for caching the initial level inside the peak_level_estimator and then allow subsequent reinitialization (where typically the ApplyConfig call is not made). The caching can for sure be done differently, but this is one way to do it. However,... As I think we anyway will eventually en up doing reconstruction instead of reinitialization, and it is better to cache the config instead of caching the separate value of the config where it is used, I'll rewrite this. PTAL. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.h (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.h:36: float initial_peak_level_ = kTargetLcPeakLevel; On 2016/09/16 12:40:25, the sun wrote: > Is this default value really needed? Won't it be set up from the LC default > config and ApplyConfig when the LC is initialized? Not in the way it is now, as it basically constitutes the cached value from the config which is reused each time the level controller is reinitialized (for reasons such as sample rate change, changes in the number of channels, etc.) This is, however, changed in response to another of your comments. Done. https://codereview.webrtc.org/2337083002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_unittest.cc:2794: EXPECT_NEAR(-6.0206f, apm->config_.level_controller.initial_peak_level_dbfs, On 2016/09/19 15:06:06, aleloi wrote: > Maybe we should compare with the value in lc_constants.h instead? This comment > applies to all comparisons with the default value. Good point! Done. https://codereview.webrtc.org/2337083002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_unittest.cc:2800: // that is created only when the that is specified in the config. On 2016/09/19 15:06:06, aleloi wrote: > 'the that' - remove 'the' Done.
https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:551: config.level_controller.enabled; On 2016/09/19 16:37:10, peah-webrtc wrote: > On 2016/09/16 12:40:24, the sun wrote: > > config_ > > where is the test case? > > > > nit: it is nice if the conditional and the assignment is ordered the same way, > > for these types of constructs. > > The test case is in audio_processing_unittests.cc. But maybe some variant of the > test is missing there? Please let me know then! > > Changed the ordering and changed to using config_ instead of config. Ah, the problem with the test is it checks the flag in config_.level_controller. Hm. You're right, it does become a mess with all the friend declarations, plus the white box nature of the test will become a burden. Ideally, we'd have had a factory method for the LC given to the APM on c-tion - then we could test out the logic without really creating an LC. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:558: } On 2016/09/19 16:37:10, peah-webrtc wrote: > On 2016/09/16 12:40:24, the sun wrote: > > Did you forget lc->ApplyConfig()? > > Oh no! Great find! I for sure did miss that. > > I'm not sure if we should add a unittest to avoid that though. It becomes quite > messy in the long run with all the friend-class statements. The best thing would > be to add tests that verify the effect of the level controller. > > WDYT, is it ok to leave it without this without any further testing to avoid > this? Let's discuss offline. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_unittest.cc:2810: apm.reset(new AudioProcessingImpl(webrtc::Config())); On 2016/09/19 16:37:10, peah-webrtc wrote: > On 2016/09/16 12:40:24, the sun wrote: > > Split up into several test cases instead - it shouldn't be too much work/code > > since you're already adding lines to reset the state. > > I'm all in for that, but in order to limit the number of > FRIEND_TEST_ALL_PREFIXES lines in the AudioProcessingImpl class when the other > submodules are also transitioned into the new parameter scheme I'll group the > tests into 4 categories that suit all submodules. Please let me know what you > think of that! > > Done. > Agree declaring friends all over APMImpl is not the way to go. https://codereview.webrtc.org/2337083002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_unittest.cc:2832: TEST(ApmConfiguration, NonValidConfigBehavior) { nit: I think "Invalid" is more common.
https://codereview.webrtc.org/2337083002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_unittest.cc:2832: TEST(ApmConfiguration, NonValidConfigBehavior) { On 2016/09/19 18:53:56, the sun wrote: > nit: I think "Invalid" is more common. Done.
https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:551: config.level_controller.enabled; On 2016/09/19 18:53:56, the sun wrote: > On 2016/09/19 16:37:10, peah-webrtc wrote: > > On 2016/09/16 12:40:24, the sun wrote: > > > config_ > > > where is the test case? > > > > > > nit: it is nice if the conditional and the assignment is ordered the same > way, > > > for these types of constructs. > > > > The test case is in audio_processing_unittests.cc. But maybe some variant of > the > > test is missing there? Please let me know then! > > > > Changed the ordering and changed to using config_ instead of config. > > Ah, the problem with the test is it checks the flag in config_.level_controller. > Hm. You're right, it does become a mess with all the friend declarations, plus > the white box nature of the test will become a burden. Ideally, we'd have had a > factory method for the LC given to the APM on c-tion - then we could test out > the logic without really creating an LC. I fully agree. We should definitely have that. For the level controller, we could probably achieve that today but for the other submodules it is not yet possible so until we can for all submodules we should have a way of doing this that works for all submodules. Let's continue the discussion offline. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:558: } On 2016/09/19 18:53:56, the sun wrote: > On 2016/09/19 16:37:10, peah-webrtc wrote: > > On 2016/09/16 12:40:24, the sun wrote: > > > Did you forget lc->ApplyConfig()? > > > > Oh no! Great find! I for sure did miss that. > > > > I'm not sure if we should add a unittest to avoid that though. It becomes > quite > > messy in the long run with all the friend-class statements. The best thing > would > > be to add tests that verify the effect of the level controller. > > > > WDYT, is it ok to leave it without this without any further testing to avoid > > this? > > Let's discuss offline. Acknowledged. https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_unittest.cc:2810: apm.reset(new AudioProcessingImpl(webrtc::Config())); On 2016/09/19 18:53:56, the sun wrote: > On 2016/09/19 16:37:10, peah-webrtc wrote: > > On 2016/09/16 12:40:24, the sun wrote: > > > Split up into several test cases instead - it shouldn't be too much > work/code > > > since you're already adding lines to reset the state. > > > > I'm all in for that, but in order to limit the number of > > FRIEND_TEST_ALL_PREFIXES lines in the AudioProcessingImpl class when the other > > submodules are also transitioned into the new parameter scheme I'll group the > > tests into 4 categories that suit all submodules. Please let me know what you > > think of that! > > > > Done. > > > > Agree declaring friends all over APMImpl is not the way to go. Acknowledged.
https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:535: << LevelController::ToString(config.level_controller) use config_ https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_unittest.cc:30: #include "webrtc/modules/audio_processing/level_controller/lc_constants.h" please don't abbreviate file names https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_unittest.cc:2799: EXPECT_TRUE(apm->config_.level_controller.enabled); Aren't you testing this in ValidConfigBehavior? Dedupe? https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_unittest.cc:2854: apm.reset(new AudioProcessingImpl(webrtc::Config())); So here's either a separate test case, or a comment is needed to explain why a new APM is created. https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/audio_processing.cc (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.cc:34: int AudioProcessing::StartDebugRecordingForPlatformFile( Can we remove this function? AudioProcessing should be a pure interface, if possible. https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.cc:271: const AudioProcessing::Config::LevelController& config) { DCHECK the config is valid https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.cc:281: << "enabled: " << (config.enabled ? "true" : "false") << "," nit: space after the comma: ", " https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/level_controller.h (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.h:53: FRIEND_TEST_ALL_PREFIXES(LevelControllerConfig, InitialLevel); You should try to make do without declaring all tests as friends. This pattern makes sense for the APM, since the APM interface is harder to change and the impl is very large at the moment, but this is fairly new code, and internal, so it can be changed to make testing easier. If absolutely necessary, add a "NnnForTesting()" utility function. Exposing ALL the internals of the class to tests blurs the expectations of the test and class interface. https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc:81: config.level_controller.initial_peak_level_dbfs = -6.0206; nit: f suffix https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc:103: config.initial_peak_level_dbfs = -6.0206; f suffix https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc:109: EXPECT_NEAR(103.62151336f, lc.peak_level_estimator_.peak_level_, kTolerance); I don't think you should be testing internal details of the peak follower here. https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.h (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/peak_level_estimator.h:29: FRIEND_TEST_ALL_PREFIXES(LevelControllerConfig, InitialLevel); Try to get by without these, please. In particular exposing the internals to the LevelControllerConfig test seems dubious. If they're so tightly coupled, why have separate classes in the first place? Adding a unit test for the peak follower is more appropriate (and then you could test the handling of the initial peak level implicitly).
Patchset #8 (id:140001) has been deleted
https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:535: << LevelController::ToString(config.level_controller) On 2016/09/30 08:51:16, the sun wrote: > use config_ Done. https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_unittest.cc:30: #include "webrtc/modules/audio_processing/level_controller/lc_constants.h" On 2016/09/30 08:51:16, the sun wrote: > please don't abbreviate file names Done. https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_unittest.cc:2799: EXPECT_TRUE(apm->config_.level_controller.enabled); On 2016/09/30 08:51:16, the sun wrote: > Aren't you testing this in ValidConfigBehavior? Dedupe? No, if I understand your question correctly, I don't think do that. -What DefaultBehavior does is to ensure that the default values are applied when no other values have been specified. The intention of this test is basically to doublecheck that the default values are not modified by accident. -What ValidConfigBehavior does is to ensure that specified values are applied for a valid config. The intention of this test is to ensure that specified settings actually take effect. -What InValidConfigBehavior does is to ensure that the specified fallback settings are applied when the config is not valid. The intention of this test is to ensure that the proper fallback behavior is used. I agree that DefaultBehavior and ValidConfigBehavior are very similar but they actually do slightly different things. WDYT? https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_unittest.cc:2854: apm.reset(new AudioProcessingImpl(webrtc::Config())); On 2016/09/30 08:51:16, the sun wrote: > So here's either a separate test case, or a comment is needed to explain why a > new APM is created. I added comments about what is being done. Ideally I'd like a separate TEST for that but as that would then as well need to be explicitly added as a friend to AudioProcessingImpl I think it is better to bundle all the valid, invalid and default configurations tests into these three tests. WDYT? https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/audio_processing.cc (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.cc:34: int AudioProcessing::StartDebugRecordingForPlatformFile( On 2016/09/30 08:51:16, the sun wrote: > Can we remove this function? AudioProcessing should be a pure interface, if > possible. I don't know. Ok if I create another CL for that? https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/level_controller.cc (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.cc:271: const AudioProcessing::Config::LevelController& config) { On 2016/09/30 08:51:16, the sun wrote: > DCHECK the config is valid Done. https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.cc:281: << "enabled: " << (config.enabled ? "true" : "false") << "," On 2016/09/30 08:51:16, the sun wrote: > nit: space after the comma: ", " Done. https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/level_controller.h (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.h:53: FRIEND_TEST_ALL_PREFIXES(LevelControllerConfig, InitialLevel); On 2016/09/30 08:51:16, the sun wrote: > You should try to make do without declaring all tests as friends. This pattern > makes sense for the APM, since the APM interface is harder to change and the > impl is very large at the moment, but this is fairly new code, and internal, so > it can be changed to make testing easier. > > If absolutely necessary, add a "NnnForTesting()" utility function. Exposing ALL > the internals of the class to tests blurs the expectations of the test and class > interface. I fully agree with you your comment about FRIEND classes. I fully agree with you your comment about FRIEND classes. After offline discussions we decided to not explicitly verify that the specified settings take effect, and instead leave that to the bitexactness tests. I have changed the code accordingly. PTAL. https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc:81: config.level_controller.initial_peak_level_dbfs = -6.0206; On 2016/09/30 08:51:16, the sun wrote: > nit: f suffix Done. https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc:103: config.initial_peak_level_dbfs = -6.0206; On 2016/09/30 08:51:16, the sun wrote: > f suffix Done. https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc:109: EXPECT_NEAR(103.62151336f, lc.peak_level_estimator_.peak_level_, kTolerance); On 2016/09/30 08:51:16, the sun wrote: > I don't think you should be testing internal details of the peak follower here. I fully agree with you your comment about FRIEND classes. After offline discussions we decided to not explicitly verify that the specified settings take effect, and instead leave that to the bitexactness tests. I have changed the code accordingly. PTAL. https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.h (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/peak_level_estimator.h:29: FRIEND_TEST_ALL_PREFIXES(LevelControllerConfig, InitialLevel); On 2016/09/30 08:51:16, the sun wrote: > Try to get by without these, please. In particular exposing the internals to the > LevelControllerConfig test seems dubious. If they're so tightly coupled, why > have separate classes in the first place? > > Adding a unit test for the peak follower is more appropriate (and then you could > test the handling of the initial peak level implicitly). I don't think this is really related to the coupling of classes but rather to how to test that the specified parameters stick. I fully agree with you your comment about FRIEND classes. After offline discussions we decided to not explicitly verify that the specified settings take effect, and instead leave that to the bitexactness tests. I have changed the code accordingly. PTAL.
https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:535: << LevelController::ToString(config.level_controller) On 2016/10/03 09:52:13, peah-webrtc wrote: > On 2016/09/30 08:51:16, the sun wrote: > > use config_ > > Done. To be precise, you are now using config, not config_. https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:19: #include "webrtc/base/gtest_prod_util.h" Sort. https://codereview.webrtc.org/2337083002/diff/160001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/160001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_unittest.cc:2818: config = AudioProcessing::Config(); Isn't config already default constructed as a result of the line above? https://codereview.webrtc.org/2337083002/diff/160001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_unittest.cc:2842: config = AudioProcessing::Config(); Same here.
https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:535: << LevelController::ToString(config.level_controller) On 2016/10/05 12:29:18, hlundin-webrtc wrote: > On 2016/10/03 09:52:13, peah-webrtc wrote: > > On 2016/09/30 08:51:16, the sun wrote: > > > use config_ > > > > Done. > > To be precise, you are now using config, not config_. In the latest patchset, this is now changed to config_ instead of config in response to the former comment. I think that comment is valid, as that will ensure that only only one variable is used (i.e., config_) for modifying/accessing the config parameters. I'm not sure whether that answers your question though. https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:19: #include "webrtc/base/gtest_prod_util.h" On 2016/10/05 12:29:18, hlundin-webrtc wrote: > Sort. Done. https://codereview.webrtc.org/2337083002/diff/160001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/160001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_unittest.cc:2818: config = AudioProcessing::Config(); On 2016/10/05 12:29:18, hlundin-webrtc wrote: > Isn't config already default constructed as a result of the line above? True! Done. https://codereview.webrtc.org/2337083002/diff/160001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_unittest.cc:2842: config = AudioProcessing::Config(); On 2016/10/05 12:29:18, hlundin-webrtc wrote: > Same here. True! Done.
lgtm % nits https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_unittest.cc:2799: EXPECT_TRUE(apm->config_.level_controller.enabled); On 2016/10/03 09:52:13, peah-webrtc wrote: > On 2016/09/30 08:51:16, the sun wrote: > > Aren't you testing this in ValidConfigBehavior? Dedupe? > > No, if I understand your question correctly, I don't think do that. > > -What DefaultBehavior does is to ensure that the default values are applied when > no other values have been specified. The intention of this test is basically to > doublecheck that the default values are not modified by accident. > -What ValidConfigBehavior does is to ensure that specified values are applied > for a valid config. The intention of this test is to ensure that specified > settings actually take effect. > -What InValidConfigBehavior does is to ensure that the specified fallback > settings are applied when the config is not valid. The intention of this test is > to ensure that the proper fallback behavior is used. > > I agree that DefaultBehavior and ValidConfigBehavior are very similar but they > actually do slightly different things. WDYT? Acknowledged. https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_unittest.cc:2854: apm.reset(new AudioProcessingImpl(webrtc::Config())); On 2016/10/03 09:52:13, peah-webrtc wrote: > On 2016/09/30 08:51:16, the sun wrote: > > So here's either a separate test case, or a comment is needed to explain why a > > new APM is created. > > I added comments about what is being done. Ideally I'd like a separate TEST for > that but as that would then as well need to be explicitly added as a friend to > AudioProcessingImpl I think it is better to bundle all the valid, invalid and > default configurations tests into these three tests. > > WDYT? Acknowledged. https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/audio_processing.cc (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.cc:34: int AudioProcessing::StartDebugRecordingForPlatformFile( On 2016/10/03 09:52:14, peah-webrtc wrote: > On 2016/09/30 08:51:16, the sun wrote: > > Can we remove this function? AudioProcessing should be a pure interface, if > > possible. > > I don't know. Ok if I create another CL for that? > sure https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.h (right): https://codereview.webrtc.org/2337083002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/peak_level_estimator.h:29: FRIEND_TEST_ALL_PREFIXES(LevelControllerConfig, InitialLevel); On 2016/10/03 09:52:14, peah-webrtc wrote: > On 2016/09/30 08:51:16, the sun wrote: > > Try to get by without these, please. In particular exposing the internals to > the > > LevelControllerConfig test seems dubious. If they're so tightly coupled, why > > have separate classes in the first place? > > > > Adding a unit test for the peak follower is more appropriate (and then you > could > > test the handling of the initial peak level implicitly). > > I don't think this is really related to the coupling of classes but rather to > how to test that the specified parameters stick. > > I fully agree with you your comment about FRIEND classes. > After offline discussions we decided to not explicitly verify that the specified > settings take effect, and instead leave that to the bitexactness tests. > I have changed the code accordingly. PTAL. > Acknowledged. https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:554: // TODO(peah): Add logline regardless of whether this is enabled or not. Why leave that as a todo? https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:132: private: nit: Add a comment that this should be removed. https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_unittest.cc:2783: const float kTolerance = 0.0001f; Can we use std::numeric_limits<float>::epsilon() instead? https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/level_controller.h (right): https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.h:18: #include "webrtc/base/gtest_prod_util.h" remove https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.h (right): https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/peak_level_estimator.h:15: #include "webrtc/base/gtest_prod_util.h" abomination! begone!
LGTM % solenberg's comments.
https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:554: // TODO(peah): Add logline regardless of whether this is enabled or not. On 2016/10/06 07:21:14, the sun wrote: > Why leave that as a todo? No good reason. I think it may have been to avoid having to change that behavior in this CL. I change it now though and remove the TODO. https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:132: private: On 2016/10/06 07:21:14, the sun wrote: > nit: Add a comment that this should be removed. Done. https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_unittest.cc:2783: const float kTolerance = 0.0001f; On 2016/10/06 07:21:14, the sun wrote: > Can we use std::numeric_limits<float>::epsilon() instead? Done. https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/level_controller.h (right): https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/level_controller.h:18: #include "webrtc/base/gtest_prod_util.h" On 2016/10/06 07:21:14, the sun wrote: > remove Done. https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.h (right): https://codereview.webrtc.org/2337083002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/peak_level_estimator.h:15: #include "webrtc/base/gtest_prod_util.h" On 2016/10/06 07:21:14, the sun wrote: > abomination! begone! :-) Done.
Description was changed from ========== This CL adds functionality in the level controller to receive a signal level to use initially, instead of the default initial signal level. The initial form of the CL (https://codereview.webrtc.org/2254973003/) was reverted due to down-stream dependencies. These have been resolved, but the CL needed to be revised according to the new scheme for passing parameters to the audio processing module. Therefore, please review this CL as if it is new. BUG=webrtc:6386 ========== to ========== This CL adds functionality in the level controller to receive a signal level to use initially, instead of the default initial signal level. The initial form of the CL (https://codereview.webrtc.org/2254973003/) was reverted due to down-stream dependencies. These have been resolved, but the CL needed to be revised according to the new scheme for passing parameters to the audio processing module. Therefore, please review this CL as if it is new. TBR=aleloi@webrtc.org BUG=webrtc:6386 ==========
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/2337083002/#ps200001 (title: "Rebase and changes in response to reviewer comments")
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_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/11970) win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/1974)
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/2337083002/#ps220001 (title: "Fixed type conversion error")
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 ========== This CL adds functionality in the level controller to receive a signal level to use initially, instead of the default initial signal level. The initial form of the CL (https://codereview.webrtc.org/2254973003/) was reverted due to down-stream dependencies. These have been resolved, but the CL needed to be revised according to the new scheme for passing parameters to the audio processing module. Therefore, please review this CL as if it is new. TBR=aleloi@webrtc.org BUG=webrtc:6386 ========== to ========== This CL adds functionality in the level controller to receive a signal level to use initially, instead of the default initial signal level. The initial form of the CL (https://codereview.webrtc.org/2254973003/) was reverted due to down-stream dependencies. These have been resolved, but the CL needed to be revised according to the new scheme for passing parameters to the audio processing module. Therefore, please review this CL as if it is new. TBR=aleloi@webrtc.org BUG=webrtc:6386 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== This CL adds functionality in the level controller to receive a signal level to use initially, instead of the default initial signal level. The initial form of the CL (https://codereview.webrtc.org/2254973003/) was reverted due to down-stream dependencies. These have been resolved, but the CL needed to be revised according to the new scheme for passing parameters to the audio processing module. Therefore, please review this CL as if it is new. TBR=aleloi@webrtc.org BUG=webrtc:6386 ========== to ========== This CL adds functionality in the level controller to receive a signal level to use initially, instead of the default initial signal level. The initial form of the CL (https://codereview.webrtc.org/2254973003/) was reverted due to down-stream dependencies. These have been resolved, but the CL needed to be revised according to the new scheme for passing parameters to the audio processing module. Therefore, please review this CL as if it is new. TBR=aleloi@webrtc.org BUG=webrtc:6386 Committed: https://crrev.com/c19f312f54b9674621c0482d7c067043643308c1 Cr-Commit-Position: refs/heads/master@{#14579} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/c19f312f54b9674621c0482d7c067043643308c1 Cr-Commit-Position: refs/heads/master@{#14579} |
