Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2826073002:
Adds AudioDeviceTest.MeasureLoopbackLatency unittest (Closed)
|
Created:
3 years, 8 months ago by henrika_webrtc Modified:
3 years, 8 months ago Reviewers:
kwiberg-webrtc CC:
webrtc-reviews_webrtc.org Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdds AudioDeviceTest.MeasureLoopbackLatency unittest.
Follow-up CL on https://codereview.webrtc.org/2788883002/ where I add a new
test which has to be enabled manually (will not run by default on bots).
Measures loopback latency and reports the min, max and average values for
a full duplex audio session.
The latency is measured like so:
- Insert impulses periodically on the output side.
- Detect the impulses on the input side.
- Measure the time difference between the transmit time and receive time.
- Store time differences in a vector and calculate min, max and average.
This test needs the '--gtest_also_run_disabled_tests' flag to run and also
some sort of audio feedback loop. E.g. a headset where the mic is placed
close to the speaker to ensure highest possible echo. It is also recommended
to run the test at highest possible output volume.
How to run:
./out/Debug/modules_unittests --gtest_filter=AudioDeviceMeasureLoopbackLatency --gtest_also_run_disabled_tests
Example output (on Linux machine):
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from AudioDeviceTest
[ RUN ] AudioDeviceTest.DISABLED_MeasureLoopbackLatency
[..........]
[..........] [min, max, avg]=[59, 67, 64] ms
[ OK ] AudioDeviceTest.DISABLED_MeasureLoopbackLatency (10034 ms)
[----------] 1 test from AudioDeviceTest (10034 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (10036 ms total)
[ PASSED ] 1 test.
BUG=webrtc:7273
Review-Url: https://codereview.webrtc.org/2826073002
Cr-Commit-Position: refs/heads/master@{#17791}
Committed: https://chromium.googlesource.com/external/webrtc/+/714e5cd6c697e90dcb716fe4c65dbb35e1a81c9a
Patch Set 1 #Patch Set 2 : Fixed comment #
Total comments: 15
Patch Set 3 : Feedback from kwiberg@ #
Total comments: 2
Patch Set 4 : nit #Patch Set 5 : Should now build on win64 #Patch Set 6 : nit #Patch Set 7 : nit #Messages
Total messages: 34 (18 generated)
Description was changed from ========== Adds AudioDeviceTest.MeasureLoopbackLatency unittest cleanup Adds AudioDeviceTest.MeasureLoopbackLatency unittest Added latency test BUG= ========== to ========== Adds AudioDeviceTest.MeasureLoopbackLatency unittest BUG=webrtc:7273 ==========
henrika@webrtc.org changed reviewers: + kwiberg@webrtc.org
Description was changed from ========== Adds AudioDeviceTest.MeasureLoopbackLatency unittest BUG=webrtc:7273 ========== to ========== Adds AudioDeviceTest.MeasureLoopbackLatency unittest. Follow-up CL on https://codereview.webrtc.org/2788883002/ where I add a new test which has to be enabled manually (will not run by default on bots). Measures loopback latency and reports the min, max and average values for a full duplex audio session. The latency is measured like so: - Insert impulses periodically on the output side. - Detect the impulses on the input side. - Measure the time difference between the transmit time and receive time. - Store time differences in a vector and calculate min, max and average. This test needs the '--gtest_also_run_disabled_tests' flag to run and also some sort of audio feedback loop. E.g. a headset where the mic is placed close to the speaker to ensure highest possible echo. It is also recommended to run the test at highest possible output volume. How to run: ./out/Debug/modules_unittests --gtest_filter=AudioDeviceMeasureLoopbackLatency --gtest_also_run_disabled_tests Example output (on Linux machine): [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from AudioDeviceTest [ RUN ] AudioDeviceTest.DISABLED_MeasureLoopbackLatency [..........] [..........] [min, max, avg]=[59, 67, 64] ms [ OK ] AudioDeviceTest.DISABLED_MeasureLoopbackLatency (10034 ms) [----------] 1 test from AudioDeviceTest (10034 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (10036 ms total) [ PASSED ] 1 test. BUG=webrtc:7273 ==========
PTAL ;-)
https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_unittest.cc (right): https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:81: static constexpr char kTag[] = "[..........] "; Can you move all or some of these closer to where they're used? At least kMeasureLatencyTimeInSec seems to be used only by a single function, and in that case it may be more readable to have the definition there. On the other hand, you may still prefer to keep them all in the same place since they seem to be documented as a unit. Do whatever you feel is more pedagogical. https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:192: const int16_t impulse = std::numeric_limits<int16_t>::max(); constexpr? https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:213: source.begin(), std::find(source.begin(), source.end(), max)); Why are you using std::find here?---you already found the max in the previous statement. Also, ArrayView's iterators are random access and support unary minus, so you don't have to use std::distance. https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:220: PRINTD("[%d]", static_cast<int>(now_time - pulse_time_)); checked_cast? https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:240: } This doesn't need to be a member function. Put it outside the class in an anonymous namespace? https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:284: int64_t pulse_time_ GUARDED_BY(lock_) = 0; You're using pulse_time_ == 0 to mean that it's unset. Could you use Optional<int64_t> instead? https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:287: size_t write_count_ = 0; Why are these two not protected by anything?
https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_unittest.cc (right): https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:213: source.begin(), std::find(source.begin(), source.end(), max)); On 2017/04/20 09:26:37, kwiberg-webrtc wrote: > Why are you using std::find here?---you already found the max in the previous > statement. > > Also, ArrayView's iterators are random access and support unary minus, so you > don't have to use std::distance. const size_t index_of_max = std::max_element(source.begin(), source.end()) - source.begin();
Thanks. PTAL https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_unittest.cc (right): https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:81: static constexpr char kTag[] = "[..........] "; Thanks. Moving and inlining kTag ;-) https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:192: const int16_t impulse = std::numeric_limits<int16_t>::max(); works as well ;-) https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:213: source.begin(), std::find(source.begin(), source.end(), max)); Thanks! https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:220: PRINTD("[%d]", static_cast<int>(now_time - pulse_time_)); On 2017/04/20 09:26:37, kwiberg-webrtc wrote: > checked_cast? Done. https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:240: } On 2017/04/20 09:26:37, kwiberg-webrtc wrote: > This doesn't need to be a member function. Put it outside the class in an > anonymous namespace? Done. https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:284: int64_t pulse_time_ GUARDED_BY(lock_) = 0; Done (I think since I have not used Optional before). https://codereview.webrtc.org/2826073002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:287: size_t write_count_ = 0; I know by design that read_count_ is only access on T1 and write_count_ on T2. Will make that more clear.
lgtm! https://codereview.webrtc.org/2826073002/diff/40001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_unittest.cc (right): https://codereview.webrtc.org/2826073002/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:43: // #define ENABLE_DEBUG_PRINTF Did you mean to do this?
https://codereview.webrtc.org/2826073002/diff/40001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_unittest.cc (right): https://codereview.webrtc.org/2826073002/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:43: // #define ENABLE_DEBUG_PRINTF Yes I did. Only used to add extra verbose debug logs for cases when the transmitted pulses are not detected.
The CQ bit was checked by henrika@webrtc.org
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: mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
The CQ bit was checked by henrika@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/2826073002/#ps60001 (title: "nit")
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: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/24856)
The CQ bit was checked by henrika@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/2826073002/#ps80001 (title: "Should now build on win64")
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: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/8420)
The CQ bit was checked by henrika@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/2826073002/#ps100001 (title: "nit")
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: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/24861)
The CQ bit was checked by henrika@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/2826073002/#ps120001 (title: "nit")
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": 1492699413338460,
"parent_rev": "103b6bfb1831fafddbe05ea971dcb67d6cd76f67", "commit_rev":
"714e5cd6c697e90dcb716fe4c65dbb35e1a81c9a"}
Message was sent while issue was closed.
Description was changed from ========== Adds AudioDeviceTest.MeasureLoopbackLatency unittest. Follow-up CL on https://codereview.webrtc.org/2788883002/ where I add a new test which has to be enabled manually (will not run by default on bots). Measures loopback latency and reports the min, max and average values for a full duplex audio session. The latency is measured like so: - Insert impulses periodically on the output side. - Detect the impulses on the input side. - Measure the time difference between the transmit time and receive time. - Store time differences in a vector and calculate min, max and average. This test needs the '--gtest_also_run_disabled_tests' flag to run and also some sort of audio feedback loop. E.g. a headset where the mic is placed close to the speaker to ensure highest possible echo. It is also recommended to run the test at highest possible output volume. How to run: ./out/Debug/modules_unittests --gtest_filter=AudioDeviceMeasureLoopbackLatency --gtest_also_run_disabled_tests Example output (on Linux machine): [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from AudioDeviceTest [ RUN ] AudioDeviceTest.DISABLED_MeasureLoopbackLatency [..........] [..........] [min, max, avg]=[59, 67, 64] ms [ OK ] AudioDeviceTest.DISABLED_MeasureLoopbackLatency (10034 ms) [----------] 1 test from AudioDeviceTest (10034 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (10036 ms total) [ PASSED ] 1 test. BUG=webrtc:7273 ========== to ========== Adds AudioDeviceTest.MeasureLoopbackLatency unittest. Follow-up CL on https://codereview.webrtc.org/2788883002/ where I add a new test which has to be enabled manually (will not run by default on bots). Measures loopback latency and reports the min, max and average values for a full duplex audio session. The latency is measured like so: - Insert impulses periodically on the output side. - Detect the impulses on the input side. - Measure the time difference between the transmit time and receive time. - Store time differences in a vector and calculate min, max and average. This test needs the '--gtest_also_run_disabled_tests' flag to run and also some sort of audio feedback loop. E.g. a headset where the mic is placed close to the speaker to ensure highest possible echo. It is also recommended to run the test at highest possible output volume. How to run: ./out/Debug/modules_unittests --gtest_filter=AudioDeviceMeasureLoopbackLatency --gtest_also_run_disabled_tests Example output (on Linux machine): [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from AudioDeviceTest [ RUN ] AudioDeviceTest.DISABLED_MeasureLoopbackLatency [..........] [..........] [min, max, avg]=[59, 67, 64] ms [ OK ] AudioDeviceTest.DISABLED_MeasureLoopbackLatency (10034 ms) [----------] 1 test from AudioDeviceTest (10034 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (10036 ms total) [ PASSED ] 1 test. BUG=webrtc:7273 Review-Url: https://codereview.webrtc.org/2826073002 Cr-Commit-Position: refs/heads/master@{#17791} Committed: https://chromium.googlesource.com/external/webrtc/+/714e5cd6c697e90dcb716fe4c... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/714e5cd6c697e90dcb716fe4c... |
