Closed Bug 1466700 Opened 2 years ago Closed 2 years ago

Refactor gfxVROculus.cpp to use gfxVRExternal interface

Categories

(Core :: WebVR, defect)

59 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: kip, Assigned: kip)

References

Details

Attachments

(2 files)

As part of an iterative implementation to move the VR api interfaces to their own process, we will first update the backend's in gfx/vr/... to communicate entirely with the rest of Gecko through the VRExternalShmem struct, defined in gfx/vr/external_api/moz_external_vr.h

This implementation will follow the conversion of gfxVROpenVR.cpp in Bug 1466699, which will lay much of the foundation needed.
Blocks: 1362578
Blocks: 1473399
Fixed android build failure found in try push.  Updated try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98ef39faa749b9005fcd0f01e80899f1b4a0481e
Please check it with dom.vr.external;true, dom.vr.service;true, dom.vr.process;true to see if there is any regression.
Corrected nits in review feedback
Moved #ifdef to fix macos and linux builds

Updated try push:

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

I am now doing some testing with physical VR hardware to verify that no regressions have occurred, as per comment 5.
Try run found another macOS build failure.  Fixed; updated try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd5dab015c69e217a04a31d2c36746005b991153
Fixed an assertion found in a try run that also enabled the VR service thread by default.  Fixed; updated try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6afe0455e4f83fff74161d86af0ac6e6a3b953df
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f536c1abf26e
Refactor gfxVROculus.cpp to use gfxVRExternal interface, r=daoshengmu
https://hg.mozilla.org/mozilla-central/rev/f536c1abf26e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Backout by dvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/32976a484a5e
Backed out changeset f536c1abf26e for causing build bustage at /build/build/src/gfx/vr/service/OculusSession.cpp
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla64 → ---
Depends on: 1498550
This was backed out due to MSVC builds returning to Tier 1 and reporting warnings:

10:45:33     INFO -  z:/build/build/src/gfx/vr/service/OculusSession.cpp(189): error C2220: warning treated as error - no 'object' file generated
10:45:33     INFO -  z:/build/build/src/gfx/vr/service/OculusSession.cpp(189): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
10:45:33     INFO -  z:/build/build/src/gfx/vr/service/OculusSession.cpp(192): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)

Change to fix this:

< +    aControllerState.buttonPressed |= (1 << aButtonIdx);
---
> +    aControllerState.buttonPressed |= ((uint64_t)1 << aButtonIdx);
653c653
< +    aControllerState.buttonTouched |= (1 << aButtonIdx);
---
> +    aControllerState.buttonTouched |= ((uint64_t)1 << aButtonIdx);
Attachment #9016807 - Flags: review?(dmu)
Comment on attachment 9016807 [details] [diff] [review]
Bug 1466700 - Refactor gfxVROculus.cpp to use gfxVRExternal interface (fixed for reland)

LGTM.
Attachment #9016807 - Flags: review?(dmu) → review+
The updated patch, "Bug 1466700 - Refactor gfxVROculus.cpp to use gfxVRExternal interface (fixed for reland)" has been updated to correct the MSVC warning and is ready for re-landing.
Flags: needinfo?(ryanvm)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/06020ce3e33a
Refactor gfxVROculus.cpp to use gfxVRExternal interface, r=daoshengmu
I'll keep an eye on the builds on autoland and re-land bug 1473399 once they're green. Thanks for the quick rebase and sorry again for the extra hassle!
Flags: needinfo?(ryanvm)
For posterity, given that the backout and relanding was all contained to the autoland repo (and will end up merging back to m-c in the same push eventually), be aware the the Nightly builds starting on m-c *now* (i.e. the second round of 20181012 Nightlies) will still contain the changes from the merge back in comment 10. In case it comes up in archaeology down the road :)
https://hg.mozilla.org/mozilla-central/rev/06020ce3e33a
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1498789
You need to log in before you can comment on or make changes to this bug.