Closed Bug 1148009 Opened 5 years ago Closed 5 years ago

[WebVR] Add support for Cardboard-like VR devices

Categories

(Core :: Graphics, defect)

35 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: vlad, Assigned: vlad)

Details

Attachments

(5 files)

This set of patches adds support for Cardboard and Cardboard-like VR devices, based on our HAL.  It expects to find a GAME_ROTATION_VECTOR sensor (I may do a followup patch to also use ROTATION_VECTOR if GAME_ isn't available) that's introduced to our HAL in bug 1144674, and then does appropriate work to translate it to something useful for VR.

Along the way, this also refactors the VR stuff -- moves it into gfx/vr, and splits out Oculus stuff into its own separate file.
Move VR stuff from gfx/thebes/* to gfx/vr
Attachment #8583951 - Flags: review?(gwright)
Attachment #8583951 - Flags: review?(gwright) → review?(jmuizelaar)
This is super simple for now; it reuses most of the existing rendering functionality and doesn't try to do anything fancy.  This is enough to get started, though.
Attachment #8583958 - Flags: review?(jmuizelaar)
Attachment #8583951 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8583956 [details] [diff] [review]
4 - add Quaternion class to Moz2D

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

::: gfx/2d/Quaternion.cpp
@@ +53,5 @@
> +  }
> +}
> +
> +void
> +Matrix4x4::SetRotationFromQuaternion(const Quaternion& q)

Move this to Matrix.cpp to surprise people less.
Attachment #8583956 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8583953 [details] [diff] [review]
3 - Move distortion vertex struct into slightly more generic system

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

::: gfx/layers/opengl/CompositorOGLVR.cpp
@@ +269,4 @@
>    gl()->fClear(LOCAL_GL_COLOR_BUFFER_BIT | LOCAL_GL_DEPTH_BUFFER_BIT);
>  
>    gl()->fUseProgram(mVR.mDistortionProgram[programIndex]);
> +  mCurrentProgram = nullptr;

Use CompositorOGL::ResetProgram() here.
Attachment #8583953 - Flags: review?(jmuizelaar) → review+
Attachment #8583952 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8583958 [details] [diff] [review]
5 - Add gfxVRCardboard based on our HAL's sensors

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

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +719,3 @@
>  
>    // This is the source texture SRV for the pixel shader
> +  // XXX, um should we cache this SRV on the source?

Yes. I think we should.

::: gfx/thebes/gfxQuaternion.h
@@ +38,5 @@
> +        r.y = y * o.w + w * o.y + z * o.x - x * o.z;
> +        r.z = z * o.w + w * o.z + x * o.y - y * o.x;
> +        r.w = w * o.w - x * o.x - y * o.y - z * o.z;
> +        return r;
> +    }

Should go away.

::: gfx/vr/gfxVRCardboard.cpp
@@ +212,5 @@
> +}
> +
> +static Matrix4x4
> +ConstructProjectionMatrix(const VRFieldOfView& fov, bool rightHanded, double zNear, double zFar)
> +{

Add a comment about where this came from if you can.
Attachment #8583958 - Flags: review?(jmuizelaar) → review+
Attachment #8583956 - Flags: review?(jacob.benoit.1)
Comment on attachment 8583956 [details] [diff] [review]
4 - add Quaternion class to Moz2D

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

The math looks OK although I didn't seriously check the calculations.

Some inline comments below.

One thing that isn't clear from this patch is what benefit do Quaternions bring here, compared to just dealing with matrices? Quaternions are a representation of certain special linear transformations. They allow running certain operations faster, and allow implementing some other operations more conveniently. But here, I'm not seeing optimized code, so I take it that performance is out of scope; and I'm also not seeing the features that are made more convenient to implement with quaternions (thinking of SLERP). So I wonder, why bother with Quaternions at all?

::: gfx/2d/Quaternion.h
@@ +68,5 @@
> +  {
> +    return sqrt(x*x + y*y + z*z + w*w);
> +  }
> +
> +  Quaternion& Conjugate()

That raises the concern that people will call Conjugate() to get the conjugated value, and won't realize that it changed *this in-place.

A common way to address that is to have on the one hand an in-place method returning void, and on the other hand a Conjugated()  (note the d) method that's const qualified and not changing *this.

@@ +78,5 @@
> +  Quaternion& Normalize()
> +  {
> +    Float l = Length();
> +    if (l > -std::numeric_limits<Float>::epsilon() &&
> +        l < std::numeric_limits<Float>::epsilon())

Not clear to me how epsilon() is relevant here. The only floats that are special wrt multiplication/division are the zeroes and denormals.

Since min() returns the min normal positive value, you could address this by replacing epsilon() by min().

For speed, it's also better to do a single test on the absolute value, than two separate tests.

Unless you're worried about loss of precision with denormals (I don't think you should worry about that here), you can simply write

if (l)

which will be faster.

Finally, you might simply want to just do the division, in the worst case the result will be non-finite. The caller of this API probably shouldn't be normalizing quaternions that might be zero!
Attachment #8583956 - Flags: review?(jacob.benoit.1)
Thanks for looking at this!

(In reply to Benoit Jacob [:bjacob] (mostly away) from comment #9)
> Comment on attachment 8583956 [details] [diff] [review]
> 
> One thing that isn't clear from this patch is what benefit do Quaternions
> bring here, compared to just dealing with matrices?

The incoming sensor values are coming in as a quaternion, and the web-visible API also uses quaternions.  So while I could probably get a way with just adding methods on Matrix4x4 to set a rotation from 4 values and to read the translation as a quaternion, but I think this is cleaner.

In the near future, I expect to have prediction code somewhere in here which will actually want to work with quaternions, doing SLERP and other operations though.

> ::: gfx/2d/Quaternion.h
> @@ +68,5 @@
> > +  {
> > +    return sqrt(x*x + y*y + z*z + w*w);
> > +  }
> > +
> > +  Quaternion& Conjugate()
> 
> That raises the concern that people will call Conjugate() to get the
> conjugated value, and won't realize that it changed *this in-place.
> 
> A common way to address that is to have on the one hand an in-place method
> returning void, and on the other hand a Conjugated()  (note the d) method
> that's const qualified and not changing *this.

Yeah, unfortunate that conjugate is both a verb and a noun.   I'll.. blah. I'll do that in a followup when I extend the Quaternion class.

> @@ +78,5 @@

> Unless you're worried about loss of precision with denormals (I don't think
> you should worry about that here), you can simply write
> 
> if (l)
> 
> which will be faster.

Good point.  Fixed.
You need to log in before you can comment on or make changes to this bug.