1186578 - [webvr] add support for OpenVR/HTC VIVE

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Add support for OpenVR/SteamVR devices.

Comment 1

[:kip (Kearwood Gilbert)]

As you are busy with the VSync work, would you like me to take this one?

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: OpenVR/SteamVR support (patch needs update)

I've actually had this mostly done for a while :( It just took a back burner to a bunch of other stuff.. it also really needs the vsync bits.  But want to update it and let's get it checked in regardless so it doesn't bitrot further?

Comment 3

[:kip (Kearwood Gilbert)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #2)
> Created attachment 8709178 [details] [diff] [review]
> OpenVR/SteamVR support (patch needs update)
> 
> I've actually had this mostly done for a while :( It just took a back burner
> to a bunch of other stuff.. it also really needs the vsync bits.  But want
> to update it and let's get it checked in regardless so it doesn't bitrot
> further?

I agree about checking in, even if more work is to be done.  Should we put it behind a pref?

Comment 4

[:kip (Kearwood Gilbert)]

To free you up for the vsync work, I'd be glad to un-bitrot your patch since I made the changes that bit-rotted it :-)

Would you like me to take this on?

Comment 5

[:kip (Kearwood Gilbert)]

I have pulled down the patch and it looks easy to update.  I'll get it updated, thanks Vlad!

Comment 6

[Diego Marcos [:dmarcos]]

Is there any ETA for this?

Comment 7

[:kip (Kearwood Gilbert)]

I have just returned from GDC and now have Vive hardware to test against.  There are some other priorities currently (WebVR 1.0 API, some CSS Transform Fixes); however, I will find some time over the next week to review these patches to see how far they need to go and land them (disabled by default if needed) if that prevents further bitrotting.

Comment 8

[:kip (Kearwood Gilbert)]

NI'ing myself as a reminder to file a licensing bug for the Valve BSD license addition.

Comment 9

[:kip (Kearwood Gilbert)]

While updating these patches, I'm rebasing them onto the WIP patches for WebVR 1.0 support in Bug 1250244.

Comment 10

[:kip (Kearwood Gilbert)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=262e158170ea

Comment 11

[:kip (Kearwood Gilbert)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d9a68fa362e

Comment 12

[:kip (Kearwood Gilbert)]

Attachment: Bug 1186578 - [webvr] Implement OpenVR/SteamVR support

Please apply the patchset in Bug 1250244 first before this patch.

There is a crash that occurs when accessing VRStageParameters.sittingToStandingTransform; otherwise, HTC Vive support is working.

HTC Tracked controller support will be landing separately.

Once I've fixed the crash with sittingToStandingTransform and Bug 1250244 is landed, I'll upload the patch to MozReview and assign out for review and landing.

Comment 13

[:kip (Kearwood Gilbert)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46922dae4aa3

Comment 14

[:kip (Kearwood Gilbert)]

Attachment: Bug 1186578 - [webvr] Implement OpenVR/SteamVR support

I have rebased this patch on top of Bug 1295951, which fixes a crash when accessing VRDisplay.stageParameters from within requestAnimationFrame callbacks.

Comment 15

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8782646 [details] [diff] [review]
Bug 1186578 - [webvr] Implement OpenVR/SteamVR support

Review of attachment 8782646 [details] [diff] [review]:
-----------------------------------------------------------------

No real concerns with this, just some minor comments.

::: gfx/thebes/gfxPrefs.h
@@ +308,5 @@
>    DECL_GFX_PREF(Live, "dom.ipc.plugins.asyncdrawing.enabled",  PluginAsyncDrawingEnabled, bool, false);
>    DECL_GFX_PREF(Live, "dom.meta-viewport.enabled",             MetaViewportEnabled, bool, false);
>    DECL_GFX_PREF(Once, "dom.vr.enabled",                        VREnabled, bool, false);
>    DECL_GFX_PREF(Once, "dom.vr.oculus.enabled",                 VROculusEnabled, bool, true);
> +  DECL_GFX_PREF(Once, "dom.vr.openvr.enabled",                  VROpenVREnabled, bool, false);

Nit: alignment is off

::: gfx/vr/VRManager.cpp
@@ +76,5 @@
>  
> +  mgr = VRDisplayManagerOpenVR::Create();
> +  if (mgr) {
> +    mManagers.AppendElement(mgr);
> +  }

Does this need to be in an ifdef like OSVR's? This doesn't work on Android, right?

::: gfx/vr/gfxVR.h
@@ +73,5 @@
> +  Cap_StageParameters = 1 << 7,
> +  /**
> +  * Cap_StageParameters is set if the VRDisplay is capable of room scale VR
> +  * and can report the StageParameters to describe the space.
> +  */

The descriptions here seem to be in the wrong order

::: gfx/vr/gfxVROpenVR.cpp
@@ +15,5 @@
> +#include "mozilla/gfx/Quaternion.h"
> +
> +#ifdef XP_WIN
> +#include "../layers/d3d11/CompositorD3D11.h"
> +#include "../layers/d3d11/TextureD3D11.h"

Should we add layers/d3d11 to moz.build's include path?

@@ +24,5 @@
> +#else
> +#pragma comment(lib, "c:/proj/openvr/lib/win32/openvr_api.lib")
> +#endif
> +#endif
> +#endif

Can you add // XP_WIN here for readability

@@ +74,5 @@
> +    return false;
> +
> +#define REQUIRE_FUNCTION(_x) do {                                       \
> +    *(void **)&vr_##_x = (void *) PR_FindSymbol(openvrLib, "VR_" #_x);  \
> +    if (!vr_##_x) { printf_stderr("VR_" #_x " symbol missing\n"); goto fail; } \

can we just return false here instead of having a goto

@@ +111,5 @@
> +  mDisplayInfo.mCapabilityFlags |= VRDisplayCapabilityFlags::Cap_Orientation;
> +  mDisplayInfo.mCapabilityFlags |= VRDisplayCapabilityFlags::Cap_Position;
> +  mDisplayInfo.mCapabilityFlags |= VRDisplayCapabilityFlags::Cap_External;
> +  mDisplayInfo.mCapabilityFlags |= VRDisplayCapabilityFlags::Cap_Present;
> +  mDisplayInfo.mCapabilityFlags |= VRDisplayCapabilityFlags::Cap_StageParameters;

nit: just | all the items on a single assignment instead of multiple |=

@@ +236,5 @@
> +    while (mVRSystem->PollNextEvent(&event, sizeof(event))) {
> +      // ignore
> +    }
> +  }
> +  

nit: trailing spaces

@@ +319,5 @@
> +  if (!mIsPresenting) {
> +    return;
> +  }
> +
> +  //virtual EVRCompositorError Submit(EVREye eEye, const Texture_t *pTexture, const VRTextureBounds_t* pBounds = 0, EVRSubmitFlags nSubmitFlags = Submit_Default) = 0;

should this be here?

@@ +361,5 @@
> +
> +void
> +VRDisplayOpenVR::NotifyVSync()
> +{
> +  mDisplayInfo.mIsConnected = vr_IsHmdPresent();

I'm not sure what this has to do with VSync?

@@ +397,5 @@
> +  if (!vr_IsRuntimeInstalled())
> +    return false;
> +
> +  mOpenVRInitialized = true;
> +  return true;

This method doesn't appear to initialise anything. If this is deliberate (I'm assuming in the future it may do something more involved?) can you add a comment here explaining why. Otherwise it may just be worth calling it "mOpenVRInstalled" as that's what it's being used for.

::: gfx/vr/openvr/LICENSE
@@ +23,5 @@
> +(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> +LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> +ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> +SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

I don't know if you're going to need approval from someone else to import this code.

Comment 16

[:kip (Kearwood Gilbert)]

I have added a Bug 1297532 for updating about:license to include the Valve BSD license.

Comment 17

[:kip (Kearwood Gilbert)]

Attachment: Bug 1186578 - [webvr] Implement OpenVR/SteamVR support

I have updated the patch based on the feedback in Comment 15.

Comment 18

[:kip (Kearwood Gilbert)]

Comment on attachment 8784157 [details] [diff] [review]
Bug 1186578 - [webvr] Implement OpenVR/SteamVR support

The log for this file shows that :gerv has done most of the reviews here lately.  Would you be able to review this change? Thanks!

Comment 19

[:kip (Kearwood Gilbert)]

Comment on attachment 8784157 [details] [diff] [review]
Bug 1186578 - [webvr] Implement OpenVR/SteamVR support

Sorry, was wrong bug!  Unassigned :gerv..

Comment 20

[:kip (Kearwood Gilbert)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=989209f01d3e

Comment 21

[:kip (Kearwood Gilbert)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b2a897690b39803e005a1c90d477623e7585b3b
Bug 1186578 - [webvr] Implement OpenVR/SteamVR support,r=gw280

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2b2a897690b3