Closed Bug 1466699 Opened Last year Closed Last year

Refactor gfxVROpenVR to use gfxVRExternal interface

Categories

(Core :: WebVR, enhancement)

59 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: kip, Assigned: kip)

References

Details

(Whiteboard: geckoview:fxr)

Attachments

(1 file, 7 obsolete 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 will first be done while keeping the implementation in the current process, to prepare for the later refactor to its own "VR process".

This bug tracks the first conversion, for gfxVROpenVR.cpp.  This includes changes to other classes such as VRDisplayHost to support gamepad updates and VR frame submission through the struct.
Whiteboard: geckoview:fxr
WIP, transferring immersive mode layer information over Shmem
Attachment #8983623 - Attachment is obsolete: true
Almost there now..  Have to implement DX11 context creation for the VRService thread.
Attachment #8983985 - Attachment is obsolete: true
OpenVR presentation is now working perfectly through the Shmem interface.
Now need to test all edge cases, hook up telemetry, plumb in controllers, and clean up the patch.
Attachment #8984331 - Attachment is obsolete: true
Introduced VRSession abstract class to ease implementation of multiple vr API backends in the VR Service.
Attachment #8984628 - Attachment is obsolete: true
VRService main loop now continuously checks status of VR system messages, and will respond correctly to shutdown.
Attachment #8985193 - Attachment is obsolete: true
Fixed edge case where user would need to refresh the browser when exiting and returning to a WebVR page with specific timing.

WIP controller enumeration in OpenVRSession
Attachment #8986324 - Attachment is obsolete: true
Depends on: 1469967
Attachment #8986328 - Attachment is obsolete: true
I have split the controller implementation into Bug 1470527 for more incremental landing.
@kip. After your refactor work of the shared memory and VRExternal landed, do we still need VRManagerParent and VRManagerChild IPCs? Ideally, we could be able to let VR services be communicated between GPU process and Content process with shared memory instead of IPCs.
Flags: needinfo?(kgilbert)
(In reply to Daosheng Mu[:daoshengmu] from comment #10)
> @kip. After your refactor work of the shared memory and VRExternal landed,
> do we still need VRManagerParent and VRManagerChild IPCs? Ideally, we could
> be able to let VR services be communicated between GPU process and Content
> process with shared memory instead of IPCs.

I imagine the real-time data (pose, frames, controller updates) could be communicated directly between the content process and the VR service process.

After landing, we will support both the old gfxVR... classes and the new VRService behind a pref.

Once we have all features implemented (eg, controllers and haptic feedback), we can flip the pref by default and start to refactor out some of the old code.

I would imagine first we could remove the old gfxVR... classes and merge gfxVRExternal into VRDisplayHost.

We could greatly simplify the PVRManager protocol and likely remove PVRLayer by using the same Shmem structure directly in the content process.  There may be some function left in VRManagerParent and VRManagerChild; however, as it may be necessary to do validation outside of the content process and to manage VR requests between multiple content processes.
Flags: needinfo?(kgilbert)
Attachment #8986938 - Flags: review?(dmu)
Attachment #8986938 - Flags: review?(dmu)
Some static analysis warnings are causing try to fail with WIP controller implementation.  I'm removing the WIP bits, to be landed separately in Bug 1470527.

I'll re-assign for review once Try passes.
Attachment #8986938 - Flags: review?(dmu)
Try run is now green
Comment on attachment 8986938 [details]
Bug 1466699 - Implement VRService thread

https://reviewboard.mozilla.org/r/252178/#review261948

lgtm! thanks.

::: gfx/vr/service/OpenVRSession.cpp:477
(Diff revision 7)
> +  mVRCompositor->GetCumulativeStats(&stats, sizeof(::vr::Compositor_CumulativeStats));
> +  // TODO - Need to send telemetry back to browser.  Refactor original gfxVROpenVR code:
> +  //   const uint32_t droppedFramesPerSec = (stats.m_nNumReprojectedFrames -
> +  //                                      mTelemetry.mLastDroppedFrameCount) / duration.ToSeconds();
> +  // Telemetry::Accumulate(Telemetry::WEBVR_DROPPED_FRAMES_IN_OPENVR, droppedFramesPerSec);
> +}

Why do we want to mark these lines? If we has some reasons and we will re-enable it later, please give a bug number at the comment.
Attachment #8986938 - Flags: review?(dmu) → review+
Comment on attachment 8986938 [details]
Bug 1466699 - Implement VRService thread

https://reviewboard.mozilla.org/r/252178/#review261948

> Why do we want to mark these lines? If we has some reasons and we will re-enable it later, please give a bug number at the comment.

I've updated the comment with the Bug number that will re-enable these lines.
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fda31bd96fa1
Implement VRService thread r=daoshengmu
https://hg.mozilla.org/mozilla-central/rev/fda31bd96fa1
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1473881
You need to log in before you can comment on or make changes to this bug.