Closed Bug 1316279 Opened 3 years ago Closed 3 years ago

[webvr] WebVR in out-of-process compositing mode.

Categories

(Core :: Graphics, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: daoshengmu, Assigned: daoshengmu)

References

Details

(Whiteboard: [webvr][gfx-noted])

Attachments

(4 files)

That is a regression bug related to GPU process works (Bug 1264543). We will see “WebVR supported, but no VRDisplays found." from https://webvr.info/samples/04-simple-mirroring.html. If we disable e10s, it will be good.

That is because mVRDisplays and mControllerManagers in VRManager are not initialized successfully when VR manager is running at an individual GPU process.
Blocks: 1298612
Whiteboard: [webvr]
Depends on: e10s-gpu-win, 1314133
Priority: -- → P3
Whiteboard: [webvr] → [webvr][gfx-noted]
mVRDisplays and mControllerManagers of VRManager are not initialized from VRManager::ManagerInit()

We suppose VRManager::ManagerInit() would be called at GPUParent::Init() when we are launching GPU process. But, it seems to not happen so...
I think I find out the root cause. This is because we use gfxPrefs::VREnabled(), !gfxPrefs::VROpenVREnabled for checking if we enable some configs. But gfxPrefs seems to not work in OOP compositing. Do we have any approach to fix this kind of problem?
Flags: needinfo?(dvander)
gfxPrefs does work in the GPU process, but Preferences does not... could stuff like this not be working?

http://searchfox.org/mozilla-central/source/gfx/vr/gfxVROpenVR.cpp#103

