Closed Bug 1182048 Opened 9 years ago Closed 8 years ago

Make webvr and e10s work together

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
e10s + ---
firefox42 --- affected
firefox46 --- verified

People

(Reporter: vlad, Assigned: kip)

References

Details

(Whiteboard: [webvr][vrm2])

Attachments

(2 files, 3 obsolete files)

MozReview Request: Bug 1182048 - Part 1: Allow e10s to be enabled
58 bytes, text/x-review-board-request
vlad
: review+
Details
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR
58 bytes, text/x-review-board-request
vlad
: review+
Details 
Right now, webvr disables access e10s, because we end up needing to access the HMD from both the content process and the compositor process.  We need to resolve this.  There are two ways we can do this, depending on whether we are allowed to talk to the HMD runtime from two different processes (only one of which is doing output).  This seems to be the case for both Oculus and OpenVR right now with the latest runtimes, since they all delegate the output of the HMD to a compositor process, and have a service that all apps interact with.

If we can talk to the HMD from two different processes, each frame should roughly look like this:

[content process] - read current/predicted HMD and sensor state
[content process] - do updates and requestAnimationFrame callbacks
[content process] - dispatch new layers update, including HMD pose used for rendering the frame
[compositor process] - take the HMD pose that frame was rendered with, do whatever postprocessing
[compositor process] - present frame to HMD

If we can only use the HMD from one process, then we need the HMD to be owned by the compositor process, with the content process asking for the render pose at the start of each frame.  That's not much different from the above, it just requires an IPC call to happen at the start.  Alternatively, the compositor can provide the next frame's pose after the layer updates are sent.
Assignee: nobody → vladimir
Whiteboard: [webvr]
Whiteboard: [webvr] → [webvr][vrm2]
Assignee: vladimir → kgilbert
Blocks: 1212486
Blocks: 1218482
Bug 1182048 - Part 1: Allow e10s to be enabled
- Removed code that disabled e10s when the dom.vr.enabled pref was
  set to true.
Bug 1182048 - Part 2: Add new protocol for ipc
* * *
Experimenting with threads..
Bug 1218482 - Enable WebVR By Default
- dom.vr.enabled is now "true" by default.
- dom.vr.add-test-devices is now "0" by default, so that content may detect
  VR devices accurately.
Attachment #8679018 - Attachment is obsolete: true
Uploaded WIP patches, will assign for review later today after a couple fixes:

- Doesn’t cleanly shutdown - need to review reference counting and check for memory leaks (look for FINDME KIP! HACK!’s)
- Need to split up patches to be more granular
- Need to format, adjust spacing to Mozilla code standards
Comment on attachment 8679016 [details]
MozReview Request: Bug 1182048 - Part 1: Allow e10s to be enabled

Bug 1182048 - Part 1: Allow e10s to be enabled
- Removed code that disabled e10s when the dom.vr.enabled pref was
  set to true.
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

Bug 1182048 - Part 2: Add new protocol for ipc
* * *
Experimenting with threads..
Comment on attachment 8679016 [details]
MozReview Request: Bug 1182048 - Part 1: Allow e10s to be enabled

Bug 1182048 - Part 1: Allow e10s to be enabled
- Removed code that disabled e10s when the dom.vr.enabled pref was
  set to true.
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

Bug 1182048 - Part 2: Add new protocol for ipc
* * *
Experimenting with threads..
The updated patchset in Comment 8 includes a couple of fixes:
- Cardboard VR fixed
- Navigator.GetVRDevices now returns the same Javascript object in multiple calls for VR Devices that have not been remove since the last call to GetVRDevices.  Previously returned a new object wrapping the same device.
These patches are now working in OSX with Oculus 0.5, Windows with Oculus 0.7 and Cardboard VR.

I'm splitting the patches now and cleaning up for review.
Comment on attachment 8679016 [details]
MozReview Request: Bug 1182048 - Part 1: Allow e10s to be enabled

