[webvr] add support for OpenVR/HTC VIVE

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: vlad, Assigned: kip)

Tracking

Trunk
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected, firefox51 fixed)

Details

(Whiteboard: [webvr][vrm2])

Attachments

(1 attachment, 3 obsolete attachments)

Add support for OpenVR/SteamVR devices.
Assignee: nobody → vladimir
Whiteboard: [webvr] → [webvr][vrm2]
As you are busy with the VSync work, would you like me to take this one?
Flags: needinfo?(vladimir)
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?
Flags: needinfo?(vladimir)
(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?
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?
I have pulled down the patch and it looks easy to update.  I'll get it updated, thanks Vlad!
Assignee: vladimir → kgilbert
Is there any ETA for this?
Flags: needinfo?(kgilbert)
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.
Flags: needinfo?(kgilbert)
NI'ing myself as a reminder to file a licensing bug for the Valve BSD license addition.
Flags: needinfo?(kgilbert)
While updating these patches, I'm rebasing them onto the WIP patches for WebVR 1.0 support in Bug 1250244.
Depends on: 1250244
Flags: needinfo?(kgilbert)
Flags: needinfo?(kgilbert)
Summary: [webvr] add support for OpenVR → [webvr] add support for OpenVR/HTC VIVE
Blocks: 1260563
Depends on: 1287545
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.
Attachment #8709178 - Attachment is obsolete: true
Flags: needinfo?(kgilbert)
Depends on: 1295951
I have rebased this patch on top of Bug 1295951, which fixes a crash when accessing VRDisplay.stageParameters from within requestAnimationFrame callbacks.
Attachment #8779885 - Attachment is obsolete: true
Attachment #8782646 - Flags: review?(gwright)
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.
Attachment #8782646 - Flags: review?(gwright) → review+
Depends on: 1297532
I have added a Bug 1297532 for updating about:license to include the Valve BSD license.
I have updated the patch based on the feedback in Comment 15.
Attachment #8782646 - Attachment is obsolete: true
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!
Attachment #8784157 - Flags: review?(gerv)
Comment on attachment 8784157 [details] [diff] [review]
Bug 1186578 - [webvr] Implement OpenVR/SteamVR support

Sorry, was wrong bug!  Unassigned :gerv..
Attachment #8784157 - Flags: review?(gerv)
https://hg.mozilla.org/mozilla-central/rev/2b2a897690b3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Blocks: 1299926
No longer blocks: 1260563
You need to log in before you can comment on or make changes to this bug.