There could be other XPCOM things missing for VR too, but Preferences is the most obvious. Maybe we should add nsCString support to gfxPrefs and move VR to that.
Flags: needinfo?(dvander)
(In reply to David Anderson [:dvander] from comment #3)
> gfxPrefs does work in the GPU process, but Preferences does not... could
> stuff like this not be working?
> 
> http://searchfox.org/mozilla-central/source/gfx/vr/gfxVROpenVR.cpp#103
> 
> There could be other XPCOM things missing for VR too, but Preferences is the
> most obvious. Maybe we should add nsCString support to gfxPrefs and move VR
> to that.

I am not sure why gfxPrefs::VREnabled() and gfxPrefs::VROpenVREnabled are not true because I have adjusted them in about:config, and non-e10s mode is good.

nsCString in Preferences is an another issue. Like you said, I can't get my dll path from http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/gfx/vr/gfxVROpenVR.cpp#103.
(In reply to Daosheng Mu[:daoshengmu] from comment #4)
> (In reply to David Anderson [:dvander] from comment #3)
> > gfxPrefs does work in the GPU process, but Preferences does not... could
> > stuff like this not be working?
> > 
> > http://searchfox.org/mozilla-central/source/gfx/vr/gfxVROpenVR.cpp#103
> > 
> > There could be other XPCOM things missing for VR too, but Preferences is the
> > most obvious. Maybe we should add nsCString support to gfxPrefs and move VR
> > to that.
> 
> I am not sure why gfxPrefs::VREnabled() and gfxPrefs::VROpenVREnabled are
> not true because I have adjusted them in about:config, and non-e10s mode is
> good.
> 
> nsCString in Preferences is an another issue. Like you said, I can't get my
> dll path from
> http://searchfox.org/mozilla-central/rev/
> 4b6cab91f93c73ae591dafaea40fd5704b41810e/gfx/vr/gfxVROpenVR.cpp#103.

Ah, the reason might be something silly. We initialize VRManager here:

http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/gfx/ipc/GPUParent.cpp#108

But this occurs before we actually get the gfxPrefs blob, here:

http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/gfx/ipc/GPUParent.cpp#147

So I think we need to move VRManager initialization to the end of RecvInit. We still need to fix places that use Preferences, though.
Flags: needinfo?(dmu)
(In reply to David Anderson [:dvander] from comment #5)
> (In reply to Daosheng Mu[:daoshengmu] from comment #4)
> > (In reply to David Anderson [:dvander] from comment #3)
> > > gfxPrefs does work in the GPU process, but Preferences does not... could
> > > stuff like this not be working?
> > > 
> > > http://searchfox.org/mozilla-central/source/gfx/vr/gfxVROpenVR.cpp#103
> > > 
> > > There could be other XPCOM things missing for VR too, but Preferences is the
> > > most obvious. Maybe we should add nsCString support to gfxPrefs and move VR
> > > to that.
> > 
> > I am not sure why gfxPrefs::VREnabled() and gfxPrefs::VROpenVREnabled are
> > not true because I have adjusted them in about:config, and non-e10s mode is
> > good.
> > 
> > nsCString in Preferences is an another issue. Like you said, I can't get my
> > dll path from
> > http://searchfox.org/mozilla-central/rev/
> > 4b6cab91f93c73ae591dafaea40fd5704b41810e/gfx/vr/gfxVROpenVR.cpp#103.
> 
> Ah, the reason might be something silly. We initialize VRManager here:
> 
> http://searchfox.org/mozilla-central/rev/
> 886d5186f0598ab2f8a9953eb5a4dab9750ef834/gfx/ipc/GPUParent.cpp#108
> 
> But this occurs before we actually get the gfxPrefs blob, here:
> 
> http://searchfox.org/mozilla-central/rev/
> 886d5186f0598ab2f8a9953eb5a4dab9750ef834/gfx/ipc/GPUParent.cpp#147
> 
> So I think we need to move VRManager initialization to the end of RecvInit.
> We still need to fix places that use Preferences, though.

After moving VRManager initialization to the end of RecvInit, we can use gfxPrefs::VREnabled() and gfxPrefs::VROpenVREnabled. Just remaining the nsCString support of Preferences for getting the dll path.
Flags: needinfo?(dmu)
What's the expected ETA of fixing this? WebVR is not working for users anymore in Firefox Nightly (unless they know to and aware of having to disable multiprocessing).
Flags: needinfo?(dmu)
Depends on: 1318618
Flags: needinfo?(dmu)
(In reply to Christopher Van Wiemeersch [:cvan] from comment #7)
> What's the expected ETA of fixing this? WebVR is not working for users
> anymore in Firefox Nightly (unless they know to and aware of having to
> disable multiprocessing).

I just file Bug 1318618 to trace it, and I am going to jump in to fix it. I hope it would be resolved next week.
I have confirmed that we have no other issues besides Bug 1318618. After Bug 1318618 is resolved, this bug can be resolved as well.
Assignee: nobody → dmu
(In reply to Daosheng Mu[:daoshengmu] from comment #10)
> Created attachment 8813487 [details]
> Bug 1316279 - Fix gfxPrefs support for WebVR in out-of-process compositing;
> 
> Review commit: https://reviewboard.mozilla.org/r/94902/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/94902/

Kip, Should I need to modify the way you get ovr_lib in gfxVROculus?, https://dxr.mozilla.org/mozilla-central/rev/0534254e9a40b4bade2577c631fe4cfa0b5db41d/gfx/vr/gfxVROculus.cpp#167.

I am not sure if "dom.vr.ovr_lib_path" and "dom.vr.ovr_lib_name" are unused now? Because I don't see you define it in all.js.
(In reply to Daosheng Mu[:daoshengmu] from comment #11)
> (In reply to Daosheng Mu[:daoshengmu] from comment #10)
> > Created attachment 8813487 [details]
> > Bug 1316279 - Fix gfxPrefs support for WebVR in out-of-process compositing;
> > 
> > Review commit: https://reviewboard.mozilla.org/r/94902/diff/#index_header
> > See other reviews: https://reviewboard.mozilla.org/r/94902/
> 
> Kip, Should I need to modify the way you get ovr_lib in gfxVROculus?,
> https://dxr.mozilla.org/mozilla-central/rev/
> 0534254e9a40b4bade2577c631fe4cfa0b5db41d/gfx/vr/gfxVROculus.cpp#167.
> 
> I am not sure if "dom.vr.ovr_lib_path" and "dom.vr.ovr_lib_name" are unused
> now? Because I don't see you define it in all.js.

We can remove dom.vr.ovr_lib_path and dom.vr.ovr_lib_name.  I don't believe anyone has needed to set these to get Oculus running with the current Oculus runtimes.
(In reply to :kip (Kearwood Gilbert) from comment #12)
> (In reply to Daosheng Mu[:daoshengmu] from comment #11)
> > (In reply to Daosheng Mu[:daoshengmu] from comment #10)
> > > Created attachment 8813487 [details]
> > > Bug 1316279 - Fix gfxPrefs support for WebVR in out-of-process compositing;
> > > 
> > > Review commit: https://reviewboard.mozilla.org/r/94902/diff/#index_header
> > > See other reviews: https://reviewboard.mozilla.org/r/94902/
> > 
> > Kip, Should I need to modify the way you get ovr_lib in gfxVROculus?,
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 0534254e9a40b4bade2577c631fe4cfa0b5db41d/gfx/vr/gfxVROculus.cpp#167.
> > 
> > I am not sure if "dom.vr.ovr_lib_path" and "dom.vr.ovr_lib_name" are unused
> > now? Because I don't see you define it in all.js.
> 
> We can remove dom.vr.ovr_lib_path and dom.vr.ovr_lib_name.  I don't believe
> anyone has needed to set these to get Oculus running with the current Oculus
> runtimes.

We will require one string preference for HTC Vive / OpenVR, "gfx.vr.openvr-runtime":

https://dxr.mozilla.org/mozilla-central/source/gfx/vr/gfxVROpenVR.cpp#103
Comment on attachment 8813487 [details]
Bug 1316279 - Part 1: Fix gfxPrefs support for WebVR in out-of-process compositing;

https://reviewboard.mozilla.org/r/94902/#review95370

r=me with the small nit on alphabetical order in gfxPrefs.h

::: gfx/thebes/gfxPrefs.h:323
(Diff revision 1)
>    DECL_GFX_PREF(Live, "dom.meta-viewport.enabled",             MetaViewportEnabled, bool, false);
>    DECL_GFX_PREF(Once, "dom.vr.enabled",                        VREnabled, bool, false);
>    DECL_GFX_PREF(Once, "dom.vr.oculus.enabled",                 VROculusEnabled, bool, true);
>    DECL_GFX_PREF(Once, "dom.vr.openvr.enabled",                 VROpenVREnabled, bool, false);
>    DECL_GFX_PREF(Once, "dom.vr.osvr.enabled",                   VROSVREnabled, bool, false);
> +  DECL_GFX_PREF(Once, "gfx.vr.openvr-runtime",                 VROpenVRRuntime, std::string, "");

nit: Move openvr-runtime pref up one line to maintain alphabetical order.
Attachment #8813487 - Flags: review?(kgilbert) → review+
The Oculus CV1 with firmware 78/34a904e8da and the software from Oculus downloaded and installed 23/11 do not work with Nightly 53.0a1 (2016-11-23) (64-bit)

Very frustrating as no WebVR can be tested in the headset. Chromium has the same symptom.

Headset works perfectly in Oculus and SteamVR.

Head tracking works.
(In reply to philippeback from comment #15)
> The Oculus CV1 with firmware 78/34a904e8da and the software from Oculus
> downloaded and installed 23/11 do not work with Nightly 53.0a1 (2016-11-23)
> (64-bit)
> 
> Very frustrating as no WebVR can be tested in the headset. Chromium has the
> same symptom.
> 
> Headset works perfectly in Oculus and SteamVR.
> 
> Head tracking works.

Dreaded WebVR Supported but no display found.
Come on, that cannot be like this, how are U guys testing this thing?
(In reply to philippeback from comment #16)
> (In reply to philippeback from comment #15)
> > The Oculus CV1 with firmware 78/34a904e8da and the software from Oculus
> > downloaded and installed 23/11 do not work with Nightly 53.0a1 (2016-11-23)
> > (64-bit)
> > 
> > Very frustrating as no WebVR can be tested in the headset. Chromium has the
> > same symptom.
> > 
> > Headset works perfectly in Oculus and SteamVR.
> > 
> > Head tracking works.
> 
> Dreaded WebVR Supported but no display found.
> Come on, that cannot be like this, how are U guys testing this thing?

Automated testing to reduce chances of regressions like this is a high priority in the next quarter.  For now, may I suggest trying Firefox Dev Edition which is a bit farther from the bleeding edge to reduce your exposure?  WebVR and Oculus support are enabled by default in Dev Edition as well.
Attached image 24-11-16 09-49-40.png
That's where the current Firefox Dev Edition I downloaded this morning crashes in the panorama WebVR sample.

The no display message is gone but same "Firefox takes a long time to load".
Ok loaded Dev Edition, loaded the WebVR panorama sample. No more No Display but "Firefox takes a long time to load" in the headset.

Also, see https://bug1316279.bmoattachments.org/attachment.cgi?id=8814006 for a screenshot in VS2015 whith symbols I tried to add from mozilla. But it still had an issue there b/c of mozalloc_abort.cpp not being found or something.

I dug into the disassembly, there seems to be something usable at the bottom.
(In reply to philippeback from comment #20)
> Created attachment 8814007 [details]
> FFX Crash in Entering VR

Thanks for reporting the crashes.  Could we move this to a separate bug so it can be tracked once this bug with e10s compatibility has been closed?
It now works in Nightly with:

about:config

browser.tabs.remote.autostart.2 set to false.
openvr.enabled set to false


Enable multi process Nightly unchecked

Then I was able to use all samples, including stuff from aframe.io

Life is great now. Hope to get no further regressions.

Ah yes, spatial sound didn't sound so spatial. Is there a more comprehensive test I can run?
Keywords: checkin-needed
Keywords: checkin-needed
(In reply to Daosheng Mu[:daoshengmu] from comment #24)
> Created attachment 8814334 [details]
> Bug 1316279 - Part 2: Fix gfxPrefs::PrefGet string when using default value;
> 
> Review commit: https://reviewboard.mozilla.org/r/95608/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/95608/

dvander, I find a bug at gfxPrefs::PrefGet string when users didn't set the value. Please help me review it. thanks.
Comment on attachment 8814334 [details]
Bug 1316279 - Part 2: Fix gfxPrefs::PrefGet string when using default value;

https://reviewboard.mozilla.org/r/95608/#review96148

::: gfx/thebes/gfxPrefs.cpp:200
(Diff revision 1)
>    MOZ_ASSERT(IsPrefsServiceAvailable());
>  
>    nsAdoptingCString result;
>    Preferences::GetCString(aPref, &result);
> +
> +  if (result.IsEmpty())

nit: need braces here
Attachment #8814334 - Flags: review?(dvander) → review+
Follow Comment 28 to add a brace at if(result.IsEmpty())
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/48164669baec
Part 1: Fix gfxPrefs support for WebVR in out-of-process compositing; r=kip
https://hg.mozilla.org/integration/autoland/rev/211067e97639
Part 2: Fix gfxPrefs::PrefGet string when using default value; r=dvander
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/48164669baec
https://hg.mozilla.org/mozilla-central/rev/211067e97639
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Question: Are the following settings

about:config

browser.tabs.remote.autostart.2 set to false.
openvr.enabled set to false

Enable multi process Nightly unchecked

Still to be put that way?

Basically, how can I test in Nightly with a fresh set of settings?
(In reply to philippeback from comment #32)
> Question: Are the following settings
> 
> about:config
> 
> browser.tabs.remote.autostart.2 set to false.
> openvr.enabled set to false
> 
> Enable multi process Nightly unchecked
> 
> Still to be put that way?
> 
> Basically, how can I test in Nightly with a fresh set of settings?

Hi,

You can help us test it by

browser.tabs.remote.autostart.2 set to false.
openvr.enabled set to true

Enable multi process Nightly checked

Please feel free to give us feedback.
Interestingly,the Enable multi process Nightly is grayed out in the Nightly I have.

53.0a1 (2016-11-30) (64-bit)
I can enter VR and leave it without trouble with these settings:

browser.tabs.remote.autostart.2 set to false.
openvr.enabled set to true


But cannot check the Enable multi process Nighly checkbox, sorry.
You need to log in before you can comment on or make changes to this bug.