Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1907223003:
Extension and refactoring of the audioproc_f tool to be a fully fledged tool for audio processing m… (Closed)
|
Created:
4 years, 8 months ago by peah-webrtc Modified:
4 years, 6 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, the sun, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionThis CL updates and extends the audioproc_f command line
tool to support all the functionality needed for simulating
and analyzing the audio processing module behavior during
calls.
BUG=
Committed: https://crrev.com/60a189f1bfa5f822cb9e384b1fea514a66fa3a68
Cr-Commit-Position: refs/heads/master@{#12882}
Patch Set 1 #Patch Set 2 : Rebase with master in order to include the refined adaptive filter functionality and the descriptio… #Patch Set 3 : Corrected the activation of the refined filter and aec3 experimental features when operating on aec… #Patch Set 4 : Fixed the bitexactness check when operating on aecdump data #
Total comments: 58
Patch Set 5 : Changes in response to reviewer comments #
Total comments: 36
Patch Set 6 : Changes in response to reviewer comments #
Total comments: 22
Patch Set 7 : Changes in response to reviewer comments #
Total comments: 48
Patch Set 8 : Changes in response to reviewer comments #
Total comments: 2
Patch Set 9 : Changes in response to latest reviewer comments #
Total comments: 2
Patch Set 10 : Changes in response to reviewer comments #Patch Set 11 : Changed from using command-line flags for feature (de-)activation to using 1/0 specifiers. #Patch Set 12 : Rebase with latest master #
Total comments: 2
Patch Set 13 : Changed the command line tool description #
Total comments: 4
Patch Set 14 : Changes in response to reviewer comments #Patch Set 15 : Rebase with latest master #Messages
Total messages: 45 (10 generated)
peah@webrtc.org changed reviewers: + aluebs@webrtc.org, minyue@webrtc.org
Hi, The new version of audioproc_f is not ready. The changes are quite big so there are very likely some bugs present that will need to be found and removed. I will start using it now as the default analysis tool and will address any found issues along the way. As I don't know of any issues, and as I don't foresee any major changes being needed to address the issues I now send the CL out for review.
Description was changed from ========== Extension and refactoring of the audioproc_f tool to be a fully fledged tool for audio processing module simulations BUG= ========== to ========== This CL updates and extends the audioproc_f command line tool to support all the functionality needed for simulating and analyzing the audio processing module behavior during calls. BUG= ==========
Oups, as a critical typo went into the text, I resend the
message ("not" has been exchanged for the indended "now")
Hi,
The new version of audioproc_f is now ready. The changes
are quite big so there are very likely some bugs present
that will need to be found and removed. I will start
using it now as the default analysis tool and will address
any found issues along the way.
As I don't know of any issues, and as I don't foresee any
major changes being needed to address the issues I now send
the CL out for review.
On 2016/04/22 11:24:13, peah-webrtc wrote:
> Hi,
> The new version of audioproc_f is not ready. The changes
> are quite big so there are very likely some bugs present
> that will need to be found and removed. I will start
> using it now as the default analysis tool and will address
> any found issues along the way.
>
> As I don't know of any issues, and as I don't foresee any
> major changes being needed to address the issues I now send
> the CL out for review.
Next time, please separate major refactoring from features in different CLs. Because of the size of this CL I only reviewed audioproc_float and will review other files in later iterations. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. I don't think the year should be updated because of a refactoring. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:37: DEFINE_string(o, "", "Forward stream output wav filename"); Don't we ant some default value for the output file? https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:42: "Number of forward stream output channels (1-2)"); Any number of channels is supported, right? https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:64: DEFINE_bool(aec, false, "Activate the canceller"); echo canceller? https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:83: DEFINE_bool(no_le, false, "Dectivate the level estimator"); Why do we need 2 flags to specify if a component is enabled or disabled? (if I remember correctly, each "flag" has its correspondent "noflag") Also, why are both false? Isn't it easier to have only one flag with the right default value that can be overwritten? Then you can also drop the all_default flag. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:138: DEFINE_bool(discard_settings_in_aecdump, Probably more intuitive to use the positive opposite use_aecdump_settings. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:189: &settings.reverse_output_num_channels); For these parameters it would be great to ave the same name for the flag and the settings field. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:215: SetSettingIfFlagSet(FLAGS_no_le, &settings.use_le, false); This can be simplified so much with my suggestion above. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:263: auto report_conditional_error_and_exit = [](bool condition, Why not making this a function directly? https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:272: report_conditional_error_and_exit(!!settings.aec_dump_input_filename, double negation? https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:299: *settings.output_sample_rate_hz > 48000), Why do we impose this limit? The APM can handle an arbitrary sample rate. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:302: report_conditional_error_and_exit( All these checks for a single parameter can be implemented in the gflags framework. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:310: *settings.output_num_channels > 2), Why do we impose this limit? The APM can handle an arbitrary number of channels, right? https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:325: settings.target_angle_degrees < 0 || settings.target_angle_degrees > 90, The angle can definitively go between 0 and 359 degrees. And I think that if it isn't in that range it would wrap to this range anyways. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:359: auto valid_wav_name = [](const std::string& wav_file_name) { Why isn't this a normal function? https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:407: int num_fwd_chunks_processed = processor->get_num_process_stream_calls(); I don't think this temp variable is necessary, but in any case it should be const and should be moved inside the if scope. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:422: if (settings.report_bitexactness && settings.aec_dump_input_filename) { It should probably be checked in your sanity function that it has an aecdump when asking for bit exactness testing.
Thanks for the review comments! As mentioned offline, I'd be happy to split it into several separate CLs if that is desired. I personally don't think it is motivated in this case as the new functionality basically constitutes a complete rewrite of audioproc_float.cc and the other source files it reuses. My feeling was that it would require much more reviewing work to split it in separate CLs than do do it in one CL. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2016/04/26 04:33:39, aluebs-webrtc wrote: > I don't think the year should be updated because of a refactoring. Done. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:37: DEFINE_string(o, "", "Forward stream output wav filename"); On 2016/04/26 04:33:39, aluebs-webrtc wrote: > Don't we ant some default value for the output file? I don't think so. In many cases the output is not needed when running audioproc(_f) and I don't see why we would need a default for the output. This is also according to how it was done in process_test.cc. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:42: "Number of forward stream output channels (1-2)"); On 2016/04/26 04:33:39, aluebs-webrtc wrote: > Any number of channels is supported, right? I'll change that to support any channels. Done. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:64: DEFINE_bool(aec, false, "Activate the canceller"); On 2016/04/26 04:33:39, aluebs-webrtc wrote: > echo canceller? Done. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:83: DEFINE_bool(no_le, false, "Dectivate the level estimator"); On 2016/04/26 04:33:39, aluebs-webrtc wrote: > Why do we need 2 flags to specify if a component is enabled or disabled? (if I > remember correctly, each "flag" has its correspondent "noflag") Also, why are > both false? Isn't it easier to have only one flag with the right default value > that can be overwritten? Then you can also drop the all_default flag. This was the most simple way (for the user of the audioproc_f) that I could come up with but there are probably better alternatives. The underlying idea behind this is that each parameter basically should constitute a tri-state value, where it either turns off the feature, turns on the feature, or keeps it as its default value. This is important for this to work well with aecdump recordings where there are default values for the settings stored in the recording that the simulator should act upon. When reading the config event from the aecdump the simulator will then either 1) apply the setting in the config if no parameter value has been explicitly specified on the command line. 2) apply the parameter setting specified if that has been explicitly specified on the command line. For a on/off feature, for instance turning on the aec a tri-state variable is needed for the above. And it is not possible to use a default value as that would then overwrite the value in the config. For non on/off parameters I solved this by using a default value kParameterNotSpecifiedValue represent that the parameter had not been set, but for boolean flags that is not possible. Afaics, this is not possible to achieve using the no-flag-feature of gflags, as that enforces the options to be two-state (bundling the no_aec and aec only gives two values for the parameter and therefore it cannot be used as a tri-state to control the feature). One alternative could be to use integer flags instead of specifying the on/off flags using boolean flags, i.e., instead of using the command line --no_aec use --aec 0. I did not choose that option as that was not how the process_test.cc and audioproc_float.cc used it before, but if you think that is a better option I'd be happy to use that. That is some background to the choice of implementation. If you have a better approach, I'd be happy to change this. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:138: DEFINE_bool(discard_settings_in_aecdump, On 2016/04/26 04:33:39, aluebs-webrtc wrote: > Probably more intuitive to use the positive opposite use_aecdump_settings. The absolutely most common thing to do when analyzing aecdumps is to reapply the settings stored in the aecdump, so therefore I wanted that to be the default option. If the option is called use_aecdump_settings, and the default value is true, then one must negate that in the command line to achieve discarding the settings. Would it be possible to do that nicely using the "no" flags option you mentioned? https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:189: &settings.reverse_output_num_channels); On 2016/04/26 04:33:39, aluebs-webrtc wrote: > For these parameters it would be great to ave the same name for the flag and the > settings field. Good point! Done. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:215: SetSettingIfFlagSet(FLAGS_no_le, &settings.use_le, false); On 2016/04/26 04:33:39, aluebs-webrtc wrote: > This can be simplified so much with my suggestion above. Yes, probably. But please explain more on how that would be used. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:263: auto report_conditional_error_and_exit = [](bool condition, On 2016/04/26 04:33:39, aluebs-webrtc wrote: > Why not making this a function directly? Done. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:272: report_conditional_error_and_exit(!!settings.aec_dump_input_filename, On 2016/04/26 04:33:39, aluebs-webrtc wrote: > double negation? aec_dump_input_filename is an optional, and I want to true to be passed if it is set. The double negation is a way to achieve that in a simple manner. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:302: report_conditional_error_and_exit( On 2016/04/26 04:33:39, aluebs-webrtc wrote: > All these checks for a single parameter can be implemented in the gflags > framework. No, that does not work if you want to have tri-state variables. The problem then is that the validator enforces the default value to be valid, and therefore the default value that is used as an indicator that the parameter is not set must be changed to be in the valid range. For some of the options that is possible but not for some of the others. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:310: *settings.output_num_channels > 2), On 2016/04/26 04:33:39, aluebs-webrtc wrote: > Why do we impose this limit? The APM can handle an arbitrary number of channels, > right? Done. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:325: settings.target_angle_degrees < 0 || settings.target_angle_degrees > 90, On 2016/04/26 04:33:39, aluebs-webrtc wrote: > The angle can definitively go between 0 and 359 degrees. And I think that if it > isn't in that range it would wrap to this range anyways. My mistake. I think it is anyway nice to impose a sane angle between 0 and 359. Or do you instead prefer no limit and rely on wrapping behavior? Done. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:359: auto valid_wav_name = [](const std::string& wav_file_name) { On 2016/04/26 04:33:39, aluebs-webrtc wrote: > Why isn't this a normal function? The lambda/function is only used in the lines below and nowhere else. If the functionality in the function/lambda is very light-weight, I typically try to include it as a lambda and place it close to where it is used in order to simplify the code readability. In this case it is just used to simplify the if-statements by removing repeated code, and if it would be implemented as a function it would need to be placed ~100 lines above where it is used. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:407: int num_fwd_chunks_processed = processor->get_num_process_stream_calls(); On 2016/04/26 04:33:39, aluebs-webrtc wrote: > I don't think this temp variable is necessary, but in any case it should be > const and should be moved inside the if scope. Done. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:422: if (settings.report_bitexactness && settings.aec_dump_input_filename) { On 2016/04/26 04:33:39, aluebs-webrtc wrote: > It should probably be checked in your sanity function that it has an aecdump > when asking for bit exactness testing. Good point! Done.
I think it always is better to separate functionality and refactor and I would go further to say whenever it can be separated in more CLs it should. But now that I started looking at it, let's keep this as is. I also am not sure why a complete rewrite was necessary instead of adding the functionality to the existing tool, but I don't mind. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2016/04/26 07:19:29, peah-webrtc wrote: > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > I don't think the year should be updated because of a refactoring. > > Done. It was 2014 though. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:37: DEFINE_string(o, "", "Forward stream output wav filename"); On 2016/04/26 07:19:29, peah-webrtc wrote: > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > Don't we ant some default value for the output file? > > I don't think so. In many cases the output is not needed when running > audioproc(_f) and I don't see why we would need a default for the output. This > is also according to how it was done in process_test.cc. I think in most of the cases the output is of interest and we avoid having to specify this parameter, but I see your point. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:83: DEFINE_bool(no_le, false, "Dectivate the level estimator"); On 2016/04/26 07:19:29, peah-webrtc wrote: > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > Why do we need 2 flags to specify if a component is enabled or disabled? (if I > > remember correctly, each "flag" has its correspondent "noflag") Also, why are > > both false? Isn't it easier to have only one flag with the right default value > > that can be overwritten? Then you can also drop the all_default flag. > > This was the most simple way (for the user of the audioproc_f) that I could come > up with but there are probably better alternatives. > > The underlying idea behind this is that each parameter basically should > constitute a tri-state value, where it > either turns off the feature, turns on the feature, or keeps it as its default > value. > This is important for this to work well with aecdump recordings where there are > default values for the settings stored in the recording that the simulator > should act upon. > When reading the config event from the aecdump the simulator will then either > 1) apply the setting in the config if no parameter value has been explicitly > specified on the command line. > 2) apply the parameter setting specified if that has been explicitly specified > on the command line. > > For a on/off feature, for instance turning on the aec a tri-state variable is > needed for the above. And it is not possible to use a default value as that > would then overwrite the value in the config. For non on/off parameters I solved > this by using a default value kParameterNotSpecifiedValue represent that the > parameter had not been set, but for boolean flags that is not possible. > > Afaics, this is not possible to achieve using the no-flag-feature of gflags, as > that enforces the options to be two-state (bundling the no_aec and aec only > gives two values for the parameter and therefore it cannot be used as a > tri-state to control the feature). > > One alternative could be to use integer flags instead of specifying the on/off > flags using boolean flags, i.e., instead of using the command line --no_aec use > --aec 0. I did not choose that option as that was not how the process_test.cc > and audioproc_float.cc used it before, but if you think that is a better option > I'd be happy to use that. > > That is some background to the choice of implementation. If you have a better > approach, I'd be happy to change this. > > I personally prefer 1 int flag than 2 different bool flags that add more code and is confusing for a user, since you could technically specify "flag" and "no_flag" at the same time. But what about this. One global flag that forces to use the aecdump values for all flags and then just boolean flags with the correct default value? https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:138: DEFINE_bool(discard_settings_in_aecdump, On 2016/04/26 07:19:29, peah-webrtc wrote: > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > Probably more intuitive to use the positive opposite use_aecdump_settings. > > The absolutely most common thing to do when analyzing aecdumps is to reapply the > settings stored in the aecdump, so therefore I wanted that to be the default > option. If the option is called use_aecdump_settings, and the default value is > true, then one must negate that in the command line to achieve discarding the > settings. > Would it be possible to do that nicely using the "no" flags option you > mentioned? Yes, you just --use nouse_aecdump_settings https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:215: SetSettingIfFlagSet(FLAGS_no_le, &settings.use_le, false); On 2016/04/26 07:19:29, peah-webrtc wrote: > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > This can be simplified so much with my suggestion above. > > Yes, probably. But please explain more on how that would be used. See above. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:272: report_conditional_error_and_exit(!!settings.aec_dump_input_filename, On 2016/04/26 07:19:29, peah-webrtc wrote: > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > double negation? > > aec_dump_input_filename is an optional, and I want to true to be passed if it is > set. The double negation is a way to achieve that in a simple manner. Oh, I thought it would cast automatically to bool. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:302: report_conditional_error_and_exit( On 2016/04/26 07:19:29, peah-webrtc wrote: > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > All these checks for a single parameter can be implemented in the gflags > > framework. > > No, that does not work if you want to have tri-state variables. The problem then > is that the validator enforces the default value to be valid, and therefore the > default value that is used as an indicator that the parameter is not set must be > changed to be in the valid range. For some of the options that is possible but > not for some of the others. Good point. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:325: settings.target_angle_degrees < 0 || settings.target_angle_degrees > 90, On 2016/04/26 07:19:29, peah-webrtc wrote: > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > The angle can definitively go between 0 and 359 degrees. And I think that if > it > > isn't in that range it would wrap to this range anyways. > > My mistake. I think it is anyway nice to impose a sane angle between 0 and 359. > Or do you instead prefer no limit and rely on wrapping behavior? > > Done. I don't mind imposing this limit. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:359: auto valid_wav_name = [](const std::string& wav_file_name) { On 2016/04/26 07:19:29, peah-webrtc wrote: > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > Why isn't this a normal function? > > The lambda/function is only used in the lines below and nowhere else. If the > functionality in the function/lambda is very light-weight, I typically try to > include it as a lambda and place it close to where it is used in order to > simplify the code readability. > > In this case it is just used to simplify the if-statements by removing repeated > code, and if it would be implemented as a function it would need to be placed > ~100 lines above where it is used. Personally I find interleaved lambdas to be less readable than a function. Specially because finding a lambda makes at least me assume it will be passed as a function pointer or it will grab some environment variables. And I don't think it matters too much where it is defined for such a simple method as long as the name is intuitive. But if you think it is better, I don't have such a strong opinion about it. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:30: src.data_[sample * src.num_channels_ + ch] / 32767.0f; There are de-interleave and format methods in audio_util which take care of this in a better way. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:37: char counter_string[10]; Where does this 10 come from? Also it should probably be a constant. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:43: new_name += wav_name.substr(wav_name.size() - 4); For many concatenations it is better and simpler to use a stringstream. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:71: CopyFromAudioFrame(fwd_frame_, out_buf_.get()); The st should have a tighter scope to not take into account this copying. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:90: CopyFromAudioFrame(rev_frame_, out_buf_.get()); rev_out_buf_ https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:119: AudioProcessingSimulator::kChunksPerSecond), AudioProcessingSimulator:: not needed. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:179: new WavWriter(filename, out_config_.sample_rate_hz(), Is this going to work for the AudioFrame interface that forces the same input and output sample rates? https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:231: !settings_.use_extended_filter) { Instead of these complicated conditions, wouldn't it be easier to set the default value before and then overwrite it like the others? https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.h (right): https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:14: #include <algorithm> Is this include really needed here? https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:38: int target_angle_degrees = 90; Why are there some settings that are not optional? https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:135: size_t output_reset_counter_ = 0; These default values should be set in the constructor instead. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:139: protected: Why 2 protected and private blocks? It is confusing. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:153: const SimulationSettings& settings_; Why a reference? I think this is confusing.
Thanks again for the great comments! I've updated the CL with a new patch. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2016/04/27 16:06:47, aluebs-webrtc wrote: > On 2016/04/26 07:19:29, peah-webrtc wrote: > > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > > I don't think the year should be updated because of a refactoring. > > > > Done. > > It was 2014 though. Sorry. Done. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:37: DEFINE_string(o, "", "Forward stream output wav filename"); On 2016/04/27 16:06:47, aluebs-webrtc wrote: > On 2016/04/26 07:19:29, peah-webrtc wrote: > > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > > Don't we ant some default value for the output file? > > > > I don't think so. In many cases the output is not needed when running > > audioproc(_f) and I don't see why we would need a default for the output. This > > is also according to how it was done in process_test.cc. > > I think in most of the cases the output is of interest and we avoid having to > specify this parameter, but I see your point. Acknowledged. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:83: DEFINE_bool(no_le, false, "Dectivate the level estimator"); On 2016/04/27 16:06:47, aluebs-webrtc wrote: > On 2016/04/26 07:19:29, peah-webrtc wrote: > > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > > Why do we need 2 flags to specify if a component is enabled or disabled? (if > I > > > remember correctly, each "flag" has its correspondent "noflag") Also, why > are > > > both false? Isn't it easier to have only one flag with the right default > value > > > that can be overwritten? Then you can also drop the all_default flag. > > > > This was the most simple way (for the user of the audioproc_f) that I could > come > > up with but there are probably better alternatives. > > > > The underlying idea behind this is that each parameter basically should > > constitute a tri-state value, where it > > either turns off the feature, turns on the feature, or keeps it as its default > > value. > > This is important for this to work well with aecdump recordings where there > are > > default values for the settings stored in the recording that the simulator > > should act upon. > > When reading the config event from the aecdump the simulator will then either > > 1) apply the setting in the config if no parameter value has been explicitly > > specified on the command line. > > 2) apply the parameter setting specified if that has been explicitly specified > > on the command line. > > > > For a on/off feature, for instance turning on the aec a tri-state variable is > > needed for the above. And it is not possible to use a default value as that > > would then overwrite the value in the config. For non on/off parameters I > solved > > this by using a default value kParameterNotSpecifiedValue represent that the > > parameter had not been set, but for boolean flags that is not possible. > > > > Afaics, this is not possible to achieve using the no-flag-feature of gflags, > as > > that enforces the options to be two-state (bundling the no_aec and aec only > > gives two values for the parameter and therefore it cannot be used as a > > tri-state to control the feature). > > > > One alternative could be to use integer flags instead of specifying the on/off > > flags using boolean flags, i.e., instead of using the command line --no_aec > use > > --aec 0. I did not choose that option as that was not how the process_test.cc > > and audioproc_float.cc used it before, but if you think that is a better > option > > I'd be happy to use that. > > > > That is some background to the choice of implementation. If you have a better > > approach, I'd be happy to change this. > > > > > > I personally prefer 1 int flag than 2 different bool flags that add more code > and is confusing for a user, since you could technically specify "flag" and > "no_flag" at the same time. > But what about this. One global flag that forces to use the aecdump values for > all flags and then just boolean flags with the correct default value? That sounds like a good option. But I see the drawback of having to do double-bookkeeping in the sense that the default options in the simulator needs to be up2date with what the default options in the AECdump. Furthermore, there are not really any globally default options that can be assumed I think. I then prefer the int flag option. But lets ask @minyue what he prefers before doing any changes here. Would you be ok with using the int flag option instead of the "global flag" alternative? https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:138: DEFINE_bool(discard_settings_in_aecdump, On 2016/04/27 16:06:47, aluebs-webrtc wrote: > On 2016/04/26 07:19:29, peah-webrtc wrote: > > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > > Probably more intuitive to use the positive opposite use_aecdump_settings. > > > > The absolutely most common thing to do when analyzing aecdumps is to reapply > the > > settings stored in the aecdump, so therefore I wanted that to be the default > > option. If the option is called use_aecdump_settings, and the default value is > > true, then one must negate that in the command line to achieve discarding the > > settings. > > Would it be possible to do that nicely using the "no" flags option you > > mentioned? > > Yes, you just --use nouse_aecdump_settings Yes, but that would then mean that "use_aecdump_settings" is never used (since it is anyway the default) and the only thing the user would specify is the "nouse_aecdump_settings" which at least to me is more awkward than to use "discard_settings_in_aecdump". Furthermore, use_aecdump_settings sounds more like the the settings stored in the aecdump are used, which is not really true as they may be overridden by any of the other command-line settings (e.g., agc_mode). Therefore, I think discard_settings_in_aecdump is better in that it exactly specifies what is being done. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:215: SetSettingIfFlagSet(FLAGS_no_le, &settings.use_le, false); On 2016/04/27 16:06:46, aluebs-webrtc wrote: > On 2016/04/26 07:19:29, peah-webrtc wrote: > > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > > This can be simplified so much with my suggestion above. > > > > Yes, probably. But please explain more on how that would be used. > > See above. Acknowledged. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:299: *settings.output_sample_rate_hz > 48000), On 2016/04/26 04:33:39, aluebs-webrtc wrote: > Why do we impose this limit? The APM can handle an arbitrary sample rate. Done. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:359: auto valid_wav_name = [](const std::string& wav_file_name) { On 2016/04/27 16:06:46, aluebs-webrtc wrote: > On 2016/04/26 07:19:29, peah-webrtc wrote: > > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > > Why isn't this a normal function? > > > > The lambda/function is only used in the lines below and nowhere else. If the > > functionality in the function/lambda is very light-weight, I typically try to > > include it as a lambda and place it close to where it is used in order to > > simplify the code readability. > > > > In this case it is just used to simplify the if-statements by removing > repeated > > code, and if it would be implemented as a function it would need to be placed > > ~100 lines above where it is used. > > Personally I find interleaved lambdas to be less readable than a function. > Specially because finding a lambda makes at least me assume it will be passed as > a function pointer or it will grab some environment variables. And I don't think > it matters too much where it is defined for such a simple method as long as the > name is intuitive. But if you think it is better, I don't have such a strong > opinion about it. Acknowledged. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:30: src.data_[sample * src.num_channels_ + ch] / 32767.0f; On 2016/04/27 16:06:47, aluebs-webrtc wrote: > There are de-interleave and format methods in audio_util which take care of this > in a better way. Thanks! Done. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:37: char counter_string[10]; On 2016/04/27 16:06:47, aluebs-webrtc wrote: > Where does this 10 come from? Also it should probably be a constant. This is now removed as the stringstream is now used. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:43: new_name += wav_name.substr(wav_name.size() - 4); On 2016/04/27 16:06:47, aluebs-webrtc wrote: > For many concatenations it is better and simpler to use a stringstream. Thanks! Very much better indeed! Done. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:71: CopyFromAudioFrame(fwd_frame_, out_buf_.get()); On 2016/04/27 16:06:47, aluebs-webrtc wrote: > The st should have a tighter scope to not take into account this copying. Good find! Done. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:90: CopyFromAudioFrame(rev_frame_, out_buf_.get()); On 2016/04/27 16:06:47, aluebs-webrtc wrote: > rev_out_buf_ Great find! Done. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:119: AudioProcessingSimulator::kChunksPerSecond), On 2016/04/27 16:06:47, aluebs-webrtc wrote: > AudioProcessingSimulator:: not needed. Done. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:179: new WavWriter(filename, out_config_.sample_rate_hz(), On 2016/04/27 16:06:47, aluebs-webrtc wrote: > Is this going to work for the AudioFrame interface that forces the same input > and output sample rates? It will if the simulation is set up properly. I did not add the checks for that so that will need to be reported as errors by the APM. Even though it is easy to check the command line arguments for this particular error when operating on .wav files, it is not as straightforward when operating on aecdumps, as the information on which interface is used is stored as part of the recording. Furthermore, even for the .wav file case, the .wav file headers needs to be parsed to be able to ensure this. Therefore, I chose to instead rely on the APM internal error reporting. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:231: !settings_.use_extended_filter) { On 2016/04/27 16:06:47, aluebs-webrtc wrote: > Instead of these complicated conditions, wouldn't it be easier to set the > default value before and then overwrite it like the others? Definitely, and I think that the reason that it is written like this is now longer valid. I'll change this, as it is not even correct but please let me know if it can be further simplified! Note, however, that the default values for these are not correct as to how the APM is by default run. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.h (right): https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:14: #include <algorithm> On 2016/04/27 16:06:48, aluebs-webrtc wrote: > Is this include really needed here? git cl upload complains if it is not there. I think it is needed for the std::min operation. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:38: int target_angle_degrees = 90; On 2016/04/27 16:06:48, aluebs-webrtc wrote: > Why are there some settings that are not optional? I tried to make those that did not need to be optional nonoptional. For instance, the target_angle_degrees does not need to be optional since it always needs to be specified if the beamformer is activated (correct?) so it can piggyback on the use_bf optional. Furthermore, the target_angle_degrees is not stored in any protobuf recording and therefore there is no need to use an optional to determine whether the values in the protobuf recording should be overridden. There are similar arguments for the other non-optional values. In general I tried to make as few things optional as possible. I'd be happy for any suggestions for changes in this scheme. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:135: size_t output_reset_counter_ = 0; On 2016/04/27 16:06:48, aluebs-webrtc wrote: > These default values should be set in the constructor instead. I think the guideline is agnostic to that (right?). I've actually often gotten feedback from others that it is better to put the default initializers here rather than in the constructor. I don't have a strong opinion on this but I prefer this way as it does not clutter the constructor, produces fewer lines of code and keeps the initialization close to where the variables are. Wdyt? Are you ok with keeping them as they are. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:139: protected: On 2016/04/27 16:06:48, aluebs-webrtc wrote: > Why 2 protected and private blocks? It is confusing. My mistake. Done. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:153: const SimulationSettings& settings_; On 2016/04/27 16:06:48, aluebs-webrtc wrote: > Why a reference? I think this is confusing. Done.
https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:83: DEFINE_bool(no_le, false, "Dectivate the level estimator"); On 2016/04/28 07:41:11, peah-webrtc wrote: > On 2016/04/27 16:06:47, aluebs-webrtc wrote: > > On 2016/04/26 07:19:29, peah-webrtc wrote: > > > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > > > Why do we need 2 flags to specify if a component is enabled or disabled? > (if > > I > > > > remember correctly, each "flag" has its correspondent "noflag") Also, why > > are > > > > both false? Isn't it easier to have only one flag with the right default > > value > > > > that can be overwritten? Then you can also drop the all_default flag. > > > > > > This was the most simple way (for the user of the audioproc_f) that I could > > come > > > up with but there are probably better alternatives. > > > > > > The underlying idea behind this is that each parameter basically should > > > constitute a tri-state value, where it > > > either turns off the feature, turns on the feature, or keeps it as its > default > > > value. > > > This is important for this to work well with aecdump recordings where there > > are > > > default values for the settings stored in the recording that the simulator > > > should act upon. > > > When reading the config event from the aecdump the simulator will then > either > > > 1) apply the setting in the config if no parameter value has been explicitly > > > specified on the command line. > > > 2) apply the parameter setting specified if that has been explicitly > specified > > > on the command line. > > > > > > For a on/off feature, for instance turning on the aec a tri-state variable > is > > > needed for the above. And it is not possible to use a default value as that > > > would then overwrite the value in the config. For non on/off parameters I > > solved > > > this by using a default value kParameterNotSpecifiedValue represent that the > > > parameter had not been set, but for boolean flags that is not possible. > > > > > > Afaics, this is not possible to achieve using the no-flag-feature of gflags, > > as > > > that enforces the options to be two-state (bundling the no_aec and aec only > > > gives two values for the parameter and therefore it cannot be used as a > > > tri-state to control the feature). > > > > > > One alternative could be to use integer flags instead of specifying the > on/off > > > flags using boolean flags, i.e., instead of using the command line --no_aec > > use > > > --aec 0. I did not choose that option as that was not how the > process_test.cc > > > and audioproc_float.cc used it before, but if you think that is a better > > option > > > I'd be happy to use that. > > > > > > That is some background to the choice of implementation. If you have a > better > > > approach, I'd be happy to change this. > > > > > > > > > > I personally prefer 1 int flag than 2 different bool flags that add more code > > and is confusing for a user, since you could technically specify "flag" and > > "no_flag" at the same time. > > But what about this. One global flag that forces to use the aecdump values for > > all flags and then just boolean flags with the correct default value? > > That sounds like a good option. But I see the drawback of having to do > double-bookkeeping in the sense that the default options in the simulator needs > to be up2date with what the default options in the AECdump. Furthermore, there > are not really any globally default options that can be assumed I think. > > I then prefer the int flag option. But lets ask @minyue what he prefers before > doing any changes here. > > Would you be ok with using the int flag option instead of the "global flag" > alternative? I prefer the global flag suggestion, but I am open to an int flag. But agreed on getting minyue's opinion here. I don't understand why the simulator would need to be up to date with the aecdump defaults. How is this different in my proposal than in the one you implemented? Anyway, I think there could be some reasonable global defaults when the ones from the aecdump are not used. Maybe something like the components that are enabled in a standard WebRTC call, but I see how this is fuzzy. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:138: DEFINE_bool(discard_settings_in_aecdump, On 2016/04/28 07:41:11, peah-webrtc wrote: > On 2016/04/27 16:06:47, aluebs-webrtc wrote: > > On 2016/04/26 07:19:29, peah-webrtc wrote: > > > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > > > Probably more intuitive to use the positive opposite use_aecdump_settings. > > > > > > The absolutely most common thing to do when analyzing aecdumps is to reapply > > the > > > settings stored in the aecdump, so therefore I wanted that to be the default > > > option. If the option is called use_aecdump_settings, and the default value > is > > > true, then one must negate that in the command line to achieve discarding > the > > > settings. > > > Would it be possible to do that nicely using the "no" flags option you > > > mentioned? > > > > Yes, you just --use nouse_aecdump_settings > > Yes, but that would then mean that "use_aecdump_settings" is never used (since > it is anyway the default) and the only thing the user would specify is the > "nouse_aecdump_settings" which at least to me is more awkward than to use > "discard_settings_in_aecdump". > > Furthermore, use_aecdump_settings sounds more like the the settings stored in > the aecdump are used, which is not really true as they may be overridden by any > of the other command-line settings (e.g., agc_mode). Therefore, I think > discard_settings_in_aecdump is better in that it exactly specifies what is being > done. Agreed. Let's leave it as is. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:179: new WavWriter(filename, out_config_.sample_rate_hz(), On 2016/04/28 07:41:11, peah-webrtc wrote: > On 2016/04/27 16:06:47, aluebs-webrtc wrote: > > Is this going to work for the AudioFrame interface that forces the same input > > and output sample rates? > > It will if the simulation is set up properly. I did not add the checks for that > so that will need to be reported as errors by the APM. > > Even though it is easy to check the command line arguments for this particular > error when operating on .wav files, it is not as straightforward when operating > on aecdumps, as the information on which interface is used is stored as part of > the recording. Furthermore, even for the .wav file case, the .wav file headers > needs to be parsed to be able to ensure this. > > Therefore, I chose to instead rely on the APM internal error reporting. Sounds good. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:231: !settings_.use_extended_filter) { On 2016/04/28 07:41:11, peah-webrtc wrote: > On 2016/04/27 16:06:47, aluebs-webrtc wrote: > > Instead of these complicated conditions, wouldn't it be easier to set the > > default value before and then overwrite it like the others? > > Definitely, and I think that the reason that it is written like this is now > longer valid. I'll change this, as it is not even correct but please let me know > if it can be further simplified! > > Note, however, that the default values for these are not correct as to how the > APM is by default run. It can be simplified as: config.Set<ExtendedFilter>(new ExtendedFilter(!settings_.use_extended_filter || *settings_.use_extended_filter)); https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.h (right): https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:14: #include <algorithm> On 2016/04/28 07:41:12, peah-webrtc wrote: > On 2016/04/27 16:06:48, aluebs-webrtc wrote: > > Is this include really needed here? > > git cl upload complains if it is not there. I think it is needed for the > std::min operation. Acknowledged. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:38: int target_angle_degrees = 90; On 2016/04/28 07:41:12, peah-webrtc wrote: > On 2016/04/27 16:06:48, aluebs-webrtc wrote: > > Why are there some settings that are not optional? > > I tried to make those that did not need to be optional nonoptional. > For instance, the target_angle_degrees does not need to be optional since it > always needs to be specified if the beamformer is activated (correct?) so it can > piggyback on the use_bf optional. Furthermore, the target_angle_degrees is not > stored in any protobuf recording and therefore there is no need to use an > optional to determine whether the values in the protobuf recording should be > overridden. > > There are similar arguments for the other non-optional values. In general I > tried to make as few things optional as possible. > I'd be happy for any suggestions for changes in this scheme. Sounds good. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:135: size_t output_reset_counter_ = 0; On 2016/04/28 07:41:12, peah-webrtc wrote: > On 2016/04/27 16:06:48, aluebs-webrtc wrote: > > These default values should be set in the constructor instead. > I think the guideline is agnostic to that (right?). I've actually often gotten > feedback from others that it is better to put the default initializers here > rather than in the constructor. > > I don't have a strong opinion on this but I prefer this way as it does not > clutter the constructor, produces fewer lines of code and keeps the > initialization close to where the variables are. > Wdyt? Are you ok with keeping them as they are. Agreed on leaving as is. https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/wav_based_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:31: ap_->set_stream_key_pressed(true); Isn't there a setting for this? For example if the TS is enabled? https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:42: last_specified_microphone_level_)); last_specified_microphone_level_ is never modified. Should it be a constant? https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:66: switch (call_chain_[call_chain_index]) { Do we really need this enum-function-vector-switch complexity to just toggle between ProcessStream and ProcessReverseStream? Can't we just call one after the other in the loop directly? https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:84: num_forward_chunks_processed * 1.f / You can cast to avoid a multiplication here. https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:93: if (buffer_reader_->Read(in_buf_.get())) { I like functions with only one return statement: bool samples_left_to_process = buffer_reader_->Read(in_buf_.get()); if (samples_left_to_process) { ... } return samples_left_to_process; But anyway, you don't need the else clause. https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:95: ProcessStream(settings_.fixed_interface); Why do we need to pass in this setting? Can't AudioProcessingSimulator access itself? https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/wav_based_simulator.h (right): https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.h:44: std::vector<WavBasedSimulator::SimulationEventType> GetDefaultEventChain() WavBasedSimulator:: not necessary and below.
Again, thanks for the great comments! I have updated the CL with a new patch according to these. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:83: DEFINE_bool(no_le, false, "Dectivate the level estimator"); On 2016/04/30 02:08:04, aluebs-webrtc wrote: > On 2016/04/28 07:41:11, peah-webrtc wrote: > > On 2016/04/27 16:06:47, aluebs-webrtc wrote: > > > On 2016/04/26 07:19:29, peah-webrtc wrote: > > > > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > > > > Why do we need 2 flags to specify if a component is enabled or disabled? > > (if > > > I > > > > > remember correctly, each "flag" has its correspondent "noflag") Also, > why > > > are > > > > > both false? Isn't it easier to have only one flag with the right default > > > value > > > > > that can be overwritten? Then you can also drop the all_default flag. > > > > > > > > This was the most simple way (for the user of the audioproc_f) that I > could > > > come > > > > up with but there are probably better alternatives. > > > > > > > > The underlying idea behind this is that each parameter basically should > > > > constitute a tri-state value, where it > > > > either turns off the feature, turns on the feature, or keeps it as its > > default > > > > value. > > > > This is important for this to work well with aecdump recordings where > there > > > are > > > > default values for the settings stored in the recording that the simulator > > > > should act upon. > > > > When reading the config event from the aecdump the simulator will then > > either > > > > 1) apply the setting in the config if no parameter value has been > explicitly > > > > specified on the command line. > > > > 2) apply the parameter setting specified if that has been explicitly > > specified > > > > on the command line. > > > > > > > > For a on/off feature, for instance turning on the aec a tri-state variable > > is > > > > needed for the above. And it is not possible to use a default value as > that > > > > would then overwrite the value in the config. For non on/off parameters I > > > solved > > > > this by using a default value kParameterNotSpecifiedValue represent that > the > > > > parameter had not been set, but for boolean flags that is not possible. > > > > > > > > Afaics, this is not possible to achieve using the no-flag-feature of > gflags, > > > as > > > > that enforces the options to be two-state (bundling the no_aec and aec > only > > > > gives two values for the parameter and therefore it cannot be used as a > > > > tri-state to control the feature). > > > > > > > > One alternative could be to use integer flags instead of specifying the > > on/off > > > > flags using boolean flags, i.e., instead of using the command line > --no_aec > > > use > > > > --aec 0. I did not choose that option as that was not how the > > process_test.cc > > > > and audioproc_float.cc used it before, but if you think that is a better > > > option > > > > I'd be happy to use that. > > > > > > > > That is some background to the choice of implementation. If you have a > > better > > > > approach, I'd be happy to change this. > > > > > > > > > > > > > > I personally prefer 1 int flag than 2 different bool flags that add more > code > > > and is confusing for a user, since you could technically specify "flag" and > > > "no_flag" at the same time. > > > But what about this. One global flag that forces to use the aecdump values > for > > > all flags and then just boolean flags with the correct default value? > > > > That sounds like a good option. But I see the drawback of having to do > > double-bookkeeping in the sense that the default options in the simulator > needs > > to be up2date with what the default options in the AECdump. Furthermore, there > > are not really any globally default options that can be assumed I think. > > > > I then prefer the int flag option. But lets ask @minyue what he prefers before > > doing any changes here. > > > > Would you be ok with using the int flag option instead of the "global flag" > > alternative? > > I prefer the global flag suggestion, but I am open to an int flag. But agreed on > getting minyue's opinion here. > I don't understand why the simulator would need to be up to date with the > aecdump defaults. How is this different in my proposal than in the one you > implemented? > Anyway, I think there could be some reasonable global defaults when the ones > from the aecdump are not used. Maybe something like the components that are > enabled in a standard WebRTC call, but I see how this is fuzzy. Maybe I misunderstood you but my concern is what I would do if I'd want to run an aecdump recording and customize some feature: for instance just turn off the noise suppressor. Then I'd either have to rely on the all the other flags to have default values that matches the values in the recording, or manually specify all the values that were present in the recording. Both ways to do that seems error prone to me and I prefer analysis of aecdump recordings to something that should be possible to done quickly in a way that can cause as few mistakes as possible. There are definitely some reasonable global defaults that could be used but if we can avoid adding a dependency on them I think we should, both to reduce the risk of errors and also to avoid having to update the defaults in the simulator each time those defaults are changed. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:231: !settings_.use_extended_filter) { On 2016/04/30 02:08:04, aluebs-webrtc wrote: > On 2016/04/28 07:41:11, peah-webrtc wrote: > > On 2016/04/27 16:06:47, aluebs-webrtc wrote: > > > Instead of these complicated conditions, wouldn't it be easier to set the > > > default value before and then overwrite it like the others? > > > > Definitely, and I think that the reason that it is written like this is now > > longer valid. I'll change this, as it is not even correct but please let me > know > > if it can be further simplified! > > > > Note, however, that the default values for these are not correct as to how the > > APM is by default run. > > It can be simplified as: > config.Set<ExtendedFilter>(new ExtendedFilter(!settings_.use_extended_filter || > *settings_.use_extended_filter)); Of course! Nice! Done. https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/wav_based_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:31: ap_->set_stream_key_pressed(true); On 2016/04/30 02:08:05, aluebs-webrtc wrote: > Isn't there a setting for this? For example if the TS is enabled? True. Done. https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:42: last_specified_microphone_level_)); On 2016/04/30 02:08:05, aluebs-webrtc wrote: > last_specified_microphone_level_ is never modified. Should it be a constant? That is a mistake. The idea is to query the last level from the AGC but that code got lost. I readded it. PTAL. https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:66: switch (call_chain_[call_chain_index]) { On 2016/04/30 02:08:05, aluebs-webrtc wrote: > Do we really need this enum-function-vector-switch complexity to just toggle > between ProcessStream and ProcessReverseStream? Can't we just call one after the > other in the loop directly? The idea is to simplify changing the default call-order if needed. For instance, it is sometimes important to evaluate call-orders like RFRF... or RRFRRFFF... instead of the standard FRFR... (F=forward, R=reverse). This is much simpler to alter if the order is stored in a vector rather than hardcoded by toggling between the calls. https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:84: num_forward_chunks_processed * 1.f / On 2016/04/30 02:08:05, aluebs-webrtc wrote: > You can cast to avoid a multiplication here. Done. https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:93: if (buffer_reader_->Read(in_buf_.get())) { On 2016/04/30 02:08:05, aluebs-webrtc wrote: > I like functions with only one return statement: > > bool samples_left_to_process = buffer_reader_->Read(in_buf_.get()); > if (samples_left_to_process) { > ... > } > return samples_left_to_process; > > But anyway, you don't need the else clause. Nice! Done. https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:95: ProcessStream(settings_.fixed_interface); On 2016/04/30 02:08:05, aluebs-webrtc wrote: > Why do we need to pass in this setting? Can't AudioProcessingSimulator access > itself? It could for the wav-based simulation but it cannot for the aecdump-based simulation as this is stored in the aecdump and the reading of that is done outside AudioProcessingSimulator. Therefore, in order to make the operations in the wav-based and aecdump-based processing as simular as possibly I implemented it like this. I could add another ProcessStream call in AudioProcessingSimulator that takes no arguments and which determines the interface based on the settings but I prefer this variant since 1) I think it simplifies the code not to extende the AudioProcessingSimulator api further. 2) The alternative will require more lines of code. WDYT? As it is now https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/wav_based_simulator.h (right): https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.h:44: std::vector<WavBasedSimulator::SimulationEventType> GetDefaultEventChain() On 2016/04/30 02:08:05, aluebs-webrtc wrote: > WavBasedSimulator:: not necessary and below. Done.
https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:83: DEFINE_bool(no_le, false, "Dectivate the level estimator"); On 2016/05/02 06:18:42, peah-webrtc wrote: > On 2016/04/30 02:08:04, aluebs-webrtc wrote: > > On 2016/04/28 07:41:11, peah-webrtc wrote: > > > On 2016/04/27 16:06:47, aluebs-webrtc wrote: > > > > On 2016/04/26 07:19:29, peah-webrtc wrote: > > > > > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > > > > > Why do we need 2 flags to specify if a component is enabled or > disabled? > > > (if > > > > I > > > > > > remember correctly, each "flag" has its correspondent "noflag") Also, > > why > > > > are > > > > > > both false? Isn't it easier to have only one flag with the right > default > > > > value > > > > > > that can be overwritten? Then you can also drop the all_default flag. > > > > > > > > > > This was the most simple way (for the user of the audioproc_f) that I > > could > > > > come > > > > > up with but there are probably better alternatives. > > > > > > > > > > The underlying idea behind this is that each parameter basically should > > > > > constitute a tri-state value, where it > > > > > either turns off the feature, turns on the feature, or keeps it as its > > > default > > > > > value. > > > > > This is important for this to work well with aecdump recordings where > > there > > > > are > > > > > default values for the settings stored in the recording that the > simulator > > > > > should act upon. > > > > > When reading the config event from the aecdump the simulator will then > > > either > > > > > 1) apply the setting in the config if no parameter value has been > > explicitly > > > > > specified on the command line. > > > > > 2) apply the parameter setting specified if that has been explicitly > > > specified > > > > > on the command line. > > > > > > > > > > For a on/off feature, for instance turning on the aec a tri-state > variable > > > is > > > > > needed for the above. And it is not possible to use a default value as > > that > > > > > would then overwrite the value in the config. For non on/off parameters > I > > > > solved > > > > > this by using a default value kParameterNotSpecifiedValue represent that > > the > > > > > parameter had not been set, but for boolean flags that is not possible. > > > > > > > > > > Afaics, this is not possible to achieve using the no-flag-feature of > > gflags, > > > > as > > > > > that enforces the options to be two-state (bundling the no_aec and aec > > only > > > > > gives two values for the parameter and therefore it cannot be used as a > > > > > tri-state to control the feature). > > > > > > > > > > One alternative could be to use integer flags instead of specifying the > > > on/off > > > > > flags using boolean flags, i.e., instead of using the command line > > --no_aec > > > > use > > > > > --aec 0. I did not choose that option as that was not how the > > > process_test.cc > > > > > and audioproc_float.cc used it before, but if you think that is a better > > > > option > > > > > I'd be happy to use that. > > > > > > > > > > That is some background to the choice of implementation. If you have a > > > better > > > > > approach, I'd be happy to change this. > > > > > > > > > > > > > > > > > > I personally prefer 1 int flag than 2 different bool flags that add more > > code > > > > and is confusing for a user, since you could technically specify "flag" > and > > > > "no_flag" at the same time. > > > > But what about this. One global flag that forces to use the aecdump values > > for > > > > all flags and then just boolean flags with the correct default value? > > > > > > That sounds like a good option. But I see the drawback of having to do > > > double-bookkeeping in the sense that the default options in the simulator > > needs > > > to be up2date with what the default options in the AECdump. Furthermore, > there > > > are not really any globally default options that can be assumed I think. > > > > > > I then prefer the int flag option. But lets ask @minyue what he prefers > before > > > doing any changes here. > > > > > > Would you be ok with using the int flag option instead of the "global flag" > > > alternative? > > > > I prefer the global flag suggestion, but I am open to an int flag. But agreed > on > > getting minyue's opinion here. > > I don't understand why the simulator would need to be up to date with the > > aecdump defaults. How is this different in my proposal than in the one you > > implemented? > > Anyway, I think there could be some reasonable global defaults when the ones > > from the aecdump are not used. Maybe something like the components that are > > enabled in a standard WebRTC call, but I see how this is fuzzy. > > Maybe I misunderstood you but my concern is what I would do if I'd want to run > an aecdump recording and customize some feature: for instance just turn off the > noise suppressor. Then I'd either have to rely on the all the other flags to > have default values that matches the values in the recording, or manually > specify all the values that were present in the recording. > Both ways to do that seems error prone to me and I prefer analysis of aecdump > recordings to something that should be possible to done quickly in a way that > can cause as few mistakes as possible. > > There are definitely some reasonable global defaults that could be used but if > we can avoid adding a dependency on them I think we should, both to reduce the > risk of errors and also to avoid having to update the defaults in the simulator > each time those defaults are changed. I see the point when you want to use the aecdump values for all except a few parameters it can be painful. But in that case I still feel the int flags would be less bug prone and confusing. But as said, let's get minyue's view on this. https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/wav_based_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:42: last_specified_microphone_level_)); On 2016/05/02 06:18:42, peah-webrtc wrote: > On 2016/04/30 02:08:05, aluebs-webrtc wrote: > > last_specified_microphone_level_ is never modified. Should it be a constant? > > That is a mistake. The idea is to query the last level from the AGC but that > code got lost. I readded it. PTAL. Maybe I am not familiar enough with this setting, but why do we need to query it, to set it again here? Doesn't it maintain its value internally? https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:66: switch (call_chain_[call_chain_index]) { On 2016/05/02 06:18:42, peah-webrtc wrote: > On 2016/04/30 02:08:05, aluebs-webrtc wrote: > > Do we really need this enum-function-vector-switch complexity to just toggle > > between ProcessStream and ProcessReverseStream? Can't we just call one after > the > > other in the loop directly? > > The idea is to simplify changing the default call-order if needed. For instance, > it is sometimes important to evaluate call-orders like RFRF... or RRFRRFFF... > instead of the standard FRFR... (F=forward, R=reverse). > This is much simpler to alter if the order is stored in a vector rather than > hardcoded by toggling between the calls. If you think this is a enough frequent use case to justify the higher code complexity, that is ok with me. https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:95: ProcessStream(settings_.fixed_interface); On 2016/05/02 06:18:42, peah-webrtc wrote: > On 2016/04/30 02:08:05, aluebs-webrtc wrote: > > Why do we need to pass in this setting? Can't AudioProcessingSimulator access > > itself? > > It could for the wav-based simulation but it cannot for the aecdump-based > simulation as this is stored in the aecdump and the reading of that is done > outside AudioProcessingSimulator. > Therefore, in order to make the operations in the wav-based and aecdump-based > processing as simular as possibly I implemented it like this. > > I could add another ProcessStream call in AudioProcessingSimulator that takes no > arguments and which determines the interface based on the settings but I prefer > this variant since > 1) I think it simplifies the code not to extende the AudioProcessingSimulator > api further. > 2) The alternative will require more lines of code. > > WDYT? > As it is now If that is the case, it makes sense to leave as is. I didn't know we stored the interface used in the aecdump. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:29: if (msg.output_data().data()[k] != frame.data_[k]) { Did you verify that it is possible to achieve bitexactness with this tool using the same configuration? Because maybe we need to introduce a tolerance here. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:43: return false; Why does this return false while the above CHECKs on the size? I don't have a preference, but I think it should be consistent. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:61: if (msg.has_input_data()) { Why does this decides the interface? Is it just a horrible naming? https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:68: // ProcessStream could have changed this for the output frame. It doesn't though, right? https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:72: RTC_CHECK_EQ(sizeof(int16_t) * fwd_frame_.samples_per_channel_ * sizeof(fwd_frame_.data[0])? https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:87: for (int i = 0; i < msg.input_channel_size(); ++i) { size_t? https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:96: if (msg.has_delay()) { You probably still want to set_stream_delay_ms to 0 when the msg has no delay, right? Also, you can probably simplify this with conditional operators. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:114: if (msg.has_keypress()) { If it doesn't, set it depending on use_ts. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:117: } Aren't you missing set_stream_analog_level here? https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:157: reverse_in_buf_->num_frames() * sizeof(*in_buf_->channels()[i]), reverse_in_buf_ https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:200: num_forward_chunks_processed * 1.f / Again, cast to avoid multiplication? https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:213: printf("Config at frame: "); Use cout instead of printf? You also probably want to add a new-line after the colon. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:224: if (msg.has_aec_enabled()) { This logic makes it so that if you set a parameter from the terminal but it is not specified in the aecdump it doesn't get set. That sounds wrong to me. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:343: settings_.use_ts ? *settings_.use_ts : msg.noise_robust_agc_enabled(); What does the noise_robust_agc has to do with TS? https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.h (right): https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.h:47: enum InterfaceType { This is defined in wav_based_simulator and here. Can't we have it only once in audio_processing_simulator?
https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:83: DEFINE_bool(no_le, false, "Dectivate the level estimator"); On 2016/05/03 22:30:40, aluebs-webrtc wrote: > On 2016/05/02 06:18:42, peah-webrtc wrote: > > On 2016/04/30 02:08:04, aluebs-webrtc wrote: > > > On 2016/04/28 07:41:11, peah-webrtc wrote: > > > > On 2016/04/27 16:06:47, aluebs-webrtc wrote: > > > > > On 2016/04/26 07:19:29, peah-webrtc wrote: > > > > > > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > > > > > > Why do we need 2 flags to specify if a component is enabled or > > disabled? > > > > (if > > > > > I > > > > > > > remember correctly, each "flag" has its correspondent "noflag") > Also, > > > why > > > > > are > > > > > > > both false? Isn't it easier to have only one flag with the right > > default > > > > > value > > > > > > > that can be overwritten? Then you can also drop the all_default > flag. > > > > > > > > > > > > This was the most simple way (for the user of the audioproc_f) that I > > > could > > > > > come > > > > > > up with but there are probably better alternatives. > > > > > > > > > > > > The underlying idea behind this is that each parameter basically > should > > > > > > constitute a tri-state value, where it > > > > > > either turns off the feature, turns on the feature, or keeps it as its > > > > default > > > > > > value. > > > > > > This is important for this to work well with aecdump recordings where > > > there > > > > > are > > > > > > default values for the settings stored in the recording that the > > simulator > > > > > > should act upon. > > > > > > When reading the config event from the aecdump the simulator will then > > > > either > > > > > > 1) apply the setting in the config if no parameter value has been > > > explicitly > > > > > > specified on the command line. > > > > > > 2) apply the parameter setting specified if that has been explicitly > > > > specified > > > > > > on the command line. > > > > > > > > > > > > For a on/off feature, for instance turning on the aec a tri-state > > variable > > > > is > > > > > > needed for the above. And it is not possible to use a default value as > > > that > > > > > > would then overwrite the value in the config. For non on/off > parameters > > I > > > > > solved > > > > > > this by using a default value kParameterNotSpecifiedValue represent > that > > > the > > > > > > parameter had not been set, but for boolean flags that is not > possible. > > > > > > > > > > > > Afaics, this is not possible to achieve using the no-flag-feature of > > > gflags, > > > > > as > > > > > > that enforces the options to be two-state (bundling the no_aec and aec > > > only > > > > > > gives two values for the parameter and therefore it cannot be used as > a > > > > > > tri-state to control the feature). > > > > > > > > > > > > One alternative could be to use integer flags instead of specifying > the > > > > on/off > > > > > > flags using boolean flags, i.e., instead of using the command line > > > --no_aec > > > > > use > > > > > > --aec 0. I did not choose that option as that was not how the > > > > process_test.cc > > > > > > and audioproc_float.cc used it before, but if you think that is a > better > > > > > option > > > > > > I'd be happy to use that. > > > > > > > > > > > > That is some background to the choice of implementation. If you have a > > > > better > > > > > > approach, I'd be happy to change this. > > > > > > > > > > > > > > > > > > > > > > I personally prefer 1 int flag than 2 different bool flags that add more > > > code > > > > > and is confusing for a user, since you could technically specify "flag" > > and > > > > > "no_flag" at the same time. > > > > > But what about this. One global flag that forces to use the aecdump > values > > > for > > > > > all flags and then just boolean flags with the correct default value? > > > > > > > > That sounds like a good option. But I see the drawback of having to do > > > > double-bookkeeping in the sense that the default options in the simulator > > > needs > > > > to be up2date with what the default options in the AECdump. Furthermore, > > there > > > > are not really any globally default options that can be assumed I think. > > > > > > > > I then prefer the int flag option. But lets ask @minyue what he prefers > > before > > > > doing any changes here. > > > > > > > > Would you be ok with using the int flag option instead of the "global > flag" > > > > alternative? > > > > > > I prefer the global flag suggestion, but I am open to an int flag. But > agreed > > on > > > getting minyue's opinion here. > > > I don't understand why the simulator would need to be up to date with the > > > aecdump defaults. How is this different in my proposal than in the one you > > > implemented? > > > Anyway, I think there could be some reasonable global defaults when the ones > > > from the aecdump are not used. Maybe something like the components that are > > > enabled in a standard WebRTC call, but I see how this is fuzzy. > > > > Maybe I misunderstood you but my concern is what I would do if I'd want to run > > an aecdump recording and customize some feature: for instance just turn off > the > > noise suppressor. Then I'd either have to rely on the all the other flags to > > have default values that matches the values in the recording, or manually > > specify all the values that were present in the recording. > > Both ways to do that seems error prone to me and I prefer analysis of aecdump > > recordings to something that should be possible to done quickly in a way that > > can cause as few mistakes as possible. > > > > There are definitely some reasonable global defaults that could be used but if > > we can avoid adding a dependency on them I think we should, both to reduce the > > risk of errors and also to avoid having to update the defaults in the > simulator > > each time those defaults are changed. > > I see the point when you want to use the aecdump values for all except a few > parameters it can be painful. But in that case I still feel the int flags would > be less bug prone and confusing. But as said, let's get minyue's view on this. Acknowledged. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:138: DEFINE_bool(discard_settings_in_aecdump, On 2016/04/30 02:08:04, aluebs-webrtc wrote: > On 2016/04/28 07:41:11, peah-webrtc wrote: > > On 2016/04/27 16:06:47, aluebs-webrtc wrote: > > > On 2016/04/26 07:19:29, peah-webrtc wrote: > > > > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > > > > Probably more intuitive to use the positive opposite > use_aecdump_settings. > > > > > > > > The absolutely most common thing to do when analyzing aecdumps is to > reapply > > > the > > > > settings stored in the aecdump, so therefore I wanted that to be the > default > > > > option. If the option is called use_aecdump_settings, and the default > value > > is > > > > true, then one must negate that in the command line to achieve discarding > > the > > > > settings. > > > > Would it be possible to do that nicely using the "no" flags option you > > > > mentioned? > > > > > > Yes, you just --use nouse_aecdump_settings > > > > Yes, but that would then mean that "use_aecdump_settings" is never used (since > > it is anyway the default) and the only thing the user would specify is the > > "nouse_aecdump_settings" which at least to me is more awkward than to use > > "discard_settings_in_aecdump". > > > > Furthermore, use_aecdump_settings sounds more like the the settings stored in > > the aecdump are used, which is not really true as they may be overridden by > any > > of the other command-line settings (e.g., agc_mode). Therefore, I think > > discard_settings_in_aecdump is better in that it exactly specifies what is > being > > done. > > Agreed. Let's leave it as is. Acknowledged. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:272: report_conditional_error_and_exit(!!settings.aec_dump_input_filename, On 2016/04/27 16:06:47, aluebs-webrtc wrote: > On 2016/04/26 07:19:29, peah-webrtc wrote: > > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > > double negation? > > > > aec_dump_input_filename is an optional, and I want to true to be passed if it > is > > set. The double negation is a way to achieve that in a simple manner. > > Oh, I thought it would cast automatically to bool. Acknowledged. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:302: report_conditional_error_and_exit( On 2016/04/27 16:06:47, aluebs-webrtc wrote: > On 2016/04/26 07:19:29, peah-webrtc wrote: > > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > > All these checks for a single parameter can be implemented in the gflags > > > framework. > > > > No, that does not work if you want to have tri-state variables. The problem > then > > is that the validator enforces the default value to be valid, and therefore > the > > default value that is used as an indicator that the parameter is not set must > be > > changed to be in the valid range. For some of the options that is possible but > > not for some of the others. > > Good point. Acknowledged. https://codereview.webrtc.org/1907223003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:325: settings.target_angle_degrees < 0 || settings.target_angle_degrees > 90, On 2016/04/27 16:06:47, aluebs-webrtc wrote: > On 2016/04/26 07:19:29, peah-webrtc wrote: > > On 2016/04/26 04:33:39, aluebs-webrtc wrote: > > > The angle can definitively go between 0 and 359 degrees. And I think that if > > it > > > isn't in that range it would wrap to this range anyways. > > > > My mistake. I think it is anyway nice to impose a sane angle between 0 and > 359. > > Or do you instead prefer no limit and rely on wrapping behavior? > > > > Done. > > I don't mind imposing this limit. Acknowledged. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:179: new WavWriter(filename, out_config_.sample_rate_hz(), On 2016/04/30 02:08:04, aluebs-webrtc wrote: > On 2016/04/28 07:41:11, peah-webrtc wrote: > > On 2016/04/27 16:06:47, aluebs-webrtc wrote: > > > Is this going to work for the AudioFrame interface that forces the same > input > > > and output sample rates? > > > > It will if the simulation is set up properly. I did not add the checks for > that > > so that will need to be reported as errors by the APM. > > > > Even though it is easy to check the command line arguments for this particular > > error when operating on .wav files, it is not as straightforward when > operating > > on aecdumps, as the information on which interface is used is stored as part > of > > the recording. Furthermore, even for the .wav file case, the .wav file headers > > needs to be parsed to be able to ensure this. > > > > Therefore, I chose to instead rely on the APM internal error reporting. > > Sounds good. Acknowledged. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.h (right): https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:14: #include <algorithm> On 2016/04/30 02:08:04, aluebs-webrtc wrote: > On 2016/04/28 07:41:12, peah-webrtc wrote: > > On 2016/04/27 16:06:48, aluebs-webrtc wrote: > > > Is this include really needed here? > > > > git cl upload complains if it is not there. I think it is needed for the > > std::min operation. > > Acknowledged. Acknowledged. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:38: int target_angle_degrees = 90; On 2016/04/30 02:08:04, aluebs-webrtc wrote: > On 2016/04/28 07:41:12, peah-webrtc wrote: > > On 2016/04/27 16:06:48, aluebs-webrtc wrote: > > > Why are there some settings that are not optional? > > > > I tried to make those that did not need to be optional nonoptional. > > For instance, the target_angle_degrees does not need to be optional since it > > always needs to be specified if the beamformer is activated (correct?) so it > can > > piggyback on the use_bf optional. Furthermore, the target_angle_degrees is not > > stored in any protobuf recording and therefore there is no need to use an > > optional to determine whether the values in the protobuf recording should be > > overridden. > > > > There are similar arguments for the other non-optional values. In general I > > tried to make as few things optional as possible. > > I'd be happy for any suggestions for changes in this scheme. > > Sounds good. Acknowledged. https://codereview.webrtc.org/1907223003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:135: size_t output_reset_counter_ = 0; On 2016/04/30 02:08:04, aluebs-webrtc wrote: > On 2016/04/28 07:41:12, peah-webrtc wrote: > > On 2016/04/27 16:06:48, aluebs-webrtc wrote: > > > These default values should be set in the constructor instead. > > I think the guideline is agnostic to that (right?). I've actually often gotten > > feedback from others that it is better to put the default initializers here > > rather than in the constructor. > > > > I don't have a strong opinion on this but I prefer this way as it does not > > clutter the constructor, produces fewer lines of code and keeps the > > initialization close to where the variables are. > > Wdyt? Are you ok with keeping them as they are. > > Agreed on leaving as is. Acknowledged. https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/wav_based_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:42: last_specified_microphone_level_)); On 2016/05/03 22:30:40, aluebs-webrtc wrote: > On 2016/05/02 06:18:42, peah-webrtc wrote: > > On 2016/04/30 02:08:05, aluebs-webrtc wrote: > > > last_specified_microphone_level_ is never modified. Should it be a constant? > > > > That is a mistake. The idea is to query the last level from the AGC but that > > code got lost. I readded it. PTAL. > > Maybe I am not familiar enough with this setting, but why do we need to query > it, to set it again here? Doesn't it maintain its value internally? I'm not sure how it works in APM but this is how process_test.cc does it. And I think it makes sense. analog_stream_level() reports what level APM thinks the analog level should be at, and set_analog_stream_level() reports what the analog level was actually set to. So by doing it like this we basically simulate an analog microphone level that is set exactly to what APM wants. https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:66: switch (call_chain_[call_chain_index]) { On 2016/05/03 22:30:40, aluebs-webrtc wrote: > On 2016/05/02 06:18:42, peah-webrtc wrote: > > On 2016/04/30 02:08:05, aluebs-webrtc wrote: > > > Do we really need this enum-function-vector-switch complexity to just toggle > > > between ProcessStream and ProcessReverseStream? Can't we just call one after > > the > > > other in the loop directly? > > > > The idea is to simplify changing the default call-order if needed. For > instance, > > it is sometimes important to evaluate call-orders like RFRF... or RRFRRFFF... > > instead of the standard FRFR... (F=forward, R=reverse). > > This is much simpler to alter if the order is stored in a vector rather than > > hardcoded by toggling between the calls. > > If you think this is a enough frequent use case to justify the higher code > complexity, that is ok with me. Acknowledged. https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:95: ProcessStream(settings_.fixed_interface); On 2016/05/03 22:30:40, aluebs-webrtc wrote: > On 2016/05/02 06:18:42, peah-webrtc wrote: > > On 2016/04/30 02:08:05, aluebs-webrtc wrote: > > > Why do we need to pass in this setting? Can't AudioProcessingSimulator > access > > > itself? > > > > It could for the wav-based simulation but it cannot for the aecdump-based > > simulation as this is stored in the aecdump and the reading of that is done > > outside AudioProcessingSimulator. > > Therefore, in order to make the operations in the wav-based and aecdump-based > > processing as simular as possibly I implemented it like this. > > > > I could add another ProcessStream call in AudioProcessingSimulator that takes > no > > arguments and which determines the interface based on the settings but I > prefer > > this variant since > > 1) I think it simplifies the code not to extende the AudioProcessingSimulator > > api further. > > 2) The alternative will require more lines of code. > > > > WDYT? > > As it is now > > If that is the case, it makes sense to leave as is. I didn't know we stored the > interface used in the aecdump. Acknowledged. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:29: if (msg.output_data().data()[k] != frame.data_[k]) { On 2016/05/03 22:30:40, aluebs-webrtc wrote: > Did you verify that it is possible to achieve bitexactness with this tool using > the same configuration? Because maybe we need to introduce a tolerance here. It is/should definite be possible to achieve bitexactness for a recording done singlethreadedly which is processed on the very same machine. I'm not sure whether it would make sense to have a threshold for the bitexactness. I'd rather then have the maximum difference being reported as a value. Would it be fine to wait with adding a threshold value until we have used the tool more? An alternative would also be to remove the bitexactness testing functionality fully for now, and wait to introduce it until later. WDYT? https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:43: return false; On 2016/05/03 22:30:40, aluebs-webrtc wrote: > Why does this return false while the above CHECKs on the size? I don't have a > preference, but I think it should be consistent. Good point! Will fix that. Done. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:61: if (msg.has_input_data()) { On 2016/05/03 22:30:40, aluebs-webrtc wrote: > Why does this decides the interface? Is it just a horrible naming? Yes, that is how it is currently in the protobuf file. Regardless that naming I think for backward compatibility we should keep it as it is. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:68: // ProcessStream could have changed this for the output frame. On 2016/05/03 22:30:40, aluebs-webrtc wrote: > It doesn't though, right? This is how it is currently done in process_test.cc, and the comment comes from that file. With the current implementation of the resampling and deinterlieving this is very hard to ensure and verify by looking at the code. But as far as I can see, if the beamformer is activated, it will actually explicitly change the number of channels in capture_audio. And since capture_audio is then copied back into the AudioFrame I cannot see anything that prevents the number of channels to change. I think this line has to be there as long as the beamformer could be activated. Wdyt? https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:72: RTC_CHECK_EQ(sizeof(int16_t) * fwd_frame_.samples_per_channel_ * On 2016/05/03 22:30:40, aluebs-webrtc wrote: > sizeof(fwd_frame_.data[0])? Done. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:87: for (int i = 0; i < msg.input_channel_size(); ++i) { On 2016/05/03 22:30:40, aluebs-webrtc wrote: > size_t? No, msg.input_channel_size() is of type int. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:96: if (msg.has_delay()) { On 2016/05/03 22:30:40, aluebs-webrtc wrote: > You probably still want to set_stream_delay_ms to 0 when the msg has no delay, > right? Also, you can probably simplify this with conditional operators. No, I don't think so. If the recording has no stream delay stored I don't think that should be masked by defaulting to a default value. If there is no stream delay stored I think that should best be reflected with that no call was made to the set_stream_delay() method and we should then let APM handle that according to its implementation. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:114: if (msg.has_keypress()) { On 2016/05/03 22:30:40, aluebs-webrtc wrote: > If it doesn't, set it depending on use_ts. Done. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:117: } On 2016/05/03 22:30:41, aluebs-webrtc wrote: > Aren't you missing set_stream_analog_level here? Great find! Done. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:157: reverse_in_buf_->num_frames() * sizeof(*in_buf_->channels()[i]), On 2016/05/03 22:30:40, aluebs-webrtc wrote: > reverse_in_buf_ Good find! Done. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:200: num_forward_chunks_processed * 1.f / On 2016/05/03 22:30:40, aluebs-webrtc wrote: > Again, cast to avoid multiplication? Done. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:213: printf("Config at frame: "); On 2016/05/03 22:30:40, aluebs-webrtc wrote: > Use cout instead of printf? You also probably want to add a new-line after the > colon. Fixed the new-line. Not sure about using cout though. printf is used elsewhere in this file and it is nice to use the same printout functionality in the file. Do you see any particular advantage of using cout here? https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:224: if (msg.has_aec_enabled()) { On 2016/05/03 22:30:40, aluebs-webrtc wrote: > This logic makes it so that if you set a parameter from the terminal but it is > not specified in the aecdump it doesn't get set. That sounds wrong to me. Agree! Good find! Done. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:343: settings_.use_ts ? *settings_.use_ts : msg.noise_robust_agc_enabled(); On 2016/05/03 22:30:40, aluebs-webrtc wrote: > What does the noise_robust_agc has to do with TS? Good find! I modified this code. PTAL! https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.h (right): https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.h:47: enum InterfaceType { On 2016/05/03 22:30:41, aluebs-webrtc wrote: > This is defined in wav_based_simulator and here. Can't we have it only once in > audio_processing_simulator? I don't really follow this comment. As far as I can see, this enum is only defined here, and it is only used within aec_dump_based_simlator.h. Could you please clarify?
minyue, could you please have a look at this CL, in particular what you think should be done with the command line flags. https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/wav_based_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:42: last_specified_microphone_level_)); On 2016/05/09 11:37:31, peah-webrtc wrote: > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > On 2016/05/02 06:18:42, peah-webrtc wrote: > > > On 2016/04/30 02:08:05, aluebs-webrtc wrote: > > > > last_specified_microphone_level_ is never modified. Should it be a > constant? > > > > > > That is a mistake. The idea is to query the last level from the AGC but that > > > code got lost. I readded it. PTAL. > > > > Maybe I am not familiar enough with this setting, but why do we need to query > > it, to set it again here? Doesn't it maintain its value internally? > > I'm not sure how it works in APM but this is how process_test.cc does it. > > And I think it makes sense. analog_stream_level() reports what level APM thinks > the analog level should be at, and set_analog_stream_level() reports what the > analog level was actually set to. So by doing it like this we basically simulate > an analog microphone level that is set exactly to what APM wants. > Sounds good. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:29: if (msg.output_data().data()[k] != frame.data_[k]) { On 2016/05/09 11:37:31, peah-webrtc wrote: > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > Did you verify that it is possible to achieve bitexactness with this tool > using > > the same configuration? Because maybe we need to introduce a tolerance here. > > It is/should definite be possible to achieve bitexactness for a recording done > singlethreadedly which is processed on the very same machine. > > I'm not sure whether it would make sense to have a threshold for the > bitexactness. I'd rather then have the maximum difference being reported as a > value. > > Would it be fine to wait with adding a threshold value until we have used the > tool more? > > An alternative would also be to remove the bitexactness testing functionality > fully for now, and wait to introduce it until later. > > WDYT? As I said before it would have been better to first refactor and only then add new fuctionality in different CLs. But since I already reviewed this let's leave it as is. We can add the threshold or maximum difference reporting later, but please add a TODO for it in this CL. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:61: if (msg.has_input_data()) { On 2016/05/09 11:37:31, peah-webrtc wrote: > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > Why does this decides the interface? Is it just a horrible naming? > > Yes, that is how it is currently in the protobuf file. Regardless that naming I > think for backward compatibility we should keep it as it is. Agreed. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:68: // ProcessStream could have changed this for the output frame. On 2016/05/09 11:37:31, peah-webrtc wrote: > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > It doesn't though, right? > > This is how it is currently done in process_test.cc, and the comment comes from > that file. > > With the current implementation of the resampling and deinterlieving this is > very hard to ensure and verify by looking at the code. > But as far as I can see, if the beamformer is activated, it will actually > explicitly change the number of channels in capture_audio. And since > capture_audio is then copied back into the AudioFrame I cannot see anything that > prevents the number of channels to change. > > I think this line has to be there as long as the beamformer could be activated. > Wdyt? > The reason of moving away from process_test is to get rid of all the unnecessary code, not copying it over. The beamformer changes the number of channels on the AudioBuffer, but when copying back to the AudioFrame this gets upmixed. I think that is quite simple to see from the code because the properties of the AudioFrame are never modified. Another way of seeing it is that the output number of channels is set equal to the input number of channels for the AudioFrame interface. I find this line of code and the comment misleading. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:87: for (int i = 0; i < msg.input_channel_size(); ++i) { On 2016/05/09 11:37:31, peah-webrtc wrote: > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > size_t? > > No, msg.input_channel_size() is of type int. Acknowledged. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:96: if (msg.has_delay()) { On 2016/05/09 11:37:31, peah-webrtc wrote: > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > You probably still want to set_stream_delay_ms to 0 when the msg has no delay, > > right? Also, you can probably simplify this with conditional operators. > > No, I don't think so. If the recording has no stream delay stored I don't think > that should be masked by defaulting to a default value. If there is no stream > delay stored I think that should best be reflected with that no call was made to > the set_stream_delay() method and we should then let APM handle that according > to its implementation. Good point. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:213: printf("Config at frame: "); On 2016/05/09 11:37:31, peah-webrtc wrote: > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > Use cout instead of printf? You also probably want to add a new-line after the > > colon. > > Fixed the new-line. Not sure about using cout though. printf is used elsewhere > in this file and it is nice to use the same printout functionality in the file. > > Do you see any particular advantage of using cout here? I meant that we probably want to use cout everywhere. It is the C++ alternative to the C printf. It is more type-safe and less error prone with it's static inputs instead of the dynamic %s. It also makes the code simpler. Here are some more reasons: https://isocpp.org/wiki/faq/input-output#iostream-vs-stdio https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:343: settings_.use_ts ? *settings_.use_ts : msg.noise_robust_agc_enabled(); On 2016/05/09 11:37:31, peah-webrtc wrote: > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > What does the noise_robust_agc has to do with TS? > > Good find! I modified this code. > > PTAL! Now it makes more sense. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.h (right): https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.h:47: enum InterfaceType { On 2016/05/09 11:37:31, peah-webrtc wrote: > On 2016/05/03 22:30:41, aluebs-webrtc wrote: > > This is defined in wav_based_simulator and here. Can't we have it only once in > > audio_processing_simulator? > > I don't really follow this comment. As far as I can see, this enum is only > defined here, and it is only used within aec_dump_based_simlator.h. Could you > please clarify? You are right. This is only defined here. My bad. https://codereview.webrtc.org/1907223003/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:189: const int kOneBykChunksPerSecond = float
On 2016/05/09 16:27:32, aluebs-webrtc wrote: > minyue, could you please have a look at this CL, in particular what you think > should be done with the command line flags. > > https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... > File webrtc/modules/audio_processing/test/wav_based_simulator.cc (right): > > https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/test/wav_based_simulator.cc:42: > last_specified_microphone_level_)); > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > On 2016/05/02 06:18:42, peah-webrtc wrote: > > > > On 2016/04/30 02:08:05, aluebs-webrtc wrote: > > > > > last_specified_microphone_level_ is never modified. Should it be a > > constant? > > > > > > > > That is a mistake. The idea is to query the last level from the AGC but > that > > > > code got lost. I readded it. PTAL. > > > > > > Maybe I am not familiar enough with this setting, but why do we need to > query > > > it, to set it again here? Doesn't it maintain its value internally? > > > > I'm not sure how it works in APM but this is how process_test.cc does it. > > > > And I think it makes sense. analog_stream_level() reports what level APM > thinks > > the analog level should be at, and set_analog_stream_level() reports what the > > analog level was actually set to. So by doing it like this we basically > simulate > > an analog microphone level that is set exactly to what APM wants. > > > > Sounds good. > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:29: if > (msg.output_data().data()[k] != frame.data_[k]) { > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > Did you verify that it is possible to achieve bitexactness with this tool > > using > > > the same configuration? Because maybe we need to introduce a tolerance here. > > > > It is/should definite be possible to achieve bitexactness for a recording done > > singlethreadedly which is processed on the very same machine. > > > > I'm not sure whether it would make sense to have a threshold for the > > bitexactness. I'd rather then have the maximum difference being reported as a > > value. > > > > Would it be fine to wait with adding a threshold value until we have used the > > tool more? > > > > An alternative would also be to remove the bitexactness testing functionality > > fully for now, and wait to introduce it until later. > > > > WDYT? > > As I said before it would have been better to first refactor and only then add > new fuctionality in different CLs. But since I already reviewed this let's leave > it as is. We can add the threshold or maximum difference reporting later, but > please add a TODO for it in this CL. > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:61: if > (msg.has_input_data()) { > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > Why does this decides the interface? Is it just a horrible naming? > > > > Yes, that is how it is currently in the protobuf file. Regardless that naming > I > > think for backward compatibility we should keep it as it is. > > Agreed. > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:68: // > ProcessStream could have changed this for the output frame. > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > It doesn't though, right? > > > > This is how it is currently done in process_test.cc, and the comment comes > from > > that file. > > > > With the current implementation of the resampling and deinterlieving this is > > very hard to ensure and verify by looking at the code. > > But as far as I can see, if the beamformer is activated, it will actually > > explicitly change the number of channels in capture_audio. And since > > capture_audio is then copied back into the AudioFrame I cannot see anything > that > > prevents the number of channels to change. > > > > I think this line has to be there as long as the beamformer could be > activated. > > Wdyt? > > > > The reason of moving away from process_test is to get rid of all the unnecessary > code, not copying it over. The beamformer changes the number of channels on the > AudioBuffer, but when copying back to the AudioFrame this gets upmixed. I think > that is quite simple to see from the code because the properties of the > AudioFrame are never modified. Another way of seeing it is that the output > number of channels is set equal to the input number of channels for the > AudioFrame interface. I find this line of code and the comment misleading. > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:87: for (int i > = 0; i < msg.input_channel_size(); ++i) { > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > size_t? > > > > No, msg.input_channel_size() is of type int. > > Acknowledged. > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:96: if > (msg.has_delay()) { > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > You probably still want to set_stream_delay_ms to 0 when the msg has no > delay, > > > right? Also, you can probably simplify this with conditional operators. > > > > No, I don't think so. If the recording has no stream delay stored I don't > think > > that should be masked by defaulting to a default value. If there is no stream > > delay stored I think that should best be reflected with that no call was made > to > > the set_stream_delay() method and we should then let APM handle that according > > to its implementation. > > Good point. > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:213: > printf("Config at frame: "); > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > Use cout instead of printf? You also probably want to add a new-line after > the > > > colon. > > > > Fixed the new-line. Not sure about using cout though. printf is used elsewhere > > in this file and it is nice to use the same printout functionality in the > file. > > > > Do you see any particular advantage of using cout here? > > I meant that we probably want to use cout everywhere. It is the C++ alternative > to the C printf. It is more type-safe and less error prone with it's static > inputs instead of the dynamic %s. It also makes the code simpler. Here are some > more reasons: https://isocpp.org/wiki/faq/input-output#iostream-vs-stdio > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:343: > settings_.use_ts ? *settings_.use_ts : msg.noise_robust_agc_enabled(); > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > What does the noise_robust_agc has to do with TS? > > > > Good find! I modified this code. > > > > PTAL! > > Now it makes more sense. > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > File webrtc/modules/audio_processing/test/aec_dump_based_simulator.h (right): > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/test/aec_dump_based_simulator.h:47: enum > InterfaceType { > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:41, aluebs-webrtc wrote: > > > This is defined in wav_based_simulator and here. Can't we have it only once > in > > > audio_processing_simulator? > > > > I don't really follow this comment. As far as I can see, this enum is only > > defined here, and it is only used within aec_dump_based_simlator.h. Could you > > please clarify? > > You are right. This is only defined here. My bad. > > https://codereview.webrtc.org/1907223003/diff/140001/webrtc/modules/audio_pro... > File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): > > https://codereview.webrtc.org/1907223003/diff/140001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:189: const int > kOneBykChunksPerSecond = > float One option to achieve tri-statedness for the command line flags seems could be to use the GetCommandLineOption method that returns a bool for whether the parameter was set. If that works, we should be able to skip all the --no_aec type of flags.
That actually sounds optimal to me.
https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/wav_based_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/wav_based_simulator.cc:42: last_specified_microphone_level_)); On 2016/05/09 16:27:31, aluebs-webrtc wrote: > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > On 2016/05/02 06:18:42, peah-webrtc wrote: > > > > On 2016/04/30 02:08:05, aluebs-webrtc wrote: > > > > > last_specified_microphone_level_ is never modified. Should it be a > > constant? > > > > > > > > That is a mistake. The idea is to query the last level from the AGC but > that > > > > code got lost. I readded it. PTAL. > > > > > > Maybe I am not familiar enough with this setting, but why do we need to > query > > > it, to set it again here? Doesn't it maintain its value internally? > > > > I'm not sure how it works in APM but this is how process_test.cc does it. > > > > And I think it makes sense. analog_stream_level() reports what level APM > thinks > > the analog level should be at, and set_analog_stream_level() reports what the > > analog level was actually set to. So by doing it like this we basically > simulate > > an analog microphone level that is set exactly to what APM wants. > > > > Sounds good. Acknowledged. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:29: if (msg.output_data().data()[k] != frame.data_[k]) { On 2016/05/09 16:27:32, aluebs-webrtc wrote: > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > Did you verify that it is possible to achieve bitexactness with this tool > > using > > > the same configuration? Because maybe we need to introduce a tolerance here. > > > > It is/should definite be possible to achieve bitexactness for a recording done > > singlethreadedly which is processed on the very same machine. > > > > I'm not sure whether it would make sense to have a threshold for the > > bitexactness. I'd rather then have the maximum difference being reported as a > > value. > > > > Would it be fine to wait with adding a threshold value until we have used the > > tool more? > > > > An alternative would also be to remove the bitexactness testing functionality > > fully for now, and wait to introduce it until later. > > > > WDYT? > > As I said before it would have been better to first refactor and only then add > new fuctionality in different CLs. But since I already reviewed this let's leave > it as is. We can add the threshold or maximum difference reporting later, but > please add a TODO for it in this CL. Done. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:61: if (msg.has_input_data()) { On 2016/05/09 16:27:32, aluebs-webrtc wrote: > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > Why does this decides the interface? Is it just a horrible naming? > > > > Yes, that is how it is currently in the protobuf file. Regardless that naming > I > > think for backward compatibility we should keep it as it is. > > Agreed. Acknowledged. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:68: // ProcessStream could have changed this for the output frame. On 2016/05/09 16:27:32, aluebs-webrtc wrote: > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > It doesn't though, right? > > > > This is how it is currently done in process_test.cc, and the comment comes > from > > that file. > > > > With the current implementation of the resampling and deinterlieving this is > > very hard to ensure and verify by looking at the code. > > But as far as I can see, if the beamformer is activated, it will actually > > explicitly change the number of channels in capture_audio. And since > > capture_audio is then copied back into the AudioFrame I cannot see anything > that > > prevents the number of channels to change. > > > > I think this line has to be there as long as the beamformer could be > activated. > > Wdyt? > > > > The reason of moving away from process_test is to get rid of all the unnecessary > code, not copying it over. The beamformer changes the number of channels on the > AudioBuffer, but when copying back to the AudioFrame this gets upmixed. I think > that is quite simple to see from the code because the properties of the > AudioFrame are never modified. Another way of seeing it is that the output > number of channels is set equal to the input number of channels for the > AudioFrame interface. I find this line of code and the comment misleading. You mean that implicitly this happens in the line capture_.capture_audio->InterleaveTo(frame, output_copy_needed()); right? That is quite an implicit thing, as it is not mentioned in the comment to the method and is actually only allowed to happen when the number of channels is one. Having looked at the implementation of the InterleaveTo method I see now that it behaves like that though. However, as that there is not anything in the APM API that prevents this from happening I think we should add an assert for this inside APM. The properties of the AudioFrame may never actually be modified in the code but there is nothing in the code that explicitly prevents that from happening, e.g, no const specifier, so you really need to know all the code to verify this. Regarding the setting of the input number of channels to the output number of channels for the AudioFrame interface that does not guarantee this either, as the number of channels is later changed when the beamformer is on (but in the end changed back in the interleaveTo call). Of course we should not keep unnecessary code from process_test and after having this pointed out I definitely agree this code is not necessary. Done. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:87: for (int i = 0; i < msg.input_channel_size(); ++i) { On 2016/05/09 16:27:32, aluebs-webrtc wrote: > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > size_t? > > > > No, msg.input_channel_size() is of type int. > > Acknowledged. Acknowledged. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:96: if (msg.has_delay()) { On 2016/05/09 16:27:32, aluebs-webrtc wrote: > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > You probably still want to set_stream_delay_ms to 0 when the msg has no > delay, > > > right? Also, you can probably simplify this with conditional operators. > > > > No, I don't think so. If the recording has no stream delay stored I don't > think > > that should be masked by defaulting to a default value. If there is no stream > > delay stored I think that should best be reflected with that no call was made > to > > the set_stream_delay() method and we should then let APM handle that according > > to its implementation. > > Good point. Acknowledged. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:213: printf("Config at frame: "); On 2016/05/09 16:27:32, aluebs-webrtc wrote: > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > Use cout instead of printf? You also probably want to add a new-line after > the > > > colon. > > > > Fixed the new-line. Not sure about using cout though. printf is used elsewhere > > in this file and it is nice to use the same printout functionality in the > file. > > > > Do you see any particular advantage of using cout here? > > I meant that we probably want to use cout everywhere. It is the C++ alternative > to the C printf. It is more type-safe and less error prone with it's static > inputs instead of the dynamic %s. It also makes the code simpler. Here are some > more reasons: https://isocpp.org/wiki/faq/input-output#iostream-vs-stdio I think the only reason that applies here is the type-safety, which is not really an issue in this case. The main drawback with streams of speed is neither applicable though. I'll change all printouts to use cout. Done. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:343: settings_.use_ts ? *settings_.use_ts : msg.noise_robust_agc_enabled(); On 2016/05/09 16:27:32, aluebs-webrtc wrote: > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > What does the noise_robust_agc has to do with TS? > > > > Good find! I modified this code. > > > > PTAL! > > Now it makes more sense. Acknowledged. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.h (right): https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.h:47: enum InterfaceType { On 2016/05/09 16:27:32, aluebs-webrtc wrote: > On 2016/05/09 11:37:31, peah-webrtc wrote: > > On 2016/05/03 22:30:41, aluebs-webrtc wrote: > > > This is defined in wav_based_simulator and here. Can't we have it only once > in > > > audio_processing_simulator? > > > > I don't really follow this comment. As far as I can see, this enum is only > > defined here, and it is only used within aec_dump_based_simlator.h. Could you > > please clarify? > > You are right. This is only defined here. My bad. Acknowledged. https://codereview.webrtc.org/1907223003/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:189: const int kOneBykChunksPerSecond = Good find! Done.
Regarding GetCommandLineOption I could not get that to work as I thought it should. And I have not found any examples of where it is used that can give me a hint of a proper usage of it. Therefore I left the command line options unchanged and the question for how to implement those remains.
If GetCommandLineOption is not an option I still prefer int flags instead of 2 boolean. But let's hear what minyue has to say about it. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:68: // ProcessStream could have changed this for the output frame. On 2016/05/11 12:19:27, peah-webrtc wrote: > On 2016/05/09 16:27:32, aluebs-webrtc wrote: > > On 2016/05/09 11:37:31, peah-webrtc wrote: > > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > > It doesn't though, right? > > > > > > This is how it is currently done in process_test.cc, and the comment comes > > from > > > that file. > > > > > > With the current implementation of the resampling and deinterlieving this is > > > very hard to ensure and verify by looking at the code. > > > But as far as I can see, if the beamformer is activated, it will actually > > > explicitly change the number of channels in capture_audio. And since > > > capture_audio is then copied back into the AudioFrame I cannot see anything > > that > > > prevents the number of channels to change. > > > > > > I think this line has to be there as long as the beamformer could be > > activated. > > > Wdyt? > > > > > > > The reason of moving away from process_test is to get rid of all the > unnecessary > > code, not copying it over. The beamformer changes the number of channels on > the > > AudioBuffer, but when copying back to the AudioFrame this gets upmixed. I > think > > that is quite simple to see from the code because the properties of the > > AudioFrame are never modified. Another way of seeing it is that the output > > number of channels is set equal to the input number of channels for the > > AudioFrame interface. I find this line of code and the comment misleading. > > > You mean that implicitly this happens in the line > capture_.capture_audio->InterleaveTo(frame, output_copy_needed()); > right? > That is quite an implicit thing, as it is not mentioned in the comment to the > method and is actually only allowed to happen when the number of channels is > one. Having looked at the implementation of the InterleaveTo method I see now > that it behaves like that though. > > However, as that there is not anything in the APM API that prevents this from > happening I think we should add an assert for this inside APM. > The properties of the AudioFrame may never actually be modified in the code but > there is nothing in the code that explicitly prevents that from happening, e.g, > no const specifier, so you really need to know all the code to verify this. > Regarding the setting of the input number of channels to the output number of > channels for the AudioFrame interface that does not guarantee this either, as > the number of channels is later changed when the beamformer is on (but in the > end changed back in the interleaveTo call). > > Of course we should not keep unnecessary code from process_test and after having > this pointed out I definitely agree this code is not necessary. > > > Done. > > It seemed you agree with removing this line, but the line is still there. The beamformer changes the current number of channels, but doesn't change the input and output number of channels, which is the important point. https://codereview.webrtc.org/1907223003/diff/160001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/1907223003/diff/160001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audioproc_float.cc:267: fprintf(stderr, "%s", message.c_str()); You can use cerr here.
Thanks again for the comments. https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:68: // ProcessStream could have changed this for the output frame. On 2016/05/11 15:41:55, aluebs-webrtc wrote: > On 2016/05/11 12:19:27, peah-webrtc wrote: > > On 2016/05/09 16:27:32, aluebs-webrtc wrote: > > > On 2016/05/09 11:37:31, peah-webrtc wrote: > > > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > > > It doesn't though, right? > > > > > > > > This is how it is currently done in process_test.cc, and the comment comes > > > from > > > > that file. > > > > > > > > With the current implementation of the resampling and deinterlieving this > is > > > > very hard to ensure and verify by looking at the code. > > > > But as far as I can see, if the beamformer is activated, it will actually > > > > explicitly change the number of channels in capture_audio. And since > > > > capture_audio is then copied back into the AudioFrame I cannot see > anything > > > that > > > > prevents the number of channels to change. > > > > > > > > I think this line has to be there as long as the beamformer could be > > > activated. > > > > Wdyt? > > > > > > > > > > The reason of moving away from process_test is to get rid of all the > > unnecessary > > > code, not copying it over. The beamformer changes the number of channels on > > the > > > AudioBuffer, but when copying back to the AudioFrame this gets upmixed. I > > think > > > that is quite simple to see from the code because the properties of the > > > AudioFrame are never modified. Another way of seeing it is that the output > > > number of channels is set equal to the input number of channels for the > > > AudioFrame interface. I find this line of code and the comment misleading. > > > > > > You mean that implicitly this happens in the line > > capture_.capture_audio->InterleaveTo(frame, output_copy_needed()); > > right? > > That is quite an implicit thing, as it is not mentioned in the comment to the > > method and is actually only allowed to happen when the number of channels is > > one. Having looked at the implementation of the InterleaveTo method I see now > > that it behaves like that though. > > > > However, as that there is not anything in the APM API that prevents this from > > happening I think we should add an assert for this inside APM. > > The properties of the AudioFrame may never actually be modified in the code > but > > there is nothing in the code that explicitly prevents that from happening, > e.g, > > no const specifier, so you really need to know all the code to verify this. > > Regarding the setting of the input number of channels to the output number of > > channels for the AudioFrame interface that does not guarantee this either, as > > the number of channels is later changed when the beamformer is on (but in the > > end changed back in the interleaveTo call). > > > > Of course we should not keep unnecessary code from process_test and after > having > > this pointed out I definitely agree this code is not necessary. > > > > > > Done. > > > > > > It seemed you agree with removing this line, but the line is still there. > The beamformer changes the current number of channels, but doesn't change the > input and output number of channels, which is the important point. Sorry, my mistake! Thanks for catching that! Done. https://codereview.webrtc.org/1907223003/diff/160001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/1907223003/diff/160001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audioproc_float.cc:267: fprintf(stderr, "%s", message.c_str()); On 2016/05/11 15:41:55, aluebs-webrtc wrote: > You can use cerr here. Thanks! Done.
On 2016/05/18 12:18:50, peah-webrtc wrote: > Thanks again for the comments. > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:68: // > ProcessStream could have changed this for the output frame. > On 2016/05/11 15:41:55, aluebs-webrtc wrote: > > On 2016/05/11 12:19:27, peah-webrtc wrote: > > > On 2016/05/09 16:27:32, aluebs-webrtc wrote: > > > > On 2016/05/09 11:37:31, peah-webrtc wrote: > > > > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > > > > It doesn't though, right? > > > > > > > > > > This is how it is currently done in process_test.cc, and the comment > comes > > > > from > > > > > that file. > > > > > > > > > > With the current implementation of the resampling and deinterlieving > this > > is > > > > > very hard to ensure and verify by looking at the code. > > > > > But as far as I can see, if the beamformer is activated, it will > actually > > > > > explicitly change the number of channels in capture_audio. And since > > > > > capture_audio is then copied back into the AudioFrame I cannot see > > anything > > > > that > > > > > prevents the number of channels to change. > > > > > > > > > > I think this line has to be there as long as the beamformer could be > > > > activated. > > > > > Wdyt? > > > > > > > > > > > > > The reason of moving away from process_test is to get rid of all the > > > unnecessary > > > > code, not copying it over. The beamformer changes the number of channels > on > > > the > > > > AudioBuffer, but when copying back to the AudioFrame this gets upmixed. I > > > think > > > > that is quite simple to see from the code because the properties of the > > > > AudioFrame are never modified. Another way of seeing it is that the output > > > > number of channels is set equal to the input number of channels for the > > > > AudioFrame interface. I find this line of code and the comment misleading. > > > > > > > > > You mean that implicitly this happens in the line > > > capture_.capture_audio->InterleaveTo(frame, output_copy_needed()); > > > right? > > > That is quite an implicit thing, as it is not mentioned in the comment to > the > > > method and is actually only allowed to happen when the number of channels is > > > one. Having looked at the implementation of the InterleaveTo method I see > now > > > that it behaves like that though. > > > > > > However, as that there is not anything in the APM API that prevents this > from > > > happening I think we should add an assert for this inside APM. > > > The properties of the AudioFrame may never actually be modified in the code > > but > > > there is nothing in the code that explicitly prevents that from happening, > > e.g, > > > no const specifier, so you really need to know all the code to verify this. > > > Regarding the setting of the input number of channels to the output number > of > > > channels for the AudioFrame interface that does not guarantee this either, > as > > > the number of channels is later changed when the beamformer is on (but in > the > > > end changed back in the interleaveTo call). > > > > > > Of course we should not keep unnecessary code from process_test and after > > having > > > this pointed out I definitely agree this code is not necessary. > > > > > > > > > Done. > > > > > > > > > > It seemed you agree with removing this line, but the line is still there. > > The beamformer changes the current number of channels, but doesn't change the > > input and output number of channels, which is the important point. > > Sorry, my mistake! Thanks for catching that! > Done. > > https://codereview.webrtc.org/1907223003/diff/160001/webrtc/modules/audio_pro... > File webrtc/modules/audio_processing/test/audioproc_float.cc (right): > > https://codereview.webrtc.org/1907223003/diff/160001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/test/audioproc_float.cc:267: fprintf(stderr, > "%s", message.c_str()); > On 2016/05/11 15:41:55, aluebs-webrtc wrote: > > You can use cerr here. > > Thanks! > Done. I agree that it should be better to use int flags. Minyue: do you have any comments on that? Otherwise I will change the command line to use flags instead.
On 2016/05/18 12:20:03, peah-webrtc wrote: > On 2016/05/18 12:18:50, peah-webrtc wrote: > > Thanks again for the comments. > > > > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > > File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): > > > > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:68: // > > ProcessStream could have changed this for the output frame. > > On 2016/05/11 15:41:55, aluebs-webrtc wrote: > > > On 2016/05/11 12:19:27, peah-webrtc wrote: > > > > On 2016/05/09 16:27:32, aluebs-webrtc wrote: > > > > > On 2016/05/09 11:37:31, peah-webrtc wrote: > > > > > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > > > > > It doesn't though, right? > > > > > > > > > > > > This is how it is currently done in process_test.cc, and the comment > > comes > > > > > from > > > > > > that file. > > > > > > > > > > > > With the current implementation of the resampling and deinterlieving > > this > > > is > > > > > > very hard to ensure and verify by looking at the code. > > > > > > But as far as I can see, if the beamformer is activated, it will > > actually > > > > > > explicitly change the number of channels in capture_audio. And since > > > > > > capture_audio is then copied back into the AudioFrame I cannot see > > > anything > > > > > that > > > > > > prevents the number of channels to change. > > > > > > > > > > > > I think this line has to be there as long as the beamformer could be > > > > > activated. > > > > > > Wdyt? > > > > > > > > > > > > > > > > The reason of moving away from process_test is to get rid of all the > > > > unnecessary > > > > > code, not copying it over. The beamformer changes the number of channels > > on > > > > the > > > > > AudioBuffer, but when copying back to the AudioFrame this gets upmixed. > I > > > > think > > > > > that is quite simple to see from the code because the properties of the > > > > > AudioFrame are never modified. Another way of seeing it is that the > output > > > > > number of channels is set equal to the input number of channels for the > > > > > AudioFrame interface. I find this line of code and the comment > misleading. > > > > > > > > > > > > You mean that implicitly this happens in the line > > > > capture_.capture_audio->InterleaveTo(frame, output_copy_needed()); > > > > right? > > > > That is quite an implicit thing, as it is not mentioned in the comment to > > the > > > > method and is actually only allowed to happen when the number of channels > is > > > > one. Having looked at the implementation of the InterleaveTo method I see > > now > > > > that it behaves like that though. > > > > > > > > However, as that there is not anything in the APM API that prevents this > > from > > > > happening I think we should add an assert for this inside APM. > > > > The properties of the AudioFrame may never actually be modified in the > code > > > but > > > > there is nothing in the code that explicitly prevents that from happening, > > > e.g, > > > > no const specifier, so you really need to know all the code to verify > this. > > > > Regarding the setting of the input number of channels to the output number > > of > > > > channels for the AudioFrame interface that does not guarantee this either, > > as > > > > the number of channels is later changed when the beamformer is on (but in > > the > > > > end changed back in the interleaveTo call). > > > > > > > > Of course we should not keep unnecessary code from process_test and after > > > having > > > > this pointed out I definitely agree this code is not necessary. > > > > > > > > > > > > Done. > > > > > > > > > > > > > > It seemed you agree with removing this line, but the line is still there. > > > The beamformer changes the current number of channels, but doesn't change > the > > > input and output number of channels, which is the important point. > > > > Sorry, my mistake! Thanks for catching that! > > Done. > > > > > https://codereview.webrtc.org/1907223003/diff/160001/webrtc/modules/audio_pro... > > File webrtc/modules/audio_processing/test/audioproc_float.cc (right): > > > > > https://codereview.webrtc.org/1907223003/diff/160001/webrtc/modules/audio_pro... > > webrtc/modules/audio_processing/test/audioproc_float.cc:267: fprintf(stderr, > > "%s", message.c_str()); > > On 2016/05/11 15:41:55, aluebs-webrtc wrote: > > > You can use cerr here. > > > > Thanks! > > Done. > > I agree that it should be better to use int flags. Minyue: do you have any > comments on that? Otherwise I will change the command line to use flags instead. Sorry for a delayed answer. I just saw that you were waiting for my opinion. It is a pity that gflags do not have Unspecified. But yes, it would be better to use int instead of bool, like what you already did for int parameters. Then if at some point, gflags is ready to support Unspecified, the change to it will be smooth.
On 2016/05/23 05:07:43, minyue-webrtc wrote: > On 2016/05/18 12:20:03, peah-webrtc wrote: > > On 2016/05/18 12:18:50, peah-webrtc wrote: > > > Thanks again for the comments. > > > > > > > > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > > > File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc > (right): > > > > > > > > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > > > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:68: // > > > ProcessStream could have changed this for the output frame. > > > On 2016/05/11 15:41:55, aluebs-webrtc wrote: > > > > On 2016/05/11 12:19:27, peah-webrtc wrote: > > > > > On 2016/05/09 16:27:32, aluebs-webrtc wrote: > > > > > > On 2016/05/09 11:37:31, peah-webrtc wrote: > > > > > > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > > > > > > It doesn't though, right? > > > > > > > > > > > > > > This is how it is currently done in process_test.cc, and the comment > > > comes > > > > > > from > > > > > > > that file. > > > > > > > > > > > > > > With the current implementation of the resampling and deinterlieving > > > this > > > > is > > > > > > > very hard to ensure and verify by looking at the code. > > > > > > > But as far as I can see, if the beamformer is activated, it will > > > actually > > > > > > > explicitly change the number of channels in capture_audio. And since > > > > > > > capture_audio is then copied back into the AudioFrame I cannot see > > > > anything > > > > > > that > > > > > > > prevents the number of channels to change. > > > > > > > > > > > > > > I think this line has to be there as long as the beamformer could be > > > > > > activated. > > > > > > > Wdyt? > > > > > > > > > > > > > > > > > > > The reason of moving away from process_test is to get rid of all the > > > > > unnecessary > > > > > > code, not copying it over. The beamformer changes the number of > channels > > > on > > > > > the > > > > > > AudioBuffer, but when copying back to the AudioFrame this gets > upmixed. > > I > > > > > think > > > > > > that is quite simple to see from the code because the properties of > the > > > > > > AudioFrame are never modified. Another way of seeing it is that the > > output > > > > > > number of channels is set equal to the input number of channels for > the > > > > > > AudioFrame interface. I find this line of code and the comment > > misleading. > > > > > > > > > > > > > > > You mean that implicitly this happens in the line > > > > > capture_.capture_audio->InterleaveTo(frame, output_copy_needed()); > > > > > right? > > > > > That is quite an implicit thing, as it is not mentioned in the comment > to > > > the > > > > > method and is actually only allowed to happen when the number of > channels > > is > > > > > one. Having looked at the implementation of the InterleaveTo method I > see > > > now > > > > > that it behaves like that though. > > > > > > > > > > However, as that there is not anything in the APM API that prevents this > > > from > > > > > happening I think we should add an assert for this inside APM. > > > > > The properties of the AudioFrame may never actually be modified in the > > code > > > > but > > > > > there is nothing in the code that explicitly prevents that from > happening, > > > > e.g, > > > > > no const specifier, so you really need to know all the code to verify > > this. > > > > > Regarding the setting of the input number of channels to the output > number > > > of > > > > > channels for the AudioFrame interface that does not guarantee this > either, > > > as > > > > > the number of channels is later changed when the beamformer is on (but > in > > > the > > > > > end changed back in the interleaveTo call). > > > > > > > > > > Of course we should not keep unnecessary code from process_test and > after > > > > having > > > > > this pointed out I definitely agree this code is not necessary. > > > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > It seemed you agree with removing this line, but the line is still there. > > > > The beamformer changes the current number of channels, but doesn't change > > the > > > > input and output number of channels, which is the important point. > > > > > > Sorry, my mistake! Thanks for catching that! > > > Done. > > > > > > > > > https://codereview.webrtc.org/1907223003/diff/160001/webrtc/modules/audio_pro... > > > File webrtc/modules/audio_processing/test/audioproc_float.cc (right): > > > > > > > > > https://codereview.webrtc.org/1907223003/diff/160001/webrtc/modules/audio_pro... > > > webrtc/modules/audio_processing/test/audioproc_float.cc:267: fprintf(stderr, > > > "%s", message.c_str()); > > > On 2016/05/11 15:41:55, aluebs-webrtc wrote: > > > > You can use cerr here. > > > > > > Thanks! > > > Done. > > > > I agree that it should be better to use int flags. Minyue: do you have any > > comments on that? Otherwise I will change the command line to use flags > instead. > > Sorry for a delayed answer. I just saw that you were waiting for my opinion. It > is a pity that gflags do not have Unspecified. But yes, it would be better to > use int instead of bool, like what you already did for int parameters. Then if > at some point, gflags is ready to support Unspecified, the change to it will be > smooth. BTW, could you do a rebasing? I tried to download the patch but had conflicts.
On 2016/05/23 05:07:43, minyue-webrtc wrote: > On 2016/05/18 12:20:03, peah-webrtc wrote: > > On 2016/05/18 12:18:50, peah-webrtc wrote: > > > Thanks again for the comments. > > > > > > > > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > > > File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc > (right): > > > > > > > > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > > > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:68: // > > > ProcessStream could have changed this for the output frame. > > > On 2016/05/11 15:41:55, aluebs-webrtc wrote: > > > > On 2016/05/11 12:19:27, peah-webrtc wrote: > > > > > On 2016/05/09 16:27:32, aluebs-webrtc wrote: > > > > > > On 2016/05/09 11:37:31, peah-webrtc wrote: > > > > > > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > > > > > > It doesn't though, right? > > > > > > > > > > > > > > This is how it is currently done in process_test.cc, and the comment > > > comes > > > > > > from > > > > > > > that file. > > > > > > > > > > > > > > With the current implementation of the resampling and deinterlieving > > > this > > > > is > > > > > > > very hard to ensure and verify by looking at the code. > > > > > > > But as far as I can see, if the beamformer is activated, it will > > > actually > > > > > > > explicitly change the number of channels in capture_audio. And since > > > > > > > capture_audio is then copied back into the AudioFrame I cannot see > > > > anything > > > > > > that > > > > > > > prevents the number of channels to change. > > > > > > > > > > > > > > I think this line has to be there as long as the beamformer could be > > > > > > activated. > > > > > > > Wdyt? > > > > > > > > > > > > > > > > > > > The reason of moving away from process_test is to get rid of all the > > > > > unnecessary > > > > > > code, not copying it over. The beamformer changes the number of > channels > > > on > > > > > the > > > > > > AudioBuffer, but when copying back to the AudioFrame this gets > upmixed. > > I > > > > > think > > > > > > that is quite simple to see from the code because the properties of > the > > > > > > AudioFrame are never modified. Another way of seeing it is that the > > output > > > > > > number of channels is set equal to the input number of channels for > the > > > > > > AudioFrame interface. I find this line of code and the comment > > misleading. > > > > > > > > > > > > > > > You mean that implicitly this happens in the line > > > > > capture_.capture_audio->InterleaveTo(frame, output_copy_needed()); > > > > > right? > > > > > That is quite an implicit thing, as it is not mentioned in the comment > to > > > the > > > > > method and is actually only allowed to happen when the number of > channels > > is > > > > > one. Having looked at the implementation of the InterleaveTo method I > see > > > now > > > > > that it behaves like that though. > > > > > > > > > > However, as that there is not anything in the APM API that prevents this > > > from > > > > > happening I think we should add an assert for this inside APM. > > > > > The properties of the AudioFrame may never actually be modified in the > > code > > > > but > > > > > there is nothing in the code that explicitly prevents that from > happening, > > > > e.g, > > > > > no const specifier, so you really need to know all the code to verify > > this. > > > > > Regarding the setting of the input number of channels to the output > number > > > of > > > > > channels for the AudioFrame interface that does not guarantee this > either, > > > as > > > > > the number of channels is later changed when the beamformer is on (but > in > > > the > > > > > end changed back in the interleaveTo call). > > > > > > > > > > Of course we should not keep unnecessary code from process_test and > after > > > > having > > > > > this pointed out I definitely agree this code is not necessary. > > > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > It seemed you agree with removing this line, but the line is still there. > > > > The beamformer changes the current number of channels, but doesn't change > > the > > > > input and output number of channels, which is the important point. > > > > > > Sorry, my mistake! Thanks for catching that! > > > Done. > > > > > > > > > https://codereview.webrtc.org/1907223003/diff/160001/webrtc/modules/audio_pro... > > > File webrtc/modules/audio_processing/test/audioproc_float.cc (right): > > > > > > > > > https://codereview.webrtc.org/1907223003/diff/160001/webrtc/modules/audio_pro... > > > webrtc/modules/audio_processing/test/audioproc_float.cc:267: fprintf(stderr, > > > "%s", message.c_str()); > > > On 2016/05/11 15:41:55, aluebs-webrtc wrote: > > > > You can use cerr here. > > > > > > Thanks! > > > Done. > > > > I agree that it should be better to use int flags. Minyue: do you have any > > comments on that? Otherwise I will change the command line to use flags > instead. > > Sorry for a delayed answer. I just saw that you were waiting for my opinion. It > is a pity that gflags do not have Unspecified. But yes, it would be better to > use int instead of bool, like what you already did for int parameters. Then if > at some point, gflags is ready to support Unspecified, the change to it will be > smooth. Thanks! That makes sense! I have now changed the command line options to work like that.
On 2016/05/23 05:08:39, minyue-webrtc wrote: > On 2016/05/23 05:07:43, minyue-webrtc wrote: > > On 2016/05/18 12:20:03, peah-webrtc wrote: > > > On 2016/05/18 12:18:50, peah-webrtc wrote: > > > > Thanks again for the comments. > > > > > > > > > > > > > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > > > > File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc > > (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/1907223003/diff/120001/webrtc/modules/audio_pro... > > > > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:68: // > > > > ProcessStream could have changed this for the output frame. > > > > On 2016/05/11 15:41:55, aluebs-webrtc wrote: > > > > > On 2016/05/11 12:19:27, peah-webrtc wrote: > > > > > > On 2016/05/09 16:27:32, aluebs-webrtc wrote: > > > > > > > On 2016/05/09 11:37:31, peah-webrtc wrote: > > > > > > > > On 2016/05/03 22:30:40, aluebs-webrtc wrote: > > > > > > > > > It doesn't though, right? > > > > > > > > > > > > > > > > This is how it is currently done in process_test.cc, and the > comment > > > > comes > > > > > > > from > > > > > > > > that file. > > > > > > > > > > > > > > > > With the current implementation of the resampling and > deinterlieving > > > > this > > > > > is > > > > > > > > very hard to ensure and verify by looking at the code. > > > > > > > > But as far as I can see, if the beamformer is activated, it will > > > > actually > > > > > > > > explicitly change the number of channels in capture_audio. And > since > > > > > > > > capture_audio is then copied back into the AudioFrame I cannot see > > > > > anything > > > > > > > that > > > > > > > > prevents the number of channels to change. > > > > > > > > > > > > > > > > I think this line has to be there as long as the beamformer could > be > > > > > > > activated. > > > > > > > > Wdyt? > > > > > > > > > > > > > > > > > > > > > > The reason of moving away from process_test is to get rid of all the > > > > > > unnecessary > > > > > > > code, not copying it over. The beamformer changes the number of > > channels > > > > on > > > > > > the > > > > > > > AudioBuffer, but when copying back to the AudioFrame this gets > > upmixed. > > > I > > > > > > think > > > > > > > that is quite simple to see from the code because the properties of > > the > > > > > > > AudioFrame are never modified. Another way of seeing it is that the > > > output > > > > > > > number of channels is set equal to the input number of channels for > > the > > > > > > > AudioFrame interface. I find this line of code and the comment > > > misleading. > > > > > > > > > > > > > > > > > > You mean that implicitly this happens in the line > > > > > > capture_.capture_audio->InterleaveTo(frame, output_copy_needed()); > > > > > > right? > > > > > > That is quite an implicit thing, as it is not mentioned in the comment > > to > > > > the > > > > > > method and is actually only allowed to happen when the number of > > channels > > > is > > > > > > one. Having looked at the implementation of the InterleaveTo method I > > see > > > > now > > > > > > that it behaves like that though. > > > > > > > > > > > > However, as that there is not anything in the APM API that prevents > this > > > > from > > > > > > happening I think we should add an assert for this inside APM. > > > > > > The properties of the AudioFrame may never actually be modified in the > > > code > > > > > but > > > > > > there is nothing in the code that explicitly prevents that from > > happening, > > > > > e.g, > > > > > > no const specifier, so you really need to know all the code to verify > > > this. > > > > > > Regarding the setting of the input number of channels to the output > > number > > > > of > > > > > > channels for the AudioFrame interface that does not guarantee this > > either, > > > > as > > > > > > the number of channels is later changed when the beamformer is on (but > > in > > > > the > > > > > > end changed back in the interleaveTo call). > > > > > > > > > > > > Of course we should not keep unnecessary code from process_test and > > after > > > > > having > > > > > > this pointed out I definitely agree this code is not necessary. > > > > > > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > It seemed you agree with removing this line, but the line is still > there. > > > > > The beamformer changes the current number of channels, but doesn't > change > > > the > > > > > input and output number of channels, which is the important point. > > > > > > > > Sorry, my mistake! Thanks for catching that! > > > > Done. > > > > > > > > > > > > > > https://codereview.webrtc.org/1907223003/diff/160001/webrtc/modules/audio_pro... > > > > File webrtc/modules/audio_processing/test/audioproc_float.cc (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/1907223003/diff/160001/webrtc/modules/audio_pro... > > > > webrtc/modules/audio_processing/test/audioproc_float.cc:267: > fprintf(stderr, > > > > "%s", message.c_str()); > > > > On 2016/05/11 15:41:55, aluebs-webrtc wrote: > > > > > You can use cerr here. > > > > > > > > Thanks! > > > > Done. > > > > > > I agree that it should be better to use int flags. Minyue: do you have any > > > comments on that? Otherwise I will change the command line to use flags > > instead. > > > > Sorry for a delayed answer. I just saw that you were waiting for my opinion. > It > > is a pity that gflags do not have Unspecified. But yes, it would be better to > > use int instead of bool, like what you already did for int parameters. Then if > > at some point, gflags is ready to support Unspecified, the change to it will > be > > smooth. > > BTW, could you do a rebasing? I tried to download the patch but had conflicts. Absolutely. Done!
I have now changed the command line options and done a rebase with the latest master. Please take another look.
https://codereview.webrtc.org/1907223003/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/1907223003/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audioproc_float.cc:28: const char kUsageDescription[] = I found this not being correct.
https://codereview.webrtc.org/1907223003/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/1907223003/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audioproc_float.cc:28: const char kUsageDescription[] = On 2016/05/23 09:59:38, minyue-webrtc wrote: > I found this not being correct. Done.
lgtm % after addressing my comments https://codereview.webrtc.org/1907223003/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/1907223003/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audioproc_float.cc:115: "Activate (1) or deactivate(0) the level estimator"); These are not all the "level estimator". https://codereview.webrtc.org/1907223003/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audioproc_float.cc:250: settings.fixed_interface = FLAGS_fixed_interface == 1 ? true : false; Isn't this a bool?
On 2016/05/24 00:22:30, aluebs-webrtc wrote: > lgtm % after addressing my comments > > https://codereview.webrtc.org/1907223003/diff/240001/webrtc/modules/audio_pro... > File webrtc/modules/audio_processing/test/audioproc_float.cc (right): > > https://codereview.webrtc.org/1907223003/diff/240001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/test/audioproc_float.cc:115: "Activate (1) or > deactivate(0) the level estimator"); > These are not all the "level estimator". > > https://codereview.webrtc.org/1907223003/diff/240001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/test/audioproc_float.cc:250: > settings.fixed_interface = FLAGS_fixed_interface == 1 ? true : false; > Isn't this a bool? Great! Again, thanks for the the great comments! Will revise accordingly. Minyue, do you have any further comments?
On 2016/05/24 13:38:05, peah-webrtc wrote: > On 2016/05/24 00:22:30, aluebs-webrtc wrote: > > lgtm % after addressing my comments > > > > > https://codereview.webrtc.org/1907223003/diff/240001/webrtc/modules/audio_pro... > > File webrtc/modules/audio_processing/test/audioproc_float.cc (right): > > > > > https://codereview.webrtc.org/1907223003/diff/240001/webrtc/modules/audio_pro... > > webrtc/modules/audio_processing/test/audioproc_float.cc:115: "Activate (1) or > > deactivate(0) the level estimator"); > > These are not all the "level estimator". > > > > > https://codereview.webrtc.org/1907223003/diff/240001/webrtc/modules/audio_pro... > > webrtc/modules/audio_processing/test/audioproc_float.cc:250: > > settings.fixed_interface = FLAGS_fixed_interface == 1 ? true : false; > > Isn't this a bool? > > Great! Again, thanks for the the great comments! Will revise accordingly. > > Minyue, do you have any further comments? good to commit it now. and I have started using it. If I find anything during using it, we can fix it later. LGTM
https://codereview.webrtc.org/1907223003/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/1907223003/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audioproc_float.cc:115: "Activate (1) or deactivate(0) the level estimator"); On 2016/05/24 00:22:30, aluebs-webrtc wrote: > These are not all the "level estimator". Ah, good catch!!! Done. https://codereview.webrtc.org/1907223003/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audioproc_float.cc:250: settings.fixed_interface = FLAGS_fixed_interface == 1 ? true : false; On 2016/05/24 00:22:30, aluebs-webrtc wrote: > Isn't this a bool? It most certainly is! Great catch!!! Done.
On 2016/05/24 21:22:57, peah-webrtc wrote: > https://codereview.webrtc.org/1907223003/diff/240001/webrtc/modules/audio_pro... > File webrtc/modules/audio_processing/test/audioproc_float.cc (right): > > https://codereview.webrtc.org/1907223003/diff/240001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/test/audioproc_float.cc:115: "Activate (1) or > deactivate(0) the level estimator"); > On 2016/05/24 00:22:30, aluebs-webrtc wrote: > > These are not all the "level estimator". > > Ah, good catch!!! > > Done. > > https://codereview.webrtc.org/1907223003/diff/240001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/test/audioproc_float.cc:250: > settings.fixed_interface = FLAGS_fixed_interface == 1 ? true : false; > On 2016/05/24 00:22:30, aluebs-webrtc wrote: > > Isn't this a bool? > > It most certainly is! Great catch!!! > > Done. Great! Thanks again for reviewing this huge CL. The final code is much better than the initial version thanks to your feedback!
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from aluebs@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/1907223003/#ps280001 (title: "Rebase with latest master")
The CQ bit was unchecked by peah@webrtc.org
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1907223003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1907223003/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1907223003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1907223003/280001
Message was sent while issue was closed.
Description was changed from ========== This CL updates and extends the audioproc_f command line tool to support all the functionality needed for simulating and analyzing the audio processing module behavior during calls. BUG= ========== to ========== This CL updates and extends the audioproc_f command line tool to support all the functionality needed for simulating and analyzing the audio processing module behavior during calls. BUG= ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== This CL updates and extends the audioproc_f command line tool to support all the functionality needed for simulating and analyzing the audio processing module behavior during calls. BUG= ========== to ========== This CL updates and extends the audioproc_f command line tool to support all the functionality needed for simulating and analyzing the audio processing module behavior during calls. BUG= Committed: https://crrev.com/60a189f1bfa5f822cb9e384b1fea514a66fa3a68 Cr-Commit-Position: refs/heads/master@{#12882} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/60a189f1bfa5f822cb9e384b1fea514a66fa3a68 Cr-Commit-Position: refs/heads/master@{#12882} |
