Closed Bug 1406327 Opened 7 years ago Closed 7 years ago

Detect VR content to manage VR listener thread

Categories

(Core :: WebVR, defect, P1)

Product:
Core
Component:
WebVR
Type:
defect

Tracking

()

Status:
RESOLVED INCOMPLETE
Tracking Flags:
Tracking Status
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- affected

People

(Reporter: daoshengmu, Assigned: daoshengmu)

References

Details

Attachments

(6 files)

Bug 1406327 - Part 1: Shutdown VR listener thread when no VR content in seconds;
59 bytes, text/x-review-board-request
kip
: review+
dvander
: review+
Details
Bug 1406327 - Part 2: When loading VR content, launching the VR listener thread;
59 bytes, text/x-review-board-request
kip
: review+
Details
Bug 1406327 - Part 3: VRSystemManager for multi-threads;
59 bytes, text/x-review-board-request
kip
: review+
Details
Bug 1406327 - Part 4: RefreshVRDisplays needs to be at VRListenerThread;
59 bytes, text/x-review-board-request
kip
: review+
Details
Bug 1406327 - Part 5: Checking VRListenerThread is shutdowning.
59 bytes, text/x-review-board-request
Details
Bug 1406327 - Part 6: Avoid other threads to access the same member data simultaneously(wip).
59 bytes, text/x-review-board-request
Details 
In Bug 1392216, we start launching VR listener thread when Compositor thread is launched. We can do more optimize to let it only be launched when VR content is detected and shut down the thread when no WebVR content is present.
See Also: → 1392216
Blocks: 1392215
Assignee: nobody → dmu
Comment on attachment 8920086 [details]
Bug 1406327 - Part 1: Shutdown VR listener thread when no VR content in seconds;

https://reviewboard.mozilla.org/r/191088/#review196282

::: gfx/vr/VRManager.cpp:315
(Diff revision 1)
> -    DispatchVRDisplayInfoUpdate();
> +   // DispatchVRDisplayInfoUpdate();
> +    MessageLoop* loop = CompositorThreadHolder::Loop();
> +    loop->PostTask(
> +      NewRunnableMethod("gfx::VRManager::DispatchVRDisplayInfoUpdate",
> +                        this,
> +                        &VRManager::DispatchVRDisplayInfoUpdate));

When receving event changes, we need to send them to Compositor thread to ask it to post these message back via PVRManager because PVRManager is bound in Compositor thread.

