Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2804083003:
Add POLQA to low bandwidth audio test (Closed)
|
Created:
3 years, 8 months ago by oprypin_webrtc Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, tlegrand-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd POLQA to low bandwidth audio test
BUG=webrtc:7229
Review-Url: https://codereview.webrtc.org/2804083003
Cr-Commit-Position: refs/heads/master@{#17671}
Committed: https://chromium.googlesource.com/external/webrtc/+/f2501004759a8d634ae5b2426e76ec0c8e5b557a
Patch Set 1 : Add POLQA to low bandwidth audio test #
Total comments: 10
Patch Set 2 : Address review comments #
Total comments: 3
Patch Set 3 : Rebase #
Total comments: 6
Patch Set 4 : Address review feedback #
Total comments: 2
Patch Set 5 : Remove unused argument #
Messages
Total messages: 28 (10 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
oprypin@webrtc.org changed reviewers: + kjellander@webrtc.org, kwiberg@webrtc.org
https://codereview.webrtc.org/2804083003/diff/40001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2804083003/diff/40001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.cc:27: DEFINE_int32(bitrate, 16000, "Bitrate of the produced audio file."); This should probably be called sampling_frequency (Hz), since that's what it is (sorry I didn't spot this earlier). I thought it looked a little strange in previous reviews but I forgot to follow up. See https://cs.chromium.org/chromium/src/third_party/webrtc/test/fake_audio_devic..., which you call below at line 66. https://codereview.webrtc.org/2804083003/diff/40001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.py (right): https://codereview.webrtc.org/2804083003/diff/40001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:112: def _RunPesq(executable_path, reference_file, degraded_file, bitrate=16000): bitrate -> sampling_frequency_in_hz https://codereview.webrtc.org/2804083003/diff/40001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:153: # Find the scores in stdout of polqa. polqa -> POLQA https://codereview.webrtc.org/2804083003/diff/40001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:183: analyzers = [(_RunPesq, pesq_path, 16000)] How about using this instead for improved readability: import collections Analyzer = collections.namedtuple('Analyzer', ['func', 'executable', 'sampling_frequency_in_hz']) https://codereview.webrtc.org/2804083003/diff/40001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:188: analyzers.insert(0, (_RunPolqa, polqa_path, 48000)) any reason you don't just use .append instead?
I assume you made sure they're uploaded to the internal GS bucket?
https://codereview.webrtc.org/2804083003/diff/40001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2804083003/diff/40001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.cc:27: DEFINE_int32(bitrate, 16000, "Bitrate of the produced audio file."); On 2017/04/07 18:26:12, kjellander_webrtc wrote: > This should probably be called sampling_frequency (Hz), since that's what it is > (sorry I didn't spot this earlier). I thought it looked a little strange in > previous reviews but I forgot to follow up. > See > https://cs.chromium.org/chromium/src/third_party/webrtc/test/fake_audio_devic..., > which you call below at line 66. Done. https://codereview.webrtc.org/2804083003/diff/40001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.py (right): https://codereview.webrtc.org/2804083003/diff/40001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:112: def _RunPesq(executable_path, reference_file, degraded_file, bitrate=16000): On 2017/04/07 18:26:12, kjellander_webrtc wrote: > bitrate -> sampling_frequency_in_hz Thanks. I really should be more careful about this terminology. https://codereview.webrtc.org/2804083003/diff/40001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:153: # Find the scores in stdout of polqa. On 2017/04/07 18:26:12, kjellander_webrtc wrote: > polqa -> POLQA Done. https://codereview.webrtc.org/2804083003/diff/40001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:183: analyzers = [(_RunPesq, pesq_path, 16000)] On 2017/04/07 18:26:12, kjellander_webrtc wrote: > How about using this instead for improved readability: > > import collections > Analyzer = collections.namedtuple('Analyzer', ['func', 'executable', > 'sampling_frequency_in_hz']) > Done. https://codereview.webrtc.org/2804083003/diff/40001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:188: analyzers.insert(0, (_RunPolqa, polqa_path, 48000)) On 2017/04/07 18:26:12, kjellander_webrtc wrote: > any reason you don't just use .append instead? This was meant to run POLQA first, but it doesn't matter. Done.
lgtm with a suggestion. https://codereview.webrtc.org/2804083003/diff/60001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.py (right): https://codereview.webrtc.org/2804083003/diff/60001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:198: test_process = subprocess.Popen( What happens in the case where we get a really bad run and the tool fails (like the problem you were facing earlier) ? I think we should at least catch + re-raise the error and print what command was run, since the Exception being thrown usually don't reveal that kind of information and is most likely harder to understand. An alternative is to add a little more logging, like the logging.info in _RunCommand (and/or verbose logging triggered by a --verbose flag).
https://codereview.webrtc.org/2804083003/diff/60001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.py (right): https://codereview.webrtc.org/2804083003/diff/60001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:198: test_process = subprocess.Popen( On 2017/04/10 12:30:02, kjellander_webrtc wrote: > What happens in the case where we get a really bad run and the tool fails (like > the problem you were facing earlier) ? > > I think we should at least catch + re-raise the error and print what command was > run, since the Exception being thrown usually don't reveal that kind of > information and is most likely harder to understand. > > An alternative is to add a little more logging, like the logging.info in > _RunCommand (and/or verbose logging triggered by a --verbose flag). I don't quite understand. The executed command is logged with `_LogCommand`. The status code is propagated with `return test_process.wait()` into `sys.exit()`. The stdout is echoed (though without a logging prefix). By the way, this CL doesn't make any change to the described process. What exactly can I improve?
https://codereview.webrtc.org/2804083003/diff/60001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.py (right): https://codereview.webrtc.org/2804083003/diff/60001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:198: test_process = subprocess.Popen( On 2017/04/10 12:36:51, oprypin_webrtc wrote: > On 2017/04/10 12:30:02, kjellander_webrtc wrote: > > What happens in the case where we get a really bad run and the tool fails > (like > > the problem you were facing earlier) ? > > > > I think we should at least catch + re-raise the error and print what command > was > > run, since the Exception being thrown usually don't reveal that kind of > > information and is most likely harder to understand. > > > > An alternative is to add a little more logging, like the logging.info in > > _RunCommand (and/or verbose logging triggered by a --verbose flag). > > I don't quite understand. > > The executed command is logged with `_LogCommand`. The status code is propagated > with `return test_process.wait()` into `sys.exit()`. The stdout is echoed > (though without a logging prefix). By the way, this CL doesn't make any change > to the described process. What exactly can I improve? You're right - this is fine. I didn't pay enough attention to that the .wait() call is happening at line 228. I guess I assumed it to be inside the try+finally block or something.
I just remembered that I had one question. This code issues WARNING logs under relatively normal conditions (POLQA will be missing on most bots). Can that affect the "greenness" of dashboard entries for these runs? In that case, should this be changed to a lower log level?
On 2017/04/11 09:22:15, oprypin_webrtc wrote: > I just remembered that I had one question. > This code issues WARNING logs under relatively normal conditions (POLQA will be > missing on most bots). Can that affect the "greenness" of dashboard entries for > these runs? In that case, should this be changed to a lower log level? Logging levels don't affect how results are presented, but I'm personally very diligent about not having ERROR showing up for non-fatal problems (unfortunately that's a very common problem). I think WARNING is fine in this case, but you could also change it to INFO if you think it suits better. I'm leaning towards that.
The CQ bit was checked by oprypin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2804083003/#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/16076)
Right... Karl, could you review the .cc file?
Looked at the .cc file. https://codereview.webrtc.org/2804083003/diff/80001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2804083003/diff/80001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.cc:24: DEFINE_int32(sampling_frequency, 16000, As always, I'm going to suggest that you call this sample_rate_hz. But you decide. https://codereview.webrtc.org/2804083003/diff/80001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.cc:25: "Sampling frequency (Hz) of the produced audio files."); Look it up before you take my advice because I'm not too familiar with gflags, but I think you may be supposed to put them in the global namespace. https://codereview.webrtc.org/2804083003/diff/80001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.cc:54: "_" + std::to_string(FLAGS_sampling_frequency / 1000) + ".wav"; You construct this name more than once. Utility function?
https://codereview.webrtc.org/2804083003/diff/80001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2804083003/diff/80001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.cc:24: DEFINE_int32(sampling_frequency, 16000, On 2017/04/12 07:13:49, kwiberg-webrtc wrote: > As always, I'm going to suggest that you call this sample_rate_hz. But you > decide. Done. https://codereview.webrtc.org/2804083003/diff/80001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.cc:25: "Sampling frequency (Hz) of the produced audio files."); On 2017/04/12 07:13:49, kwiberg-webrtc wrote: > Look it up before you take my advice because I'm not too familiar with gflags, > but I think you may be supposed to put them in the global namespace. OK. Indeed, flags are usually put in the global namespace. https://codereview.webrtc.org/2804083003/diff/80001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.cc:54: "_" + std::to_string(FLAGS_sampling_frequency / 1000) + ".wav"; On 2017/04/12 07:13:49, kwiberg-webrtc wrote: > You construct this name more than once. Utility function? Done.
lgtm https://codereview.webrtc.org/2804083003/diff/100001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2804083003/diff/100001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:29: std::string FileSampleRateSuffix(int sample_rate_hz = FLAGS_sample_rate_hz) { You don't use the argument; remove it?
https://codereview.webrtc.org/2804083003/diff/100001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2804083003/diff/100001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:29: std::string FileSampleRateSuffix(int sample_rate_hz = FLAGS_sample_rate_hz) { On 2017/04/12 11:35:06, kwiberg-webrtc wrote: > You don't use the argument; remove it? Oops. Thanks.
The CQ bit was checked by oprypin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2804083003/#ps120001 (title: "Remove unused argument")
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": 120001, "attempt_start_ts": 1491997378793580,
"parent_rev": "8d8185c7743f2713908a1823656493c13fc8ea67", "commit_rev":
"f2501004759a8d634ae5b2426e76ec0c8e5b557a"}
Message was sent while issue was closed.
Description was changed from ========== Add POLQA to low bandwidth audio test BUG=webrtc:7229 ========== to ========== Add POLQA to low bandwidth audio test BUG=webrtc:7229 Review-Url: https://codereview.webrtc.org/2804083003 Cr-Commit-Position: refs/heads/master@{#17671} Committed: https://chromium.googlesource.com/external/webrtc/+/f2501004759a8d634ae5b2426... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/f2501004759a8d634ae5b2426... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
