Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2718133002:
APM Quality Assessment tool, environmental noise generator (Closed)
|
Created:
3 years, 9 months ago by AleBzk Modified:
3 years, 8 months ago Reviewers:
peah-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionEnvironmental noise generator implemented.
BUG=webrtc:7218
NOTRY=True
Review-Url: https://codereview.webrtc.org/2718133002
Cr-Commit-Position: refs/heads/master@{#17506}
Committed: https://chromium.googlesource.com/external/webrtc/+/8a1b3c9d1129a326afde9b853482a86969d720d2
Patch Set 1 #
Total comments: 6
Patch Set 2 : comments from Per addressed #
Total comments: 32
Patch Set 3 : Comments from Per addressed #
Total comments: 39
Patch Set 4 : comments from Per addressed #
Total comments: 4
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 24 (8 generated)
Description was changed from ========== Envorinmental noise generator implemented. BUG=webrtc:7218 NOTRY=True ========== to ========== Envorinmental noise generator implemented. BUG=webrtc:7218 NOTRY=True ==========
alessiob@webrtc.org changed reviewers: + ehmaldonado@webrtc.org
Hi Edward, I'm working on a Python tool to evaluate the APM module in WebRTC. This is done by also simulating different noise conditions. This CL implements a generator that adds environmental background noise.
On 2017/02/28 16:17:28, AleBzk wrote: > Hi Edward, > > I'm working on a Python tool to evaluate the APM module in WebRTC. This is done > by also simulating different noise conditions. This CL implements a generator > that adds environmental background noise. Hi :) Can you please add an OWNER from https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud... to review this and the other CLs?
On 2017/03/01 02:52:42, ehmaldonado_webrtc wrote: > On 2017/02/28 16:17:28, AleBzk wrote: > > Hi Edward, > > > > I'm working on a Python tool to evaluate the APM module in WebRTC. This is > done > > by also simulating different noise conditions. This CL implements a generator > > that adds environmental background noise. > > Hi :) > > Can you please add an OWNER from > https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud... > to review this and the other CLs? Thanks Edward, I created a separate CL: https://codereview.webrtc.org/2320043002
Hi again Edward, Just wanted to tell you that in order to review, you don't need to be an owner (I was told so). However, I created the OWNER file (and I'm in there). Best, Alessio
On 2017/03/03 13:23:10, AleBzk wrote: > Hi again Edward, > > Just wanted to tell you that in order to review, you don't need to be an owner > (I was told so). However, I created the OWNER file (and I'm in there). > > Best, > Alessio Hi Alessio, This looks good to me, but I don't know enough to give you useful feedback, that's why I asked you to add someone from that OWNERS file as a reviewer. :)
Description was changed from ========== Envorinmental noise generator implemented. BUG=webrtc:7218 NOTRY=True ========== to ========== Environmental noise generator implemented. BUG=webrtc:7218 NOTRY=True ==========
alessiob@webrtc.org changed reviewers: + peah@webrtc.org
Hi, As Edward asked, I added one of the owners as a reviewer of this CL. Best
Hi, Some initial comments/questions, since I'm new to the code. https://codereview.webrtc.org/2718133002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py (right): https://codereview.webrtc.org/2718133002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:66: def generate( This is not part of this review, but I think that it would make sense to have documentation in the code that describes the purpose and usage of the public methods. This is in particular true when the naming of the methods is so general as this one. What this method does, is to EnvironmentalNoiseGenerator::_generate(...), right? But what does that return? The documentation of NoiseGenerator says that two streams are returned. Is that correct? To me it looks as it files are created. Or am I wrong in that? https://codereview.webrtc.org/2718133002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:237: class EnvironmentalNoiseGenerator(NoiseGenerator): How is this class to be used. Afaics, there is yet no code using this class, nor does this CL include any such. Would it be possible for you to add a unit test that demonstrates the usage of the class. https://codereview.webrtc.org/2718133002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:239: Additive environmental noise generator. I would like a more descriptive documentation of the class, that describes the usage of the class, or alternatively, a unit test that illustrates how the class is to be used.
Hi Per, Thanks so much for your valuable requests. Finally someone pushed me to write at least one decent unit test :) This was really useful. I hope I answered to your questions, I look forward to getting more questions if there are still unclear points. Best, Alessio https://codereview.webrtc.org/2718133002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py (right): https://codereview.webrtc.org/2718133002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:66: def generate( On 2017/03/09 12:08:12, peah-webrtc wrote: > This is not part of this review, but I think that it would make sense to have > documentation in the code that describes the purpose and usage of the public > methods. This is in particular true when the naming of the methods is so general > as this one. > > What this method does, is to EnvironmentalNoiseGenerator::_generate(...), right? > > But what does that return? The documentation of NoiseGenerator says that two > streams are returned. Is that correct? To me it looks as it files are created. > Or am I wrong in that? Thanks and sorry for the lack of documentation. I removed return since _generate() does not return anything, and added the doc for both generate() and _generate(). This should already help. Plus, I added other missing doc for each concrete class in another CL (https://codereview.webrtc.org/2715233003/diff2/20001:40001/webrtc/modules/aud...) - I got the same request. https://codereview.webrtc.org/2718133002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:237: class EnvironmentalNoiseGenerator(NoiseGenerator): On 2017/03/09 12:08:12, peah-webrtc wrote: > How is this class to be used. Afaics, there is yet no code using this class, nor > does this CL include any such. Would it be possible for you to add a unit test > that demonstrates the usage of the class. Acknowledged. https://codereview.webrtc.org/2718133002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:239: Additive environmental noise generator. On 2017/03/09 12:08:11, peah-webrtc wrote: > I would like a more descriptive documentation of the class, that describes the > usage of the class, or alternatively, a unit test that illustrates how the class > is to be used. Acknowledged. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py (right): https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py:37: self.assertGreater(len(registered_classes), 0) registered_classes includes the environmental noise generator that is the main contrib of this CL. All the registered generators are tested in this test case. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py:42: self.assertTrue(os.path.exists(input_signal_filepath)) tone-880.wav is used as clean input signal (in practice it often is clean speech) https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py:58: base_output_path=self._base_output_path) This is the way to let a generator generate the pairs of noisy input and reference signals. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py:83: """ This is the most interesting test. I check that the noisy signal and the reference are not shorter than the clean input. For instance, with echo due to the delay the noisy signal becomes longer (same for the reference).
Great! Some more comments. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py (right): https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:67: """Given a (clean) input signal, create a set of noisy input and reference Probably it would be good to somehow specify that the signals referred to are stored in files. As it is now, it could possibly be misinterpreted that they are stored as vectors/lists/arrays. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:73: method implemented in a concrete class. Regarding "concrete", I'm not 100% sure about that term. Is that a standard term in python for referring to a class implementing an interface? https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:88: generator produces a specific number of pairs, depending on its internal I don't think you need the same description here as for generate(). The abstract method description is needed though, and maybe something beyond that but I don't think there is any point in repeating the description from generate(). https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:256: _NOISY_SIGNAL_FILENAME_TEMPLATE = '{0}_{1:d}_SNR.wav' Suggestion: what about NOISE_SIGNAL_FILENAME_TEMPLATE as everything in this signal is to be considered noise, right? https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:258: # TODO(alessiob): allow the user to store the noise tracks in a custom path. Allow https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:268: # values indicate the noise-to-signal ratio. Therefore a higher value means I don't think it is good to invert the definition of SNR just because the way it is used. If absolutely needed, you should call it NSR instead of SNR but I see no need to doing that. Why not just specify the values in SNR and then invert that when you use the value? That is much less confusing I think and will also eliminate the need to add this comment to explain what is done. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:297: if not os.path.exists(noise_track_filepath): Do we need to log this? Can't we just assert/crash in a controlled manner with an explanation? Is there any point in continuing if one of the signals is missing? I think that in production code it would make sense to log and fail gracefully, but for a command line analysis tool like this I think it is better to fail in a more obvious manner, and not bother with the boilerplate code required to handled a graceful failure. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:314: noise_signal, input_signal, snr) I think the use of mix_signals in this way is confusing, as the mix_signals descriptions states: "signal_0: AudioSegment instance (signal). signal_1: AudioSegment instance (noise)." I think it is better to really use snr as the snr and not the nsr. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py (right): https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:14: import pydub.generators Why is this added? Nothing else in the code seems to have changed. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py (right): https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py:1: # Copyright (c) 2017 The WebRTC project authors. All Rights Reserved. I'm not sure about the name of the file. In C++, the filename is usually noisegen_unittest.cc. Is that different for Python? https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py:84: input_signal_length = ( Is the extra parenthesis needed for the indentation? https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py:96: noisy_signal_length = ( Also here, are the extra parenthesis needed for the indentation? https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py:98: self.assertGreaterEqual(noisy_signal_length, input_signal_length) Why should the reference signal be longer than the input signal? https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py:110: self.assertGreaterEqual(reference_signal_length, input_signal_length) Why can the reference signal be longer than the input signal?
Thanks for your comments Per! I made some changes, see my comments below. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py (right): https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:67: """Given a (clean) input signal, create a set of noisy input and reference On 2017/03/14 11:33:26, peah-webrtc wrote: > Probably it would be good to somehow specify that the signals referred to are > stored in files. As it is now, it could possibly be misinterpreted that they are > stored as vectors/lists/arrays. Done. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:73: method implemented in a concrete class. On 2017/03/14 11:33:26, peah-webrtc wrote: > Regarding "concrete", I'm not 100% sure about that term. Is that a standard term > in python for referring to a class implementing an interface? There is for sure "abstract" as standard term (see the abc python package doc). Just wanted to highlight that this function calls an abstract method (see raise NotImplementedError() in _generate()) which is implemented in every class that extends NoiseGenerator. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:88: generator produces a specific number of pairs, depending on its internal On 2017/03/14 11:33:26, peah-webrtc wrote: > I don't think you need the same description here as for generate(). The abstract > method description is needed though, and maybe something beyond that but I don't > think there is any point in repeating the description from generate(). Done. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:256: _NOISY_SIGNAL_FILENAME_TEMPLATE = '{0}_{1:d}_SNR.wav' On 2017/03/14 11:33:26, peah-webrtc wrote: > Suggestion: what about NOISE_SIGNAL_FILENAME_TEMPLATE as everything in this > signal is to be considered noise, right? Something I haven't documented well is my naming convention: - input signal: the clean signal - noise signal: the noise to be summed up to the input signal - noisy signal: input + noise I use this convention in all the additive generators. I added a file description at the beginning of the file to explain this better. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:258: # TODO(alessiob): allow the user to store the noise tracks in a custom path. On 2017/03/14 11:33:26, peah-webrtc wrote: > Allow Thanks, I ack this style remark but leave it as it is to facilitate rebasing. This comment is deleted in a depending CL. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:268: # values indicate the noise-to-signal ratio. Therefore a higher value means On 2017/03/14 11:33:26, peah-webrtc wrote: > I don't think it is good to invert the definition of SNR just because the way it > is used. If absolutely needed, you should call it NSR instead of SNR but I see > no need to doing that. > Why not just specify the values in SNR and then invert that when you use the > value? That is much less confusing I think and will also eliminate the need to > add this comment to explain what is done. > Done. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:297: if not os.path.exists(noise_track_filepath): On 2017/03/14 11:33:26, peah-webrtc wrote: > Do we need to log this? Can't we just assert/crash in a controlled manner with > an explanation? Is there any point in continuing if one of the signals is > missing? > > I think that in production code it would make sense to log and fail gracefully, > but for a command line analysis tool like this I think it is better to fail in a > more obvious manner, and not bother with the boilerplate code required to > handled a graceful failure. Thanks, I added continue since I prefer to notify the user and keep running. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:314: noise_signal, input_signal, snr) On 2017/03/14 11:33:26, peah-webrtc wrote: > I think the use of mix_signals in this way is confusing, as the mix_signals > descriptions states: > "signal_0: AudioSegment instance (signal). > signal_1: AudioSegment instance (noise)." > > I think it is better to really use snr as the snr and not the nsr. Once again thanks to a comment from you the code improved a lot :) I also had to change mix_signals() in signal_processing.py and I corrected another generator from a previous CL. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py (right): https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:14: import pydub.generators On 2017/03/14 11:33:26, peah-webrtc wrote: > Why is this added? Nothing else in the code seems to have changed. This is not directly included when pydub is imported. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py (right): https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py:1: # Copyright (c) 2017 The WebRTC project authors. All Rights Reserved. On 2017/03/14 11:33:26, peah-webrtc wrote: > I'm not sure about the name of the file. In C++, the filename is usually > noisegen_unittest.cc. Is that different for Python? Good point. You're right, that's our naming convention for Python as well. I was using the default one, which is useful when you use python -m unittest discover (it looks for test_*.py files). https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py:84: input_signal_length = ( On 2017/03/14 11:33:26, peah-webrtc wrote: > Is the extra parenthesis needed for the indentation? Yes. That's the drawback of having no ";" :) https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py:96: noisy_signal_length = ( On 2017/03/14 11:33:26, peah-webrtc wrote: > Also here, are the extra parenthesis needed for the indentation? Acknowledged. https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py:98: self.assertGreaterEqual(noisy_signal_length, input_signal_length) On 2017/03/14 11:33:26, peah-webrtc wrote: > Why should the reference signal be longer than the input signal? See the answer to "Why can the reference signal be longer than the input signal?" (below). https://codereview.webrtc.org/2718133002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py:110: self.assertGreaterEqual(reference_signal_length, input_signal_length) On 2017/03/14 11:33:26, peah-webrtc wrote: > Why can the reference signal be longer than the input signal? Premise: the reference signal is compared to the APM input signal when computing a quality score. A shorter reference does not make sense because the reference is APM input + noise. When noise is purely additive (e.g., white) the number of samples is exactly the same. While when there is convolutional noise (e.g., echo) then the reference is longer.
https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment_export.py (right): https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment_export.py:2: # Copyright (c) 2017 The WebRTC project authors. All Rights Reserved. What is changed in this file? Is it added due to a rebase? I realize that it is already in the repository, but what is the reason for having an empty file (apart from the comments)? https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py (right): https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:11: used as input for APM, and it is generated by making a clean signal noisy. Regarding "used". Is it not more accurate to write "intended to be used"? Or is the output always directly used as APM input? https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:11: used as input for APM, and it is generated by making a clean signal noisy. "making a clean signal noisy" -> "adding noise to a signal" ? https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:12: The reference is the expected APM output. Ideally, one would like to hear the Suggestion: "The reference is the expected APM output" -> "The reference is the expected APM output when using the generated input" ? https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:13: clean signal; however it is more realistic to assume that residual noise is Regarding "Ideally, one would like to hear the clean signal; however it is more realistic to assume that residual noise is still present. This choice will also make the evaluation more realistic.": I'm not sure about this comment. It relates to the intended behavior of APM when applied to the signals generated by this class. I'd prefer if the comment here only relates to what this class does. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:18: - noise signal: the noise to be summed up to the input signal (e.g., white), "white" -> "white noise" or "Gaussian noise" https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:20: The noise signal may or may not be a function of the clean signal. For Regarding "The noise signal may or may not be a function of the clean signal.": This does seem strange to me and I don't think it is true with the comment below regarding the echo. To me this translates into noise_signal = F(clean_signal) and I don't see any such functionality in place. Could you give an example of a case when the noise signal can be generated as this? https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:22: convolving the input signal with an impulse response. Regarding "whereas echo is obtained by convolving the input signal with an impulse response.": Do you plan to do the echo generation within the noisegenerator class/framework as well? That could make sense but then I think the name NoiseGenerator does no longer match what it is doing. I think that that name should probably be changed, as it is already now a bit misleading. It does not always generate noise, right? https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:36: the latter goes trhough the same deterioration process, but more "gently". type: trhough https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:42: the background noise or all the echo. Hence, the process that generates the I'm not sure that this comment fits here. Would it be possible to have these comments in code that more directly work with APM? (this code does actually not have anything to do with APM). https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:85: Each concrete generator produces a specific number of pairs, depending on I think the initial part of the comment may be redundant, as it basically states the same as was stated above. WDYT? https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:88: This method initialized an empty set of pairs and calls the _generate() initializes https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:187: [20, 30], # Smallest noise. How do yo handle the case of clean signals, e.g. no noise? https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:335: class EchoNoiseGenerator(NoiseGenerator): It think the term "noise" is becoming a bit too widely used herein. To me, a NoiseGenerator is something that generates noise. I think it may possibly be used for something that scales a prerecorded noise signal but that is a bit of a stretch I think. I definitely think it is misleading when specified to echo as in echo noise generator. In this case, you by noise mean noise for the purpose of evaluating the quality of the APM output, right? I think the code would benefit in readability if you could find another name for this. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py (right): https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:117: Mix two signals up to a desired SNR by scaling signal_1 (noise). Since you use the term SNR (== signal to noise ratio), why not rename the signals signal_0 and signal_1 to signal and noise so that their names matches the SNR term? https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:122: target_snr: float (dB). How do you handle infinite SNR (no noise)? https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:127: # overlay() method implictly pads signal_1. Hence, the only case to handle I think it would make more sense to limit the signal lengths to the shortest one rather than to pad the signals. Padding signal_1 (the noise) may drastically change the results as that padding will achieve an infinite SNR in that region. Padding signal_0 is safer, but I don't see a usecase where it would be preferred to pad that instead of truncating it. WDYT? https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:137: logging.debug(' padding: %d ms', padding_duration) Is the output of this logging always shown? The padding needs to be clearly signalled I think as it will have a huge impact of the results. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:146: power_0 = float(signal_0.dBFS) How do you handle zero powered signals? (dBFS==negative infinity) https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py (left): https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py:11: from . import noise_generation Is this file empty? Would it not then instead make sense to remove it?
New unit tests added for signal_processing.SignalProcessingUtils.mix_signals() (see signal_processing_unittest.py). Doc corrected. NoiseGenerator naming issue acknowledged. It will be addressed in a separate CL in order to land the pending CLs asap. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment_export.py (right): https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment_export.py:2: # Copyright (c) 2017 The WebRTC project authors. All Rights Reserved. On 2017/03/22 09:57:03, peah-webrtc wrote: > What is changed in this file? Is it added due to a rebase? I realize that it is > already in the repository, but what is the reason for having an empty file > (apart from the comments)? It is already called by the .sh script even if it does nothing. See https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud... https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py (right): https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:11: used as input for APM, and it is generated by making a clean signal noisy. On 2017/03/22 09:57:03, peah-webrtc wrote: > "making a clean signal noisy" -> "adding noise to a signal" ? Done. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:12: The reference is the expected APM output. Ideally, one would like to hear the On 2017/03/22 09:57:03, peah-webrtc wrote: > Suggestion: "The reference is the expected APM output" -> "The reference is the > expected APM output when using the generated input" ? Done. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:13: clean signal; however it is more realistic to assume that residual noise is On 2017/03/22 09:57:03, peah-webrtc wrote: > Regarding "Ideally, one would like to hear the clean signal; however it is more > realistic to assume that residual noise is still present. This choice will also > make the evaluation more realistic.": > I'm not sure about this comment. It relates to the intended behavior of APM when > applied to the signals generated by this class. I'd prefer if the comment here > only relates to what this class does. Done. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:18: - noise signal: the noise to be summed up to the input signal (e.g., white), On 2017/03/22 09:57:03, peah-webrtc wrote: > "white" -> "white noise" or "Gaussian noise" Done. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:20: The noise signal may or may not be a function of the clean signal. For On 2017/03/22 09:57:03, peah-webrtc wrote: > Regarding "The noise signal may or may not be a function of the clean signal.": > This does seem strange to me and I don't think it is true with the comment below > regarding the echo. To me this translates into noise_signal = F(clean_signal) > and I don't see any such functionality in place. Could you give an example of a > case when the noise signal can be generated as this? We clarified that it is not echo but reverberation. Let's leave the "echo" keyword for echo path simulation (which will appear in the future). I guess that "reverberation" is enough - no need to provide an example. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:22: convolving the input signal with an impulse response. On 2017/03/22 09:57:03, peah-webrtc wrote: > Regarding "whereas echo is obtained by convolving the input signal with an > impulse response.": Do you plan to do the echo generation within the > noisegenerator class/framework as well? That could make sense but then I think > the name NoiseGenerator does no longer match what it is doing. I think that that > name should probably be changed, as it is already now a bit misleading. It does > not always generate noise, right? A better name could be InputReferencePairGenerator. We agreed on leaving the names as they are since there are several pending lgtm'ed CLs depedning on this. I will then write a refactoring CL to address your renaming request. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:36: the latter goes trhough the same deterioration process, but more "gently". On 2017/03/22 09:57:03, peah-webrtc wrote: > type: trhough Done. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:42: the background noise or all the echo. Hence, the process that generates the On 2017/03/22 09:57:03, peah-webrtc wrote: > I'm not sure that this comment fits here. Would it be possible to have these > comments in code that more directly work with APM? (this code does actually not > have anything to do with APM). Done. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:85: Each concrete generator produces a specific number of pairs, depending on On 2017/03/22 09:57:03, peah-webrtc wrote: > I think the initial part of the comment may be redundant, as it basically states > the same as was stated above. WDYT? Done. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:88: This method initialized an empty set of pairs and calls the _generate() On 2017/03/22 09:57:03, peah-webrtc wrote: > initializes Done. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:187: [20, 30], # Smallest noise. On 2017/03/22 09:57:03, peah-webrtc wrote: > How do yo handle the case of clean signals, e.g. no noise? I have a separate generator that does nothing. It's called IdentityGenerator (see below in the same file). https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:335: class EchoNoiseGenerator(NoiseGenerator): On 2017/03/22 09:57:03, peah-webrtc wrote: > It think the term "noise" is becoming a bit too widely used herein. To me, a > NoiseGenerator is something that generates noise. I think it may possibly be > used for something that scales a prerecorded noise signal but that is a bit of a > stretch I think. I definitely think it is misleading when specified to echo as > in echo noise generator. > > In this case, you by noise mean noise for the purpose of evaluating the quality > of the APM output, right? I think the code would benefit in readability if you > could find another name for this. Great point. As discussed above, I will first land all the pending CLs and then write a refactoring CL to improve naming. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py (right): https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:117: Mix two signals up to a desired SNR by scaling signal_1 (noise). On 2017/03/22 09:57:04, peah-webrtc wrote: > Since you use the term SNR (== signal to noise ratio), why not rename the > signals signal_0 and signal_1 to signal and noise so that their names matches > the SNR term? Done. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:122: target_snr: float (dB). On 2017/03/22 09:57:04, peah-webrtc wrote: > How do you handle infinite SNR (no noise)? Yet another good point. I now check if SNR == +/- numpy.Inf and return a copy of signal/noise respectively. I also added a unit test for this function (see signal_processing_unittest.py). https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:127: # overlay() method implictly pads signal_1. Hence, the only case to handle On 2017/03/22 09:57:03, peah-webrtc wrote: > I think it would make more sense to limit the signal lengths to the shortest one > rather than to pad the signals. Padding signal_1 (the noise) may drastically > change the results as that padding will achieve an infinite SNR in that region. > Padding signal_0 is safer, but I don't see a usecase where it would be preferred > to pad that instead of truncating it. WDYT? True, but let me explain the usecase. In the APM-QA tool, the noise is longer than the signal when, for instance, we simulate reverberation. In this case, the difference in duration is little and hence it shouldn't really harm. Then, we should keep in mind the way we evaluate in the AMP-QA tool. For instance, when computing POLQA, a DTW is performed to align the reference to the APM output. If the reference is shorter than the APM input signal (achieved by truncating), then the reference will also be shorter than the APM output. And this may cause a POLQA score drop - POLQA looks quite sensitive. In general, I'd say that the correct strategy (truncating or padding) may depend on the score. While padding is better for POLQA, it may not be the best for speech level. Handling this would increase the complexity of the code enormously. For now I'd prefer to keep padding because I'm more concerned by POLQA (which we compute with an external tool) than audio/speech level scores. Let me know what you also think about this point. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:137: logging.debug(' padding: %d ms', padding_duration) On 2017/03/22 09:57:04, peah-webrtc wrote: > Is the output of this logging always shown? The padding needs to be clearly > signalled I think as it will have a huge impact of the results. Done. https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:146: power_0 = float(signal_0.dBFS) On 2017/03/22 09:57:04, peah-webrtc wrote: > How do you handle zero powered signals? (dBFS==negative infinity) Right. I wasn't handling the -Inf case. I won't allow that, I added a check at the beginning of the function (+ unittest). https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py (left): https://codereview.webrtc.org/2718133002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_noisegen.py:11: from . import noise_generation On 2017/03/22 09:57:03, peah-webrtc wrote: > Is this file empty? Would it not then instead make sense to remove it? This diff is because the file has been renamed to noise_generation_unittest.py. So it's been removed.
lgtm with some comments. It would, however, be good to again go through this code once the usage of the code is in place. Then I think it will be more clear if things should be changed. https://codereview.webrtc.org/2718133002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.sh (right): https://codereview.webrtc.org/2718133002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.sh:1: #!/bin/bash Would it not be possible to do this as well in python instead of in bash? That would not require users to both have bash and python installed. https://codereview.webrtc.org/2718133002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py (right): https://codereview.webrtc.org/2718133002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:285: input_signal = SignalProcessingUtils.normalize(input_signal) It is fine for now, but I don't see why the normalization should be needed here. Why not do that as part of the mixing? Also, to me it is not really clear that we always want normalized signals. But I think that will be clearer in upcoming CLs.
alessiob@webrtc.org changed reviewers: - ehmaldonado@webrtc.org
Thanks Per! I replied to the last comments and took note of the open issues to be addressed in the refactoring CLs. https://codereview.webrtc.org/2718133002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.sh (right): https://codereview.webrtc.org/2718133002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.sh:1: #!/bin/bash On 2017/04/03 12:15:55, peah-webrtc wrote: > Would it not be possible to do this as well in python instead of in bash? That > would not require users to both have bash and python installed. Well, this is meant to be a sample bash script to easily run in parallel the APM QA tool. In bash, I can just use a for loop, the ampersand and the wait command. Parallelizing in Python requires instead much more code and the performance may not be exactly the same as bash background processing (read the first lines at https://wiki.python.org/moin/GlobalInterpreterLock). We can safely assume that everyone has bash. https://codereview.webrtc.org/2718133002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py (right): https://codereview.webrtc.org/2718133002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:285: input_signal = SignalProcessingUtils.normalize(input_signal) On 2017/04/03 12:15:55, peah-webrtc wrote: > It is fine for now, but I don't see why the normalization should be needed here. > Why not do that as part of the mixing? Also, to me it is not really clear that > we always want normalized signals. But I think that will be clearer in upcoming > CLs. Thanks for this comment. I took note of it for further off-line discussion on how to optimize the signal processing pipeline.
The CQ bit was checked by alessiob@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491227524822800,
"parent_rev": "653063f6fd8b5406da6a6117c41470e0a47b1eb8", "commit_rev":
"8a1b3c9d1129a326afde9b853482a86969d720d2"}
Message was sent while issue was closed.
Description was changed from ========== Environmental noise generator implemented. BUG=webrtc:7218 NOTRY=True ========== to ========== Environmental noise generator implemented. BUG=webrtc:7218 NOTRY=True Review-Url: https://codereview.webrtc.org/2718133002 Cr-Commit-Position: refs/heads/master@{#17506} Committed: https://chromium.googlesource.com/external/webrtc/+/8a1b3c9d1129a326afde9b853... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/8a1b3c9d1129a326afde9b853... |
