1466702 - Refactor gfxVRPuppet.cpp to use gfxVRExternal interface

Status: RESOLVED FIXED

(Core :: WebVR, enhancement)

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.

The control of the puppet devices themselves will continue to use the existing IPC/messages on the Compositor thread until later refactoring to use a puppet testing specific interface from the VR process.

Comment 1

[:kip (Kearwood Gilbert)]

The dom/vr/tests/mochitest tests have been disabled in Bug 1473397 due to the test code replaced by VRServicePuppet in this patch needing to be updated to support asynchronous display and controller instantiation.

The dom/vr/tests/cmohitest tests will be re-enabled with this patch.

Comment 2

[thomasmo]

It might also be good to also move the runtime call from SpecialPowers.pushPrefEnv (in runVRTest.js) to config declarations in mochitest.ini when these are re-enabled.

Comment 3

[:kip (Kearwood Gilbert)]

Attachment: Bug 1466702 - Refactor / reimplement gfxVRPuppet and VRServiceTest to use gfxVRExternal

gfxVRPuppet will be replaced with a fully asynchronous puppet automation that runs in the VR process.

Comment 4

[:kip (Kearwood Gilbert)]

Early try run to find build errors:

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

Comment 5

[:kip (Kearwood Gilbert)]

Updated try push:

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

Comment 6

[:kip (Kearwood Gilbert)]

Updated try push:

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

Comment 7

[:kip (Kearwood Gilbert)]

New try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ffcab6bf85e4a3a852a027a1592ad14affbef07

Comment 8

[:kip (Kearwood Gilbert)]

Fixed some static analysis warnings and errors found on try. Updated push:

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

Comment 9

[:kip (Kearwood Gilbert)]

Fixed Linux compile errors, updated push:

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

Comment 10

[:kip (Kearwood Gilbert)]

Fixed more static analysis warnings, addressed review feedback, fixed memory leak.

Updated push:

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

There is likely still compile errors on Android, will fix now.

Comment 11

[:kip (Kearwood Gilbert)]

I have completed making all the changes suggested in review feedback and static analysis.

Pushed to try again to weed out any platform-specific failures:

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

Comment 12

[:kip (Kearwood Gilbert)]

Updated try push, rebased and attempting to fix Android and Linux build errors:

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

Comment 13

[:kip (Kearwood Gilbert)]

Try looks good, except for Win 7 WPT. Investigating...

Comment 14

[:kip (Kearwood Gilbert)]

It appears that Win 7 try is failing due to having the VR process enabled but not the GPU process. Updating patch to ensure that the logic is consistent everywhere, testing for both the GPU process and VR process enable prefs.

Comment 15

[:kip (Kearwood Gilbert)]

Updated try push, with Win 7 fixes:

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

Comment 16

[:kip (Kearwood Gilbert)]

Updated patch and try push, to correct shutdown crash introduced in last patch:

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

Comment 17

[:kip (Kearwood Gilbert)]

New patch and try push, to correct implicit VRServiceHost constructor warning:

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

Comment 18

[:kip (Kearwood Gilbert)]

VRServiceHost was shutting down too early, causing VRManager's DTOR to crash. Updated patch and try push:

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

Comment 19

[:kip (Kearwood Gilbert)]

Rebase, backport pref changes, fix try build errors.

Try push for new patch:

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

Comment 20

[:kip (Kearwood Gilbert)]

This build is looking good. I have also done some sanity checks on device locally with the OpenVR backend.

Comment 21

[:kip (Kearwood Gilbert)]

Changed unsigned long arguments to uint32_t based on review feedback from BZ, rebased. Try push:

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

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/133ac0d11428
Refactor / reimplement gfxVRPuppet and VRServiceTest to use gfxVRExternal r=daoshengmu,thomasmo,bzbarsky

Comment 23

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/133ac0d11428