Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1348903004:
Adding APM configuration in AEC dump. (Closed)
|
Created:
5 years, 3 months ago by minyue-webrtc Modified:
5 years, 2 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, kwiberg-webrtc, aluebs-webrtc, bjornv1, mgraczyk Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding APM configuration in AEC dump.
The AEC dump was not self-contented enough in the sense that APM configuration is missing, and therefore, given an AEC dump, it is sometimes not clear how to reproduce problems.
This CL tries to address the problem.
Note that this cannot guarantee a perfect reproduction in all cases. Dumping from the middle of a call makes the initial states unknown and thus may make the result non-reproducible.
BUG=
TEST= 1. new dump in Chromium and unpack
2. unpack old dump
R=andrew@webrtc.org, peah@webrtc.org
Committed: https://crrev.com/13b96ba90f72164134019cbfc07d4a47cf1fd091
Cr-Commit-Position: refs/heads/master@{#10155}
Patch Set 1 #
Total comments: 22
Patch Set 2 : update #
Total comments: 2
Patch Set 3 : fixing an issue and let unittest pass #
Total comments: 31
Patch Set 4 : Remove ApmConfig and use audioproc::Config #
Total comments: 18
Patch Set 5 : removing an unnecessary test #
Total comments: 11
Patch Set 6 : removing macro and more #
Total comments: 12
Patch Set 7 : serialize Config #Patch Set 8 : unmerge a wrong change #
Total comments: 7
Patch Set 9 : renaming two fields #
Total comments: 14
Patch Set 10 : fixing some nits #
Total comments: 2
Patch Set 11 : last commit was hasty, having an error #Messages
Total messages: 78 (14 generated)
minyue@webrtc.org changed reviewers: + ivoc@webrtc.org, peah@webrtc.org
Hi Per and Ivo, Since you are APM and protobuf experts, I choose you to review this CL. It would be good to automate process_test for dump files, but I think we can do on a separate CL.
Nice, seems like a useful addition! Most of the changes look fine to me, beside my comment below. Are there any unit tests for the AEC dump code? If so, it might be good to add some tests for these new fields. https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:470: int err = WriteInitMessage(); Can you explain why this call needs to be added here? Won't it lead to multiple init messages being written?
On 2015/09/22 11:18:54, ivoc wrote: > Nice, seems like a useful addition! Most of the changes look fine to me, beside > my comment below. > Are there any unit tests for the AEC dump code? If so, it might be good to add > some tests for these new fields. I could not find any existing test. But why not adding one? I will try to do it. Nevertheless, it may be better to do it in a follow up CL (since it may be big) so that we can land this CL some time before Chrome M47 cut, and let it bake in Canary channels for some time. > > https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/audio_processing_impl.cc:470: int err = > WriteInitMessage(); > Can you explain why this call needs to be added here? Won't it lead to multiple > init messages being written?
and some more https://codereview.chromium.org/1348903004/diff/1/webrtc/modules/audio_proces... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.chromium.org/1348903004/diff/1/webrtc/modules/audio_proces... webrtc/modules/audio_processing/audio_processing_impl.cc:470: int err = WriteInitMessage(); On 2015/09/22 11:18:54, ivoc wrote: > Can you explain why this call needs to be added here? Won't it lead to multiple > init messages being written? Yes, it may lead to multiple init messages, but won't miss any re-config. BTW, even with current code, it gives a new init message when input/output format (sample rate/channel) changes. Since SetExtraOptions should only happen at beginning of a call, there should be no harm to add an init here.
https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:470: int err = WriteInitMessage(); On 2015/09/22 11:39:28, minyue-webrtc wrote: > On 2015/09/22 11:18:54, ivoc wrote: > > Can you explain why this call needs to be added here? Won't it lead to > multiple > > init messages being written? > > Yes, it may lead to multiple init messages, but won't miss any re-config. > > BTW, even with current code, it gives a new init message when input/output > format (sample rate/channel) changes. > > Since SetExtraOptions should only happen at beginning of a call, there should be > no harm to add an init here. I'm very new to the protobuf concept, what is the problem of having multiple init messages? I don't think we can rely on that SetExtraOptions happens only at the beginning of the call, or can we? https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:1253: msg->set_aec_delay_agnostic(echo_cancellation_->is_delay_logging_enabled()); The delay logging is set if the delay agnostic aec is active but it may also be set explicitly without that being active. Therefore this is not the correct indicator to tell whether the delay agnostic aec is active. Most likely we should add a member function for reporting that. https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.h:40: SuppressionLevel suppression_level() const override; Is it really needed to change the public/private statuses of the member functions? These are public in the EchoCancellation parent class and they are publicly accessed from elsewhere so I don't think this api change is needed. https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_control_mobile_impl.h (right): https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_control_mobile_impl.h:40: bool is_comfort_noise_enabled() const override; Is it really needed to change the public/private statuses of the member functions? These are public in the EchoControlMobile parent class and they are publicly accessed from elsewhere so I don't think this api change is needed. https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/gain_control_impl.h (right): https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/gain_control_impl.h:43: Mode mode() const override; Is it really needed to change the public/private statuses of the member functions? These are public in the GainControl parent class and they are publicly accessed from elsewhere so I don't think this api change is needed. https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/noise_suppression_impl.h (right): https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/noise_suppression_impl.h:36: Level level() const override; Is it really needed to change the public/private statuses of this member function? It is public in the NoiseSuppression parent class and it is publicly accessed from elsewhere so I don't think this api change is needed.
Thanks for the comments! Please see a new patch set and inline comments. https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:470: int err = WriteInitMessage(); On 2015/09/22 12:05:29, peah-webrtc wrote: > On 2015/09/22 11:39:28, minyue-webrtc wrote: > > On 2015/09/22 11:18:54, ivoc wrote: > > > Can you explain why this call needs to be added here? Won't it lead to > > multiple > > > init messages being written? > > > > Yes, it may lead to multiple init messages, but won't miss any re-config. > > > > BTW, even with current code, it gives a new init message when input/output > > format (sample rate/channel) changes. > > > > Since SetExtraOptions should only happen at beginning of a call, there should > be > > no harm to add an init here. > > I'm very new to the protobuf concept, what is the problem of having multiple > init messages? I do not know better, and so I included Ivo in the reviewers. Init message is defined here and not special to protobuf, you can call it anything. Messages are serialized. So if there is a SetExtraOptions invoked during a WebRTC call, it will stuff in a "init" message, and then behave as normal. There is problem with our unpack.cc is that it overwrite the audio files (input, ref_out, reverse.wav/float/pcm) when a new init message is found. So we now always see one set of audio files although there can be multiple init messages in settings.txt. This can be fixed in unpack.cc. > > I don't think we can rely on that SetExtraOptions happens only at the beginning > of the call, or can we? There is no guarantee that SetExtraOptions should not happen during a call, since APM is not fully RAII. But it is generally true. In addition, as said, adding init messages should be no harm. (probably better with a fix in unpack.cc) https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:1253: msg->set_aec_delay_agnostic(echo_cancellation_->is_delay_logging_enabled()); On 2015/09/22 12:05:29, peah-webrtc wrote: > The delay logging is set if the delay agnostic aec is active but it may also be > set explicitly without that being active. Therefore this is not the correct > indicator to tell whether the delay agnostic aec is active. Most likely we > should add a member function for reporting that. Sorry, it is my bad, I chose a wrong flag to report https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.h:40: SuppressionLevel suppression_level() const override; On 2015/09/22 12:05:29, peah-webrtc wrote: > Is it really needed to change the public/private statuses of the member > functions? These are public in the EchoCancellation parent class and they are > publicly accessed from elsewhere so I don't think this api change is needed. The problem is the instantiate of echo canceler in APM is an EchoCancellationImpl https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_control_mobile_impl.h (right): https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_control_mobile_impl.h:40: bool is_comfort_noise_enabled() const override; On 2015/09/22 12:05:29, peah-webrtc wrote: > Is it really needed to change the public/private statuses of the member > functions? These are public in the EchoControlMobile parent class and they are > publicly accessed from elsewhere so I don't think this api change is needed. Similar to suppression_level() in echo_cancellation_impl.h https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/gain_control_impl.h (right): https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/gain_control_impl.h:43: Mode mode() const override; On 2015/09/22 12:05:29, peah-webrtc wrote: > Is it really needed to change the public/private statuses of the member > functions? These are public in the GainControl parent class and they are > publicly accessed from elsewhere so I don't think this api change is needed. Similar to suppression_level() in echo_cancellation_impl.cc https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/noise_suppression_impl.h (right): https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/noise_suppression_impl.h:36: Level level() const override; On 2015/09/22 12:05:29, peah-webrtc wrote: > Is it really needed to change the public/private statuses of this member > function? It is public in the NoiseSuppression parent class and it is publicly > accessed from elsewhere so I don't think this api change is needed. similar to suppression_level() in echo_cancellation_impl.cc
andrew@webrtc.org changed reviewers: + andrew@webrtc.org
Drive-by. Thanks for looking at this Minyue. We've long considered it, but deferred due to the churn in configuration. A text log might be a more appropriate place to capture this information. If we do decide to store configuration in the protobuf, I think we want to wait anyway. We're currently in discussions about removing all these run-time setters and allowing configuration only at construction. Since protobuf fields are "forever", I think holding off is prudent. What do you think?
On 2015/09/23 01:43:22, Andrew MacDonald wrote: > Drive-by. > > Thanks for looking at this Minyue. We've long considered it, but deferred due to > the churn in configuration. A text log might be a more appropriate place to > capture this information. > > If we do decide to store configuration in the protobuf, I think we want to wait > anyway. We're currently in discussions about removing all these run-time setters > and allowing configuration only at construction. Since protobuf fields are > "forever", I think holding off is prudent. What do you think? If config churn is a concern and flexibility is important, we could also consider storing config data as a single repeated string field in the protobuf. That is similar to storing the config data as a text log, except that it's stored along with the rest of the AEC dump. By the way, since protobuf fields are "forever", aren't we stuck with the current init message anyway? Would it really be so bad to add a few extra fields to it if we have to keep it around anyway?
On 2015/09/23 01:43:22, Andrew MacDonald wrote: > Drive-by. > > Thanks for looking at this Minyue. We've long considered it, but deferred due to > the churn in configuration. A text log might be a more appropriate place to > capture this information. > > If we do decide to store configuration in the protobuf, I think we want to wait > anyway. We're currently in discussions about removing all these run-time setters > and allowing configuration only at construction. Since protobuf fields are > "forever", I think holding off is prudent. What do you think? The reason of the this CL is that we have been suffering from lack of information when debugging with AEC dumps. So factors to decide whether we wait can be 1. how soon the config-at-construction will happen? I think it is better not to be longer than 2 Chrome releases, given than AEC dump is becoming a main debugging tool. 2. are there any drawback of doing this? Only problem I can think of is compatibility. a) If we want to change to something other than protobuf, that means we do not care compatibility. b) If we continue to use the same protobuf, protobuf gives convenient ways to add and delete fields, particularly optional fields.
On 2015/09/23 07:58:45, ivoc wrote: > On 2015/09/23 01:43:22, Andrew MacDonald wrote: > > Drive-by. > > > > Thanks for looking at this Minyue. We've long considered it, but deferred due > to > > the churn in configuration. A text log might be a more appropriate place to > > capture this information. > > > > If we do decide to store configuration in the protobuf, I think we want to > wait > > anyway. We're currently in discussions about removing all these run-time > setters > > and allowing configuration only at construction. Since protobuf fields are > > "forever", I think holding off is prudent. What do you think? > > If config churn is a concern and flexibility is important, we could also > consider storing config data as a single repeated string field in the protobuf. > That is similar to storing the config data as a text log, except that it's > stored along with the rest of the AEC dump. Serializing the configuration to a string is a very interesting idea. We should consider that! > > By the way, since protobuf fields are "forever", aren't we stuck with the > current init message anyway? Would it really be so bad to add a few extra fields > to it if we have to keep it around anyway? There's nothing wrong with the current init message. We will probably continue to allow on-the-fly audio format changes (sample rate, channels etc), but not component configuration, which is what is added here.
On 2015/09/23 08:31:27, minyue wrote: > The reason of the this CL is that we have been suffering from lack of > information when debugging with AEC dumps. > So factors to decide whether we wait can be > 1. how soon the config-at-construction will happen? Depends on the voice engine refactoring work, but I would think certainly by M49. > I think it is better not to be longer than 2 Chrome releases, given than AEC > dump is becoming a main debugging tool. aecdumps have been _the_ main audio debugging tool since webrtc's inception. We've waited 4 years, I think we can wait a bit longer to get it right :) > 2. are there any drawback of doing this? > Only problem I can think of is compatibility. > a) If we want to change to something other than protobuf, that means we do > not care compatibility. > b) If we continue to use the same protobuf, protobuf gives convenient ways to > add and delete fields, particularly optional fields. It's true that we can remove these later, but keep in mind you won't be able to decode the configuration from old messages once that's done. What do you think? Is it worth delaying?
https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:470: int err = WriteInitMessage(); On 2015/09/22 13:19:29, minyue-webrtc wrote: > On 2015/09/22 12:05:29, peah-webrtc wrote: > > On 2015/09/22 11:39:28, minyue-webrtc wrote: > > > On 2015/09/22 11:18:54, ivoc wrote: > > > > Can you explain why this call needs to be added here? Won't it lead to > > > multiple > > > > init messages being written? > > > > > > Yes, it may lead to multiple init messages, but won't miss any re-config. > > > > > > BTW, even with current code, it gives a new init message when input/output > > > format (sample rate/channel) changes. > > > > > > Since SetExtraOptions should only happen at beginning of a call, there > should > > be > > > no harm to add an init here. > > > > I'm very new to the protobuf concept, what is the problem of having multiple > > init messages? > > I do not know better, and so I included Ivo in the reviewers. Init message is > defined here and not special to protobuf, you can call it anything. Messages are > serialized. So if there is a SetExtraOptions invoked during a WebRTC call, it > will stuff in a "init" message, and then behave as normal. > > There is problem with our unpack.cc is that it overwrite the audio files (input, > ref_out, reverse.wav/float/pcm) when a new init message is found. So we now > always see one set of audio files although there can be multiple init messages > in settings.txt. This can be fixed in unpack.cc. > > > > > I don't think we can rely on that SetExtraOptions happens only at the > beginning > > of the call, or can we? > There is no guarantee that SetExtraOptions should not happen during a call, > since APM is not fully RAII. But it is generally true. In addition, as said, > adding init messages should be no harm. (probably better with a fix in > unpack.cc) It definitely makes sense to have unpack.cc restart the writing of the audio after an init. And I think your extension for unpack to produce separate audio outputs is great. https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:470: int err = WriteInitMessage(); On 2015/09/22 13:19:29, minyue-webrtc wrote: > On 2015/09/22 12:05:29, peah-webrtc wrote: > > On 2015/09/22 11:39:28, minyue-webrtc wrote: > > > On 2015/09/22 11:18:54, ivoc wrote: > > > > Can you explain why this call needs to be added here? Won't it lead to > > > multiple > > > > init messages being written? > > > > > > Yes, it may lead to multiple init messages, but won't miss any re-config. > > > > > > BTW, even with current code, it gives a new init message when input/output > > > format (sample rate/channel) changes. > > > > > > Since SetExtraOptions should only happen at beginning of a call, there > should > > be > > > no harm to add an init here. > > > > I'm very new to the protobuf concept, what is the problem of having multiple > > init messages? > > I do not know better, and so I included Ivo in the reviewers. Init message is > defined here and not special to protobuf, you can call it anything. Messages are > serialized. So if there is a SetExtraOptions invoked during a WebRTC call, it > will stuff in a "init" message, and then behave as normal. > > There is problem with our unpack.cc is that it overwrite the audio files (input, > ref_out, reverse.wav/float/pcm) when a new init message is found. So we now > always see one set of audio files although there can be multiple init messages > in settings.txt. This can be fixed in unpack.cc. > > > > > I don't think we can rely on that SetExtraOptions happens only at the > beginning > > of the call, or can we? > There is no guarantee that SetExtraOptions should not happen during a call, > since APM is not fully RAII. But it is generally true. In addition, as said, > adding init messages should be no harm. (probably better with a fix in > unpack.cc) Acknowledged. https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.h:40: SuppressionLevel suppression_level() const override; On 2015/09/22 13:19:29, minyue-webrtc wrote: > On 2015/09/22 12:05:29, peah-webrtc wrote: > > Is it really needed to change the public/private statuses of the member > > functions? These are public in the EchoCancellation parent class and they are > > publicly accessed from elsewhere so I don't think this api change is needed. > > The problem is the instantiate of echo canceler in APM is an > EchoCancellationImpl After a discussion together we agreed that to comply with the ongoing locking refactoring the submodules apis needs to anyway change according to this so it is motivated to do the change you propose. And the mismatch in public/private could as well be fixed at the same time. https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.h:40: SuppressionLevel suppression_level() const override; On 2015/09/22 13:19:29, minyue-webrtc wrote: > On 2015/09/22 12:05:29, peah-webrtc wrote: > > Is it really needed to change the public/private statuses of the member > > functions? These are public in the EchoCancellation parent class and they are > > publicly accessed from elsewhere so I don't think this api change is needed. > > The problem is the instantiate of echo canceler in APM is an > EchoCancellationImpl Acknowledged. https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_control_mobile_impl.h (right): https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_control_mobile_impl.h:40: bool is_comfort_noise_enabled() const override; On 2015/09/22 13:19:29, minyue-webrtc wrote: > On 2015/09/22 12:05:29, peah-webrtc wrote: > > Is it really needed to change the public/private statuses of the member > > functions? These are public in the EchoControlMobile parent class and they are > > publicly accessed from elsewhere so I don't think this api change is needed. > > Similar to suppression_level() in echo_cancellation_impl.h Acknowledged. https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/gain_control_impl.h (right): https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/gain_control_impl.h:43: Mode mode() const override; On 2015/09/22 13:19:29, minyue-webrtc wrote: > On 2015/09/22 12:05:29, peah-webrtc wrote: > > Is it really needed to change the public/private statuses of the member > > functions? These are public in the GainControl parent class and they are > > publicly accessed from elsewhere so I don't think this api change is needed. > > Similar to suppression_level() in echo_cancellation_impl.cc Acknowledged. https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/noise_suppression_impl.h (right): https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/noise_suppression_impl.h:36: Level level() const override; On 2015/09/22 13:19:30, minyue-webrtc wrote: > On 2015/09/22 12:05:29, peah-webrtc wrote: > > Is it really needed to change the public/private statuses of this member > > function? It is public in the NoiseSuppression parent class and it is publicly > > accessed from elsewhere so I don't think this api change is needed. > > similar to suppression_level() in echo_cancellation_impl.cc Acknowledged.
On 2015/09/23 18:37:51, Andrew MacDonald wrote: > On 2015/09/23 08:31:27, minyue wrote: > > The reason of the this CL is that we have been suffering from lack of > > information when debugging with AEC dumps. > > So factors to decide whether we wait can be > > 1. how soon the config-at-construction will happen? > > Depends on the voice engine refactoring work, but I would think certainly by > M49. > > > I think it is better not to be longer than 2 Chrome releases, given than > AEC > > dump is becoming a main debugging tool. > > aecdumps have been _the_ main audio debugging tool since webrtc's inception. > We've waited 4 years, I think we can wait a bit longer to get it right :) > > > 2. are there any drawback of doing this? > > Only problem I can think of is compatibility. > > a) If we want to change to something other than protobuf, that means we do > > not care compatibility. > > b) If we continue to use the same protobuf, protobuf gives convenient ways > to > > add and delete fields, particularly optional fields. > > It's true that we can remove these later, but keep in mind you won't be able to > decode the configuration from old messages once that's done. > > What do you think? Is it worth delaying? I think we should move forward with this now. It is a small burden to keep these "forever". Sugest that we group them all in a sub-struct within the protobuf definition.
Patchset #2 (id:20001) has been deleted
The audio team discussed over this a bit today. We see some immediate advantages so that we so hope not to wait more than necessary. Also, backward compatibility in the long run should not be crucial as we anyway have a dependency on the rest of the audio stack, meaning that we for old recording anyway cannot fully replicate what was happening using the new audio stack. That is best solved with build using the old audio stack and that will also fix the protobuf support for the recording. Andrew, please comment whether this suits you. Also, to make these field more self-contained, Henrik suggested that we move them to a separate message block, which I have implemented. https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:1277: msg->set_ns_experiment(noise_suppression_->is_enabled()); This was not right. noise suppression experiment is used to determine transient suppressor, so here it should be whether transient suppressor is on or not.
some more comments on the new patch set. https://codereview.webrtc.org/1348903004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:560: if (current_config != config_) { I changed the way of detecting changes in the configurations, so no changes-on-the-fly will be missed. https://codereview.webrtc.org/1348903004/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/debug.proto:55: optional bool agc_experiment = 22; I found agc_experiment a crucial information, and added it. There can be other things that we may eventually want to add, like VAD, beamformer, intelligibility. I find that either they do not play a roll in signal processing (VAD, intelligibility) or are not much used (beamformer), and it may be ok for this moment to skip them. To maintain readability for future changes, I tried to re-assigne ids to these fields.
A few more remarks. https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1276: AudioProcessingImpl::ApmConfig AudioProcessingImpl::GetCurrentConfig() const { Wouldn't it be better to return an audioproc::Config from this function, so you don't have to convert it later in the WriteConfigMessage function? https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/debug.proto:4: Could you add a comment here to explain that the Init message can occur multiple times in the log? Would make it a bit clearer for the uninitiated. https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/debug.proto:50: optional bool aecm_enabled = 11; Shouldn't we continue with field number 6 here? Or are you reserving space for future fields? https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/debug.proto:71: CONFIG = 4; Shouldn't this be value 3? https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_unittest.cc (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_unittest.cc:1909: // remove(ref_filename.c_str()); Why is this commented out? I think it's needed, isn't it?
https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1276: AudioProcessingImpl::ApmConfig AudioProcessingImpl::GetCurrentConfig() const { On 2015/09/25 09:24:21, ivoc wrote: > Wouldn't it be better to return an audioproc::Config from this function, so you > don't have to convert it later in the WriteConfigMessage function? I think it is very valid. I also felt it cumbersome adding ApmConfig. https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/debug.proto:4: On 2015/09/25 09:24:21, ivoc wrote: > Could you add a comment here to explain that the Init message can occur multiple > times in the log? Would make it a bit clearer for the uninitiated. ok will do. https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/debug.proto:50: optional bool aecm_enabled = 11; On 2015/09/25 09:24:21, ivoc wrote: > Shouldn't we continue with field number 6 here? Or are you reserving space for > future fields? Yes, precisely. I feel that there can be info to add in future, and giving some gaps will help readability for future. Ivo, are there any disadvantages that I may not be aware of? https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/debug.proto:71: CONFIG = 4; On 2015/09/25 09:24:21, ivoc wrote: > Shouldn't this be value 3? Yes indeed https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_unittest.cc (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_unittest.cc:1909: // remove(ref_filename.c_str()); On 2015/09/25 09:24:21, ivoc wrote: > Why is this commented out? I think it's needed, isn't it? sorry sorry, I needed they to help debugging.
https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/debug.proto:50: optional bool aecm_enabled = 11; On 2015/09/25 09:49:09, minyue-webrtc wrote: > On 2015/09/25 09:24:21, ivoc wrote: > > Shouldn't we continue with field number 6 here? Or are you reserving space for > > future fields? > > Yes, precisely. I feel that there can be info to add in future, and giving some > gaps will help readability for future. Ivo, are there any disadvantages that I > may not be aware of? Well, fields 1 to 15 take a single byte to encode, while fields 16 to 2047 take 2 bytes to encode. However, I think this is not that important for this message, since there will not be that many Config messages in a typical log file.
https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:169: // AEC I think the style guide says that comments should be ended by a . http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Comm...
On 2015/09/24 12:57:14, hlundin-webrtc wrote: > I think we should move forward with this now. It is a small burden to keep these > "forever". Sugest that we group them all in a sub-struct within the protobuf > definition. Sounds good, and agreed about grouping them.
https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:166: struct ApmConfig { Why do you need this thing? https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/debug.proto:50: optional bool aecm_enabled = 11; On 2015/09/25 10:22:35, ivoc wrote: > > Yes, precisely. I feel that there can be info to add in future, and giving > some > > gaps will help readability for future. Ivo, are there any disadvantages that I > > may not be aware of? Personally I wouldn't bother with this. You can keep a "next field number" comment at the top, so people don't need to scan the list to find it. https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.h:40: SuppressionLevel suppression_level() const override; The first two implement EchoCancellation, right? Move them up to that block. Same thing in all the other ProcessingComponents. https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_unittest.cc (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_unittest.cc:1747: } else if (event_msg.type() == audioproc::Event::CONFIG) { Correct me if I'm wrong, but because the ref03.aecdump won't contain this message, this isn't testing anything, right? Instead, add a new focused test where you set a few of these settings, start debug recording, read the file and verify the config message contains what you expect. Too bad StartDebugRecording doesn't take some kind of abstract Writer class so you wouldn't have to involve the file-system. https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_unittest.cc:1813: apm_->SetExtraOptions(config); This API is deprecated. Don't add more calls to it. https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/unpack.cc (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/unpack.cc:231: if (msg.has_aec_enabled()) { Might be cooler to use a macro for this: #define PRINT_CONFIG(field_name) \ if (msg.has_##field_name()) { \ fprintf(settings_file, #field_name ": %d\n", msg.field_name()); \ } ... PRINT_CONFIG(aec_enabled) PRINT_CONFIG(aec_delay_agnostic) ... Looks like the full protobuf Message class can provide descriptors that would let us do this with complete agnosticism to the fields, but alas we only have MessageLite.
Hi Thanks for the review. Here is my update. https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:166: struct ApmConfig { On 2015/09/26 02:36:19, Andrew MacDonald wrote: > Why do you need this thing? Ivo had a good suggestion. It is now removed https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/debug.proto:50: optional bool aecm_enabled = 11; On 2015/09/26 02:36:19, Andrew MacDonald wrote: > On 2015/09/25 10:22:35, ivoc wrote: > > > Yes, precisely. I feel that there can be info to add in future, and giving > > some > > > gaps will help readability for future. Ivo, are there any disadvantages that > I > > > may not be aware of? > > Personally I wouldn't bother with this. You can keep a "next field number" > comment at the top, so people don't need to scan the list to find it. I think that is nice. https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_cancellation_impl.h (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_cancellation_impl.h:40: SuppressionLevel suppression_level() const override; On 2015/09/26 02:36:19, Andrew MacDonald wrote: > The first two implement EchoCancellation, right? Move them up to that block. > > Same thing in all the other ProcessingComponents. Acknowledged. https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_unittest.cc (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_unittest.cc:1747: } else if (event_msg.type() == audioproc::Event::CONFIG) { On 2015/09/26 02:36:20, Andrew MacDonald wrote: > Correct me if I'm wrong, but because the ref03.aecdump won't contain this > message, this isn't testing anything, right? > > Instead, add a new focused test where you set a few of these settings, start > debug recording, read the file and verify the config message contains what you > expect. > > Too bad StartDebugRecording doesn't take some kind of abstract Writer class so > you wouldn't have to involve the file-system. ref03.aecdump doesn't contain Config message. But the test is valid. The test does the following 1. takes ref03.aecdump and re-process it to ref_xxx, now it contains Config 2. use ref_xxx and re-process it to out_xxx, also contains Config 3. check if ref_xxx === out_xxx So it checks both backward compatibility and new fields https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_unittest.cc:1813: apm_->SetExtraOptions(config); On 2015/09/26 02:36:20, Andrew MacDonald wrote: > This API is deprecated. Don't add more calls to it. Ok, thanks for the info. But how should one set ns_experiment / da-aec / aec-extended for now? https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/unpack.cc (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/unpack.cc:231: if (msg.has_aec_enabled()) { On 2015/09/26 02:36:20, Andrew MacDonald wrote: > Might be cooler to use a macro for this: > > #define PRINT_CONFIG(field_name) \ > if (msg.has_##field_name()) { \ > fprintf(settings_file, #field_name ": %d\n", msg.field_name()); \ > } > > ... > > PRINT_CONFIG(aec_enabled) > PRINT_CONFIG(aec_delay_agnostic) > ... > > Looks like the full protobuf Message class can provide descriptors that would > let us do this with complete agnosticism to the fields, but alas we only have > MessageLite. yes I like a macro, much easier to read and maintain. https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:419: for (size_t i = 0; i < kNumNativeSampleRates; ++i) { oh, this must come from rebasing. Sorry, I worked on a different computer and could not separate this out. https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:426: if (echo_control_mobile_->is_enabled() && rebase https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:615: frame->sample_rate_hz_ > kMaxAECMSampleRateHz) { rebase
https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_unittest.cc (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_unittest.cc:1747: } else if (event_msg.type() == audioproc::Event::CONFIG) { On 2015/09/26 16:26:44, minyue-webrtc wrote: > On 2015/09/26 02:36:20, Andrew MacDonald wrote: > > Correct me if I'm wrong, but because the ref03.aecdump won't contain this > > message, this isn't testing anything, right? > > > > Instead, add a new focused test where you set a few of these settings, start > > debug recording, read the file and verify the config message contains what you > > expect. > > > > Too bad StartDebugRecording doesn't take some kind of abstract Writer class so > > you wouldn't have to involve the file-system. > > ref03.aecdump doesn't contain Config message. But the test is valid. The test > does the following > 1. takes ref03.aecdump and re-process it to ref_xxx, now it contains Config > 2. use ref_xxx and re-process it to out_xxx, also contains Config > 3. check if ref_xxx === out_xxx > > So it checks both backward compatibility and new fields OK, thanks. Still not totally convinced this is a great way to test this. All components are enabled in this test, and if one of those fails to get logged, this wouldn't catch it. Right? Also, we want to remove all these run-time setters anyway. So this feels like a step backwards. https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_unittest.cc:1813: apm_->SetExtraOptions(config); On 2015/09/26 16:26:44, minyue-webrtc wrote: > On 2015/09/26 02:36:20, Andrew MacDonald wrote: > > This API is deprecated. Don't add more calls to it. > > Ok, thanks for the info. But how should one set ns_experiment / da-aec / > aec-extended for now? Through Create().
https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_unittest.cc (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_unittest.cc:1747: } else if (event_msg.type() == audioproc::Event::CONFIG) { On 2015/09/28 06:22:33, Andrew MacDonald wrote: > On 2015/09/26 16:26:44, minyue-webrtc wrote: > > On 2015/09/26 02:36:20, Andrew MacDonald wrote: > > > Correct me if I'm wrong, but because the ref03.aecdump won't contain this > > > message, this isn't testing anything, right? > > > > > > Instead, add a new focused test where you set a few of these settings, start > > > debug recording, read the file and verify the config message contains what > you > > > expect. > > > > > > Too bad StartDebugRecording doesn't take some kind of abstract Writer class > so > > > you wouldn't have to involve the file-system. > > > > ref03.aecdump doesn't contain Config message. But the test is valid. The test > > does the following > > 1. takes ref03.aecdump and re-process it to ref_xxx, now it contains Config > > 2. use ref_xxx and re-process it to out_xxx, also contains Config > > 3. check if ref_xxx === out_xxx > > > > So it checks both backward compatibility and new fields > > OK, thanks. Still not totally convinced this is a great way to test this. All > components are enabled in this test, and if one of those fails to get logged, > this wouldn't catch it. Right? > > Also, we want to remove all these run-time setters anyway. So this feels like a > step backwards. True, it does not test all combination of configs,which is an over-kill also. I may do this try many combinations offline (just need to change line 1883) , and maybe leave a test or two in there. https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_unittest.cc:1813: apm_->SetExtraOptions(config); On 2015/09/28 06:22:33, Andrew MacDonald wrote: > On 2015/09/26 16:26:44, minyue-webrtc wrote: > > On 2015/09/26 02:36:20, Andrew MacDonald wrote: > > > This API is deprecated. Don't add more calls to it. > > > > Ok, thanks for the info. But how should one set ns_experiment / da-aec / > > aec-extended for now? > > Through Create(). Yes I see, do you suggest recreate APM here?
https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_unittest.cc (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_unittest.cc:1813: apm_->SetExtraOptions(config); On 2015/09/29 04:51:49, minyue-webrtc wrote: > On 2015/09/28 06:22:33, Andrew MacDonald wrote: > > Through Create(). > > Yes I see, do you suggest recreate APM here? No, I suggest not testing this feature here, as in the other comment.
https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_unittest.cc (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_unittest.cc:1813: apm_->SetExtraOptions(config); On 2015/09/29 05:05:17, Andrew MacDonald wrote: > On 2015/09/29 04:51:49, minyue-webrtc wrote: > > On 2015/09/28 06:22:33, Andrew MacDonald wrote: > > > Through Create(). > > > > Yes I see, do you suggest recreate APM here? > > No, I suggest not testing this feature here, as in the other comment. Ok. I'd remove these changes. I will try to add some tests later, preferably in a separate CL. I'd do tests (without submit) with these modifies offline anyway to verify the new fields.
https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_unittest.cc (right): https://codereview.webrtc.org/1348903004/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_unittest.cc:1813: apm_->SetExtraOptions(config); On 2015/09/29 21:05:05, minyue-webrtc wrote: > On 2015/09/29 05:05:17, Andrew MacDonald wrote: > > On 2015/09/29 04:51:49, minyue-webrtc wrote: > > > On 2015/09/28 06:22:33, Andrew MacDonald wrote: > > > > Through Create(). > > > > > > Yes I see, do you suggest recreate APM here? > > > > No, I suggest not testing this feature here, as in the other comment. > > Ok. I'd remove these changes. > > I will try to add some tests later, preferably in a separate CL. I'd do tests > (without submit) with these modifies offline anyway to verify the new fields. Why in a separate CL? If you're going to add a unit test (and I think you should) you should do it in this CL.
https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1286: UPDATE_CONFIG(aec_enabled, echo_cancellation_->is_enabled()); Sorry, missed to send this before, had a draft that was never sent. Sending it now instead. I'd prefer a function instead of a macro in this case. Using a macro instead of a function "only" saves 2 parameters. Furthermore, it optionally allows a more descriptive printout than rather just the field name. The style guide also also cautious about macros (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preprocessor_...) If you want to have a macro anyway, I'm fine with that as well though.
https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1286: UPDATE_CONFIG(aec_enabled, echo_cancellation_->is_enabled()); On 2015/09/30 13:13:54, peah-webrtc wrote: > Sorry, missed to send this before, had a draft that was never sent. > Sending it now instead. > I'd prefer a function instead of a macro in this case. Using a macro instead of > a function "only" saves 2 parameters. Furthermore, it optionally allows a more > descriptive printout than rather just the field name. The style guide also also > cautious about macros > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preprocessor_...) > > If you want to have a macro anyway, I'm fine with that as well though. I would like to use function but it does not seem to be as convenient. The config field name determines the function name (see the string concatenation ## in the macro). Is there a better way you may suggest me to maintain this convenience of macro?
https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:46: message Config { FYI: I just wanted to mention that Michael and I briefly discussed using a protobuf interface when moving to create-time configuration. We'd of course want to reuse this message in that case.
https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:79: required Type type = 1; I need to mention that adding CONFIG makes Type non-forward-compatible, i.e., old unpacker cannot unpack new dump. Ivo, you may suggest me if there is a work around. Adding those optional field in existing Event type, i.e., INIT, as I did at the beginning is of no problem. But is forward compatibility needed?
https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1286: UPDATE_CONFIG(aec_enabled, echo_cancellation_->is_enabled()); On 2015/09/30 18:44:52, minyue-webrtc wrote: > On 2015/09/30 13:13:54, peah-webrtc wrote: > > Sorry, missed to send this before, had a draft that was never sent. > > Sending it now instead. > > I'd prefer a function instead of a macro in this case. Using a macro instead > of > > a function "only" saves 2 parameters. Furthermore, it optionally allows a more > > descriptive printout than rather just the field name. The style guide also > also > > cautious about macros > > > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preprocessor_...) > > > > If you want to have a macro anyway, I'm fine with that as well though. > > I would like to use function but it does not seem to be as convenient. The > config field name determines the function name (see the string concatenation ## > in the macro). Is there a better way you may suggest me to maintain this > convenience of macro? I'd use a function instead with more parameters. The only benefit I see is that the macro implementation is the convenience of requiring 2 parameters instead of the 3 (or 4) that a function implementation would. Personally, I don't think that warrants the cost in readability and debuggability that a macro incurs.
https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (left): https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:45: #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP Why was this removed? https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:571: #endif Combine these #ifdef blocks. https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1282: bool AudioProcessingImpl::UpdateCurrentConfig() { I wonder if rather than checking every individual field, it would be cleaner to generate the message, serialize it to a string and check if that was different than the existing serialization. You might have to rework how the messages get written. https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1286: UPDATE_CONFIG(aec_enabled, echo_cancellation_->is_enabled()); On 2015/10/01 05:31:31, peah-webrtc wrote: > On 2015/09/30 18:44:52, minyue-webrtc wrote: > > On 2015/09/30 13:13:54, peah-webrtc wrote: > > > Sorry, missed to send this before, had a draft that was never sent. > > > Sending it now instead. > > > I'd prefer a function instead of a macro in this case. Using a macro instead > > of > > > a function "only" saves 2 parameters. Furthermore, it optionally allows a > more > > > descriptive printout than rather just the field name. The style guide also > > also > > > cautious about macros > > > > > > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preprocessor_...) > > > > > > If you want to have a macro anyway, I'm fine with that as well though. > > > > I would like to use function but it does not seem to be as convenient. The > > config field name determines the function name (see the string concatenation > ## > > in the macro). Is there a better way you may suggest me to maintain this > > convenience of macro? > > I'd use a function instead with more parameters. > > The only benefit I see is that the macro implementation is the convenience of > requiring 2 parameters instead of the 3 (or 4) that a function implementation > would. Personally, I don't think that warrants the cost in readability and > debuggability that a macro incurs. Agree with Per, I probably would not use a macro for this. https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/unpack.cc (right): https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/unpack.cc:237: PRINT_CONFIG(aec_enabled, "AEC enabled"); I wouldn't add the second string parameter here. The main point of the macro is that it can produce a string straight from the first parameter.
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1286: UPDATE_CONFIG(aec_enabled, echo_cancellation_->is_enabled()); On 2015/10/01 06:33:28, Andrew MacDonald wrote: > On 2015/10/01 05:31:31, peah-webrtc wrote: > > On 2015/09/30 18:44:52, minyue-webrtc wrote: > > > On 2015/09/30 13:13:54, peah-webrtc wrote: > > > > Sorry, missed to send this before, had a draft that was never sent. > > > > Sending it now instead. > > > > I'd prefer a function instead of a macro in this case. Using a macro > instead > > > of > > > > a function "only" saves 2 parameters. Furthermore, it optionally allows a > > more > > > > descriptive printout than rather just the field name. The style guide also > > > also > > > > cautious about macros > > > > > > > > > > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preprocessor_...) > > > > > > > > If you want to have a macro anyway, I'm fine with that as well though. > > > > > > I would like to use function but it does not seem to be as convenient. The > > > config field name determines the function name (see the string concatenation > > ## > > > in the macro). Is there a better way you may suggest me to maintain this > > > convenience of macro? > > > > I'd use a function instead with more parameters. > > > > The only benefit I see is that the macro implementation is the convenience of > > requiring 2 parameters instead of the 3 (or 4) that a function implementation > > would. Personally, I don't think that warrants the cost in readability and > > debuggability that a macro incurs. > > Agree with Per, I probably would not use a macro for this. A good compromise might be to implement almost all of it as a function with 3-4 arguments, and have a macro with 2 arguments whose only job is to magick up the 1-2 extra parameters and call the function.
https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:79: required Type type = 1; On 2015/10/01 05:10:21, minyue-webrtc wrote: > I need to mention that adding CONFIG makes Type non-forward-compatible, i.e., > old unpacker cannot unpack new dump. Ivo, you may suggest me if there is a work > around. > > Adding those optional field in existing Event type, i.e., INIT, as I did at the > beginning is of no problem. But is forward compatibility needed? First off, I'm not sure how important forward compatibility is, does it happen often that you use old versions of tools to analyze an AEC dump? What will happen during the parsing is that the old parser will find the unknown value (CONFIG - 3) and assign it to the default value instead (in this case INIT, the first one) instead. That can lead to problems if the parser expects an INIT Event to have a valid init member, which it won't have in this case. This is the reason that it's recommended to always have an UNKNOWN_EVENT = 0 entry as the first entry in a protobuf enum, so that you can detect unknown values that are added in the future. Since this enum is already made this way, it's hard to avoid the problem. It might be a good idea to add an UNKNOWN_EVENT = 4 field now, and assign it as the default value of the type (add [default = UNKNOWN_EVENT]), that way we can at least avoid this problem in the future when adding additional event types, by allowing the parser to ignore events it doesn't recognize.
On 2015/10/01 08:55:39, ivoc wrote: > https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... > File webrtc/modules/audio_processing/debug.proto (right): > > https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/debug.proto:79: required Type type = 1; > On 2015/10/01 05:10:21, minyue-webrtc wrote: > > I need to mention that adding CONFIG makes Type non-forward-compatible, i.e., > > old unpacker cannot unpack new dump. Ivo, you may suggest me if there is a > work > > around. > > > > Adding those optional field in existing Event type, i.e., INIT, as I did at > the > > beginning is of no problem. But is forward compatibility needed? > > First off, I'm not sure how important forward compatibility is, does it happen > often that you use old versions of tools to analyze an AEC dump? > > What will happen during the parsing is that the old parser will find the unknown > value (CONFIG - 3) and assign it to the default value instead (in this case > INIT, the first one) instead. That can lead to problems if the parser expects an > INIT Event to have a valid init member, which it won't have in this case. > > This is the reason that it's recommended to always have an UNKNOWN_EVENT = 0 > entry as the first entry in a protobuf enum, so that you can detect unknown > values that are added in the future. Since this enum is already made this way, > it's hard to avoid the problem. > > It might be a good idea to add an UNKNOWN_EVENT = 4 field now, and assign it as > the default value of the type (add [default = UNKNOWN_EVENT]), that way we can > at least avoid this problem in the future when adding additional event types, by > allowing the parser to ignore events it doesn't recognize. Yes, nice idea. but given that the M47 cut is at present, I'd add it after the cut with good test, to be on the safe side.
https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:79: required Type type = 1; On 2015/10/01 08:55:39, ivoc wrote: > On 2015/10/01 05:10:21, minyue-webrtc wrote: > > I need to mention that adding CONFIG makes Type non-forward-compatible, i.e., > > old unpacker cannot unpack new dump. Ivo, you may suggest me if there is a > work > > around. > > > > Adding those optional field in existing Event type, i.e., INIT, as I did at > the > > beginning is of no problem. But is forward compatibility needed? > > First off, I'm not sure how important forward compatibility is, does it happen > often that you use old versions of tools to analyze an AEC dump? > > What will happen during the parsing is that the old parser will find the unknown > value (CONFIG - 3) and assign it to the default value instead (in this case > INIT, the first one) instead. That can lead to problems if the parser expects an > INIT Event to have a valid init member, which it won't have in this case. > > This is the reason that it's recommended to always have an UNKNOWN_EVENT = 0 > entry as the first entry in a protobuf enum, so that you can detect unknown > values that are added in the future. Since this enum is already made this way, > it's hard to avoid the problem. > > It might be a good idea to add an UNKNOWN_EVENT = 4 field now, and assign it as > the default value of the type (add [default = UNKNOWN_EVENT]), that way we can > at least avoid this problem in the future when adding additional event types, by > allowing the parser to ignore events it doesn't recognize. Thanks for the explanation Ivo, interesting! Minyue, why not add this now? You still need to make other changes anyway ;)
Thanks all for the comments! A new patch set is formed, mainly removing macros. Other questions are also addresses. https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (left): https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:45: #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP On 2015/10/01 06:33:28, Andrew MacDonald wrote: > Why was this removed? This is moved to .h since audioproc::Config is needed there. https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:571: #endif On 2015/10/01 06:33:28, Andrew MacDonald wrote: > Combine these #ifdef blocks. Done. https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1282: bool AudioProcessingImpl::UpdateCurrentConfig() { On 2015/10/01 06:33:28, Andrew MacDonald wrote: > I wonder if rather than checking every individual field, it would be cleaner to > generate the message, serialize it to a string and check if that was different > than the existing serialization. You might have to rework how the messages get > written. I did a similar trial (not a string compare, but a struct compare) before patch set 4. But it feels that an additional struct is redundant, since audioproc::Config is a good container of all configs that need to be stored. The reason of not generating a temp audioproc::Config and compare it with the current audioproc::Config is 1) there is no "==" defined, we need to define it; 2) We omit memory usage of a temp audioproc::Config Or do you suggest dropping protobuf for Config at all? Then do we need to invent a new dump format? https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1286: UPDATE_CONFIG(aec_enabled, echo_cancellation_->is_enabled()); On 2015/10/01 08:26:22, kwiberg-webrtc wrote: > On 2015/10/01 06:33:28, Andrew MacDonald wrote: > > On 2015/10/01 05:31:31, peah-webrtc wrote: > > > On 2015/09/30 18:44:52, minyue-webrtc wrote: > > > > On 2015/09/30 13:13:54, peah-webrtc wrote: > > > > > Sorry, missed to send this before, had a draft that was never sent. > > > > > Sending it now instead. > > > > > I'd prefer a function instead of a macro in this case. Using a macro > > instead > > > > of > > > > > a function "only" saves 2 parameters. Furthermore, it optionally allows > a > > > more > > > > > descriptive printout than rather just the field name. The style guide > also > > > > also > > > > > cautious about macros > > > > > > > > > > > > > > > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preprocessor_...) > > > > > > > > > > If you want to have a macro anyway, I'm fine with that as well though. > > > > > > > > I would like to use function but it does not seem to be as convenient. The > > > > config field name determines the function name (see the string > concatenation > > > ## > > > > in the macro). Is there a better way you may suggest me to maintain this > > > > convenience of macro? > > > > > > I'd use a function instead with more parameters. > > > > > > The only benefit I see is that the macro implementation is the convenience > of > > > requiring 2 parameters instead of the 3 (or 4) that a function > implementation > > > would. Personally, I don't think that warrants the cost in readability and > > > debuggability that a macro incurs. > > > > Agree with Per, I probably would not use a macro for this. > > A good compromise might be to implement almost all of it as a function with 3-4 > arguments, and have a macro with 2 arguments whose only job is to magick up the > 1-2 extra parameters and call the function. It is rather complicated to use inline function, since it needs a callback to a non-static function, "set_xxx". Plus the fact that if we do not take the advantage of ## in macro, the benefit of inline function is quite minor, I suggest just do a plain coding. See the new patch. https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:46: message Config { On 2015/10/01 04:37:45, Andrew MacDonald wrote: > FYI: I just wanted to mention that Michael and I briefly discussed using a > protobuf interface when moving to create-time configuration. > > We'd of course want to reuse this message in that case. Acknowledged. https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:79: required Type type = 1; On 2015/10/01 17:35:21, Andrew MacDonald wrote: > On 2015/10/01 08:55:39, ivoc wrote: > > On 2015/10/01 05:10:21, minyue-webrtc wrote: > > > I need to mention that adding CONFIG makes Type non-forward-compatible, > i.e., > > > old unpacker cannot unpack new dump. Ivo, you may suggest me if there is a > > work > > > around. > > > > > > Adding those optional field in existing Event type, i.e., INIT, as I did at > > the > > > beginning is of no problem. But is forward compatibility needed? > > > > First off, I'm not sure how important forward compatibility is, does it happen > > often that you use old versions of tools to analyze an AEC dump? > > > > What will happen during the parsing is that the old parser will find the > unknown > > value (CONFIG - 3) and assign it to the default value instead (in this case > > INIT, the first one) instead. That can lead to problems if the parser expects > an > > INIT Event to have a valid init member, which it won't have in this case. > > > > This is the reason that it's recommended to always have an UNKNOWN_EVENT = 0 > > entry as the first entry in a protobuf enum, so that you can detect unknown > > values that are added in the future. Since this enum is already made this way, > > it's hard to avoid the problem. > > > > It might be a good idea to add an UNKNOWN_EVENT = 4 field now, and assign it > as > > the default value of the type (add [default = UNKNOWN_EVENT]), that way we can > > at least avoid this problem in the future when adding additional event types, > by > > allowing the parser to ignore events it doesn't recognize. > > Thanks for the explanation Ivo, interesting! > > Minyue, why not add this now? You still need to make other changes anyway ;) Yes, adding it in enum is very simple, my concern is that it cannot be properly tested in a short time. I added it. But Ivo, can you help to make sure that it works the way it is supposed to? https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/unpack.cc (right): https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/unpack.cc:237: PRINT_CONFIG(aec_enabled, "AEC enabled"); On 2015/10/01 06:33:28, Andrew MacDonald wrote: > I wouldn't add the second string parameter here. The main point of the macro is > that it can produce a string straight from the first parameter. Ok. I think it is better to use inline function then
https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/unpack.cc (right): https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/unpack.cc:237: PRINT_CONFIG(aec_enabled, "AEC enabled"); On 2015/10/01 20:18:26, minyue-webrtc wrote: > On 2015/10/01 06:33:28, Andrew MacDonald wrote: > > I wouldn't add the second string parameter here. The main point of the macro > is > > that it can produce a string straight from the first parameter. > > Ok. I think it is better to use inline function then Why not just have the macro "stringify" the single input parameter as I originally proposed?
https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1282: bool AudioProcessingImpl::UpdateCurrentConfig() { On 2015/10/01 20:18:26, minyue-webrtc wrote: > On 2015/10/01 06:33:28, Andrew MacDonald wrote: > > I wonder if rather than checking every individual field, it would be cleaner > to > > generate the message, serialize it to a string and check if that was different > > than the existing serialization. You might have to rework how the messages get > > written. > > I did a similar trial (not a string compare, but a struct compare) before patch > set 4. But it feels that an additional struct is redundant, since > audioproc::Config is a good container of all configs that need to be stored. > > The reason of not generating a temp audioproc::Config and compare it with the > current audioproc::Config is > 1) there is no "==" defined, we need to define it; > 2) We omit memory usage of a temp audioproc::Config I wouldn't worry about that, as it would only be used when recording aecdumps. > > Or do you suggest dropping protobuf for Config at all? Then do we need to invent > a new dump format? No. I meant serializing to a string and comparing the strings. Defining an operator== for audioproc::Config is an interesting option, but I think we'd have problems keeping it updated.
https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1282: bool AudioProcessingImpl::UpdateCurrentConfig() { On 2015/10/02 02:15:19, Andrew MacDonald wrote: > On 2015/10/01 20:18:26, minyue-webrtc wrote: > > On 2015/10/01 06:33:28, Andrew MacDonald wrote: > > > I wonder if rather than checking every individual field, it would be cleaner > > to > > > generate the message, serialize it to a string and check if that was > different > > > than the existing serialization. You might have to rework how the messages > get > > > written. > > > > I did a similar trial (not a string compare, but a struct compare) before > patch > > set 4. But it feels that an additional struct is redundant, since > > audioproc::Config is a good container of all configs that need to be stored. > > > > The reason of not generating a temp audioproc::Config and compare it with the > > current audioproc::Config is > > 1) there is no "==" defined, we need to define it; > > 2) We omit memory usage of a temp audioproc::Config > > I wouldn't worry about that, as it would only be used when recording aecdumps. > > > > > Or do you suggest dropping protobuf for Config at all? Then do we need to > invent > > a new dump format? > > No. I meant serializing to a string and comparing the strings. Defining an > operator== for audioproc::Config is an interesting option, but I think we'd have > problems keeping it updated. I still have some questions on this. Do you mean class AudioProcessingImpl { private: std::string config_; } std::string current_config = GetCurrentConfig(); // serialize all configs to a string. if (strcmp(current_config, config_) != 0) { WriteConfigMessage(); config_ = current_config; } If this, we may need to check through all configs twice, one in GetCurrentConfig and one in WriteConfigMessage. The cumbersomeness is similar to Patch set 3. Or do you even mean to write the string as a message, i.e., doing, in protobuf message Config { optional string all_configs; } Then what is benefit of protobuf, we need to write encode and decode. https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/unpack.cc (right): https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/unpack.cc:237: PRINT_CONFIG(aec_enabled, "AEC enabled"); On 2015/10/02 00:37:20, Andrew MacDonald wrote: > On 2015/10/01 20:18:26, minyue-webrtc wrote: > > On 2015/10/01 06:33:28, Andrew MacDonald wrote: > > > I wouldn't add the second string parameter here. The main point of the macro > > is > > > that it can produce a string straight from the first parameter. > > > > Ok. I think it is better to use inline function then > > Why not just have the macro "stringify" the single input parameter as I > originally proposed? Because I think the variable name is not sufficiently descriptive. In particular, to make the variables shorter, most bool variables does not have "enabled" in the names, but they are bool and should be self-clear. When printing them out, it may be better to make it clearer. Also the variable name string (lower case with underscores) does not look nice, and inconsistent with other lines in settings.txt.
https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1282: bool AudioProcessingImpl::UpdateCurrentConfig() { On 2015/10/02 05:29:01, minyue-webrtc wrote: > I still have some questions on this. Do you mean > > class AudioProcessingImpl { > private: > std::string config_; > } > > std::string current_config = GetCurrentConfig(); // serialize all configs to a > string. > if (strcmp(current_config, config_) != 0) { > WriteConfigMessage(); > config_ = current_config; > } > > If this, we may need to check through all configs twice, one in GetCurrentConfig > and one in WriteConfigMessage. The cumbersomeness is similar to Patch set 3. > > Or do you even mean to write the string as a message, i.e., doing, in protobuf > message Config { > optional string all_configs; > } > Then what is benefit of protobuf, we need to write encode and decode. To be clear, I mean this: https://codereview.webrtc.org/1384653002/ (based on your CL, and essentially the same as the first snippet you posted) Yes, it serializes Config twice, but since this is only enabled during debug recordings, I'm not that worried. https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/unpack.cc (right): https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/unpack.cc:237: PRINT_CONFIG(aec_enabled, "AEC enabled"); On 2015/10/02 05:29:01, minyue-webrtc wrote: > On 2015/10/02 00:37:20, Andrew MacDonald wrote: > > On 2015/10/01 20:18:26, minyue-webrtc wrote: > > > On 2015/10/01 06:33:28, Andrew MacDonald wrote: > > > > I wouldn't add the second string parameter here. The main point of the > macro > > > is > > > > that it can produce a string straight from the first parameter. > > > > > > Ok. I think it is better to use inline function then > > > > Why not just have the macro "stringify" the single input parameter as I > > originally proposed? > > Because I think the variable name is not sufficiently descriptive. In > particular, to make the variables shorter, most bool variables does not have > "enabled" in the names, but they are bool and should be self-clear. When > printing them out, it may be better to make it clearer. Also the variable name > string (lower case with underscores) does not look nice, and inconsistent with > other lines in settings.txt. I'm more concerned about the code being clear than this debug file. If your variable name is good enough for the protobuf field, I think it's good enough for the debug file.
https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1271: bool AudioProcessingImpl::UpdateCurrentConfig() { This function became very long (for good reasons) but I don't think the scope limiting and all the if-statements is as they should. I'd rather implement this as something like: bool AudioProcessingImpl::UpdateCurrentConfig() { const bool aec_drift_compensation = echo_cancellation_->is_drift_compensation_enabled(); const bool aec_extended_filter = echo_cancellation_->is_extended_filter_enabled(); const int aec_suppression_level = static_cast<int>(echo_cancellation_->suppression_level()); ... bool changed = false; changed |= aec_drift_compensation != config_->aec_drift_compensation(); changed |= aec_extended_filter != config_->aec_extended_filter(); changed |= aec_suppression_level != config_->aec_suppression_level(); ... if (changed) { config_->set_aec_drift_compensation(aec_drift_compensation); config_->set_aec_extended_filter(aec_extended_filter); config_->set_aec_suppression_level(aec_suppression_level); ... } return changed; } I think that is more readable and more compact (and hence less error prone) and also more efficient as the fewer if-statements makes it more pipeline-able and more efficient. It uses more stack space but that should not be a problem. https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:22: #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP Is this really the correct way to do this? Why not keep this in the .cc file as before and instead do a forward declaration, similarly to what is done for the Event class? https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audio_processing_unittest.cc (right): https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_processing_unittest.cc:1840: // dsadadasaadasdfewafscdeeds This comment should be updated to be more proper :-) https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/unpack.cc (right): https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/unpack.cc:43: inline void print_config(FILE* settings_file, Is this really a proper filename according to the style guideline? (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names) https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/unpack.cc:97: FILE* settings_file = OpenFile(FLAGS_settings_file, "wb"); This is something that holds for the version currently in master as well, but should we not close the file? I cannot find any corresponding file close?
Patchset #8 (id:160001) has been deleted
On 2015/10/02 06:07:39, Andrew MacDonald wrote: > https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > https://codereview.webrtc.org/1348903004/diff/80001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:1282: bool > AudioProcessingImpl::UpdateCurrentConfig() { > On 2015/10/02 05:29:01, minyue-webrtc wrote: > > I still have some questions on this. Do you mean > > > > class AudioProcessingImpl { > > private: > > std::string config_; > > } > > > > std::string current_config = GetCurrentConfig(); // serialize all configs to > a > > string. > > if (strcmp(current_config, config_) != 0) { > > WriteConfigMessage(); > > config_ = current_config; > > } > > > > If this, we may need to check through all configs twice, one in > GetCurrentConfig > > and one in WriteConfigMessage. The cumbersomeness is similar to Patch set 3. > > > > Or do you even mean to write the string as a message, i.e., doing, in protobuf > > message Config { > > optional string all_configs; > > } > > Then what is benefit of protobuf, we need to write encode and decode. > > To be clear, I mean this: > https://codereview.webrtc.org/1384653002/ > (based on your CL, and essentially the same as the first snippet you posted) This is very nice. I did not know "config.SerializeAsString()", so I thought that what you meant by string is string << aaa_enabled << bbb_enabled ... That would mean "cumbersomeness". Mine is still different from yours a bit, because we must force write config upon dump start. See patch set 7. > > Yes, it serializes Config twice, but since this is only enabled during debug > recordings, I'm not that worried. > > https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... > File webrtc/modules/audio_processing/test/unpack.cc (right): > > https://codereview.webrtc.org/1348903004/diff/100001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/test/unpack.cc:237: PRINT_CONFIG(aec_enabled, > "AEC enabled"); > On 2015/10/02 05:29:01, minyue-webrtc wrote: > > On 2015/10/02 00:37:20, Andrew MacDonald wrote: > > > On 2015/10/01 20:18:26, minyue-webrtc wrote: > > > > On 2015/10/01 06:33:28, Andrew MacDonald wrote: > > > > > I wouldn't add the second string parameter here. The main point of the > > macro > > > > is > > > > > that it can produce a string straight from the first parameter. > > > > > > > > Ok. I think it is better to use inline function then > > > > > > Why not just have the macro "stringify" the single input parameter as I > > > originally proposed? > > > > Because I think the variable name is not sufficiently descriptive. In > > particular, to make the variables shorter, most bool variables does not have > > "enabled" in the names, but they are bool and should be self-clear. When > > printing them out, it may be better to make it clearer. Also the variable name > > string (lower case with underscores) does not look nice, and inconsistent with > > other lines in settings.txt. > > I'm more concerned about the code being clear than this debug file. If your > variable name is good enough for the protobuf field, I think it's good enough > for the debug file. Ok. I changed it in Patch set 7. To make it even better, I renamed some protobuf fields.
Hi, Andrew's suggestion is formed into patch set 7. Patch set 6 had a wrong file (due to my local merge), which is removed in patch set 8. https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1271: bool AudioProcessingImpl::UpdateCurrentConfig() { On 2015/10/02 08:34:27, peah-webrtc wrote: > This function became very long (for good reasons) but I don't think the scope > limiting and all the if-statements is as they should. I'd rather implement this > as something like: > > bool AudioProcessingImpl::UpdateCurrentConfig() { > const bool aec_drift_compensation = > echo_cancellation_->is_drift_compensation_enabled(); > const bool aec_extended_filter = > echo_cancellation_->is_extended_filter_enabled(); > const int aec_suppression_level = > static_cast<int>(echo_cancellation_->suppression_level()); > ... > > bool changed = false; > changed |= aec_drift_compensation != config_->aec_drift_compensation(); > changed |= aec_extended_filter != config_->aec_extended_filter(); > changed |= aec_suppression_level != config_->aec_suppression_level(); > ... > > > if (changed) { > config_->set_aec_drift_compensation(aec_drift_compensation); > config_->set_aec_extended_filter(aec_extended_filter); > config_->set_aec_suppression_level(aec_suppression_level); > ... > } > > return changed; > } > > I think that is more readable and more compact (and hence less error prone) and > also more efficient as the fewer if-statements makes it more pipeline-able and > more efficient. It uses more stack space but that should not be a problem. also goes away per Andrew's suggestion https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:22: #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP On 2015/10/02 08:34:27, peah-webrtc wrote: > Is this really the correct way to do this? Why not keep this in the .cc file as > before and instead do a forward declaration, > similarly to what is done for the Event class? problem goes away with new change in Patch set 7 https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audio_processing_unittest.cc (right): https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_processing_unittest.cc:1840: // dsadadasaadasdfewafscdeeds On 2015/10/02 08:34:27, peah-webrtc wrote: > This comment should be updated to be more proper :-) sorry sorry, this is my local branch for testing the new filed. I might have accidentally merged it. Please check a following new patch https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/unpack.cc (right): https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/unpack.cc:43: inline void print_config(FILE* settings_file, On 2015/10/02 08:34:28, peah-webrtc wrote: > Is this really a proper filename according to the style guideline? > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names) This part has been changed back to Andrew's early suggestion (a macro). You may still concern about macro vs. inline function. To be honest, I don't know what is best, but I think Andrew's idea is not bad. https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/unpack.cc:97: FILE* settings_file = OpenFile(FLAGS_settings_file, "wb"); On 2015/10/02 08:34:27, peah-webrtc wrote: > This is something that holds for the version currently in master as well, but > should we not close the file? I cannot find any corresponding file close? Yes, could you help me find a place to add a close (since I am OOO and have limited spectrum)? or maybe make a CL or it is easier to merge.
lgtm, I don't know the protobuf recording functionality well enough yet though to vouch for that. https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/unpack.cc (right): https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/unpack.cc:43: inline void print_config(FILE* settings_file, On 2015/10/02 09:01:42, minyue-webrtc wrote: > On 2015/10/02 08:34:28, peah-webrtc wrote: > > Is this really a proper filename according to the style guideline? > > > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names) > > This part has been changed back to Andrew's early suggestion (a macro). You may > still concern about macro vs. inline function. To be honest, I don't know what > is best, but I think Andrew's idea is not bad. Acknowledged. https://codereview.webrtc.org/1348903004/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/unpack.cc:97: FILE* settings_file = OpenFile(FLAGS_settings_file, "wb"); On 2015/10/02 09:01:42, minyue-webrtc wrote: > On 2015/10/02 08:34:27, peah-webrtc wrote: > > This is something that holds for the version currently in master as well, but > > should we not close the file? I cannot find any corresponding file close? > > Yes, could you help me find a place to add a close (since I am OOO and have > limited spectrum)? or maybe make a CL or it is easier to merge. I'd just add a close of the settings_file and debug_file at the end of the do_main function, just before the return 0. This should be fine as the while-statement encompasses the whole file. I guess, however, that it should be fine to have it in a separate CL as well as it was designed like that before. Not sure about how that typically is best handled. You know more about that than me. In case you others think it is fine to keep it as it is, I'm happy with that.
andrew@webrtc.org changed reviewers: + aluebs@chromium.org, turaj@webrtc.org
lgtm, thanks for all the changes Minyue. But we should sort out the new AGC name before you land it. Turaj, Alex, please have a look at the inline comment. Also, have you tested the latest version? Significant changes from earlier. https://codereview.webrtc.org/1348903004/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:6: // when any of the fields is changed. s/is changed/are changed https://codereview.webrtc.org/1348903004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:60: optional bool agc_experiment_enabled = 10; Similarly, noise_robust_agc_enabled. Or something? We haven't really come up with a good name for this thing. Turaj, Alex, do you have any proposals? https://codereview.webrtc.org/1348903004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:67: optional bool ns_experiment_enabled = 15; Could you instead call this transient_suppression_enabled? That's what it's really referring to. The experiment business is legacy.
andrew@webrtc.org changed reviewers: + aluebs@webrtc.org - aluebs@chromium.org
Switching to Alex's webrtc address.
turajs@google.com changed reviewers: + turajs@google.com
https://codereview.webrtc.org/1348903004/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:60: optional bool agc_experiment_enabled = 10; On 2015/10/02 19:04:02, Andrew MacDonald wrote: > Similarly, noise_robust_agc_enabled. > > Or something? We haven't really come up with a good name for this thing. > > Turaj, Alex, do you have any proposals? I'm normally bad with naming. Don't have a better suggestion than "noise-robust." What differentiates it than other AGCs is its robust Voiced/Unvoiced classifier. But I don't know how to make a name out of it. It was easy for its internal VAD to call pitch-based. But I don't think pitch-based-AGC makes much of sense. I don't know if I was helpful!
https://codereview.webrtc.org/1348903004/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:60: optional bool agc_experiment_enabled = 10; On 2015/10/02 19:04:02, Andrew MacDonald wrote: > Similarly, noise_robust_agc_enabled. > > Or something? We haven't really come up with a good name for this thing. > > Turaj, Alex, do you have any proposals? I like your suggestion.
Patchset #9 (id:200001) has been deleted
On 2015/10/02 19:04:02, Andrew MacDonald wrote: > lgtm, thanks for all the changes Minyue. > > But we should sort out the new AGC name before you land it. Turaj, Alex, please > have a look at the inline comment. > > Also, have you tested the latest version? Significant changes from earlier. > > https://codereview.webrtc.org/1348903004/diff/180001/webrtc/modules/audio_pro... > File webrtc/modules/audio_processing/debug.proto (right): > > https://codereview.webrtc.org/1348903004/diff/180001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/debug.proto:6: // when any of the fields is > changed. > s/is changed/are changed > > https://codereview.webrtc.org/1348903004/diff/180001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/debug.proto:60: optional bool > agc_experiment_enabled = 10; > Similarly, noise_robust_agc_enabled. > > Or something? We haven't really come up with a good name for this thing. > > Turaj, Alex, do you have any proposals? > > https://codereview.webrtc.org/1348903004/diff/180001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/debug.proto:67: optional bool > ns_experiment_enabled = 15; > Could you instead call this transient_suppression_enabled? That's what it's > really referring to. The experiment business is legacy. I kept doing some tests, and will try a Chromium test before submitting it.
The renaming is made. Please see if it is proper. https://codereview.webrtc.org/1348903004/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:60: optional bool agc_experiment_enabled = 10; On 2015/10/02 19:16:37, aluebs-webrtc wrote: > On 2015/10/02 19:04:02, Andrew MacDonald wrote: > > Similarly, noise_robust_agc_enabled. > > > > Or something? We haven't really come up with a good name for this thing. > > > > Turaj, Alex, do you have any proposals? > > I like your suggestion. Thanks for your comments. I'd take Andrew's suggestion. A slight change to it is that I'd like "agc" to be placed as pre-fix, as other names. https://codereview.webrtc.org/1348903004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:67: optional bool ns_experiment_enabled = 15; On 2015/10/02 19:04:01, Andrew MacDonald wrote: > Could you instead call this transient_suppression_enabled? That's what it's > really referring to. The experiment business is legacy. Nice, I'd even put it at the end as a parallel to AEC, HPF etc, wdyt? https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:69: optional bool transient_suppression_enabled = 16; not sure if it is possible/better to use a short name for transient_suppression as in other fields.
lgtm https://codereview.webrtc.org/1348903004/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:60: optional bool agc_experiment_enabled = 10; On 2015/10/02 19:59:59, minyue-webrtc wrote: > On 2015/10/02 19:16:37, aluebs-webrtc wrote: > > On 2015/10/02 19:04:02, Andrew MacDonald wrote: > > > Similarly, noise_robust_agc_enabled. > > > > > > Or something? We haven't really come up with a good name for this thing. > > > > > > Turaj, Alex, do you have any proposals? > > > > I like your suggestion. > > Thanks for your comments. I'd take Andrew's suggestion. A slight change to it is > that I'd like "agc" to be placed as pre-fix, as other names. I get why you want to prefix with agc for consistency, but I'd prefer not in this case. agc_noise_robust_enabled makes it sound like a feature of the existing AGC, while noise_robust_agc_enabled makes it sound like a separate component, which is closer to the truth. https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:69: optional bool transient_suppression_enabled = 16; On 2015/10/02 19:59:59, minyue-webrtc wrote: > not sure if it is possible/better to use a short name for transient_suppression > as in other fields. That would be nice, but it's not really a recognized name so I think we'd better spell it out.
More nits. https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1256: // Acoustic echo canceler. Nit: I think you can remove all these comments. It's clear from the name. (The transient suppression one is particularly unnecessary). Put line breaks between the groups if you like. https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1266: // Mobile Acoustic echo canceler. s/Acoustic/acoustic https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:45: // added when any of the fields is changed. s/is/are https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:68: // Transcient suppression. s/Transcient/Transient But also a pretty useless comment :) https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/unpack.cc (right): https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/unpack.cc:236: // AEC. Same here, these comments are unnecessary. Use line breaks if you like.
https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1266: // Mobile Acoustic echo canceler. On 2015/10/02 21:46:04, Andrew MacDonald wrote: > s/Acoustic/acoustic Obviated by the above request.
https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1256: // Acoustic echo canceler. On 2015/10/02 21:46:04, Andrew MacDonald wrote: > Nit: I think you can remove all these comments. It's clear from the name. (The > transient suppression one is particularly unnecessary). Put line breaks between > the groups if you like. Done. https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1266: // Mobile Acoustic echo canceler. On 2015/10/02 21:46:04, Andrew MacDonald wrote: > s/Acoustic/acoustic Done. https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/debug.proto (right): https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:45: // added when any of the fields is changed. On 2015/10/02 21:46:04, Andrew MacDonald wrote: > s/is/are oh, thanks. I did not realize that I had made a similar error as in line 6. https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:68: // Transcient suppression. On 2015/10/02 21:46:04, Andrew MacDonald wrote: > s/Transcient/Transient > > But also a pretty useless comment :) Oh, sorry for the typo https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/debug.proto:69: optional bool transient_suppression_enabled = 16; On 2015/10/02 21:41:57, Andrew MacDonald wrote: > On 2015/10/02 19:59:59, minyue-webrtc wrote: > > not sure if it is possible/better to use a short name for > transient_suppression > > as in other fields. > > That would be nice, but it's not really a recognized name so I think we'd better > spell it out. Acknowledged. https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/unpack.cc (right): https://codereview.webrtc.org/1348903004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/unpack.cc:236: // AEC. On 2015/10/02 21:46:04, Andrew MacDonald wrote: > Same here, these comments are unnecessary. Use line breaks if you like. Done.
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from peah@webrtc.org, andrew@webrtc.org Link to the patchset: https://codereview.webrtc.org/1348903004/#ps240001 (title: "fixing some nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1348903004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1348903004/240001
https://codereview.webrtc.org/1348903004/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1275: config.set_noise_robust_enabled(use_new_agc_); Did you build this? This is using the wrong field name: it's missing "agc".
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_rel/builds/9598)
https://codereview.webrtc.org/1348903004/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1348903004/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1275: config.set_noise_robust_enabled(use_new_agc_); On 2015/10/02 22:00:01, Andrew MacDonald wrote: > Did you build this? This is using the wrong field name: it's missing "agc". oh, I thought I changed it. sorry, was too hasty.
should work now
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from peah@webrtc.org, andrew@webrtc.org Link to the patchset: https://codereview.webrtc.org/1348903004/#ps260001 (title: "last commit was hasty, having an error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1348903004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1348903004/260001
Message was sent while issue was closed.
Committed patchset #11 (id:260001) manually as 13b96ba90f72164134019cbfc07d4a47cf1fd091 (presubmit successful).
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/13b96ba90f72164134019cbfc07d4a47cf1fd091 Cr-Commit-Position: refs/heads/master@{#10155} |
