1470527 - Implement controller support for gfxVRExternal

Status: RESOLVED FIXED

(Core :: WebVR, enhancement)

Description

[:kip (Kearwood Gilbert)]

gfxVRExternal needs to use the information passed in through the Shmem to describe and instantiate controllers.

This bug also tracks the implementation of the OpenVRService class to populate the controller state in the shmem.

Comment 1

[:kip (Kearwood Gilbert)]

Attachment: Bug 1470527 - Implement Controller support for gfxVRExternal and VRServiceOpenVR

Review commit: https://reviewboard.mozilla.org/r/255550/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255550/

Comment 2

[:kip (Kearwood Gilbert)]

Comment on attachment 8990483 [details]
Bug 1470527 - Implement Controller support for gfxVRExternal and VRServiceOpenVR

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255550/diff/1-2/

Comment 3

[:kip (Kearwood Gilbert)]

Comment on attachment 8990483 [details]
Bug 1470527 - Implement Controller support for gfxVRExternal and VRServiceOpenVR

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255550/diff/2-3/

Comment 4

[:kip (Kearwood Gilbert)]

With the attached patch, controllers are now working through gfxVRExternal and OpenVRSession.
Once it passes the try run, I'll assign for review and land.

Comment 5

[:kip (Kearwood Gilbert)]

Comment on attachment 8990483 [details]
Bug 1470527 - Implement Controller support for gfxVRExternal and VRServiceOpenVR

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255550/diff/3-4/

Comment 6

[:kip (Kearwood Gilbert)]

Fixed warnings and debug build only errors.  Try is now green.

Comment 7

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8990483 [details]
Bug 1470527 - Implement Controller support for gfxVRExternal and VRServiceOpenVR

https://reviewboard.mozilla.org/r/255550/#review262994

LGTM. just some nits for adding assersions or checking the array size to avoid overflow when doing write/read params.