Bug 1182048 - Part 1: Allow e10s to be enabled
- Removed code that disabled e10s when the dom.vr.enabled pref was
  set to true.
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

Bug 1182048 - Part 2: Add new protocol for ipc
* * *
Experimenting with threads..
Comment on attachment 8679016 [details]
MozReview Request: Bug 1182048 - Part 1: Allow e10s to be enabled

Bug 1182048 - Part 1: Allow e10s to be enabled
- Removed code that disabled e10s when the dom.vr.enabled pref was
  set to true.
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

Bug 1182048 - Part 2: Implement e10s support for WebVR
Attachment #8679017 - Attachment description: MozReview Request: Bug 1182048 - Part 2: Add new protocol for ipc → MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR
Comment on attachment 8679016 [details]
MozReview Request: Bug 1182048 - Part 1: Allow e10s to be enabled

Bug 1182048 - Part 1: Allow e10s to be enabled
- Removed code that disabled e10s when the dom.vr.enabled pref was
  set to true.
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

Bug 1182048 - Part 2: Implement e10s support for WebVR
I have done some testing on shutting down Firefox while viewing a WebVR site that has activated the Oculus sensor tracking and found that if garbage collection can delete the VRDevice on the content side too late, after the compositor has already shut down.  This resulted in an unclean shutdown as the IPC to stop sensor tracking could not be completed on a disconnected protocol.

I have implemented a slightly different approach in the patches added in Comment 16.  Now, we send a "KeepSensorTracking" message (asynchronously) when content requests a new sensor state update, which sets a counter and starts the sensor tracking.  When this counter runs down to 0, the sensor tracking is shut down.  Effectively, sensor tracking is enabled automatically when reading a sensor state and remains enabled for up to a couple of seconds.  Content is usually continuously polling the sensor state every frame, so this keeps the tracking active.  When content is done with the sensors, it simply stops sending "KeepSensorTracking" messages.
Comment on attachment 8679016 [details]
MozReview Request: Bug 1182048 - Part 1: Allow e10s to be enabled

Bug 1182048 - Part 1: Allow e10s to be enabled
- Removed code that disabled e10s when the dom.vr.enabled pref was
  set to true.
Attachment #8679016 - Flags: review?(vladimir)
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

Bug 1182048 - Part 2: Implement e10s support for WebVR
Attachment #8679017 - Flags: review?(vladimir)
I have assigned Vlad as the first reviewer; however, as IPDL has been changed this will require at least two r+'es.

I would appreciate any feedback on this overall approach as well as the naming of the classes.  I have added assertions to ensure that the classes are used on the correct threads; however, any ideas on better naming to more clearly differentiate the classes would be welcomed.

I will continue testing on each platform with a focus on memory leaks, refcounting, and hot swapping events.
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

https://reviewboard.mozilla.org/r/23337/#review21925

Overall looks great, some nits/comments below.  The only big question I have is with regards to deviceIndex -- is there a guarantee that the device index will refer to the appropriate device in light of devices being added/removed while VR mode is active?  That is, will the index in the layer be correct?

Also, for patches of this size in the future, splitting them up into multiple steps would be helpful -- e.g. adding the ipdl interfaces, implementing parent/child, then using them, implementing device list updates, etc.  Otherwise there are a lot of hunks that are just mechanical changes or slight interface fixups that aren't really relevant to the overall review, but that clutter up the review request.

::: dom/base/nsDocument.cpp:11577
(Diff revision 6)
>  ReleaseHMDInfoRef(void *, nsIAtom*, void *aPropertyValue, void *)

Change the name of this method to match the type that it's releasing

::: dom/vr/VRDevice.h:214
(Diff revision 6)
> +           RefPtr<gfx::VRDeviceProxy> aHMD,

Don't pass RefPtr<> arguments; just use VRDeviceProxy*.

::: dom/vr/VRDevice.h:251
(Diff revision 6)
> -  HMDVRDevice(nsISupports* aParent, gfx::VRHMDInfo* aHMD)
> +  HMDVRDevice(nsISupports* aParent, RefPtr<gfx::VRDeviceProxy> aHMD)

