[webvr] Implement Cross-Browser compatible tool for emulating VR hardware

RESOLVED FIXED in Firefox 54

Status

()

Core
Graphics
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: kip, Assigned: daoshengmu)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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

2 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

2 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

2 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

2 years ago
Created attachment 8830172 [details]
WebVR test suit flow.jpg
(Assignee)

Comment 5

2 years ago
Created attachment 8830179 [details]
WebVR test suite flow.jpg

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

2 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

a year ago
Created attachment 8836709 [details]
WebVR test suite flow.jpg
Attachment #8830179 - Attachment is obsolete: true
(Assignee)

Comment 8

a year ago
Created attachment 8836712 [details] [diff] [review]
(WIP) prototype-for-VRServiceTest.patch

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

a year 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

a year 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)

Updated

a year ago
See Also: → bug 1229480
(Assignee)

Comment 11

a year 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

a year 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

a year ago
Attachment #8836712 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

a year ago
Created attachment 8839502 [details]
webvrtest.html (v1)

WebVR test API example.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 18

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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.

Comment 45

a year 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

a year 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
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1353877
Depends on: 1363037
You need to log in before you can comment on or make changes to this bug.