Closed
Bug 1323328
Opened 7 years ago
Closed 7 years ago
[webvr] Implement Cross-Browser compatible tool for emulating VR hardware
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: kip, Assigned: daoshengmu)
References
(Blocks 1 open bug)
Details
Attachments
(6 files, 3 obsolete files)
73.43 KB,
image/jpeg
|
Details | |
59 bytes,
text/x-review-board-request
|
kip
:
review+
baku
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kip
:
review+
|
Details |
2.86 KB,
text/html
|
Details | |
59 bytes,
text/x-review-board-request
|
kip
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kip
:
review+
|
Details |
In order to support cross-browser WebVR conformance tests, a tool will be needed that can emulate VR hardware. Our current idea is to build this as an OpenVR driver that responds to commands from either a WebSocket or a script file to play back a set of events with precise timing. The script file format or WebSocket protocol should be discussed in the WebVR implementer meetings and WebVR spec discussions to ensure that it can be implemented by other WebVR browser vendors that can not feasibly use our OpenVR simulation driver. More details are in the meta-bug, Bug 1323327.
Assignee | ||
Comment 1•7 years ago
|
||
After some study on Bug 1229480 and openvr_api_public.cpp, I notice that openvr_api_public.cpp just release some parts of the runtime source code. For example, GetTrackedDeviceIndexForControllerRole() function does not have its definition. So, if we wanna integrate openvr_api_public.cpp with our tests at the back-end, we have to make our own dll file to implement these functions. Probably, I think it would be more wise to make a Puppet test service because it is very simple, and we should not happen any fail when using the runtime from VR device vendors. We can choose Puppet test service to be our automatic test tool and test real device runtime manually at our first step. In the future, if we still need to test the code at gfxVROpenVR and gfxVROculus, we have to think about making our own openvr_api.dll and LibOVRRT.dll. In Puppet test service, it can help us confirm the workflow from DOM to Compositor modules. But if there is any fail at gfxVROpenVR or gfxVROculus, we would have no solution for automatic testing. Kip, how do you feel about this idea? Do you think we should only make a Puppet test service or we have to make our own customized VR runtime to support more detailed tests.
Flags: needinfo?(kgilbert)
Reporter | ||
Comment 2•7 years ago
|
||
(In reply to Daosheng Mu[:daoshengmu] from comment #1) > After some study on Bug 1229480 and openvr_api_public.cpp, I notice that > openvr_api_public.cpp just release some parts of the runtime source code. > For example, GetTrackedDeviceIndexForControllerRole() function does not have > its definition. So, if we wanna integrate openvr_api_public.cpp with our > tests at the back-end, we have to make our own dll file to implement these > functions. > > Probably, I think it would be more wise to make a Puppet test service > because it is very simple, and we should not happen any fail when using the > runtime from VR device vendors. We can choose Puppet test service to be our > automatic test tool and test real device runtime manually at our first step. > In the future, if we still need to test the code at gfxVROpenVR and > gfxVROculus, we have to think about making our own openvr_api.dll and > LibOVRRT.dll. > > In Puppet test service, it can help us confirm the workflow from DOM to > Compositor modules. But if there is any fail at gfxVROpenVR or gfxVROculus, > we would have no solution for automatic testing. > > Kip, how do you feel about this idea? Do you think we should only make a > Puppet test service or we have to make our own customized VR runtime to > support more detailed tests. I agree, we could just add a new subclass of VRDisplayHost that runs through a script rather than connecting to real hardware. OpenVR may not be implemented by all other browser vendors, so we should focus on implementing our own solution first. The interoperability in the test suite should be maintained at the DOM API and puppet device script format rather than the implementation at the C++ level.
Flags: needinfo?(kgilbert)
Reporter | ||
Comment 3•7 years ago
|
||
I have added a patch to Bug 1229480 that implements such a Puppet VR device. This is WIP, and simply allows a dummy device to be enumerated. To complete the cross-browser compatible toolchain for testing, we will need to define a cross-browser compatible DOM api for controlling the puppet VR device and an interoperable format to describe the events and their timing for playback. I'd suggest leading the way by beginning implementation of a few simple conformance tests and share the format with the other vendors in an implementers meeting for feedback.
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Comment 5•7 years ago
|
||
In this flow, I wanna describe we can make a cross-platform test suite based on the WebVR test service Web API. WebVR test service helps us push test data to VRSystemPuppet at the backend, and we can confirm if our WebVR API does work at DOM API with VRDisplay.
Attachment #8830172 -
Attachment is obsolete: true
Assignee | ||
Comment 6•7 years ago
|
||
I am thinking VRServiceTest can have VRFakeDisplay and VRFakeController that are used for managing to set VRDisaply and VRController test data separately.
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8830179 -
Attachment is obsolete: true
Assignee | ||
Comment 8•7 years ago
|
||
Kip,
Here is my first draft for the WebVR test suite. Please refer Attachment 8836709 [details], give me some feedback.
In this patch, I make a VRServiceTest DOM API that can be created from Navigator, and it can help us create VRFakeContoller and VRFakeDisplay for inserting test data. At our VR manager backend, when it receives creating displays or controllers, it will generate VRDisplay/VRControllers in VRPuppetDevice. After we have done the test data insertion, we can verify the WebVR API whether is right via VRDisaply and Gamepad API.
Attachment #8836712 -
Flags: feedback?(kgilbert)
Reporter | ||
Comment 9•7 years ago
|
||
Comment on attachment 8836712 [details] [diff] [review] (WIP) prototype-for-VRServiceTest.patch I think you are going in the right direction here. It would be nice to have a timeline scripted interface for setting poses and firing events in a time-critical manner; however, this could be added on top of what you have made here.
Attachment #8836712 -
Flags: feedback?(kgilbert) → feedback+
Reporter | ||
Comment 10•7 years ago
|
||
I also recommend following this github issue on the WebVR spec: https://github.com/w3c/webvr/issues/187 We are just beginning the conversations on what the API would look like to make the WebVR tests inter-operable between browsers.
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to :kip (Kearwood Gilbert) from comment #9) > Comment on attachment 8836712 [details] [diff] [review] > (WIP) prototype-for-VRServiceTest.patch > > I think you are going in the right direction here. > > It would be nice to have a timeline scripted interface for setting poses and > firing events in a time-critical manner; however, this could be added on top > of what you have made here. I would like to add a follow-up bug for timeline-based interface support when our VR conformance tests have this requirement.
Assignee | ||
Comment 12•7 years ago
|
||
Regarding to the cross-Browser compatible tool, we can separate it to three parts, first, we have to implement the VR mock device at our backend as Bug 1229480. Then, we need to give it the ability to insert test data into this mock device via webidl in this bug. Besides, because we hope our conformance tests can be shared by other browser vendors. We have to make this suite and its tests can be run as Web-Platform-Tests and can be integrated with Mochitest as well like attachment 8836709 [details] and Bug 1229479 mentioned.
Assignee | ||
Updated•7 years ago
|
Attachment #8836712 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
WebVR test API example.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8839498 [details] Bug 1323328 - Part 1: Implement VRServiceTest for helping insert VR test data; https://reviewboard.mozilla.org/r/114114/#review116594 This is looking great, thanks! The VR parts are good. Please also get a review from a DOM peer for the .webidl and events.
Attachment #8839498 -
Flags: review?(kgilbert) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•7 years ago
|
||
mozreview-review |
Comment on attachment 8839498 [details] Bug 1323328 - Part 1: Implement VRServiceTest for helping insert VR test data; https://reviewboard.mozilla.org/r/114114/#review117946 ::: dom/vr/VRServiceTest.h:22 (Diff revision 4) > +{ > +public: > + NS_DECL_ISUPPORTS_INHERITED > + NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(VRMockDisplay, DOMEventTargetHelper) > + > + explicit VRMockDisplay(const nsCString& aID, uint32_t aDeviceID); No explicit if more than 1 argument. ::: dom/vr/VRServiceTest.h:34 (Diff revision 4) > + const Nullable<Float32Array>& aAngularVelocity, const Nullable<Float32Array>& aAngularAcceleration); > + void Update(); > + virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override; > + > +private: > + ~VRMockDisplay() {} ~VRMockDisplay() = default; ::: dom/vr/VRServiceTest.h:47 (Diff revision 4) > +{ > +public: > + NS_DECL_ISUPPORTS_INHERITED > + NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(VRMockController, DOMEventTargetHelper) > + > + explicit VRMockController(const nsCString& aID, uint32_t aDeviceID); no explicit ::: dom/vr/VRServiceTest.h:56 (Diff revision 4) > + const Nullable<Float32Array>& aLinearAcceleration, const Nullable<Float32Array>& aOrientation, > + const Nullable<Float32Array>& aAngularVelocity, const Nullable<Float32Array>& aAngularAcceleration); > + virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override; > + > +private: > + ~VRMockController() {} = default ::: dom/vr/VRServiceTest.h:73 (Diff revision 4) > + already_AddRefed<Promise> AttachVRDisplay(const nsAString& aID, ErrorResult& aRv); > + already_AddRefed<Promise> AttachVRController(const nsAString& aID, ErrorResult& aRv); > + void Shutdown(); > + > + static already_AddRefed<VRServiceTest> CreateTestService(nsPIDOMWindowInner* aWindow); > + nsPIDOMWindowInner* GetParentObject() const { return mWindow; } no GetParentObject(). DOMEventTargetHepler already implements it for you. ::: dom/vr/VRServiceTest.cpp:318 (Diff revision 4) > + } > + > + nsCOMPtr<nsIGlobalObject> go = do_QueryInterface(mWindow); > + > + RefPtr<Promise> p = Promise::Create(go, aRv); > + if (aRv.Failed()) { NS_WARN_IF ::: dom/vr/VRServiceTest.cpp:339 (Diff revision 4) > + } > + > + nsCOMPtr<nsIGlobalObject> go = do_QueryInterface(mWindow); > + > + RefPtr<Promise> p = Promise::Create(go, aRv); > + if (aRv.Failed()) { NS_WARN_IF ::: dom/webidl/VRServiceTest.webidl:4 (Diff revision 4) > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + Add a comment saying that this is just for testing. ::: modules/libpref/init/all.js:5050 (Diff revision 4) > pref("gfx.vr.osvr.commonLibPath", ""); > pref("gfx.vr.osvr.clientLibPath", ""); > pref("gfx.vr.osvr.clientKitLibPath", ""); > - > +// Puppet device, used for simulating VR hardware within tests and dev tools > +#ifdef RELEASE_OR_BETA > +pref("dom.vr.puppet.enabled", false); This seems completely unrelated. remove it from this patch. Probably you want to enable it in tests... ? Or tell me more.
Attachment #8839498 -
Flags: review?(amarchesini) → review+
Reporter | ||
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8839499 [details] Bug 1323328 - Part 2: Implement VRServiceTest backend at VRManager; https://reviewboard.mozilla.org/r/114116/#review117948 With the nit on wrapping fixed, r=me. Please get an additional review from an IPC peer if the messages need to be sync. If we can make them async (and work around this in our tests), then r=me on that change. ::: gfx/vr/ipc/PVRManager.ipdl:65 (Diff revision 4) > async ControllerListenerAdded(); > async ControllerListenerRemoved(); > // GetControllers synchronously returns the VR controllers that have already been > // enumerated by RefreshVRControllers() but does not enumerate new ones. > sync GetControllers() returns(VRControllerInfo[] aControllerInfo); > + sync CreateVRServiceTestDisplay(nsCString aID, uint32_t aPromiseID); Could we make these sync messages async? "new sync messages must be recorded in ipc/ipdl/sync-messages.ini and reviewed by an IPC peer" If they need to be sync, then please be sure to get an additional review from an IPC peer. ::: gfx/vr/ipc/VRManagerChild.h:122 (Diff revision 4) > virtual mozilla::ipc::IPCResult RecvParentAsyncMessages(InfallibleTArray<AsyncParentMessageData>&& aMessages) override; > > virtual mozilla::ipc::IPCResult RecvNotifyVSync() override; > virtual mozilla::ipc::IPCResult RecvNotifyVRVSync(const uint32_t& aDisplayID) override; > virtual mozilla::ipc::IPCResult RecvGamepadUpdate(const GamepadChangeEvent& aGamepadEvent) override; > + virtual mozilla::ipc::IPCResult RecvReplyCreateVRServiceTestDisplay(const nsCString& aID, const uint32_t& aPromiseID, const uint32_t& aDeviceID) override; nit: should wrap to 80 cols
Attachment #8839499 -
Flags: review?(kgilbert) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 31•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8839498 [details] Bug 1323328 - Part 1: Implement VRServiceTest for helping insert VR test data; https://reviewboard.mozilla.org/r/114114/#review117946 > This seems completely unrelated. remove it from this patch. Probably you want to enable it in tests... ? > Or tell me more. Yep. I should put it at the 3rd patch to enable it in tests. I have removed it. Thanks.
Assignee | ||
Comment 32•7 years ago
|
||
mozreview-review |
Comment on attachment 8843355 [details] Bug 1323328 - Part 4: Delay create VRSystemManagerPuppet for making it only be run for tests; https://reviewboard.mozilla.org/r/117116/#review118730 ::: gfx/vr/VRManager.cpp (Diff revision 1) > } > #endif > - mgr = VRSystemManagerPuppet::Create(); > - if (mgr) { > - mManagers.AppendElement(mgr); > - } We should not enable VRSystemPuppet here because it is only needed in our tests. ::: gfx/vr/gfxVRPuppet.cpp (Diff revision 1) > > /*static*/ already_AddRefed<VRSystemManagerPuppet> > VRSystemManagerPuppet::Create() > { > - MOZ_ASSERT(NS_IsMainThread()); > - In our mochitest, it is called at the compositor thread so I have to remove this check.
Assignee | ||
Comment 33•7 years ago
|
||
In this mechanism, we can integrate WPT that is from the W3C repo with our mochitest system just need to make a converter. I prefer to make the converter later when we have enough test cases.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 38•7 years ago
|
||
(In reply to :kip (Kearwood Gilbert) from comment #26) > Comment on attachment 8839499 [details] > Bug 1323328 - Part 2: Implement VRServiceTest backend at VRManager; > > https://reviewboard.mozilla.org/r/114116/#review117948 > > With the nit on wrapping fixed, r=me. > > Please get an additional review from an IPC peer if the messages need to be > sync. If we can make them async (and work around this in our tests), then > r=me on that change. > > ::: gfx/vr/ipc/PVRManager.ipdl:65 > (Diff revision 4) > > async ControllerListenerAdded(); > > async ControllerListenerRemoved(); > > // GetControllers synchronously returns the VR controllers that have already been > > // enumerated by RefreshVRControllers() but does not enumerate new ones. > > sync GetControllers() returns(VRControllerInfo[] aControllerInfo); > > + sync CreateVRServiceTestDisplay(nsCString aID, uint32_t aPromiseID); > > Could we make these sync messages async? > > "new sync messages must be recorded in ipc/ipdl/sync-messages.ini and > reviewed by > an IPC peer" > > If they need to be sync, then please be sure to get an additional review > from an IPC peer. > > ::: gfx/vr/ipc/VRManagerChild.h:122 > (Diff revision 4) > > virtual mozilla::ipc::IPCResult RecvParentAsyncMessages(InfallibleTArray<AsyncParentMessageData>&& aMessages) override; > > > > virtual mozilla::ipc::IPCResult RecvNotifyVSync() override; > > virtual mozilla::ipc::IPCResult RecvNotifyVRVSync(const uint32_t& aDisplayID) override; > > virtual mozilla::ipc::IPCResult RecvGamepadUpdate(const GamepadChangeEvent& aGamepadEvent) override; > > + virtual mozilla::ipc::IPCResult RecvReplyCreateVRServiceTestDisplay(const nsCString& aID, const uint32_t& aPromiseID, const uint32_t& aDeviceID) override; > > nit: should wrap to 80 cols I have confirmed we just need to use asyc ipdl channels to help us.
Reporter | ||
Comment 39•7 years ago
|
||
mozreview-review |
Comment on attachment 8842510 [details] Bug 1323328 - Part 3: Add VR display request present mochitest; https://reviewboard.mozilla.org/r/116318/#review118838 This is an excellent start to our VR mochitests! Thanks for getting this in. r=me with the small whitespace formatting nit Be sure to push this to try and see if it passes on our automated test servers. ::: dom/vr/test/runVRTest.js:3 (Diff revision 4) > +function runVRTest(callback) { > + SpecialPowers.pushPrefEnv({"set" : [["dom.vr.enabled", true], > + ["dom.vr.puppet.enabled", true], nit: whitespace formatting pushing text far to the right.. Maybe a tab-to-space conversion issue?
Attachment #8842510 -
Flags: review?(kgilbert) → review+
Reporter | ||
Comment 40•7 years ago
|
||
mozreview-review |
Comment on attachment 8843355 [details] Bug 1323328 - Part 4: Delay create VRSystemManagerPuppet for making it only be run for tests; https://reviewboard.mozilla.org/r/117116/#review118844 r=me if you either confirm that a VRSystemManageRPuppet won't be reated without a display or add a check against mManagers for a pre-existing VRSystemManagerPuppet. ::: dom/vr/VRServiceTest.cpp:297 (Diff revision 2) > VRServiceTest::VRServiceTest(nsPIDOMWindowInner* aWindow) > : mWindow(aWindow), > mShuttingDown(false) > -{} > +{ > + gfx::VRManagerChild* vm = gfx::VRManagerChild::Get(); > + MOZ_ASSERT(vm); nit: gfx::VRManagerChild::Get() already has a MOZ_ASSERT(), so shouldn't need another here. ::: gfx/vr/VRManager.cpp:394 (Diff revision 2) > +VRManager::CreateVRTestSystem() > +{ > + nsTArray<VRDisplayInfo> displays; > + GetVRDisplayInfo(displays); > + > + for (auto& display : displays) { Is it possible to create a VRSystemManagerPuppet without adding a display? If so, we should probably check mManagers to avoid inserting two VRSystemManagerPuppet's.
Attachment #8843355 -
Flags: review?(kgilbert) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 43•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8843355 [details] Bug 1323328 - Part 4: Delay create VRSystemManagerPuppet for making it only be run for tests; https://reviewboard.mozilla.org/r/117116/#review118844 > Is it possible to create a VRSystemManagerPuppet without adding a display? > > If so, we should probably check mManagers to avoid inserting two VRSystemManagerPuppet's. Good suggestion! I decide to use a bool for helping check if the VRSystemManagerPuppet has already been created to avoid risk.
Assignee | ||
Comment 44•7 years ago
|
||
Try looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=d37f9eb94ae8214cfbd3d7c07826df28f86bde38.
Comment 45•7 years ago
|
||
Pushed by dmu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/41193207cd69 Part 1: Implement VRServiceTest for helping insert VR test data; r=baku,kip https://hg.mozilla.org/integration/autoland/rev/39365e72a113 Part 2: Implement VRServiceTest backend at VRManager; r=kip https://hg.mozilla.org/integration/autoland/rev/659795097ae3 Part 3: Add VR display request present mochitest; r=kip https://hg.mozilla.org/integration/autoland/rev/473acbe8453d Part 4: Delay create VRSystemManagerPuppet for making it only be run for tests; r=kip
Comment 46•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/41193207cd69 https://hg.mozilla.org/mozilla-central/rev/39365e72a113 https://hg.mozilla.org/mozilla-central/rev/659795097ae3 https://hg.mozilla.org/mozilla-central/rev/473acbe8453d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•