Don't pass RefPtr<> arguments; just pass a VRDeviceProxy *.

::: dom/vr/VRDevice.h:282
(Diff revision 6)
> -  explicit PositionSensorVRDevice(nsISupports* aParent)
> +  explicit PositionSensorVRDevice(nsISupports* aParent, RefPtr<gfx::VRDeviceProxy> aHMD)

no RefPtr<> argument

::: dom/vr/VRDevice.h:292
(Diff revision 6)
> +  HMDPositionVRDevice(nsISupports* aParent, RefPtr<gfx::VRDeviceProxy> aHMD);

no RefPtr<>

::: dom/vr/VRDevice.cpp:34
(Diff revision 6)
> +  nsTArray<RefPtr<VRDevice> > devices;

No need for "> >", we're using modern compilers

::: dom/vr/VRDevice.cpp:53
(Diff revision 6)
> +          gfx::VRDeviceInfo::State_Orientation)) {

nit: fix up indentation, and { on line by itself with multi-line if() statements [ so that when scanning down, the if clause doesn't blend in to the block statements ]

::: dom/vr/VRDevice.cpp:338
(Diff revision 6)
> +  HMDPositionVRDevice::GetState()

I think there are extraneous spaces before HMDPositionVRDevice:: here, but I can't quite tell with reviewboard

::: dom/vr/VRDevice.cpp:347
(Diff revision 6)
> +  HMDPositionVRDevice::GetImmediateState()

same spaces

::: gfx/layers/Layers.h:2186
(Diff revision 6)
> -  RefPtr<gfx::VRHMDInfo> mHMDInfo;
> +  uint32_t mVRDeviceIndex;

device index? Is this index just always unique and increasing? Or guaranteed to remain refererring to the same device at any given time?

::: gfx/vr/VRDeviceProxy.cpp:47
(Diff revision 6)
> +  } else if (mDeviceInfo.mScreenRect.width && mDeviceInfo.mScreenRect.height) {

You can nuke the FAKE_WEBVR_SCREEN stuff... that was really just for testing.  Unless you found it useful :)

::: gfx/vr/VRManager.cpp:159
(Diff revision 6)
> +    } else if(oldDevice->GetDeviceInfo() != device->GetDeviceInfo()) {

nit: missing space after if(); but you also don't need the "else" at all due to the breaks.

::: gfx/vr/ipc/PVRManager.ipdl:20
(Diff revision 6)
> +  async RefreshDevices();

Add some comments describing what each of these messages do

Ov
Attachment #8679017 - Flags: review?(vladimir)
Attachment #8679016 - Flags: review?(vladimir) → review+
Comment on attachment 8679016 [details]
MozReview Request: Bug 1182048 - Part 1: Allow e10s to be enabled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23335/diff/6-7/
Attachment #8679017 - Flags: review?(vladimir)
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23337/diff/6-7/
Please note that the updated patch in Comment 24 fixes a startup crash in B2G.
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

https://reviewboard.mozilla.org/r/23337/#review22485

So this all looks fine -- the only remaining question I have is with the VR Manager protocol.  As written, I *think* this creates an entirely new communication channel between parent and child for VR.  I'm not sure if that's necessary; can VR be a sub-protocol of one of the existing top level ones, such as PBackground, or even PImageBridge?
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #26)
> Comment on attachment 8679017 [details]
> MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR
> 
> https://reviewboard.mozilla.org/r/23337/#review22485
> 
> So this all looks fine -- the only remaining question I have is with the VR
> Manager protocol.  As written, I *think* this creates an entirely new
> communication channel between parent and child for VR.  I'm not sure if
> that's necessary; can VR be a sub-protocol of one of the existing top level
> ones, such as PBackground, or even PImageBridge?

Thanks for reviewing, Vlad!

The intent is to instantiate one VRManagerChild per content process and reduce IPC traffic as the same messages will be broadcast.  PImageBridge wasn't chosen as a parent protocol as its parent runs in a separate thread than the compositor.

Initially, I attempted to create a sub-protocol of PCompositor; however, my means to reach the correct VRManagerChild from nsGlobalWindow felt hack-ish.  Removing the association between PCompositor and PVRManager cleaned this up a fair bit.
Please note that I am still trying to hurd some memory leaks found in my try runs.  Links to try runs have been appearing in Bug 1218482.  Once these are clear, I will assign these patches to review for security of the IPDL changes and by a DOM peer for the enablign in Bug 1218482.
https://reviewboard.mozilla.org/r/23337/#review21925

> device index? Is this index just always unique and increasing? Or guaranteed to remain refererring to the same device at any given time?

DeviceIndex will always be the same for the same device, unless the device is disconnected or powered off.  When re-connected or powered on, it will get a new (incrementing) DeviceIndex

> You can nuke the FAKE_WEBVR_SCREEN stuff... that was really just for testing.  Unless you found it useful :)

Nuked
https://reviewboard.mozilla.org/r/23337/#review21925

> DeviceIndex will always be the same for the same device, unless the device is disconnected or powered off.  When re-connected or powered on, it will get a new (incrementing) DeviceIndex

I have renamed all instances of "DeviceIndex" to "DeviceID" to better describe this behavior, as per our conversation on Slack.

> Add some comments describing what each of these messages do

Added some comments
Comment on attachment 8679016 [details]
MozReview Request: Bug 1182048 - Part 1: Allow e10s to be enabled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23335/diff/7-8/
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23337/diff/7-8/
Attachment #8679017 - Flags: review?(vladimir)
Comment on attachment 8679016 [details]
MozReview Request: Bug 1182048 - Part 1: Allow e10s to be enabled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23335/diff/8-9/
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23337/diff/8-9/
ImageBridgeChild, which I used as an example while implementing VRManagerChild, is reported as leaked during Mochitests as well (Bug 1117203).  I have applied a similar fix as in the patch for Bug 1117203 for VRManagerChild and corrected a refcount issue when assigning to sVRManagerChildSingleton.

I have pushed to try to verify the fix and identify any remaining memory leaks.
The try push on 2015-11-18 shows that the leaks were fixed except for an IPC::Protocol and StoreRef (allocated by IPC Protocol).

It appears that top level protocols must asynchronously destroy their Transport (IPC::Channel) on both the Child and Parent sides within their destructors.  Pattern like this: XRE_GetIOMessageLoop()->PostTask(FROM_HERE, new DeleteTask<Transport>(GetTransport()));

I have refactored VRManagerParent to eliminate the redundant mTransport member (ITopLevelProtocol has mTrans) and to delete the transport on the child side.

Pushing to try again to see if this fixes it and if there are any more leaks or issues I haven't caught.
Comment on attachment 8679016 [details]
MozReview Request: Bug 1182048 - Part 1: Allow e10s to be enabled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23335/diff/9-10/
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23337/diff/9-10/
The last try push showed that the Transport (IPC::Channel) object leaks have been fixed; however, there are still leaks when the e10s content process crashes.  The /dom/ipc/tests mochitests are reporting leaks of PVRManagerParent's and the objects they hold in these cases.

I have pushed an updated patch to correct this to the try server.
Comment on attachment 8679016 [details]
MozReview Request: Bug 1182048 - Part 1: Allow e10s to be enabled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23335/diff/10-11/
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23337/diff/10-11/
https://reviewboard.mozilla.org/r/23337/#review22485

(Question about sub-protocol was replied in bugzilla)

The last narrow-focused try push indicated that the memory leaks have been resolved; I am doing a full try push for all platforms now to verify that there aren't any more platform specific leaks or other tests failing.

Do you have any more changes required for these patches before I pass on to the next reviewer?
Previous try run appears to show that the leaks have been fixed; however, there were a number of failures for Windows 7 debug.  I am running a try run and attempting to reproduce locally to verify if these are co-incidental.
I was able to reproduce many of the Windows 7 debug try run failures locally, without the VR e10s patches applied.  I have pushed a try run without my patches as a controlled test:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=66c9984ecf0f
As VRMessageUtils introduces new types serialized with IPC, we will require two reviews from module owners.

https://developer.mozilla.org/en-US/docs/Mozilla/IPDL/Type_Serialization

“Serializers and deserializers are security-sensitive and must always receive two reviews from module owners who understand IPC serialization well.”
Latest try runs are indicating that Windows Debug builds are timing out on some tests with these patches applied, likely due to the browser not closing cleanly.  I am investigating further...
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

https://reviewboard.mozilla.org/r/23337/#review23795

Looks fine to me. Any non-major issues we can do in followups.
Attachment #8679017 - Flags: review?(vladimir) → review+
Comment on attachment 8679016 [details]
MozReview Request: Bug 1182048 - Part 1: Allow e10s to be enabled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23335/diff/11-12/
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23337/diff/11-12/
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23337/diff/12-13/
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

I have updated the patches based on feedback on the IPC serialization review from Froydnj, replacing enumerated and flag types with specializations that validate the expected values.

Froydnj: Could you please review the diff from revision 13 to 14, which contains your recommended changes?
Attachment #8679017 - Flags: review?(nfroyd)
(In reply to :kip (Kearwood Gilbert) from comment #60)
> Comment on attachment 8679017 [details]
> MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR
> 
> I have updated the patches based on feedback on the IPC serialization review
> from Froydnj, replacing enumerated and flag types with specializations that
> validate the expected values.
> 
> Froydnj: Could you please review the diff from revision 13 to 14, which
> contains your recommended changes?

Sorry, the changes are in revision 12
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

Changes look good to me, thanks!
Attachment #8679017 - Flags: review?(nfroyd) → review+
Blocks: 1196366
Comment on attachment 8679016 [details]
MozReview Request: Bug 1182048 - Part 1: Allow e10s to be enabled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23335/diff/12-13/
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23337/diff/12-13/
Attachment #8679017 - Flags: review+
Comment on attachment 8679017 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23337/diff/13-14/
(In reply to :kip (Kearwood Gilbert) from comment #70)
> Comment on attachment 8679017 [details]
> MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/23337/diff/13-14/
(In reply to :kip (Kearwood Gilbert) from comment #69)
> Comment on attachment 8679017 [details]
> MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/23337/diff/12-13/

These updates remove the "Stop()" method from the PVRManager protocol and code that references it, as VRManagerParent::ActorDestroy already does everything needed to shut down.  This fixes a hang on shutdown with the Windows xpcshell tests and Windows mochitests.

This try push includes this version of the patchset:

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

Additionally, it was pushed to try together with the patch in Bug 1218482 to enable WebVR by default:

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

The remaining failed tests don't appear to be related to this patchset.
Blocks: 1230352
Comment on attachment 8698590 [details]
MozReview Request: Bug 1182048 - Part 1: Allow e10s to be enabled

Same patch as before; rebased.
Comment on attachment 8698591 [details]
MozReview Request: Bug 1182048 - Part 2: Implement e10s support for WebVR

Same patch as before, rebased.
Attachment #8679016 - Attachment is obsolete: true
Attachment #8679017 - Attachment is obsolete: true
Blocks: 1196507
Attachment #8698591 - Flags: review?(vladimir) → review+
Comment on attachment 8698590 [details]
MozReview Request: Bug 1182048 - Part 1: Allow e10s to be enabled

This all was r+'d before -- you don't need rereview unless you made massive changes.
Attachment #8698590 - Flags: review?(vladimir) → review+
https://hg.mozilla.org/mozilla-central/rev/04b05c76db2e
https://hg.mozilla.org/mozilla-central/rev/03e3553c83be
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1235803
Confirming this fix on latest 46 Nightly, build ID: 20160105030211.
Tested with an Oculus DK2 kit on Windows 10x64 using the mozvr.com examples.
Status: RESOLVED → VERIFIED
status-firefox46: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: