Closed Bug 1392216 Opened 2 years ago Closed 2 years ago

Implement VR listener thread for listening devices change

Categories

(Core :: WebVR, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: daoshengmu, Assigned: daoshengmu)

References

Details

Attachments

(4 files)

No description provided.
Blocks: 1392215
Assignee: nobody → dmu
Summary: Implement VR thread for Windows → Implement VR seek thread for looking for new devices join
Summary: Implement VR seek thread for looking for new devices join → Implement VR listener thread for listening devices change
I have moved all of our VR module to a separate thread, VR listener thread, for looking for any updates from devices. Currently, it works normally. However, I still need to confirm if it is safe for putting submitframe function split from the main thread.
I notice if we put TextureHost at an another thread instead of compositor thread for submitting frames, there will have errors when D3D11 textures swap buffer. However, if I post the task back to the compositor thread, we will have chance to meet this texture be recycled. Therefore, I am thinking of creating two channels, one for the new VR listener thread for seeking for updates from devices, and the other channel is for communicating with the compositor thread for submitting the textureHost to VRCompositor.
Perhaps this can be simplified if we don't rely on TextureHost any more?

I've got some patches in Bug 1381085, which eliminate the need for making our own TextureHost's.

In order to get Bug 1381085 landed in time for FF57, I have to move the refactoring parts that eliminate the TextureHost usage in our VR host side out to FF58.

