Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2715763003:
APM Quality Assessment tool, signal processing tools (Closed)
|
Created:
3 years, 10 months ago by AleBzk Modified:
3 years, 9 months ago Reviewers:
ivoc CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, aleloi, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAPM quality assessment tool, signal processing tools.
BUG=webrtc:7218
NOTRY=True
Review-Url: https://codereview.webrtc.org/2715763003
Cr-Commit-Position: refs/heads/master@{#17047}
Committed: https://chromium.googlesource.com/external/webrtc/+/21debb3ae819f91f4d7da6800502c4c628c01f46
Patch Set 1 #Patch Set 2 : Signal processing library. #
Total comments: 15
Patch Set 3 : comments from Ivo addressed #
Total comments: 3
Patch Set 4 : Typo fixed #Patch Set 5 : rebase #Patch Set 6 : Private method call removed #Patch Set 7 : call to private method replaced #Patch Set 8 : rebase #
Messages
Total messages: 34 (19 generated)
Description was changed from ========== APM quality assessment tool, signal processing tools. BUG=webrtc:7218 ========== to ========== APM quality assessment tool, signal processing tools. BUG=webrtc:7218 ==========
alessiob@webrtc.org changed reviewers: + aleloi@webrtc.org, ivoc@webrtc.org
alessiob@webrtc.org changed reviewers: - aleloi@webrtc.org
Signal processing utility class for the quality_assessment package.
Nice! See some comments/questions below. https://codereview.webrtc.org/2715763003/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/2715763003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:11: Please remove the blank line. https://codereview.webrtc.org/2715763003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:13: import pydub This is an external library, right? Is it possible to use the wav reader from the standard library instead? (see https://docs.python.org/2/library/wave.html). Or are there any features missing from that? If you need this library, you should add clear instructions about how to download and install it somewhere (maybe the readme?). https://codereview.webrtc.org/2715763003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:47: number_of_samples = signal.get_array_of_samples() Shouldn't this be len(signal.get_array_of_samples())? https://codereview.webrtc.org/2715763003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:48: assert number_of_samples % signal.channels == 0 Please add an assert to check that signal.channels is > 0. https://codereview.webrtc.org/2715763003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:54: Generate white noise with the same duration and in the same format of a same format as https://codereview.webrtc.org/2715763003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:80: convoled_samples = scipy.signal.fftconvolve( Please rename to convolved_samples.
Description was changed from ========== APM quality assessment tool, signal processing tools. BUG=webrtc:7218 ========== to ========== APM quality assessment tool, signal processing tools. BUG=webrtc:7218 NOTRY=True ==========
Once again, thank you! You can find my replies below. https://codereview.webrtc.org/2715763003/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/2715763003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:11: On 2017/03/02 01:11:48, ivoc wrote: > Please remove the blank line. The blank line is correct due to the grouping rule documented at https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#i... https://codereview.webrtc.org/2715763003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:13: import pydub On 2017/03/02 01:11:48, ivoc wrote: > This is an external library, right? Is it possible to use the wav reader from > the standard library instead? (see https://docs.python.org/2/library/wave.html). > Or are there any features missing from that? If you need this library, you > should add clear instructions about how to download and install it somewhere > (maybe the readme?). Thanks for your concern. In fact, pydub adds things that the wave module doesn't have. I have a separate CL (https://codereview.webrtc.org/2722173003/) for the README file, I explain how to add this third-party library. Sorry for the confusion generated by having the README.md file and this import into different CLs. https://codereview.webrtc.org/2715763003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:47: number_of_samples = signal.get_array_of_samples() On 2017/03/02 01:11:48, ivoc wrote: > Shouldn't this be len(signal.get_array_of_samples())? True! This is a method I'm not using anymore, that's why I missed this problem. Thanks, I fixed. https://codereview.webrtc.org/2715763003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:47: number_of_samples = signal.get_array_of_samples() On 2017/03/02 01:11:48, ivoc wrote: > Shouldn't this be len(signal.get_array_of_samples())? Done. https://codereview.webrtc.org/2715763003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:48: assert number_of_samples % signal.channels == 0 On 2017/03/02 01:11:48, ivoc wrote: > Please add an assert to check that signal.channels is > 0. Done. https://codereview.webrtc.org/2715763003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:54: Generate white noise with the same duration and in the same format of a On 2017/03/02 01:11:48, ivoc wrote: > same format as Done. https://codereview.webrtc.org/2715763003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:80: convoled_samples = scipy.signal.fftconvolve( On 2017/03/02 01:11:48, ivoc wrote: > Please rename to convolved_samples. Done.
aleloi@google.com changed reviewers: + aleloi@google.com
Wow, everything seems so simple with pydub! Couldn't find anything to complain about, back to perfcrastinating. </Drive-by> https://codereview.webrtc.org/2715763003/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/2715763003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:42: Number of samples per channel. Comment would be unnecessary if method was named count_samples_per_channel.
aleloi@google.com changed reviewers: - aleloi@google.com
lgtm. See two comments below. https://codereview.webrtc.org/2715763003/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/2715763003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:47: number_of_samples = signal.get_array_of_samples() On 2017/03/02 12:58:26, AleBzk wrote: > On 2017/03/02 01:11:48, ivoc wrote: > > Shouldn't this be len(signal.get_array_of_samples())? > > Done. Will it be used in any of the other CLs? If not it's probably better to remove it. https://codereview.webrtc.org/2715763003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:49: return number_of_samples / signal.channels I think it's better to use // instead of / here, to avoid any possibility for an implicit float conversion in python 3.
One more small comment. https://codereview.webrtc.org/2715763003/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/2715763003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:96: convoled_signal = signal._spawn(convolved_samples) Please rename to convolved_signal.
https://codereview.webrtc.org/2715763003/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/2715763003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:96: convoled_signal = signal._spawn(convolved_samples) On 2017/03/02 18:55:52, ivoc wrote: > Please rename to convolved_signal. Done.
The CQ bit was checked by alessiob@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org Link to the patchset: https://codereview.webrtc.org/2715763003/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14458)
The CQ bit was checked by alessiob@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org Link to the patchset: https://codereview.webrtc.org/2715763003/#ps100001 (title: "Private method call removed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14462)
The CQ bit was checked by alessiob@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org Link to the patchset: https://codereview.webrtc.org/2715763003/#ps120001 (title: "call to private method replaced")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14464)
The CQ bit was checked by alessiob@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org Link to the patchset: https://codereview.webrtc.org/2715763003/#ps140001 (title: "rebase")
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": 140001, "attempt_start_ts": 1488781867153150,
"parent_rev": "ef3162c3f16517a84ff3dfd08b9752b865dfc723", "commit_rev":
"21debb3ae819f91f4d7da6800502c4c628c01f46"}
Message was sent while issue was closed.
Description was changed from ========== APM quality assessment tool, signal processing tools. BUG=webrtc:7218 NOTRY=True ========== to ========== APM quality assessment tool, signal processing tools. BUG=webrtc:7218 NOTRY=True Review-Url: https://codereview.webrtc.org/2715763003 Cr-Commit-Position: refs/heads/master@{#17047} Committed: https://chromium.googlesource.com/external/webrtc/+/21debb3ae819f91f4d7da6800... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/21debb3ae819f91f4d7da6800... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
