1198518 - [webvr] add support for OSVR

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[gfrolov]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.157 Safari/537.36

Comment 1

[gfrolov]

Attachment: Patch file

Comment 2

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

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)

Comment 3

[gfrolov]

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

Comment 4

[gfrolov]

Attachment: Updated patch

This is an updated patch file based on the feedback received

Comment 5

[gfrolov]

Attachment: Updated patch (added render part)

This updates previous patch, adding display configuration, rendering and additional OSVR headers

Comment 6

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

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.

Comment 7

[gfrolov]

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.

Comment 8

[:kip (Kearwood Gilbert)]

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.

Comment 9

[gfrolov]

Attachment: Updated patch to current source code

This is an updated patch that was brought up to date with current Firefox and OSVR sources.

Comment 10

[:kip (Kearwood Gilbert)]

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.

Comment 11

[gfrolov]

Attachment: UpdatedPatchWithComms.diff

Updated the patch as per Kip's comments above.

Comment 12

[:kip (Kearwood Gilbert)]

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.

Comment 13

[gfrolov]

: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?

Comment 14

[gfrolov]

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?

Comment 15

[:kip (Kearwood Gilbert)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c8d0a956f30

Comment 16

[:kip (Kearwood Gilbert)]

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.

Comment 17

[gfrolov]

Attachment: Updated patch that includes cross platform export headers

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.

Comment 18

[:kip (Kearwood Gilbert)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f18138ffe8e

Comment 19

[:kip (Kearwood Gilbert)]

Thanks for updating the patch!

I've pushed to try again.

Comment 20

[:kip (Kearwood Gilbert)]

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!

Comment 21

[gfrolov]

Attachment: Check for OSVR status asynchronously, update API calls

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.

Comment 22

[gfrolov]

Hi Kip,

Just checking to see if you had a chance to try push the latest patch update?

Comment 23

[:kip (Kearwood Gilbert)]

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

Comment 24

[:kip (Kearwood Gilbert)]

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.

Comment 25

[gfrolov]

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.

Comment 26

[:kip (Kearwood Gilbert)]

(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

Comment 27

[gfrolov]

Attachment: UpdatedPatch.diff

Comment 28

[gfrolov]

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.

Comment 29

[:kip (Kearwood Gilbert)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a75521c90c3

Comment 30

[:kip (Kearwood Gilbert)]

Thanks again for updating the patch.  I've just pushed a new try run

Comment 31

[gfrolov]

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!

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2da4c57f4595

Comment 33

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=25329470&repo=mozilla-inbound

Comment 34

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59d3baba3cf0

Comment 35

[:kip (Kearwood Gilbert)]

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!

Comment 36

[gfrolov]

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?

Comment 37

[gfrolov]

Attachment: UpdatedPatch.diff

Comment 38

[gfrolov]

I just uploaded an updated patch with minor change that adds currentRenderTarget member. Could you please try-push this one.

Comment 39

[gfrolov]

Hi Kip,
Did you get a chance to try push with latest patch?

Comment 40

[:kip (Kearwood Gilbert)]

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

Comment 41

[gfrolov]

Looks like the try push was successful. Could we try to merge the patch

Comment 42

[:kip (Kearwood Gilbert)]

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.

Comment 43

[:kip (Kearwood Gilbert)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0443ae0763bb88d4f6242f6afa2ae74f18b49927
Bug 1198518: [webvr] add support for OSVR,r=kip

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0443ae0763bb