|
|
Created:
3 years, 10 months ago by oprypin_webrtc Modified:
3 years, 9 months ago CC:
webrtc-reviews_webrtc.org, hlundin-webrtc, skvlad Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionLow-bandwidth audio testing
The C++ part of the test uses CallTest to set up an audio-only call. It reads an audio file, plays it through a FakeAudioDevice which transfers data through a FakeNetworkPipe for another FakeAudioDevice to receive it and write it to a file. Information about these files is printed to stdout.
The test cases are meant to try different network and audio configs (more are planned in the future).
The Python part of the test runs the C++ part and scans stdout for tests to perform, runs the pairs of files (original and degraded) through the PESQ tool to receive a score and writes that to perf dashboard.
BUG=webrtc:7229
NOTRY=True
Review-Url: https://codereview.webrtc.org/2694203002
Cr-Commit-Position: refs/heads/master@{#17356}
Committed: https://chromium.googlesource.com/external/webrtc/+/92220ffe9fe31698b9113f9e5347f7bd85b557e4
Patch Set 1 : Low-bandwidth audio testing #Patch Set 2 : Fix build errors #
Total comments: 30
Patch Set 3 : Address small issues from review #
Total comments: 6
Patch Set 4 : Address more small issues from review #Patch Set 5 : Fix trailing zeros in BoundedWavFileWriter #Patch Set 6 : Move BoundedWavFileWriter and add unittests for it #
Total comments: 12
Patch Set 7 : Fix typo #Patch Set 8 : Make unittest executable in parallel #Patch Set 9 : Move call_test dependency where it belongs #
Total comments: 3
Patch Set 10 : Rebase #
Messages
Total messages: 79 (54 generated)
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Low-bandwidth audio testing BUG= ========== to ========== Low-bandwidth audio testing BUG=webrtc:7229 ==========
The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/18624)
The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/23919) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/22702)
Patchset #2 (id:80001) has been deleted
The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/23920) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/22704)
TODO: add landmine before landing https://chromium.googlesource.com/external/webrtc/+/master/tools-webrtc/get_l...
The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #2 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/22714)
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/18455)
Description was changed from ========== Low-bandwidth audio testing BUG=webrtc:7229 ========== to ========== Low-bandwidth audio testing The C++ part of the test uses CallTest to set up an audio-only call. It reads an audio file, plays it through a FakeAudioDevice which transfers data through a FakeNetworkPipe for another FakeAudioDevice to receive it and write it to a file. Information about these files is printed to stdout. The test cases are meant to try different network and audio configs (more are planned in the future). The Python part of the test runs the C++ part and scans stdout for tests to perform, runs the pairs of files (original and degraded) through the PESQ tool to receive a score and writes that to perf dashboard. BUG=webrtc:7229 ==========
oprypin@webrtc.org changed reviewers: + kjellander@webrtc.org, kwiberg@webrtc.org, stefan@webrtc.org
Nice and clean code! https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn File webrtc/audio/BUILD.gn (right): https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:94: configs += [ "..:rtc_unittests_config" ] Why do you need this? To disable the warnings listen in it?(https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?rcl=c0a0f8d5f2d5b837b4c6ea447b0cdce86723e0d6&l=361) If so, I suggest you fix them instead and remove this. If the warnings comes from the deps you depend on it's another story, but I don't see where it would come from (since that config is for making rtc_unittests compile). https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:102: "../common_audio:common_audio", Use "../common_audio" instead. It's the same (target named as the dir is implicitly used if target is omitted) https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:104: "../system_wrappers:system_wrappers", ../system_wrappers is enough. https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:113: deps += [ "../modules/video_capture:video_capture_internal_impl" ] What needs this? It seems we should try to move this dependency to that location instead of here. I assume you got linking errors without it? Can you share the output here? https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:122: suppressed_configs += [ "//build/config/clang:find_bad_constructs" ] Is it possible to remove this or do you need it to compile the code you depend on? https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:25: class BoundedWavFileWriter : public test::FakeAudioDevice::Renderer { I think BoundedWavFileWriter is large enough for its own .cc file + unit test. https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:87: AudioQualityTest::AudioQualityTest() : EndToEndTest(1000) {} It's not clear what 1000 is here. Move into a constant. https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:118: new BoundedWavFileWriter(AudioOutputFile(), 16000)); Extract into a constant or even a gflags flag, since the python script could pass it (as it also needs it for PESQ). https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:148: CodecInst{120, "OPUS", 48000, 960, 2, 64000}; This default is kind of hidden. Can you store it as a const variable for the class? https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:155: SleepMs(GetNetworkPipeConfig().queue_delay_ms + 500); Extract 500 into a constant. https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.py (right): https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.py:120: +1 blank line for top-level statements https://google.github.io/styleguide/pyguide.html?showone=Blank_Lines#Blank_Lines https://codereview.webrtc.org/2694203002/diff/160001/webrtc/test/call_test.cc File webrtc/test/call_test.cc (left): https://codereview.webrtc.org/2694203002/diff/160001/webrtc/test/call_test.cc... webrtc/test/call_test.cc:364: CreateFakeAudioDevices(); Won't this affect other tests using this code?
https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn File webrtc/audio/BUILD.gn (right): https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:94: configs += [ "..:rtc_unittests_config" ] On 2017/03/17 07:11:41, kjellander_webrtc wrote: > Why do you need this? To disable the warnings listen in > it?(https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?rcl=c0a0f8d5f2d5b837b4c6ea447b0cdce86723e0d6&l=361) > > If so, I suggest you fix them instead and remove this. If the warnings comes > from the deps you depend on it's another story, but I don't see where it would > come from (since that config is for making rtc_unittests compile). OK, turns out it's not needed, thanks. https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:102: "../common_audio:common_audio", On 2017/03/17 07:11:41, kjellander_webrtc wrote: > Use "../common_audio" instead. It's the same (target named as the dir is > implicitly used if target is omitted) Done. https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:104: "../system_wrappers:system_wrappers", On 2017/03/17 07:11:41, kjellander_webrtc wrote: > ../system_wrappers is enough. Done. https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:113: deps += [ "../modules/video_capture:video_capture_internal_impl" ] On 2017/03/17 07:11:41, kjellander_webrtc wrote: > What needs this? It seems we should try to move this dependency to that location > instead of here. I assume you got linking errors without it? Can you share the > output here? Getting this error without this line: ../../webrtc/modules/video_capture/video_capture_factory.cc:22: error: undefined reference to 'webrtc::videocapturemodule::VideoCaptureImpl::Create(char const*)' I copied this from https://cs.chromium.org/chromium/src/third_party/webrtc/video/BUILD.gn?l=96&r... https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:122: suppressed_configs += [ "//build/config/clang:find_bad_constructs" ] On 2017/03/17 07:11:41, kjellander_webrtc wrote: > Is it possible to remove this or do you need it to compile the code you depend > on? Getting this error without this line: In file included from ../../webrtc/test/call_test.h:16: In file included from ../../webrtc/call/call.h:19: ../../webrtc/call/audio_receive_stream.h:37:3: error: [chromium-style] Complex class/struct needs an explicit out-of-line constructor. ... and many more like this. I copied this from https://cs.chromium.org/chromium/src/third_party/webrtc/video/BUILD.gn?rcl=95... https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:87: AudioQualityTest::AudioQualityTest() : EndToEndTest(1000) {} On 2017/03/17 07:11:41, kjellander_webrtc wrote: > It's not clear what 1000 is here. Move into a constant. Done. https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:118: new BoundedWavFileWriter(AudioOutputFile(), 16000)); On 2017/03/17 07:11:41, kjellander_webrtc wrote: > Extract into a constant or even a gflags flag, since the python script could > pass it (as it also needs it for PESQ). PESQ has "Sample rate [...] Must select either +8000 or +16000." I hardcoded this because only 16000 makes sense under these circumstances. It's good to make it a constant (done). A flag would also be good in case other tools are used, but can we agree to do that when the need arises? https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:148: CodecInst{120, "OPUS", 48000, 960, 2, 64000}; On 2017/03/17 07:11:41, kjellander_webrtc wrote: > This default is kind of hidden. Can you store it as a const variable for the > class? Done. https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:155: SleepMs(GetNetworkPipeConfig().queue_delay_ms + 500); On 2017/03/17 07:11:41, kjellander_webrtc wrote: > Extract 500 into a constant. Done. https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.py (right): https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.py:120: On 2017/03/17 07:11:41, kjellander_webrtc wrote: > +1 blank line for top-level statements > https://google.github.io/styleguide/pyguide.html?showone=Blank_Lines#Blank_Lines Done.
Patchset #3 (id:180001) has been deleted
https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn File webrtc/audio/BUILD.gn (right): https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:113: deps += [ "../modules/video_capture:video_capture_internal_impl" ] On 2017/03/17 10:16:50, oprypin_webrtc wrote: > On 2017/03/17 07:11:41, kjellander_webrtc wrote: > > What needs this? It seems we should try to move this dependency to that > location > > instead of here. I assume you got linking errors without it? Can you share the > > output here? > > Getting this error without this line: > ../../webrtc/modules/video_capture/video_capture_factory.cc:22: error: undefined > reference to 'webrtc::videocapturemodule::VideoCaptureImpl::Create(char const*)' > > I copied this from > https://cs.chromium.org/chromium/src/third_party/webrtc/video/BUILD.gn?l=96&r... Hmm, I'm unable to see what pulls that in. That target depends on //webrtc/test:video_test_common, which has capture stuff (https://cs.chromium.org/chromium/src/third_party/webrtc/test/BUILD.gn?rcl=c0a...) but your target doesn't pull in any capture code AFAICS. We should figure this out to see if there's any refactoring needed. https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:122: suppressed_configs += [ "//build/config/clang:find_bad_constructs" ] On 2017/03/17 10:16:50, oprypin_webrtc wrote: > On 2017/03/17 07:11:41, kjellander_webrtc wrote: > > Is it possible to remove this or do you need it to compile the code you depend > > on? > > Getting this error without this line: > In file included from ../../webrtc/test/call_test.h:16: > In file included from ../../webrtc/call/call.h:19: > ../../webrtc/call/audio_receive_stream.h:37:3: error: [chromium-style] Complex > class/struct needs an explicit out-of-line constructor. > ... > and many more like this. > > I copied this from > https://cs.chromium.org/chromium/src/third_party/webrtc/video/BUILD.gn?rcl=95... I see. It's not your fault. Then we have to keep it. https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:118: new BoundedWavFileWriter(AudioOutputFile(), 16000)); On 2017/03/17 10:16:51, oprypin_webrtc wrote: > On 2017/03/17 07:11:41, kjellander_webrtc wrote: > > Extract into a constant or even a gflags flag, since the python script could > > pass it (as it also needs it for PESQ). > > PESQ has "Sample rate [...] Must select either +8000 or +16000." > I hardcoded this because only 16000 makes sense under these circumstances. It's > good to make it a constant (done). > A flag would also be good in case other tools are used, but can we agree to do > that when the need arises? Yes, let's keep it as a constant.
lgtm, but please make sure to convince kjellander@ too, since I'm not that familiar with our test infrastructure. https://codereview.webrtc.org/2694203002/diff/200001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2694203002/diff/200001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:34: // Writes to a WAV file, cutting off silence at the beginning and the end. For silence in the beginning, you the amplitude only needs to be <= kAmplitudeThreshold; for silence at the end, the amplitude is required to be zero. If this is intentional, you should probably document it here. https://codereview.webrtc.org/2694203002/diff/200001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:57: if (*begin > kAmplitudeThreshold || *begin < -kAmplitudeThreshold) { std::abs(*begin) > kAmplitudeThreshold ? https://codereview.webrtc.org/2694203002/diff/200001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:178: using LowBandwidthAudioTest = test::CallTest; You don't need the test:: prefix, since you're in that namespace.
https://codereview.webrtc.org/2694203002/diff/200001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2694203002/diff/200001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:34: // Writes to a WAV file, cutting off silence at the beginning and the end. On 2017/03/17 11:19:32, kwiberg-webrtc wrote: > For silence in the beginning, you the amplitude only needs to be <= > kAmplitudeThreshold; for silence at the end, the amplitude is required to be > zero. If this is intentional, you should probably document it here. It is intentional but it's just based on the strange effect that I noticed. The recording side gets strictly zeros when the audio hasn't come through yet, then around 20 samples smaller than 4, then strictly zeros again for a small while, and then the actual audio starts. And no, it does NOT seem to be related to silence in the audio file itself. I'd be interested to find out why this happens. https://codereview.webrtc.org/2694203002/diff/200001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:57: if (*begin > kAmplitudeThreshold || *begin < -kAmplitudeThreshold) { On 2017/03/17 11:19:32, kwiberg-webrtc wrote: > std::abs(*begin) > kAmplitudeThreshold > > ? Done. https://codereview.webrtc.org/2694203002/diff/200001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:178: using LowBandwidthAudioTest = test::CallTest; On 2017/03/17 11:19:33, kwiberg-webrtc wrote: > You don't need the test:: prefix, since you're in that namespace. Done.
https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:25: class BoundedWavFileWriter : public test::FakeAudioDevice::Renderer { On 2017/03/17 07:11:41, kjellander_webrtc wrote: > I think BoundedWavFileWriter is large enough for its own .cc file + unit test. We discussed this and came to conclusion that it's not worth putting it in a separate .cc file. Moved it to fake_audio_device and added unittest. https://codereview.webrtc.org/2694203002/diff/160001/webrtc/test/call_test.cc File webrtc/test/call_test.cc (left): https://codereview.webrtc.org/2694203002/diff/160001/webrtc/test/call_test.cc... webrtc/test/call_test.cc:364: CreateFakeAudioDevices(); On 2017/03/17 07:11:41, kjellander_webrtc wrote: > Won't this affect other tests using this code? I only moved this call outside of CreateVoiceEngines in the only place where it's called. Since it's private, other code can't be affected.
On 2017/03/20 10:39:27, oprypin_webrtc wrote: > https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... > File webrtc/audio/test/low_bandwidth_audio_test.cc (right): > > https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... > webrtc/audio/test/low_bandwidth_audio_test.cc:25: class BoundedWavFileWriter : > public test::FakeAudioDevice::Renderer { > On 2017/03/17 07:11:41, kjellander_webrtc wrote: > > I think BoundedWavFileWriter is large enough for its own .cc file + unit test. > > We discussed this and came to conclusion that it's not worth putting it in a > separate .cc file. Moved it to fake_audio_device and added unittest. > > https://codereview.webrtc.org/2694203002/diff/160001/webrtc/test/call_test.cc > File webrtc/test/call_test.cc (left): > > https://codereview.webrtc.org/2694203002/diff/160001/webrtc/test/call_test.cc... > webrtc/test/call_test.cc:364: CreateFakeAudioDevices(); > On 2017/03/17 07:11:41, kjellander_webrtc wrote: > > Won't this affect other tests using this code? > > I only moved this call outside of CreateVoiceEngines in the only place where > it's called. Since it's private, other code can't be affected. We have a similar test called video_loopback test. can we somehow merge these two.
On 2017/03/20 14:19:05, minyue-webrtc wrote: > We have a similar test called video_loopback test. can we somehow merge these > two. That's what I started from, but ever since I refactored the code to use call_test/BaseTest it has become much clearer and shorter. It has a lot more in common with BaseTest than with VideoQualityTest which reimplements a lot of the same things but is not designed to be extensible. I've actually been able to avoid using VoiceEngine directly, which is a big win. VideoQualityTest also relies on a being configurable through a struct and command line flags, but I went with a simpler approach to use "hardcoded" scenarios (yes, that sounds bad, but it's simple and flexible). Not to mention that adding an audio-only test to the video folder sounds funny.
https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:193: pipe_config.queue_length_packets = 1500; I think a longer queue should be used as thats what we've experienced in the wild https://codereview.webrtc.org/2694203002/diff/260001/webrtc/audio/BUILD.gn File webrtc/audio/BUILD.gn (right): https://codereview.webrtc.org/2694203002/diff/260001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:111: deps += [ "../modules/video_capture:video_capture_internal_impl" ] Why do we need a video capturer for the audio tests? https://codereview.webrtc.org/2694203002/diff/260001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2694203002/diff/260001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:101: SleepMs(GetNetworkPipeConfig().queue_delay_ms + kExtraRecordTimeMs); Can we perhaps wait until enough audio has been played out on the receiver instead? https://codereview.webrtc.org/2694203002/diff/260001/webrtc/test/call_test.cc File webrtc/test/call_test.cc (right): https://codereview.webrtc.org/2694203002/diff/260001/webrtc/test/call_test.cc... webrtc/test/call_test.cc:55: fake_recv_audio_device_.get()); It would be nice if this was done in roughly the same place as fake video capturers are created, see lines 119-126. Maybe those can be moved up here, or the other way around? https://codereview.webrtc.org/2694203002/diff/260001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2694203002/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:92: // Returns a Renderer instance that writes its data to a file a WAV file, "to a WAV file"
The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2694203002/diff/260001/webrtc/audio/BUILD.gn File webrtc/audio/BUILD.gn (right): https://codereview.webrtc.org/2694203002/diff/260001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:111: deps += [ "../modules/video_capture:video_capture_internal_impl" ] On 2017/03/20 14:40:10, stefan-webrtc wrote: > Why do we need a video capturer for the audio tests? I suspect that call_test pulls that in, since it also supports video. I get an error without it, see previous comments on this line: https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn#ne... https://codereview.webrtc.org/2694203002/diff/260001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2694203002/diff/260001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:101: SleepMs(GetNetworkPipeConfig().queue_delay_ms + kExtraRecordTimeMs); On 2017/03/20 14:40:10, stefan-webrtc wrote: > Can we perhaps wait until enough audio has been played out on the receiver > instead? I've given this some thought. Relying on the receive side is always a dangerous game, because what if the network is so congested that the test takes hours? (Of course, in practice that won't happen because this is a "perfect" network amd because the test can time out). Or maybe audio gets broken and never sent, or too much of it is sent, or something else. And if the audio takes that long to arrive, it should badly affect the resulting score anyway, which currently automatically happens by cutting off the receiving side. All in all, I think this is not only the simpler option, but actually the better option. https://codereview.webrtc.org/2694203002/diff/260001/webrtc/test/call_test.cc File webrtc/test/call_test.cc (right): https://codereview.webrtc.org/2694203002/diff/260001/webrtc/test/call_test.cc... webrtc/test/call_test.cc:55: fake_recv_audio_device_.get()); On 2017/03/20 14:40:10, stefan-webrtc wrote: > It would be nice if this was done in roughly the same place as fake video > capturers are created, see lines 119-126. Maybe those can be moved up here, or > the other way around? I have not changed the logic of CallTest here, only moved CreateFakeAudioDevices outside of CreateVoiceEngines and made it pluggable. This cannot be moved down because CreateVoiceEngines relies on the audio devices. Lines 119-126 cannot be moved here because CreateFrameGeneratorCapturer relies on the result of CreateVideoStreams. https://codereview.webrtc.org/2694203002/diff/260001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2694203002/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:92: // Returns a Renderer instance that writes its data to a file a WAV file, On 2017/03/20 14:40:10, stefan-webrtc wrote: > "to a WAV file" Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/23243)
https://codereview.webrtc.org/2694203002/diff/260001/webrtc/audio/BUILD.gn File webrtc/audio/BUILD.gn (right): https://codereview.webrtc.org/2694203002/diff/260001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:111: deps += [ "../modules/video_capture:video_capture_internal_impl" ] On 2017/03/20 15:09:16, oprypin_webrtc wrote: > On 2017/03/20 14:40:10, stefan-webrtc wrote: > > Why do we need a video capturer for the audio tests? > > I suspect that call_test pulls that in, since it also supports video. I get an > error without it, see previous comments on this line: > https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn#ne... Shouldn't it be added as a dependency on the target containing call_test then? https://codereview.webrtc.org/2694203002/diff/260001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2694203002/diff/260001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:101: SleepMs(GetNetworkPipeConfig().queue_delay_ms + kExtraRecordTimeMs); On 2017/03/20 15:09:16, oprypin_webrtc wrote: > On 2017/03/20 14:40:10, stefan-webrtc wrote: > > Can we perhaps wait until enough audio has been played out on the receiver > > instead? > > I've given this some thought. Relying on the receive side is always a dangerous > game, because what if the network is so congested that the test takes hours? (Of > course, in practice that won't happen because this is a "perfect" network amd > because the test can time out). Or maybe audio gets broken and never sent, or > too much of it is sent, or something else. > > And if the audio takes that long to arrive, it should badly affect the resulting > score anyway, which currently automatically happens by cutting off the receiving > side. > > All in all, I think this is not only the simpler option, but actually the better > option. Ok, convinced! https://codereview.webrtc.org/2694203002/diff/260001/webrtc/test/call_test.cc File webrtc/test/call_test.cc (right): https://codereview.webrtc.org/2694203002/diff/260001/webrtc/test/call_test.cc... webrtc/test/call_test.cc:55: fake_recv_audio_device_.get()); On 2017/03/20 15:09:16, oprypin_webrtc wrote: > On 2017/03/20 14:40:10, stefan-webrtc wrote: > > It would be nice if this was done in roughly the same place as fake video > > capturers are created, see lines 119-126. Maybe those can be moved up here, or > > the other way around? > > I have not changed the logic of CallTest here, only moved CreateFakeAudioDevices > outside of CreateVoiceEngines and made it pluggable. > This cannot be moved down because CreateVoiceEngines relies on the audio > devices. Lines 119-126 cannot be moved here because CreateFrameGeneratorCapturer > relies on the result of CreateVideoStreams. Ack
Patchset #9 (id:320001) has been deleted
https://codereview.webrtc.org/2694203002/diff/260001/webrtc/audio/BUILD.gn File webrtc/audio/BUILD.gn (right): https://codereview.webrtc.org/2694203002/diff/260001/webrtc/audio/BUILD.gn#ne... webrtc/audio/BUILD.gn:111: deps += [ "../modules/video_capture:video_capture_internal_impl" ] On 2017/03/20 15:42:32, stefan-webrtc wrote: > On 2017/03/20 15:09:16, oprypin_webrtc wrote: > > On 2017/03/20 14:40:10, stefan-webrtc wrote: > > > Why do we need a video capturer for the audio tests? > > > > I suspect that call_test pulls that in, since it also supports video. I get an > > error without it, see previous comments on this line: > > > https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/BUILD.gn#ne... > > Shouldn't it be added as a dependency on the target containing call_test then? Done. Thanks.
lgtm
https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:25: class BoundedWavFileWriter : public test::FakeAudioDevice::Renderer { On 2017/03/20 10:39:27, oprypin_webrtc wrote: > On 2017/03/17 07:11:41, kjellander_webrtc wrote: > > I think BoundedWavFileWriter is large enough for its own .cc file + unit test. > > We discussed this and came to conclusion that it's not worth putting it in a > separate .cc file. Moved it to fake_audio_device and added unittest. Actually, I thought we just agreed on where it would be put (webrtc/test) I still think it would be cleaner to move it to its own .cc file along with the unit test. https://codereview.webrtc.org/2694203002/diff/340001/webrtc/test/BUILD.gn File webrtc/test/BUILD.gn (right): https://codereview.webrtc.org/2694203002/diff/340001/webrtc/test/BUILD.gn#new... webrtc/test/BUILD.gn:413: deps += [ "../modules/video_capture:video_capture_internal_impl" ] I'm still curious about this. Does it come from :video_test_common? Can it be moved in there instead? We need to try hard not making things more complicated in the dependency tree.
The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
lgtm https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2694203002/diff/160001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:25: class BoundedWavFileWriter : public test::FakeAudioDevice::Renderer { On 2017/03/21 08:31:49, kjellander_webrtc wrote: > On 2017/03/20 10:39:27, oprypin_webrtc wrote: > > On 2017/03/17 07:11:41, kjellander_webrtc wrote: > > > I think BoundedWavFileWriter is large enough for its own .cc file + unit > test. > > > > We discussed this and came to conclusion that it's not worth putting it in a > > separate .cc file. Moved it to fake_audio_device and added unittest. > > Actually, I thought we just agreed on where it would be put (webrtc/test) I > still think it would be cleaner to move it to its own .cc file along with the > unit test. I've now been convinced this is fine. Let's keep it here. https://codereview.webrtc.org/2694203002/diff/340001/webrtc/test/BUILD.gn File webrtc/test/BUILD.gn (right): https://codereview.webrtc.org/2694203002/diff/340001/webrtc/test/BUILD.gn#new... webrtc/test/BUILD.gn:413: deps += [ "../modules/video_capture:video_capture_internal_impl" ] On 2017/03/21 08:31:49, kjellander_webrtc wrote: > I'm still curious about this. Does it come from :video_test_common? Can it be > moved in there instead? We need to try hard not making things more complicated > in the dependency tree. Let's keep this as is if you're willing to just have a quick look in a separate CL. OK?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2694203002/diff/340001/webrtc/test/BUILD.gn File webrtc/test/BUILD.gn (right): https://codereview.webrtc.org/2694203002/diff/340001/webrtc/test/BUILD.gn#new... webrtc/test/BUILD.gn:413: deps += [ "../modules/video_capture:video_capture_internal_impl" ] On 2017/03/21 08:43:41, kjellander_webrtc wrote: > On 2017/03/21 08:31:49, kjellander_webrtc wrote: > > I'm still curious about this. Does it come from :video_test_common? Can it be > > moved in there instead? We need to try hard not making things more complicated > > in the dependency tree. > > Let's keep this as is if you're willing to just have a quick look in a separate > CL. OK? OK
The CQ bit was checked by oprypin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2694203002/#ps340001 (title: "Move call_test dependency where it belongs")
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
Failed to apply patch for webrtc/audio/BUILD.gn: While running git apply --index -p1; error: patch failed: webrtc/audio/BUILD.gn:78 error: webrtc/audio/BUILD.gn: patch does not apply Patch: webrtc/audio/BUILD.gn Index: webrtc/audio/BUILD.gn diff --git a/webrtc/audio/BUILD.gn b/webrtc/audio/BUILD.gn index a6a22e652cce5cad47874baebe00acf9c1649cc2..4858c46571e06fd2ed04acfb7cb29759702c47a1 100644 --- a/webrtc/audio/BUILD.gn +++ b/webrtc/audio/BUILD.gn @@ -78,6 +78,10 @@ if (rtc_include_tests) { "//testing/gmock", "//testing/gtest", ] + if (rtc_enable_protobuf) { + deps += [ ":low_bandwidth_audio_test" ] + } + if (!build_with_chromium && is_clang) { # Suppress warnings from the Chromium Clang plugin (bugs.webrtc.org/163). suppressed_configs += [ "//build/config/clang:find_bad_constructs" ] @@ -90,12 +94,28 @@ if (rtc_include_tests) { sources = [ "test/low_bandwidth_audio_test.cc", + "test/low_bandwidth_audio_test.h", ] - deps = [] - + deps = [ + "../common_audio", + "../system_wrappers", + "../test:fake_audio_device", + "../test:run_test", + "../test:test_common", + "../test:test_main", + ] if (is_android) { - deps += [ "//testing/android/native_test:native_test_support" ] + deps += [ "//testing/android/native_test:native_test_native_code" ] + } + + data = [ + "//resources/voice_engine/audio_tiny16.wav", + ] + + if (!build_with_chromium && is_clang) { + # Suppress warnings from the Chromium Clang plugin (bugs.webrtc.org/163) + suppressed_configs += [ "//build/config/clang:find_bad_constructs" ] } } }
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, stefan@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2694203002/#ps360001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Low-bandwidth audio testing The C++ part of the test uses CallTest to set up an audio-only call. It reads an audio file, plays it through a FakeAudioDevice which transfers data through a FakeNetworkPipe for another FakeAudioDevice to receive it and write it to a file. Information about these files is printed to stdout. The test cases are meant to try different network and audio configs (more are planned in the future). The Python part of the test runs the C++ part and scans stdout for tests to perform, runs the pairs of files (original and degraded) through the PESQ tool to receive a score and writes that to perf dashboard. BUG=webrtc:7229 ========== to ========== Low-bandwidth audio testing The C++ part of the test uses CallTest to set up an audio-only call. It reads an audio file, plays it through a FakeAudioDevice which transfers data through a FakeNetworkPipe for another FakeAudioDevice to receive it and write it to a file. Information about these files is printed to stdout. The test cases are meant to try different network and audio configs (more are planned in the future). The Python part of the test runs the C++ part and scans stdout for tests to perform, runs the pairs of files (original and degraded) through the PESQ tool to receive a score and writes that to perf dashboard. BUG=webrtc:7229 NOTRY=True ==========
The CQ bit was unchecked by kjellander@webrtc.org
The CQ bit was checked by kjellander@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
On 2017/03/23 10:37:41, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/... I set NOTRY=True and bypassed the CQ as it now became urgent to get this landed asap, since linux_memcheck is now consistently failing on this test in current HEAD (but passes with this landed).
CQ is committing da patch. Bot data: {"patchset_id": 360001, "attempt_start_ts": 1490265450775590, "parent_rev": "2aa463f721c922ec93bdd3306650b0e24968b92d", "commit_rev": "92220ffe9fe31698b9113f9e5347f7bd85b557e4"}
Message was sent while issue was closed.
Description was changed from ========== Low-bandwidth audio testing The C++ part of the test uses CallTest to set up an audio-only call. It reads an audio file, plays it through a FakeAudioDevice which transfers data through a FakeNetworkPipe for another FakeAudioDevice to receive it and write it to a file. Information about these files is printed to stdout. The test cases are meant to try different network and audio configs (more are planned in the future). The Python part of the test runs the C++ part and scans stdout for tests to perform, runs the pairs of files (original and degraded) through the PESQ tool to receive a score and writes that to perf dashboard. BUG=webrtc:7229 NOTRY=True ========== to ========== Low-bandwidth audio testing The C++ part of the test uses CallTest to set up an audio-only call. It reads an audio file, plays it through a FakeAudioDevice which transfers data through a FakeNetworkPipe for another FakeAudioDevice to receive it and write it to a file. Information about these files is printed to stdout. The test cases are meant to try different network and audio configs (more are planned in the future). The Python part of the test runs the C++ part and scans stdout for tests to perform, runs the pairs of files (original and degraded) through the PESQ tool to receive a score and writes that to perf dashboard. BUG=webrtc:7229 NOTRY=True Review-Url: https://codereview.webrtc.org/2694203002 Cr-Commit-Position: refs/heads/master@{#17356} Committed: https://chromium.googlesource.com/external/webrtc/+/92220ffe9fe31698b9113f9e5... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:360001) as https://chromium.googlesource.com/external/webrtc/+/92220ffe9fe31698b9113f9e5... |