1466700 - Refactor gfxVROculus.cpp to use gfxVRExternal interface

Status: RESOLVED FIXED

(Core :: WebVR, defect)

Description

[:kip (Kearwood Gilbert)]

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.

Comment 1

[:kip (Kearwood Gilbert)]

Attachment: Bug 1466700 - Refactor gfxVROculus.cpp to use gfxVRExternal interface

Comment 2

[:kip (Kearwood Gilbert)]

Try push:

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

Comment 3

[:kip (Kearwood Gilbert)]

Fixed an #ifdef broken in merge.  Updated try push:

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

Comment 4

[:kip (Kearwood Gilbert)]

Fixed android build failure found in try push.  Updated try push:

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

Comment 5

[Daosheng Mu[:daoshengmu]]

Please check it with dom.vr.external;true, dom.vr.service;true, dom.vr.process;true to see if there is any regression.

Comment 6

[:kip (Kearwood Gilbert)]

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.

Comment 7

[:kip (Kearwood Gilbert)]

Try run found another macOS build failure.  Fixed; updated try push:

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

Comment 8

[:kip (Kearwood Gilbert)]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f536c1abf26e
Refactor gfxVROculus.cpp to use gfxVRExternal interface, r=daoshengmu

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f536c1abf26e

Comment 11

[Pulsebot]

Backout by [e-mail]:
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

Comment 12

[:kip (Kearwood Gilbert)]

Attachment: Bug 1466700 - Refactor gfxVROculus.cpp to use gfxVRExternal interface (fixed for reland)

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);

Comment 13

[Daosheng Mu[:daoshengmu]]

Comment on attachment 9016807 [details] [diff] [review]
Bug 1466700 - Refactor gfxVROculus.cpp to use gfxVRExternal interface (fixed for reland)

LGTM.

Comment 14

[:kip (Kearwood Gilbert)]

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.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06020ce3e33a
Refactor gfxVROculus.cpp to use gfxVRExternal interface, r=daoshengmu

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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!

Comment 17

[Ryan VanderMeulen [: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 :)

Comment 18

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/06020ce3e33a