It looks like this will need to land in FF58 anyways, so this may be an option for us.
Priority: -- → P1
(In reply to :kip (Kearwood Gilbert) from comment #6)
> Perhaps this can be simplified if we don't rely on TextureHost any more?
> 
> I've got some patches in Bug 1381085, which eliminate the need for making
> our own TextureHost's.
> 
> In order to get Bug 1381085 landed in time for FF57, I have to move the
> refactoring parts that eliminate the TextureHost usage in our VR host side
> out to FF58.
> 
> It looks like this will need to land in FF58 anyways, so this may be an
> option for us.

That sounds like a good idea. If we can get the D3D11Texture directly at our VR threads, that would be perfect. In this approach, we probably don't need to send the texture back to Compositor thread for rendering because we can manage it by d3dDevice at our own thread.
(In reply to Daosheng Mu[:daoshengmu] from comment #8)
> (In reply to :kip (Kearwood Gilbert) from comment #6)
> > Perhaps this can be simplified if we don't rely on TextureHost any more?
> > 
> > I've got some patches in Bug 1381085, which eliminate the need for making
> > our own TextureHost's.
> > 
> > In order to get Bug 1381085 landed in time for FF57, I have to move the
> > refactoring parts that eliminate the TextureHost usage in our VR host side
> > out to FF58.
> > 
> > It looks like this will need to land in FF58 anyways, so this may be an
> > option for us.
> 
> That sounds like a good idea. If we can get the D3D11Texture directly at our
> VR threads, that would be perfect. In this approach, we probably don't need
> to send the texture back to Compositor thread for rendering because we can
> manage it by d3dDevice at our own thread.

I have split up the patches from bug 1381085 into a few separate bugs and am working on an alternative fix for the original issue in Bug 1381085.

Could you please take a look at the patch in bug 1400407?  Bug 1400407 completely de-couples the VR frame submission from the Compositor rendering in preparation for creating the VR process.  It also refactors the code to pass in D3D11Texture's directly to the VRDisplayHost descendants.

I believe you can rebase your work on Bug 1400407 to simplify it.
Flags: needinfo?(dmu)
After applying some patches from Bug 1400457 and Bug 1400407, it works fairly well in OpenVR. The FPS at the present mode is always 90, and I can feel the vibration events from VR controllers are handled better than before. However, it seems to have a problem on the Oculus part, my screen will be stuck after switching to the present mode. I am not sure whether it is from my own patches. I will continue to investigate. Thanks!
Flags: needinfo?(dmu)
See Also: → 1401724
:kip, I still need to post my submit frame task to Compositor thread, otherwise, VR devices would be unresponsive and can't show frames in HMD. I don't know the reason though.

This separate thread improves our performance a lot. It will solve the controller jitter problem, and Vive display is always 90 FPS and Oculus would be over than 300 FPS. I think we also need to open a follow-up bug to change this listener thread launch and dead time to make it more efficiency.

After launching VR listener thread, WebVR reftest seem to not work properly. I am going to give a patch to fix it.
Comment on attachment 8904955 [details]
Bug 1392216 - Part 1: Create VR listener thread in GPU process;

https://reviewboard.mozilla.org/r/176770/#review192030

This LGTM, but could use an additional review by someone in the gfx team.  Perhaps :jgilbert ?

::: gfx/ipc/GPUParent.cpp:124
(Diff revision 6)
>    if (NS_FAILED(NS_InitMinimalXPCOM())) {
>      return false;
>    }
>  
>    CompositorThreadHolder::Start();
> +  // TODO: Start VRListenerThreadHolder when loading VR content.

Please file a separate bug to start the thread only when WebVR content is detected and to shut down the thread when no WebVR content is present (after some timeout since the last WebVR content was seen).
Attachment #8904955 - Flags: review?(kgilbert) → review+
Comment on attachment 8915540 [details]
Bug 1392216 - Part 2: Support VR listener thread in VR;

https://reviewboard.mozilla.org/r/186750/#review192032

Great work pulling this all together!  It is looking very close now.

Please implement the asynchronous callback to the VR Listener thread from the compositor to ensure that VRManager::NotifyVRVsync is called with the correct frame timing, rather than calling it immediately after posting the message to the compositor thread for submit the frame.

The rest can follow up in separate bugs.

::: gfx/vr/VRDisplayHost.cpp
(Diff revision 2)
>      default: {
>        NS_WARNING("Unsupported SurfaceDescriptor type for VR layer texture");
>        return;
>      }
>    }
> -

We should post a message back to the VR Listener thread here to start the next frame rendering.

It is important that vr->NotifyVRVsync is not called until the SubmitFrame calls to OpenVR and Oculus SDK finish blocking.

In the case of SteamVR, this is what causes the next frame to start rendering precisely 4ms before the vsync for "overlapped" rendering.

::: gfx/vr/ipc/VRLayerParent.cpp:71
(Diff revision 2)
>    if (mVRDisplayID) {
> +    MessageLoop* loop = layers::CompositorThreadHolder::Loop();
>      VRManager* vm = VRManager::Get();
>      RefPtr<VRDisplayHost> display = vm->GetDisplay(mVRDisplayID);
>      if (display) {
> -      display->SubmitFrame(this, aTexture, aFrameId,
> +      // Because VR compositor still share the same graphics device with Compositor thread.

Once Bug 1404534 lands, the VR frame submission will have its own D3D11Device, and we can change this to submit in a thread other than the Compositor thread.  There may be some benefits for having the frame submission happening on a separate thread than the VR listener thread; however, so we can still process haptic feedback updates while the SubmitFrame call is blocking.

For OpenVR and Oculus, I recommend that we have two VR threads (Frame Submission and VR Listener).  For some others (ie. Google VR / Daydream), we must submit frames in teh same thread.  It would be benefitical to ensure our code is flexible here to work either way.

We can do the frame submission in the Compositor thread for now, but please file a separate bug to create a separate "VR Frame Submission" thread.

::: gfx/vr/ipc/VRLayerParent.cpp:90
(Diff revision 2)
> +       * If NotifyVRVsync is not called here due to SubmitFrame failing, the
> +       * fallback "watchdog" code in VRDisplayHost::NotifyVSync() will cause
> +       * frames to continue at a lower refresh rate until frame submission
> +       * succeeds again.
> +       */
> +      vm->NotifyVRVsync(display->GetDisplayInfo().mDisplayID);

We don't want to call vm->NotifyVRVsync here until the Compositor / "VR Submission Thread" has finished submitting the VR frame to the underlying VR compositor.  The OpenVR API purposefully blocks until 4ms before the next VSync to enable "overlapped rendering".

We can still loop->PostTask to the compositor / "VR Submission Thread" to submit the frame asynchronously, but rather than calling vm->NotifyVRVsync immediately after the task is posted, we should wait asynchronously for a message to be sent back from the Compositor / "VR Submission Thread" indicating that the SubmitFrame call has completed.
Attachment #8915540 - Flags: review?(kgilbert) → review-
See Also: → 1406327
(In reply to Daosheng Mu[:daoshengmu] from comment #22)
> Comment on attachment 8915932 [details]
> Bug 1392216 - Part 3: VRPuppet dispatch submit frame result to VRListener
> thread;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/187194/diff/1-2/

After adjusting the FrameId mechanism and dispatching the submitframe result to VRListener thread, it only fails at OSX opt build.(https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1ff081a719a727d7a4e81ba738a9f85721f75b9) It is caused by aTexture.type() has an invalid value (268435456) at https://dxr.mozilla.org/mozilla-central/rev/19b32a138d08f73961df878a29de6f0aad441683/gfx/vr/VRDisplayHost.cpp#259.
(In reply to Daosheng Mu[:daoshengmu] from comment #26)
> Comment on attachment 8915932 [details]
> Bug 1392216 - Part 3: VRPuppet dispatch submit frame result to VRListener
> thread;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/187194/diff/2-3/

Try looks goo now. I have to send the texture as a const layers::SurfaceDescriptor to the another thread instead of a const ref. https://treeherder.mozilla.org/#/jobs?repo=try&revision=99ac242780556510f23f146d87e7cb01a530e1b4
Attachment #8904955 - Flags: review?(jgilbert) → review?(dvander)
Comment on attachment 8904955 [details]
Bug 1392216 - Part 1: Create VR listener thread in GPU process;

https://reviewboard.mozilla.org/r/176770/#review192916

GPU process is dvander's area.
Comment on attachment 8915540 [details]
Bug 1392216 - Part 2: Support VR listener thread in VR;

https://reviewboard.mozilla.org/r/186750/#review193344

This looks good, thanks!
Attachment #8915540 - Flags: review?(kgilbert) → review+
Comment on attachment 8915932 [details]
Bug 1392216 - Part 3: VRPuppet dispatch submit frame result to VRListener thread;

https://reviewboard.mozilla.org/r/187194/#review193352

This is looking really good.  Just have to ensure that VRDisplayHost::StartFrame is the only function that updates mDisplayInfo.mFrameId, then this should be perfect :-)

::: gfx/vr/VRDisplayHost.cpp:276
(Diff revision 3)
>  
>    // Ensure that we only accept the first SubmitFrame call per RAF cycle.
>    if (!mFrameStarted || aFrameId != mDisplayInfo.mFrameId) {
> +    // Reset frameId when mDisplayInfo.mFrameId updates too many times
> +    // in VR threads.
> +    mDisplayInfo.mFrameId = aFrameId;

Resetting mDisplayInfo.mFrameId to aFrameId here is likely to cause a performance regression resulting in very bad latency.

It's important that VRDisplayHost::StartFrame() is the only place that increments mDisplayInfo.mFrameId.

Please see Bug 1394561 Comment 5 for more details.

If this is causing reftests to fail, perhaps we can make adjustments to them?  Is it possible that the retests are submitting frames even when they didn't get a VRDisplay.requestAnimationFrame callback?
Attachment #8915932 - Flags: review?(kgilbert) → review-
Comment on attachment 8915932 [details]
Bug 1392216 - Part 3: VRPuppet dispatch submit frame result to VRListener thread;

https://reviewboard.mozilla.org/r/187194/#review193352

> Resetting mDisplayInfo.mFrameId to aFrameId here is likely to cause a performance regression resulting in very bad latency.
> 
> It's important that VRDisplayHost::StartFrame() is the only place that increments mDisplayInfo.mFrameId.
> 
> Please see Bug 1394561 Comment 5 for more details.
> 
> If this is causing reftests to fail, perhaps we can make adjustments to them?  Is it possible that the retests are submitting frames even when they didn't get a VRDisplay.requestAnimationFrame callback?

It seems that we don't have high refresh rate when running in Marionette. I find the avg. refresh timing in Marionette is 130 ms, compare with 30 ms in ./mach run. I think we need to open kVRDisplayRAFMaxDuration as a preference value for reftests.
One more thing for StopPresentation() in Oculus, we will be halted for a while when exiting from the presentation mode. It only happens in Oculus. I suppose it is because we submit an empty layer for ovr_SubmitFrame() when StopPresentation, it needs to be posted to Compositor thread.
Comment on attachment 8915932 [details]
Bug 1392216 - Part 3: VRPuppet dispatch submit frame result to VRListener thread;

https://reviewboard.mozilla.org/r/187194/#review193796

This looks good, thanks!
Attachment #8915932 - Flags: review?(kgilbert) → review+
Comment on attachment 8904955 [details]
Bug 1392216 - Part 1: Create VR listener thread in GPU process;

https://reviewboard.mozilla.org/r/176770/#review192940

::: gfx/layers/ipc/CompositorVsyncScheduler.cpp:142
(Diff revision 8)
> +    RefPtr<CancelableRunnable> task = NewCancelableRunnableMethod<TimeStamp>(
> +      "layers::CompositorVsyncScheduler::DispatchVREvents",
> +      this,
> +      &CompositorVsyncScheduler::DispatchVREvents,
> +      aCompositeTimestamp);
> +      mCurrentVRListenerTask = task;

It looks like this line and the next two are indented too much.
Attachment #8904955 - Flags: review?(dvander) → review+
(In reply to Daosheng Mu[:daoshengmu] from comment #38)
> Created attachment 8918183 [details]
> Bug 1392216 - Part 4: Fixing Oculus VR display has no response when stopping
> presentation in VRListener thread;
> 
> Review commit: https://reviewboard.mozilla.org/r/189050/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/189050/

I move Refresh() from StopPresentation() to SubmitFrame() because Compostior thread still has tasks to do. Besides, I mark the code for submitting a black layer when exiting from the presentation. It always makes the HMD has no response. I would like to let this VR listener thread work to land first and file an another bug to fix it.
I have resolved the issue of submitting a black layer properly. It has to be posted to Compositor thread when stopping presentation. Oculus runtime only allows ovr_SubmitFrame() be executed in Compositor thread.
Comment on attachment 8918183 [details]
Bug 1392216 - Part 4: Move drawing black layer commaneds to Compositor thread when stopping presentation;

https://reviewboard.mozilla.org/r/189050/#review194968

LGTM.  I have some upcoming refactoring for VROculusSession which will change this a bit, but this can land first.
Attachment #8918183 - Flags: review?(kgilbert) → review+
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2db537252489
Part 1: Create VR listener thread in GPU process; r=dvander,kip
https://hg.mozilla.org/integration/autoland/rev/6c06218d7c7b
Part 2: Support VR listener thread in VR; r=kip
https://hg.mozilla.org/integration/autoland/rev/a20388e22f2e
Part 3: VRPuppet dispatch submit frame result to VRListener thread; r=kip
https://hg.mozilla.org/integration/autoland/rev/bf730db86e26
Part 4: Move drawing black layer commaneds to Compositor thread when stopping presentation; r=kip
You need to log in before you can comment on or make changes to this bug.