|
|
DescriptionAdd automated VR latency tester
Adds a script to automatically measure the motion-to-photon/app latency
for VR. This requires specialized hardware to measure the latency and move
the device during the test, so it is only meant to be run on our swarming
bot that has been set up to work with it.
Has not yet been set up to run continuously, but currently works if manually
triggered to run on our bot, so can be useful for teammates that do not have
the necessary hardware to measure latency themselves.
BUG=708747
Review-Url: https://codereview.chromium.org/2799783002
Cr-Commit-Position: refs/heads/master@{#464230}
Committed: https://chromium.googlesource.com/chromium/src/+/84541d55d66a59f8b44afaad69882beb10de265b
Patch Set 1 #Patch Set 2 : Add support for args #Patch Set 3 : Working version that must be manually triggered #
Total comments: 26
Patch Set 4 : Address leilei@ comments #
Total comments: 4
Patch Set 5 : Address thakis@ comments #
Total comments: 6
Patch Set 6 : Rebase + address final comments #
Messages
Total messages: 19 (8 generated)
Description was changed from ========== WIP Add automated VR latency tester Adds a script to automatically measure the motion-to-photon/app latency for VR. This requires specialized hardware to measure the latency and move the device during the test, so it is only meant to be run on our swarming bot that has been set up to work with it. BUG=708747 ========== to ========== WIP Add automated VR latency tester Adds a script to automatically measure the motion-to-photon/app latency for VR. This requires specialized hardware to measure the latency and move the device during the test, so it is only meant to be run on our swarming bot that has been set up to work with it. Has not yet been set up to run continuously, but currently works if manually triggered to run on our bot, so can be useful for teammates that do not have the necessary hardware to measure latency themselves. BUG=708747 ==========
bsheedy@chromium.org changed reviewers: + leilei@chromium.org, thakis@chromium.org
+thakis for OWNERS +leilei for someone who's familiar with VR testing and swarming
Description was changed from ========== WIP Add automated VR latency tester Adds a script to automatically measure the motion-to-photon/app latency for VR. This requires specialized hardware to measure the latency and move the device during the test, so it is only meant to be run on our swarming bot that has been set up to work with it. Has not yet been set up to run continuously, but currently works if manually triggered to run on our bot, so can be useful for teammates that do not have the necessary hardware to measure latency themselves. BUG=708747 ========== to ========== Add automated VR latency tester Adds a script to automatically measure the motion-to-photon/app latency for VR. This requires specialized hardware to measure the latency and move the device during the test, so it is only meant to be run on our swarming bot that has been set up to work with it. Has not yet been set up to run continuously, but currently works if manually triggered to run on our bot, so can be useful for teammates that do not have the necessary hardware to measure latency themselves. BUG=708747 ==========
Description was changed from ========== Add automated VR latency tester Adds a script to automatically measure the motion-to-photon/app latency for VR. This requires specialized hardware to measure the latency and move the device during the test, so it is only meant to be run on our swarming bot that has been set up to work with it. Has not yet been set up to run continuously, but currently works if manually triggered to run on our bot, so can be useful for teammates that do not have the necessary hardware to measure latency themselves. BUG=708747 ========== to ========== Add automated VR latency tester Adds a script to automatically measure the motion-to-photon/app latency for VR. This requires specialized hardware to measure the latency and move the device during the test, so it is only meant to be run on our swarming bot that has been set up to work with it. Has not yet been set up to run continuously, but currently works if manually triggered to run on our bot, so can be useful for teammates that do not have the necessary hardware to measure latency themselves. BUG=708747 ==========
https://codereview.chromium.org/2799783002/diff/40001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2799783002/diff/40001/BUILD.gn#newcode701 BUILD.gn:701: "//chrome/test/vr/automated_motopho_latency:automated_motopho_latency_test", Why do you add automated_motopho_latency_test target to gn_all target? I think we can compile automated_motopho_latency_test directly. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... File chrome/test/vr/automated_motopho_latency/BUILD.gn (right): https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All rights reserved. How about constructing the folder as following: chrome test vr perf latency <all files about latency tests here> https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/BUILD.gn:5: group("automated_motopho_latency_test") { I prefer to remove automated prefix to short the name, since it is pretty clear this is not manual tests. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... File chrome/test/vr/automated_motopho_latency/run_latency_test.py (right): https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:10: a ShakerBot, which physically moves the test device and Motopho during the I am not sure if it is ok to mention shakerbot public, since it is our internal tool. Better to remove it. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:42: def run(self): s/run/Run to keep consistent about naming within file. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:48: print e.output It is better to use logging module for logs instead of printing it out to console. And you can also control the log level through command line. See run_tests_helper.SetLogLevel(args.verbose_count) https://cs.chromium.org/chromium/src/build/android/test_runner.py?q=test_runn... https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:80: for attempt in xrange(num_tries): nit: attempt is unused, you can add unused prefix for unused variable. unused_attempt https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:92: sys.exit(1) It is better to throw exception here instead of exiting directly. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:123: sys.exit(1) Considering to use parser.error('unrecognized arguments: %s' % ' '.join(unknown_args)) here. See test_runner.py. https://cs.chromium.org/chromium/src/build/android/test_runner.py?q=test_runn... https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:168: def RunCmdOrFail(cmd): Consider to rename it to RunCommand, no need for fail at the end. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:177: sys.exit(1) It is better to throw the original exception here. except ... logging.exception(...) throw e https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:189: 'shell', flag_string + '/data/local/tmp/chrome-command-line']) Why does it need to set the flags in two files? According to this instruction(https://www.chromium.org/developers/how-tos/run-chromium-with-flags), it only needs to update /data/local/tmp/chrome-command-line. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:246: print "Max correlation: " + str(motopho_thread.max_correlation) The latency and max correlation are also needed to be stored in a file, so it can be used to upload to chrome perf dashboard later. You can define --output_dir flag and update gn_isolate_map.pyl as following: "--output-dir=${ISOLATED_OUTDIR}", https://cs.chromium.org/chromium/src/testing/buildbot/gn_isolate_map.pyl?q=gn... ISOLATED_OUTDIR is reserved environment variable, it points to a local folder on swarming bot, but all the files under that folder will be uploaded to swarming and finally copied to the client.
https://codereview.chromium.org/2799783002/diff/40001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2799783002/diff/40001/BUILD.gn#newcode701 BUILD.gn:701: "//chrome/test/vr/automated_motopho_latency:automated_motopho_latency_test", On 2017/04/07 21:36:34, Lei Lei wrote: > Why do you add automated_motopho_latency_test target to gn_all target? > > I think we can compile automated_motopho_latency_test directly. From the top of the file: "So if you add a new build file, there must be some path of dependencies from this file to the new one or GN won't know about it." There might be a better way of meeting this requirement, but there needs to be some path from //BUILD.gn to //chrome/test/vr/automated_motopho_latency/BUILD.gn. I considered just making the test a part of //chrome/android's BUILD.gn, but since this should be applicable to desktop as well in the future, this seemed like a better approach. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... File chrome/test/vr/automated_motopho_latency/BUILD.gn (right): https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All rights reserved. On 2017/04/07 21:36:34, Lei Lei wrote: > How about constructing the folder as following: > chrome > test > vr > perf > latency > <all files about latency tests here> Done. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/BUILD.gn:5: group("automated_motopho_latency_test") { On 2017/04/07 21:36:35, Lei Lei wrote: > I prefer to remove automated prefix to short the name, since it is pretty clear > this is not manual tests. Done. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... File chrome/test/vr/automated_motopho_latency/run_latency_test.py (right): https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:10: a ShakerBot, which physically moves the test device and Motopho during the On 2017/04/07 21:36:35, Lei Lei wrote: > I am not sure if it is ok to mention shakerbot public, since it is our internal > tool. Better to remove it. Removed from the comment, and renamed the class to a very generic "RobotArm". https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:42: def run(self): On 2017/04/07 21:36:35, Lei Lei wrote: > s/run/Run to keep consistent about naming within file. It's overriding threading.Thread's run() function. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:48: print e.output On 2017/04/07 21:36:35, Lei Lei wrote: > It is better to use logging module for logs instead of printing it out to > console. > > And you can also control the log level through command line. See > run_tests_helper.SetLogLevel(args.verbose_count) > > https://cs.chromium.org/chromium/src/build/android/test_runner.py?q=test_runn... Done. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:80: for attempt in xrange(num_tries): On 2017/04/07 21:36:35, Lei Lei wrote: > nit: attempt is unused, you can add unused prefix for unused variable. > > unused_attempt Replaced with the more standard "_" for unused variables. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:92: sys.exit(1) On 2017/04/07 21:36:35, Lei Lei wrote: > It is better to throw exception here instead of exiting directly. Done. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:123: sys.exit(1) On 2017/04/07 21:36:35, Lei Lei wrote: > Considering to use parser.error('unrecognized arguments: %s' % ' > '.join(unknown_args)) here. > > See test_runner.py. > https://cs.chromium.org/chromium/src/build/android/test_runner.py?q=test_runn... Done. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:168: def RunCmdOrFail(cmd): On 2017/04/07 21:36:35, Lei Lei wrote: > Consider to rename it to RunCommand, no need for fail at the end. Done. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:177: sys.exit(1) On 2017/04/07 21:36:35, Lei Lei wrote: > It is better to throw the original exception here. > > except ... > logging.exception(...) > throw e Done. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:189: 'shell', flag_string + '/data/local/tmp/chrome-command-line']) On 2017/04/07 21:36:35, Lei Lei wrote: > Why does it need to set the flags in two files? According to this > instruction(https://www.chromium.org/developers/how-tos/run-chromium-with-flags), > it only needs to update /data/local/tmp/chrome-command-line. You're right, /data/local/chrome-command-line was kept around in some places for backwards compatibility since it was used before M33, but that's not necessary here. https://codereview.chromium.org/2799783002/diff/40001/chrome/test/vr/automate... chrome/test/vr/automated_motopho_latency/run_latency_test.py:246: print "Max correlation: " + str(motopho_thread.max_correlation) On 2017/04/07 21:36:35, Lei Lei wrote: > The latency and max correlation are also needed to be stored in a file, so it > can be used to upload to chrome perf dashboard later. > > You can define --output_dir flag and update gn_isolate_map.pyl as following: > "--output-dir=${ISOLATED_OUTDIR}", > > https://cs.chromium.org/chromium/src/testing/buildbot/gn_isolate_map.pyl?q=gn... > > > ISOLATED_OUTDIR is reserved environment variable, it points to a local folder on > swarming bot, but all the files under that folder will be uploaded to swarming > and finally copied to the client. Done.
chrome/ kind of feels like the wrong place for this. kbr, does this type of thing fit with the telemetry tests? https://codereview.chromium.org/2799783002/diff/60001/chrome/test/vr/perf/lat... File chrome/test/vr/perf/latency/run_latency_test.py (right): https://codereview.chromium.org/2799783002/diff/60001/chrome/test/vr/perf/lat... chrome/test/vr/perf/latency/run_latency_test.py:33: DEFAULT_MOTOPHO_PATH = '/home/gtvchrome/motopho/Motopho' This will be correct approximately nowhere. Either make this relative to os.path.expanduser('~') to make this a bit less bad, or if the tools isn't too large, just DEPS it in somewhere and depend on it being available there. https://codereview.chromium.org/2799783002/diff/60001/chrome/test/vr/perf/lat... chrome/test/vr/perf/latency/run_latency_test.py:145: # Taken from Daydream's perf test suite Do you have permissions to open-source that code?
https://codereview.chromium.org/2799783002/diff/60001/chrome/test/vr/perf/lat... File chrome/test/vr/perf/latency/run_latency_test.py (right): https://codereview.chromium.org/2799783002/diff/60001/chrome/test/vr/perf/lat... chrome/test/vr/perf/latency/run_latency_test.py:33: DEFAULT_MOTOPHO_PATH = '/home/gtvchrome/motopho/Motopho' On 2017/04/10 18:41:28, Nico wrote: > This will be correct approximately nowhere. Either make this relative to > os.path.expanduser('~') to make this a bit less bad, or if the tools isn't too > large, just DEPS it in somewhere and depend on it being available there. Done via first approach. Also added a TODO about maybe adding it in via DEPS in the future. https://codereview.chromium.org/2799783002/diff/60001/chrome/test/vr/perf/lat... chrome/test/vr/perf/latency/run_latency_test.py:145: # Taken from Daydream's perf test suite On 2017/04/10 18:41:28, Nico wrote: > Do you have permissions to open-source that code? I checked with the Daydream team, and it turns out it actually originated from google3. However, I checked with the internal open source licensing group, and they said it was fine to commit it here without doing anything since all it's doing is checking for certain strings in some files.
On 2017/04/10 18:41:28, Nico wrote: > chrome/ kind of feels like the wrong place for this. > > kbr, does this type of thing fit with the telemetry tests? The code could be hosted under src/content/test/gpu/ (maybe in a new subdirectory) if that would be a better place for it. This doesn't look directly related to the existing Telemetry based GPU tests though. Also, it looks like this test depends on all of Chrome, and not just the stuff in content/, so putting it under content/ may be wrong from a layering standpoint.
lgtm once leilei is happy. I didn't look at the main python script much, I'm assuming leilei will do that.
lgtm https://codereview.chromium.org/2799783002/diff/80001/chrome/test/vr/perf/lat... File chrome/test/vr/perf/latency/run_latency_test.py (right): https://codereview.chromium.org/2799783002/diff/80001/chrome/test/vr/perf/lat... chrome/test/vr/perf/latency/run_latency_test.py:46: def run(self): s/run/Run to keep consistent within file. https://codereview.chromium.org/2799783002/diff/80001/chrome/test/vr/perf/lat... chrome/test/vr/perf/latency/run_latency_test.py:57: motopho_output) How about raising exception here instead of returning 1? https://codereview.chromium.org/2799783002/diff/80001/chrome/test/vr/perf/lat... chrome/test/vr/perf/latency/run_latency_test.py:241: 'tap', '800', '800']) indent here.
https://codereview.chromium.org/2799783002/diff/80001/chrome/test/vr/perf/lat... File chrome/test/vr/perf/latency/run_latency_test.py (right): https://codereview.chromium.org/2799783002/diff/80001/chrome/test/vr/perf/lat... chrome/test/vr/perf/latency/run_latency_test.py:46: def run(self): On 2017/04/12 22:10:53, Lei Lei wrote: > s/run/Run to keep consistent within file. It's overriding threading.Thread's run() function, so it needs to be lower case. https://codereview.chromium.org/2799783002/diff/80001/chrome/test/vr/perf/lat... chrome/test/vr/perf/latency/run_latency_test.py:57: motopho_output) On 2017/04/12 22:10:53, Lei Lei wrote: > How about raising exception here instead of returning 1? Done. https://codereview.chromium.org/2799783002/diff/80001/chrome/test/vr/perf/lat... chrome/test/vr/perf/latency/run_latency_test.py:241: 'tap', '800', '800']) On 2017/04/12 22:10:53, Lei Lei wrote: > indent here. Done.
The CQ bit was checked by bsheedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, leilei@chromium.org Link to the patchset: https://codereview.chromium.org/2799783002/#ps100001 (title: "Rebase + address final comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492037524541490, "parent_rev": "c3807f86ebdb6b8e00e49b47014fb8a3b3c72499", "commit_rev": "84541d55d66a59f8b44afaad69882beb10de265b"}
Message was sent while issue was closed.
Description was changed from ========== Add automated VR latency tester Adds a script to automatically measure the motion-to-photon/app latency for VR. This requires specialized hardware to measure the latency and move the device during the test, so it is only meant to be run on our swarming bot that has been set up to work with it. Has not yet been set up to run continuously, but currently works if manually triggered to run on our bot, so can be useful for teammates that do not have the necessary hardware to measure latency themselves. BUG=708747 ========== to ========== Add automated VR latency tester Adds a script to automatically measure the motion-to-photon/app latency for VR. This requires specialized hardware to measure the latency and move the device during the test, so it is only meant to be run on our swarming bot that has been set up to work with it. Has not yet been set up to run continuously, but currently works if manually triggered to run on our bot, so can be useful for teammates that do not have the necessary hardware to measure latency themselves. BUG=708747 Review-Url: https://codereview.chromium.org/2799783002 Cr-Commit-Position: refs/heads/master@{#464230} Committed: https://chromium.googlesource.com/chromium/src/+/84541d55d66a59f8b44afaad6988... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/84541d55d66a59f8b44afaad6988... |