::: gfx/vr/ipc/VRMessageUtils.h:115
(Diff revision 4)
>      WriteParam(aMsg, aParam.mFrameId);
>      WriteParam(aMsg, aParam.mDisplayState);
>      for (int i = 0; i < mozilla::gfx::kVRMaxLatencyFrames; i++) {
>        WriteParam(aMsg, aParam.mLastSensorState[i]);
>      }
> +    for (int i = 0; i < mozilla::gfx::kVRControllerMaxCount; i++) {

Let's add an assertion for safe
 MOZ_ASSERT(mozilla::ArrayLength(aParam.mControllerState) == mozilla::gfx::kVRControllerMaxCount);
 
Or ... as the above.

::: gfx/vr/ipc/VRMessageUtils.h:130
(Diff revision 4)
>          !ReadParam(aMsg, aIter, &(aResult->mGroupMask)) ||
>          !ReadParam(aMsg, aIter, &(aResult->mFrameId)) ||
>          !ReadParam(aMsg, aIter, &(aResult->mDisplayState))) {
>        return false;
>      }
>      for (int i = 0; i < mozilla::gfx::kVRMaxLatencyFrames; i++) {

Let's add an assertion for safe
 MOZ_ASSERT(mozilla::ArrayLength(aResult->mLastSensorState) == mozilla::gfx::kVRMaxLatencyFrames);
 
or do it in this way

for (int i = 0; i < mozilla::ArrayLength(aResult->mLastSensorState); i++) {
      WriteParam(aMsg, aParam.mLastSensorState[i]);
}

::: gfx/vr/ipc/VRMessageUtils.h:135
(Diff revision 4)
>      for (int i = 0; i < mozilla::gfx::kVRMaxLatencyFrames; i++) {
>        if (!ReadParam(aMsg, aIter, &(aResult->mLastSensorState[i]))) {
>          return false;
>        }
>      }
> -
> +    for (int i = 0; i < mozilla::gfx::kVRControllerMaxCount; i++) {

Let's add an assertion for safe
 MOZ_ASSERT(mozilla::ArrayLength(aResult->mControllerState) == mozilla::gfx::kVRControllerMaxCount);
 
or do it in this way

for (int i = 0; i < mozilla::ArrayLength(aResult->mControllerState); i++) {
      WriteParam(aMsg, aParam.mLastSensorState[i]);
}

::: gfx/vr/ipc/VRMessageUtils.h:211
(Diff revision 4)
> +  {
> +    WriteParam(aMsg, aParam.timestamp);
> +    WriteParam(aMsg, aParam.inputFrameID);
> +    WriteParam(aMsg, aParam.flags);
> +    WriteParam(aMsg, aParam.pose);
> +    for (int i=0; i < 16; i++) {

Let's do this way.

for (int i = 0; i < mozilla::ArrayLength(aParam.leftViewMatrix); i++) {
      WriteParam(aMsg, aParam.leftViewMatrix[i]);
}

::: gfx/vr/ipc/VRMessageUtils.h:214
(Diff revision 4)
> +    WriteParam(aMsg, aParam.flags);
> +    WriteParam(aMsg, aParam.pose);
> +    for (int i=0; i < 16; i++) {
> +      WriteParam(aMsg, aParam.leftViewMatrix[i]);
> +    }
> +    for (int i=0; i < 16; i++) {

same as the above.

::: gfx/vr/ipc/VRMessageUtils.h:288
(Diff revision 4)
> -    WriteParam(aMsg, aParam.mButtonTouched);
> +    WriteParam(aMsg, aParam.buttonTouched);
> +    WriteParam(aMsg, aParam.flags);
> +    WriteParam(aMsg, aParam.pose);
> +    WriteParam(aMsg, aParam.isPositionValid);
> +    WriteParam(aMsg, aParam.isOrientationValid);
>      for (int i=0; i < mozilla::gfx::kVRControllerMaxAxis; i++) {

add an assersion or ...

::: gfx/vr/ipc/VRMessageUtils.h:291
(Diff revision 4)
> +    WriteParam(aMsg, aParam.isPositionValid);
> +    WriteParam(aMsg, aParam.isOrientationValid);
>      for (int i=0; i < mozilla::gfx::kVRControllerMaxAxis; i++) {
> -      WriteParam(aMsg, aParam.mAxisValue[i]);
> +      WriteParam(aMsg, aParam.axisValue[i]);
>      }
> -    for (int i=0; i < mozilla::gfx::kVRControllerMaxTriggers; i++) {
> +    for (int i=0; i < mozilla::gfx::kVRControllerMaxButtons; i++) {

add an assersion or ...

::: gfx/vr/ipc/VRMessageUtils.h:312
(Diff revision 4)
> +        !ReadParam(aMsg, aIter, &(aResult->pose)) ||
> +        !ReadParam(aMsg, aIter, &(aResult->isPositionValid)) ||
> +        !ReadParam(aMsg, aIter, &(aResult->isOrientationValid))) {
>        return false;
>      }
>      for (int i=0; i < mozilla::gfx::kVRControllerMaxAxis; i++) {

add an assersion or ...

::: gfx/vr/ipc/VRMessageUtils.h:317
(Diff revision 4)
>      for (int i=0; i < mozilla::gfx::kVRControllerMaxAxis; i++) {
> -      if (!ReadParam(aMsg, aIter, &(aResult->mAxisValue[i]))) {
> +      if (!ReadParam(aMsg, aIter, &(aResult->axisValue[i]))) {
>          return false;
>        }
>      }
> -    for (int i=0; i < mozilla::gfx::kVRControllerMaxTriggers; i++) {
> +    for (int i=0; i < mozilla::gfx::kVRControllerMaxButtons; i++) {

add an assersion or ...

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66c97a5d22ef
Implement Controller support for gfxVRExternal and VRServiceOpenVR

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad44ecfdc7b4
Backed out 1 changesets for build bustages on OpenVRSession.cpp CLOSED TREE

Comment 10

[:kip (Kearwood Gilbert)]

(In reply to Pulsebot from comment #9)
> Backout by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ad44ecfdc7b4
> Backed out 1 changesets for build bustages on OpenVRSession.cpp CLOSED TREE

Sorry for that..  Patch updated with fixes from Comment 7 was missing latest revision of changes in MozReview patch resulting in warnings (treated as errors).  Will fix and re-submit.

Comment 11

[:kip (Kearwood Gilbert)]

Attachment: Bug 1470527 - Implement Controller support for gfxVRExternal and VRServiceOpenVR

Review commit: https://reviewboard.mozilla.org/r/256340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256340/

Comment 12

[:kip (Kearwood Gilbert)]

Comment on attachment 8991456 [details]
Bug 1470527 - Implement Controller support for gfxVRExternal and VRServiceOpenVR

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256340/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e8eca5ec5c9
Implement Controller support for gfxVRExternal and VRServiceOpenVR,r=daoshengmu

Comment 14

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/1e8eca5ec5c9

Comment 15

[Chris Peterson [:cpeterson]]

status-firefox62=wontfix because this VR bug is not a Focus+GV blocker.