Closed Bug 1198518 Opened 4 years ago Closed 4 years ago

[webvr] add support for OSVR

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: gfrolov, Assigned: gfrolov)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [webvr], gfx-noted)

Attachments

(1 file, 8 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.157 Safari/537.36
Whiteboard: [webvr]
Component: Untriaged → Graphics
Product: Firefox → Core
Whiteboard: [webvr] → [webvr], gfx-noted
Attached patch Patch file (obsolete) — Splinter Review
Attachment #8653609 - Flags: review+
Attachment #8653609 - Flags: feedback+
Comment on attachment 8653609 [details] [diff] [review]
Patch file

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

Overall looks like a fine start -- some formatting/indentation issues throughout, but those can be worked out.  Other comments below.

::: gfx/vr/gfxVROSVR.cpp
@@ +60,5 @@
> +
> +
> +bool LoadOSVRRuntime()
> +{
> +    static PRLibrary *osvrUtilLib = nullptr;

spacing -- here and elsewhere in the file.  It's right now a mix of 2-space and 4-space indents; the emacs modeline at the top says 2-space, so please use 2 space indents throughout.

@@ +79,5 @@
> +
> +  osvrUtilLib = PR_LoadLibrary(osvrUtilPath.BeginReading());
> +  osvrCommonLib = PR_LoadLibrary(osvrCommonPath.BeginReading());
> +  osvrClientLib = PR_LoadLibrary(osvrClientPath.BeginReading());
> +  osvrClientKitLib = PR_LoadLibrary(osvrClientKitPath.BeginReading());

if only symbols from osvrClientKit are needed, is it explicitly needed to LoadLibrary all of the others?  They should be pulled in via dll/so/dylib deps on all platforms, right?

@@ +135,5 @@
> +  mSupportedSensorBits = State_Orientation | State_Position;
> +  
> +  OSVR_ReturnCode ret = osvr_ClientGetInterface(*m_ctx, "/me/head", &m_iface);
> +
> +  if (ret != OSVR_RETURN_SUCCESS){

nit: missing space after )

@@ +136,5 @@
> +  
> +  OSVR_ReturnCode ret = osvr_ClientGetInterface(*m_ctx, "/me/head", &m_iface);
> +
> +  if (ret != OSVR_RETURN_SUCCESS){
> +      printf("Couldn't initialize interface\n");

is m_iface nulled out if osvr_ClientGetInterface fails?  If not, it should be explicitly set to nullptr here -- otherwise the ClientFreeInterface in Destroy() will likely crash.  Also, if this can fail, it might be better to call osvr_ClientGetInterface outside and then pass in both the context and the /me/head interface to the HMDInfoOSVR() constructor... as written you could end up with a HMDInfoOSVR without a real head interface.

@@ +147,5 @@
> +void
> +HMDInfoOSVR::Destroy()
> +{
> +    //free interface
> +    osvr_ClientFreeInterface(*m_ctx, m_iface);

Do you need to nullcheck m_iface?

@@ +174,5 @@
> +                                       const Size& destViewport,
> +                                       const Rect& destRect,
> +                                       VRDistortionConstants& values)
> +{
> +    // @todo take care of that later

Does OSVR take care of distortion rendering (compositor-style approach), or does it expect apps to do it themselves (old-style Oculus approach)?

@@ +216,5 @@
> +
> +  if (ret != OSVR_RETURN_SUCCESS) {
> +      printf_stderr("No orientation state\n");
> +  }
> +  else{

nit: "} else {"

@@ +227,5 @@
> +  }
> +
> +  OSVR_PositionState position;
> +  ret = osvr_GetPositionState(m_iface, &timestamp, &position);
> +  if (ret != OSVR_RETURN_SUCCESS){

nit: space after )

@@ +268,5 @@
> +bool
> +VRHMDManagerOSVR::PlatformInit()
> +{
> +  if (mOSVRPlatformInitialized){
> +      return true;

nit: space after )

@@ +271,5 @@
> +  if (mOSVRPlatformInitialized){
> +      return true;
> +  }
> +  if (!gfxPrefs::VREnabled())
> +  {

nit: { on same line as if for one-line if clause

@@ +274,5 @@
> +  if (!gfxPrefs::VREnabled())
> +  {
> +    return false;
> +  }
> +  if (!LoadOSVRRuntime()){

nit: space after final )

@@ +294,5 @@
> +  // get client context
> +  m_ctx = osvr_ClientInit("com.osvr.webvr", 0);
> +  
> +  //initialize HMD with all necessary sensors
> +  nsRefPtr<HMDInfoOSVR> hmd = new HMDInfoOSVR(&m_ctx);

see above about getting the /me/head interface before calling the constructor, to avoid creating a HMD object here if you can't actually get a head-tracking device

@@ +304,5 @@
> +
> +void
> +VRHMDManagerOSVR::Destroy()
> +{
> +  if (!mOSVRInitialized){ return; }

nit: space after )

@@ +317,5 @@
> +void
> +VRHMDManagerOSVR::GetHMDs(nsTArray<nsRefPtr<VRHMDInfo>>& aHMDResult)
> +{
> +    Init();
> +    if (mOSVRHMD){

nit: space after )

::: modules/libpref/init/all.js
@@ +4657,5 @@
>  // true = show the VR textures in our compositing output; false = don't.
>  // true might have performance impact
>  pref("gfx.vr.mirror-textures", false);
> +// path to OSVR DLLs
> +pref("gfx.vr.osvrUtil.lib", "");

maybe name this gfx.vr.osvr.utilLibPath and similar for the others, to keep everything osvr related underneath the gfx.vr.osvr tree (along with gfx.vr.osvr.enabled, which you should add)
Attachment #8653609 - Flags: feedback?(vladimir) → feedback+
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8653609 [details] [diff] [review]
Patch file

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

::: gfx/vr/gfxVROSVR.cpp
@@ +60,5 @@
> +
> +
> +bool LoadOSVRRuntime()
> +{
> +    static PRLibrary *osvrUtilLib = nullptr;

Fixed the indentation issue. Will provide updated patch soon

@@ +79,5 @@
> +
> +  osvrUtilLib = PR_LoadLibrary(osvrUtilPath.BeginReading());
> +  osvrCommonLib = PR_LoadLibrary(osvrCommonPath.BeginReading());
> +  osvrClientLib = PR_LoadLibrary(osvrClientPath.BeginReading());
> +  osvrClientKitLib = PR_LoadLibrary(osvrClientKitPath.BeginReading());

ClientKit internally depends on those DLLs, so we need to load those first (depth order traversal of dependency graph).

@@ +136,5 @@
> +  
> +  OSVR_ReturnCode ret = osvr_ClientGetInterface(*m_ctx, "/me/head", &m_iface);
> +
> +  if (ret != OSVR_RETURN_SUCCESS){
> +      printf("Couldn't initialize interface\n");

m_iface is not nulled out if it cannot get interface for the given path. I originally had it in HMDManager but moved it to HMDInfo. You're right, HMDManager looks to be a proper place for it, so it would not initialize HMD if we cannot obtain interface

@@ +147,5 @@
> +void
> +HMDInfoOSVR::Destroy()
> +{
> +    //free interface
> +    osvr_ClientFreeInterface(*m_ctx, m_iface);

We don't need to nullcheck because function would check it

@@ +174,5 @@
> +                                       const Size& destViewport,
> +                                       const Rect& destRect,
> +                                       VRDistortionConstants& values)
> +{
> +    // @todo take care of that later

When it comes to distortion there are two options: Use Direct mode(under development and will be available soon), and it will take care do distortion rendering or use display config file to read the parameters and handle the distortion inside the client app.

@@ +294,5 @@
> +  // get client context
> +  m_ctx = osvr_ClientInit("com.osvr.webvr", 0);
> +  
> +  //initialize HMD with all necessary sensors
> +  nsRefPtr<HMDInfoOSVR> hmd = new HMDInfoOSVR(&m_ctx);

Moving the osvrClientGetInterface here and will return false if it fails, since tracking wouldn't be available
Attached patch Updated patch (obsolete) — Splinter Review
This is an updated patch file based on the feedback received
Attachment #8653609 - Attachment is obsolete: true
Attachment #8655507 - Flags: review?(vladimir)
This updates previous patch, adding display configuration, rendering and additional OSVR headers
Attachment #8655507 - Attachment is obsolete: true
Attachment #8655507 - Flags: review?(vladimir)
Attachment #8671063 - Flags: review?(vladimir)
Comment on attachment 8671063 [details] [diff] [review]
Updated patch (added render part)

This looks fine as it is, but it still won't actually render anything, right?

Note that if RenderingSupport is used, stuff like FillDistortionConstants won't ever be called.  That's for the old client-based distortion path which will likely go away soon.
Attachment #8671063 - Flags: review?(vladimir) → review+
Great, then we could probably merge this updated patch. It currently renders demos on external monitor (fullscreen mode). 
OSVR Direct render feature has been released recently and it would be able to support rendering to multiple HMDs, however we should discuss how would you prefer to integrate it.
Perhaps if this patch is updated to apply to the current source, then we could land it behind a pref so that it doesn't bitrot further.

I would be glad to help with any direction on implementing OSVR Direct Render.  Please needinfo me on Bugzilla or reach out on the WebVR slack (I am @kearwood) if I can help in any way.
This is an updated patch that was brought up to date with current Firefox and OSVR sources.
Attachment #8671063 - Attachment is obsolete: true
Attachment #8713236 - Flags: review?(kgilbert)
Comment on attachment 8713236 [details] [diff] [review]
Updated patch to current source code

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

These changes look good.  Perhaps add a comment in each function that needs to be completed with an "XXX" keyword.
As long as we are not failing any tests (with pref turned off), I think it would be okay to commit this so that it doesn't bit-rot.
Attachment #8713236 - Flags: review?(kgilbert) → review+
Attached patch UpdatedPatchWithComms.diff (obsolete) — Splinter Review
Updated the patch as per Kip's comments above.
Attachment #8713236 - Attachment is obsolete: true
Attachment #8714461 - Flags: review?(kgilbert)
Comment on attachment 8714461 [details] [diff] [review]
UpdatedPatchWithComms.diff

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

Thanks for adding comments, looks good.  With a successful try push, I feel this can be committed.
Attachment #8714461 - Flags: review?(kgilbert) → review+
:kip,
Just read up on the wiki regarding try server and wanted to clarify if you will be doing try push
because I think that my account doesn't have necessary commit permissions?
Hi Kip,
Just wanted to follow up. Could you let me know how should we do the try push. Would you be doing the try push or should I request Level 1 Commit Access to do it?
Flags: needinfo?(kgilbert)
I have pushed this to try as a "t" push.  It will build all platforms and run tests on three of them.

If this passes, I can land this patch for you.
Flags: needinfo?(kgilbert)
Based on try server logs - https://treeherder.mozilla.org/#/jobs?repo=try&revision=0735804b7f42 it appears that build failed on Linux and OSX due to export headers. This patch adds cross platform export headers that should fix the build issues.
Kip, could you give this patch another try push please.
Attachment #8714461 - Attachment is obsolete: true
Flags: needinfo?(kgilbert)
Thanks for updating the patch!

I've pushed to try again.
Flags: needinfo?(kgilbert)
It appears that the non-Windows platforms are failing in the try push due to the "Sleep()" function.

Would it be possible to do this asynchronously instead?  For example, maybe check every vsync interval to see if a response has been seen yet?  We support hot-swapping, so you don't necessarily have to complete the enumeration within the init function.

The gfxOculus.cpp/h files may give you some examples on doing this.  Also, please needinfo me if you need any help, thanks!
Flags: needinfo?(gfrolov)
Hi Kip,
Sorry about lack of updates in the past couple of weeks.
I modified the patch, removed "Sleep()" function calls and check for OSVR status asynchronously per your suggestion. I tried to use Vsync notifications but those occur inside "HMDInfoOSVR" which expects OSVR to be up and running, so instead I check every "getHMD()" call that appears to be called when you open new WebVR app/page.
I tested the patch on both Windows and Linux machines, and it should be up to date with the current source.
Attachment #8720373 - Attachment is obsolete: true
Flags: needinfo?(gfrolov) → needinfo?(kgilbert)
Hi Kip,

Just checking to see if you had a chance to try push the latest patch update?
Thanks for updating the patch!  I've started reviewing it and will follow up shortly.

Also, I have pushed a new try run, linked in Comment 23.
Flags: needinfo?(kgilbert)
Attachment #8727904 - Flags: review?(kgilbert)
Looks like the try push had failed because of unused variable. The build did pass on my Linux VM, although I might not have all the proper flags set. 
The one I had was:
ac_add_options --enable-tests
Do you know which additional flags should I be specifying in mozconfig to make sure it goes through the same tests on my machine as on try push server.
Flags: needinfo?(kgilbert)
(In reply to gfrolov from comment #25)
> Looks like the try push had failed because of unused variable. The build did
> pass on my Linux VM, although I might not have all the proper flags set. 
> The one I had was:
> ac_add_options --enable-tests
> Do you know which additional flags should I be specifying in mozconfig to
> make sure it goes through the same tests on my machine as on try push server.

We also treat warnings as errors.  You can replicate this with:

ac_add_options --enable-warnings-as-errors
Flags: needinfo?(kgilbert)
Attached patch UpdatedPatch.diff (obsolete) — Splinter Review
Attachment #8727904 - Attachment is obsolete: true
Attachment #8727904 - Flags: review?(kgilbert)
Thanks Kip! I found the warning that was causing it to fail and fixed that. It builds successfully with that flag on my Linux box now, so hopefully try push would build successfully this time.
Flags: needinfo?(kgilbert)
Thanks again for updating the patch.  I've just pushed a new try run
Flags: needinfo?(kgilbert)
Keywords: checkin-needed
Looks like the try push went through successfully on all platforms! I added flag "checkin-needed" as the guide suggested. If there are additional steps needed before merging it please let me know.
Thanks!
Flags: needinfo?(kgilbert)
It appears that the backout was due to two incompatible patches landing in mozilla-inbound simultaneously.

The other patch had removed currentRenderTarget from the parent class.  You can simply add this member to your own class and all should be good.

Also, no need to add checkin-needed this time.  Once you submit the updated patch and I have reviewed it (with r+), I can do the check-in for you.

Thanks again!
Flags: needinfo?(kgilbert)
Yeah, it looks like that's why it failed. I noticed that there were a number of additional changes to Oculus plugin which are mostly related to updating to SDK v1.3 and renaming PCCompositor to PCCompositorBridge. Just wanted to double check with you if any additional changes are needed on my part based on the recent commits, besides currentRenderTarget part?
Flags: needinfo?(kgilbert)
Attachment #8735517 - Attachment is obsolete: true
Flags: needinfo?(gfrolov)
I just uploaded an updated patch with minor change that adds currentRenderTarget member. Could you please try-push this one.
Hi Kip,
Did you get a chance to try push with latest patch?
Looks like the try push was successful. Could we try to merge the patch
Blocks: 1276711
With the imminent landing of Bug 1250244 (WebVR 1.0 support), we wish to land this patch in order to prevent it from being bit-rotted.  I have added Bug 1276712 to track the implementation of OSVR rendering support which will be required before OSVR is functional.  Bug 1250244 changes the way VR frames are submitted, and should land before OSVR rendering support starts to avoid duplicate work.
Flags: needinfo?(kgilbert)
Blocks: 1250244
https://hg.mozilla.org/mozilla-central/rev/0443ae0763bb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1497379
You need to log in before you can comment on or make changes to this bug.