1306415 - [webvr] Implement VRFrameData and VRDisplay.getFrameData

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[:kip (Kearwood Gilbert)]

The WebVR 1.1 Spec added a new interface, VRFrameData:

https://w3c.github.io/webvr/#interface-vrframedata

This is used with the new VRDisplay.getFrameData function:

https://w3c.github.io/webvr/#dom-vrdisplay-getframedata

Comment 1

[:kip (Kearwood Gilbert)]

The patches in this bug are rebased onto Bug 1306427.  Please land Bug 1306427 first.

Comment 2

[:kip (Kearwood Gilbert)]

Patches rebased onto Bug 1306429 as well.  Please land Bug 1306427 and Bug 1306429 first.

Comment 3

[:kip (Kearwood Gilbert)]

Attachment: Bug 1306415 - Part 1: (WIP Do not commit!) Implement VRFrameData and VRDisplay.getFrameDatabug1306415_part1.patch

Comment 4

[:kip (Kearwood Gilbert)]

Attachment: Bug 1306415 - Part 1: Implement VRFrameData and VRDisplay.getFrameData

Comment 5

[:kip (Kearwood Gilbert)]

Attachment: Bug 1306415 - Part 2: Add VRFrameData to test_interfaces.html

Comment 6

[:kip (Kearwood Gilbert)]

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

Comment 7

[:kip (Kearwood Gilbert)]

Comment on attachment 8798589 [details] [diff] [review]
Bug 1306415 - Part 1:  Implement VRFrameData and VRDisplay.getFrameData

@Smaug: Would you be able to review the webidl / dom bits of this one?

Perhaps @Daosheng could take a look at the rest of the implementation.

Comment 8

[:kip (Kearwood Gilbert)]

Sorry I could not upload to mozreview.  These patches are rebased onto others that have not yet landed.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8798589 [details] [diff] [review]
Bug 1306415 - Part 1:  Implement VRFrameData and VRDisplay.getFrameData

>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(VRFrameData)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mParent, mPose)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
>+  tmp->mPose = nullptr;
You already unlink mPose above

>+VRFrameData::LazyCreateMatrix(JS::Heap<JSObject*>& aArray, gfx::Matrix4x4& aMat, JSContext* aCx,
>+                 JS::MutableHandle<JSObject*> aRetval, ErrorResult& aRv)
align the latter arguments under the arguments in the first line

Comment 10

[:kip (Kearwood Gilbert)]

Attachment: Bug 1306415 - Part 1: Implement VRFrameData and VRDisplay.getFrameData, r=smaug

Updated with review feedback from Smaug

Comment 11

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8799016 [details] [diff] [review]
Bug 1306415 - Part 1:  Implement VRFrameData and VRDisplay.getFrameData, r=smaug

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

It looks good to me. Just some nits for fixing.

::: dom/vr/VRDisplay.cpp
@@ +827,5 @@
> +  matHead.SetRotationFromQuaternion(qt);
> +  matHead.PreTranslate(pos);
> +
> +  mLeftView = matHead;
> +  mLeftView.PostTranslate(

Probably, you can try to use mLeftView.PostTranslate(-aInfo.mEyeTranslation[gfx::VRDisplayInfo::Eye_Left]) to make it more concise.

@@ +839,5 @@
> +    -aInfo.mEyeTranslation[gfx::VRDisplayInfo::Eye_Right][0],
> +    -aInfo.mEyeTranslation[gfx::VRDisplayInfo::Eye_Right][1],
> +    -aInfo.mEyeTranslation[gfx::VRDisplayInfo::Eye_Right][2]
> +  );
> +

Same here. could you use  mRightView.PostTranslate(-aInfo.mEyeTranslation[gfx::VRDisplayInfo::Eye_Right])?

Comment 12

[:kip (Kearwood Gilbert)]

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

Comment 13

[:kip (Kearwood Gilbert)]

(In reply to Daosheng Mu[:daoshengmu] from comment #11)
> Comment on attachment 8799016 [details] [diff] [review]
> Bug 1306415 - Part 1:  Implement VRFrameData and VRDisplay.getFrameData,
> r=smaug
> 
> Review of attachment 8799016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks good to me. Just some nits for fixing.
> 
> ::: dom/vr/VRDisplay.cpp
> @@ +827,5 @@
> > +  matHead.SetRotationFromQuaternion(qt);
> > +  matHead.PreTranslate(pos);
> > +
> > +  mLeftView = matHead;
> > +  mLeftView.PostTranslate(
> 
> Probably, you can try to use
> mLeftView.PostTranslate(-aInfo.mEyeTranslation[gfx::VRDisplayInfo::
> Eye_Left]) to make it more concise.
> 
> @@ +839,5 @@
> > +    -aInfo.mEyeTranslation[gfx::VRDisplayInfo::Eye_Right][0],
> > +    -aInfo.mEyeTranslation[gfx::VRDisplayInfo::Eye_Right][1],
> > +    -aInfo.mEyeTranslation[gfx::VRDisplayInfo::Eye_Right][2]
> > +  );
> > +
> 
> Same here. could you use 
> mRightView.PostTranslate(-aInfo.mEyeTranslation[gfx::VRDisplayInfo::
> Eye_Right])?

Good catch, thanks!  I have corrected this.

Comment 14

[:kip (Kearwood Gilbert)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3bda4876ada99e344f4b7a7cef5b5345a3948c8
Bug 1306415 - Part 1:  Implement VRFrameData and VRDisplay.getFrameData,r=smaug,r=dmu

https://hg.mozilla.org/integration/mozilla-inbound/rev/96db3a1d6e6617777ef624ca63912f5cd260f74e
Bug 1306415 - Part 2:  Add VRFrameData to test_interfaces.html,r=smaug

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d3bda4876ada
https://hg.mozilla.org/mozilla-central/rev/96db3a1d6e66