::: gfx/vr/ipc/VRManagerParent.cpp:121
(Diff revision 1)
>  
>  /*static*/ VRManagerParent*
>  VRManagerParent::CreateSameProcess()
>  {
> -  MessageLoop* loop = VRListenerThreadHolder::Loop();
> +  MessageLoop* loop = CompositorThreadHolder::Loop();
>    RefPtr<VRManagerParent> vmp = new VRManagerParent(base::GetCurrentProcId(), false);

I need to change the message loop back to CompositorThreadHolder because PVRManager channel needs to be constructed when GPU process is created. After we have our own VR process, the message loop would be replaced with the main thread of VR process.

::: gfx/vr/ipc/VRManagerParent.cpp:172
(Diff revision 1)
> -  MOZ_ASSERT(VRListenerThreadHolder::IsInVRListenerThread());
> +  if (!VRListenerThreadHolder::IsActive()) {
> +    RefPtr<Runnable> runnable = NewRunnableMethod(
> +      "gfx::VRManagerParent::StartVRListenerThread",
> +      this, &VRManagerParent::StartVRListenerThread);
> +    NS_DispatchToMainThread(runnable.forget());
> +  }

I start the VR listener thread only when receiving RecvRefreshDisplays() command.
:kip, could you give me some feedback about my wip patch at Comment 2. I am not sure if it is a good way to go because I still have to make PVRManager be bound with Compositor thread. Thanks.
Flags: needinfo?(kgilbert)
I think we can discuss the threads we need for this VR process work more deeply.

E10s mode:

For the VR process, the main thread of VR process needs for connecting PVRManager IPC with other processes. We will use VRListenerThread for polling events from VR devices. Then, replying the messages in the main thread to the content process. We also can create a VRSumbitThread to submit frames, besides, the rendering context needs to be created at this VRSumbitThread as well because the draw calls have to at the same thread with the 3D resource creation.

nonE10s mode:
Because we only have one process. We will use the main thread of the main process to construct PVRManager IPC. Then, creating VRListenThread and VRSumbitThread to do the similar things like above. 


The creation timing of VR process:
We can consider to create the VR process at VRManagerChild::RefreshVRDisplaysWithCallback(). When loading VR content, it is going to seeking for the available VRDisplays there, then, we start to create the VR process, then constructing the connection of PVRManager IPC. In the case of nonE10s, we will create the IPC connection when the main process is launched as the present.
I think this is a good start and the patch is in the right direction.  As you mention, we need to do all of the rendering in the VRSubmitThread; however, this no longer needs to be the same thread (or process) as the compositor since we are no longer using PTexture for the frame submission.

There might be some problems with spawning processes directly from the content process due to sandboxing.  Perhaps we could keep PVRManager in the main process as it is.  VRManagerParent could then spawn the VR process as soon as it sees a request to enumerate VR displays.  This will mean we need to forward requests from the main process to the VR process.  This might require a new protocol, or perhaps we could recycle the current one for the VR process.

I would like to take Bug 1346927 (lock-free shared memory structure) and implement it after this change lands.  In bug 1346927, I intend to remove many of the functions in PVRManager, replacing them with a function that simply sets up the communication with shared memory.  A block of shared memory would be allocated for each content process by the process owning VRManagerParent.  VRManagerParent will send messages to the content process and the VR process individually to tell them about the location of the shared memory.  Once the communication is set up, the content process and the VR process can communicate directly by writing to the shared memory and polling for updates from the other end.  to  I will start with the VR rendering first, then break out the controller updates to a second bug which will follow the same approach.

You can leave PVRManager in the compositor thread in nonE10s mode.  Once Bug 1346927 lands, we won't need to use PVRManager every frame and shouldn't get frames delayed from non-vr messages in the Compositor thread's queue.  Also the shared memory approach will still work, but just between the two threads in the same process.

I hope to start on Bug 1346927 early next week -- we can coordinate the details then.
Flags: needinfo?(kgilbert)
I have updated some works for shutting down the thread and confirmed it works properly in HTC Vive. I still need to do more tests in Oculus Rift to see if there is any wrong implementations. It has been very close to be finished.
Currently, it can create VRListener thread automatically when loading VR content and shutdown the thread after timeout 30 secs. It works good in OpenVR But in Oculus, it will get "VROculusSession not thread-safe" message when StopPresentation() or closing Firefox. I also notice I will get some crash in tests about " application crashed [@ MessageLoop::PostTask_Helper" after applying my patch. Stay tuned!
I have fixed the thread-safe problem of VROculusSession and confirm try results are good.
Attachment #8920086 - Flags: review?(dvander)
Comment on attachment 8922263 [details]
Bug 1406327 - Part 3: VRSystemManager for multi-threads;

https://reviewboard.mozilla.org/r/193306/#review198808

::: gfx/vr/gfxVROpenVR.cpp:616
(Diff revision 2)
>  VRSystemManagerOpenVR::GetHMDs(nsTArray<RefPtr<VRDisplayHost>>& aHMDResult)
>  {
> +  // When running VR tests on local machines which have SteamVR runtime.
> +  // VR_IsHmdPresent() would have chance to be true. Then, it makes us can't
> +  // get the VRPuppet display.
> +  if (gfxPrefs::VRPuppetEnabled()) {

Perhaps we would need to do this for Oculus and OSVR as well?

Maybe it's easier to handle all of them at the GetHMDs callsite
Attachment #8922263 - Flags: review?(kgilbert) → review+
Comment on attachment 8922263 [details]
Bug 1406327 - Part 3: VRSystemManager for multi-threads;

https://reviewboard.mozilla.org/r/193306/#review198810

Looks good, thanks!
Comment on attachment 8922262 [details]
Bug 1406327 - Part 2: When loading VR content, launching the VR listener thread;

https://reviewboard.mozilla.org/r/193304/#review198804

This looks great, thanks!
Attachment #8922262 - Flags: review?(kgilbert) → review+
Comment on attachment 8920086 [details]
Bug 1406327 - Part 1: Shutdown VR listener thread when no VR content in seconds;

https://reviewboard.mozilla.org/r/191088/#review198792

lgtm, thanks!
Attachment #8920086 - Flags: review?(kgilbert) → review+
Comment on attachment 8920086 [details]
Bug 1406327 - Part 1: Shutdown VR listener thread when no VR content in seconds;

https://reviewboard.mozilla.org/r/191088/#review199254

::: gfx/ipc/GPUParent.cpp:436
(Diff revision 4)
>      mVsyncBridge = nullptr;
>    }
>    dom::VideoDecoderManagerParent::ShutdownVideoBridge();
>    CompositorThreadHolder::Shutdown();
> +  if (VRListenerThreadHolder::IsActive()) {
> -  VRListenerThreadHolder::Shutdown();
> +    VRListenerThreadHolder::Shutdown();

nit: Shutdown() should check IsActive() instead, it's cleaner to hide stuff like that so the shutdown steps look straightforward.

::: gfx/layers/ipc/CompositorVsyncScheduler.cpp:136
(Diff revision 4)
>        &CompositorVsyncScheduler::Composite,
>        aCompositeTimestamp);
>      mCurrentCompositeTask = task;
>      ScheduleTask(task.forget(), 0);
>    }
> -  if (mCurrentVRListenerTask == nullptr && VRListenerThreadHolder::Loop()) {
> +  if (mCurrentVRListenerTask == nullptr && VRListenerThreadHolder::IsActive() &&

If IsActive() implies Loop(), I'd drop the last condition.

::: gfx/layers/ipc/CompositorVsyncScheduler.cpp:165
(Diff revision 4)
> +}
> +
> +void
> +CompositorVsyncScheduler::ShutdownVRListenerThread()
> +{
> +  if (VRListenerThreadHolder::IsActive()) {

ditto here for IsActive ... Shutdown.

::: gfx/thebes/gfxPlatform.cpp:1070
(Diff revision 4)
>          gfx::VRManagerChild::ShutDown();
>          layers::CompositorManagerChild::Shutdown();
>          layers::ImageBridgeChild::ShutDown();
>          // This has to happen after shutting down the child protocols.
>          layers::CompositorThreadHolder::Shutdown();
> +        if (VRListenerThreadHolder::IsActive()) {

Ditto here.
Attachment #8920086 - Flags: review?(dvander) → review+
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b7a07167905c
Part 1: Shutdown VR listener thread when no VR content in seconds; r=dvander,kip
https://hg.mozilla.org/integration/autoland/rev/cfbc6262a064
Part 2: When loading VR content, launching the VR listener thread; r=kip
https://hg.mozilla.org/integration/autoland/rev/2932c914e223
Part 3: VRSystemManager for multi-threads; r=kip
(In reply to :kip (Kearwood Gilbert) from comment #16)
> Comment on attachment 8922263 [details]
> Bug 1406327 - Part 3: VRSystemManager for multi-threads;
> 
> https://reviewboard.mozilla.org/r/193306/#review198808
> 
> ::: gfx/vr/gfxVROpenVR.cpp:616
> (Diff revision 2)
> >  VRSystemManagerOpenVR::GetHMDs(nsTArray<RefPtr<VRDisplayHost>>& aHMDResult)
> >  {
> > +  // When running VR tests on local machines which have SteamVR runtime.
> > +  // VR_IsHmdPresent() would have chance to be true. Then, it makes us can't
> > +  // get the VRPuppet display.
> > +  if (gfxPrefs::VRPuppetEnabled()) {
> 
> Perhaps we would need to do this for Oculus and OSVR as well?
> 
> Maybe it's easier to handle all of them at the GetHMDs callsite

I would say this is a workaround for OpenVR only, we can consider to add it to other systems later when they occurs.
Backed out changeset b7a07167905c::2932c914e223 (bug 1406327) for failing in dom/vr/test/mochitest/test_vrController_displayId.html r=backout a=backout on a CLOSED TREE

https://hg.mozilla.org/mozilla-central/rev/1c618b1a13662de7cec429f606367db3827b6dc7

https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1412754
Status: RESOLVED → REOPENED
status-firefox58: fixed → affected
Flags: needinfo?(dmu)
Resolution: FIXED → ---
Target Milestone: mozilla58 → ---
Well. I can't reproduce it after rebase it with m-c. I have re-try it for a few time, https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3f8a9a785a804e584c88bdaba5b4cd6b6e7b35f, but it still doesn't happen.

It should be just an intermittent time-out.
Flags: needinfo?(dmu)
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1e57d418c14
Part 1: Shutdown VR listener thread when no VR content in seconds; r=dvander,kip
https://hg.mozilla.org/integration/autoland/rev/44cb5451bbde
Part 2: When loading VR content, launching the VR listener thread; r=kip
https://hg.mozilla.org/integration/autoland/rev/1784a194ced8
Part 3: VRSystemManager for multi-threads; r=kip
Backed out in https://hg.mozilla.org/integration/autoland/rev/90294812319d for https://treeherder.mozilla.org/logviewer.html#?job_id=141251153&repo=autoland - apparently at least on Android debug, you shut down slowly enough that you wind up doing so during some later test, like a random dom/workers/ test, and crashing it when you do.
Well, I can't reproduce it and retry a few times, https://treeherder.mozilla.org/#/jobs?repo=try&revision=7389c031c9eb8d229930880b8f90a05ecad60952. I decide to push it again. If it happens again, I am considering to skip it in Android because we have no support for Android.
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/996d843e1eb8
Part 1: Shutdown VR listener thread when no VR content in seconds; r=dvander,kip
https://hg.mozilla.org/integration/autoland/rev/aa67335856eb
Part 2: When loading VR content, launching the VR listener thread; r=kip
https://hg.mozilla.org/integration/autoland/rev/81c384a01762
Part 3: VRSystemManager for multi-threads; r=kip
(In reply to Daosheng Mu[:daoshengmu] from comment #36)
> Well, I can't reproduce it and retry a few times,
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7389c031c9eb8d229930880b8f90a05ecad60952. I decide to
> push it again. If it happens again, I am considering to skip it in Android
> because we have no support for Android.

I seem to can reproduce it on Windows Debug. I can see some IPC error messages when the process is shutdown. Fix it soon!
(In reply to Daosheng Mu[:daoshengmu] from comment #40)
> Created attachment 8924198 [details]
> Bug 1406327 - Part 4: RefreshVRDisplays needs to be at VRListenerThread;
> 
> Review commit: https://reviewboard.mozilla.org/r/195416/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/195416/

Because vr::VR_IsHmdPresent() is not thread-safe to be called by VRListenerThread and CompositorThread simultaneously, we have to post RefreshVRDisplays() to VRListenerThread. I also adjust VRPuppy display can be added when creating VR test system to avoid VRListenerThread is not launched but we need to RefreshVRDisplays() for getting vrdisplay info.
Flags: needinfo?(dmu)
Comment on attachment 8924198 [details]
Bug 1406327 - Part 4: RefreshVRDisplays needs to be at VRListenerThread;

https://reviewboard.mozilla.org/r/195416/#review200756

LGTM, Thanks!
Attachment #8924198 - Flags: review?(kgilbert) → review+
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/345972733daa
Part 1: Shutdown VR listener thread when no VR content in seconds; r=dvander,kip
https://hg.mozilla.org/integration/autoland/rev/63c8ee57e38c
Part 2: When loading VR content, launching the VR listener thread; r=kip
https://hg.mozilla.org/integration/autoland/rev/5890bb3a0d97
Part 3: VRSystemManager for multi-threads; r=kip
https://hg.mozilla.org/integration/autoland/rev/471c710b19b1
Part 4: RefreshVRDisplays needs to be at VRListenerThread; r=kip
After a few days of investigation, this is a race-condition problem happens on Android tests. When our VRPuppet device tries to access mVRDisplays and mVRControllers, there is an another thread is using them. So, the state of the hash table, mVRDisplays and mVRControllers, are not idle and wait for writing.

I am going to make all of functions who intend to access mVRDisplays and mVRControllers have to be at the VRListenerThread. Hope it could solve our issue.
(In reply to Daosheng Mu[:daoshengmu] from comment #48)
> After a few days of investigation, this is a race-condition problem happens
> on Android tests. When our VRPuppet device tries to access mVRDisplays and
> mVRControllers, there is an another thread is using them. So, the state of
> the hash table, mVRDisplays and mVRControllers, are not idle and wait for
> writing.
> 
> I am going to make all of functions who intend to access mVRDisplays and
> mVRControllers have to be at the VRListenerThread. Hope it could solve our
> issue.

Perhaps this can be simplified by having just one VR thread, and using our own D3D11Context (landing with Bug 1404534).

If a VRDisplayHost ancestor needs additional threads (ie. for haptics feedback or to make blocking calls act like async), they should spawn and terminate their own specialized threads when they are active.
Flags: needinfo?(dmu)
(In reply to :kip (Kearwood Gilbert) from comment #49)
> (In reply to Daosheng Mu[:daoshengmu] from comment #48)
> > After a few days of investigation, this is a race-condition problem happens
> > on Android tests. When our VRPuppet device tries to access mVRDisplays and
> > mVRControllers, there is an another thread is using them. So, the state of
> > the hash table, mVRDisplays and mVRControllers, are not idle and wait for
> > writing.
> > 
> > I am going to make all of functions who intend to access mVRDisplays and
> > mVRControllers have to be at the VRListenerThread. Hope it could solve our
> > issue.
> 
> Perhaps this can be simplified by having just one VR thread, and using our
> own D3D11Context (landing with Bug 1404534).
> 
> If a VRDisplayHost ancestor needs additional threads (ie. for haptics
> feedback or to make blocking calls act like async), they should spawn and
> terminate their own specialized threads when they are active.

Agree. I am going to stop this bug here. Currently, VRListenerThread is responsible for the connection  with PVRManager and polling events from VR devices. I think we should let it as a permanent thread. If we let two different threads to be in charge of this two tasks. It is easy to happen race condition when accessing the same data.

I am look forward Bug 1404534 landing, then we can spawn the VRSubmitFrame thread for rendering and shutdown it when it is inactive. Furthermore, if we can have share memory from Bug 1346927, we probably can relief the dependency of IPC and spawn VRListenerThread only when we need it to do polling.
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Flags: needinfo?(dmu)
Resolution: --- → INCOMPLETE
Comment on attachment 8925861 [details]
Bug 1406327 - Part 6: Avoid other threads to access the same member data simultaneously(wip).

https://reviewboard.mozilla.org/r/197062/#review202200


C/C++ static analysis found 3 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: gfx/vr/ipc/VRManagerParent.cpp:60
(Diff revision 1)
> +  }
> +  return layer.forget().take();
> +}
> +
> +void
> +VRManagerParent::GetDisplay(RefPtr<VRLayerParent> aLayer, uint32_t aDisplayID)

Warning: The parameter 'alayer' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: gfx/vr/ipc/VRManagerParent.cpp:478
(Diff revision 1)
> +        aID, aPromiseID, mDisplayTestID));
> +  }
> +}
> +
> +void
> +VRManagerParent::ReplyCreateVRServiceTestDisplayToContent(nsCString aID,

Warning: The parameter 'aid' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: gfx/vr/ipc/VRManagerParent.cpp:530
(Diff revision 1)
> +
> +  ++mControllerTestID;
> +}
> +
> +void
> +VRManagerParent::ReplyCreateVRServiceTestControllerToContent(nsCString aID,

Warning: The parameter 'aid' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: