Closed Bug 1470527 Opened 6 years ago Closed 6 years ago

Implement controller support for gfxVRExternal

Categories

(Core :: WebVR, enhancement)

Product:
Core
Component:
WebVR
Version:
59 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: kip, Assigned: kip)

References

Details

(Whiteboard: [geckoview:fxr])

Attachments

(1 file, 1 obsolete file)

Bug 1470527 - Implement Controller support for gfxVRExternal and VRServiceOpenVR
59 bytes, text/x-review-board-request
Details 
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.
Blocks: 1473399
Depends on: 1466699
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.
Whiteboard: [geckoview:fxr]
Attachment #8990483 - Flags: review?(dmu)
Fixed warnings and debug build only errors.  Try is now green.
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 ...
Attachment #8990483 - Flags: review?(dmu) → review+
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66c97a5d22ef
Implement Controller support for gfxVRExternal and VRServiceOpenVR
Backout by aciure@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad44ecfdc7b4
Backed out 1 changesets for build bustages on OpenVRSession.cpp CLOSED TREE
(In reply to Pulsebot from comment #9)
> Backout by aciure@mozilla.com:
> 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.
Attachment #8990483 - Attachment is obsolete: true
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e8eca5ec5c9
Implement Controller support for gfxVRExternal and VRServiceOpenVR,r=daoshengmu
https://hg.mozilla.org/mozilla-central/rev/1e8eca5ec5c9
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
status-firefox62=wontfix because this VR bug is not a Focus+GV blocker.
status-firefox61: --- → wontfix
status-firefox62: --- → wontfix
status-firefox-esr52: --- → wontfix
status-firefox-esr60: --- → wontfix
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: