Closed Bug 1430038 Opened 6 years ago Closed 6 years ago

Implement VR process

Categories

(Core :: WebVR, enhancement)

Product:
Core
Component:
WebVR
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: daoshengmu, Assigned: daoshengmu)

References

Details

Attachments

(4 files, 4 obsolete files)

Bug 1430038 - Part 1: Add VR process to the process list;
59 bytes, text/x-review-board-request
kip
: review+
jimm
: review+
Details
Bug 1430038 - Part 2: launch VR process and communicate with GPU process;
59 bytes, text/x-review-board-request
kip
: review+
jimm
: review+
Details
Bug 1430038 - Part 3: Construct IPC connection for VR/GPU process;
59 bytes, text/x-review-board-request
kip
: review+
mattwoodrow
: review+
jimm
: review+
Details
VR process architecture 1.0.jpg
54.51 KB, image/jpeg
Details 
There are some things need to do:

1. Implement VR process based on mozilla::ipc::GeckoChildProcessHost and mozilla::ipc::ProcessChild to define the bridge from content processes. Probably, we need to connect it with VSyncBridge as well.

2. Launch VR process when we are accessing VR content from the content process.

3. We can give a preference like "dom.vr.process.enabled" to turn on/off VR process for the current experiment and support some platforms which don't want VR be move to a separate process.
Blocks: 1362578
Assignee: nobody → dmu
(In reply to Daosheng Mu[:daoshengmu] from comment #1)
> Created attachment 8984611 [details]
> Bug 1430038 - (wip) spawning the VR process;
> 
> Review commit: https://reviewboard.mozilla.org/r/250506/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/250506/

We can spawn the VR process successfully now, the type of message loop needs to be MessageLoop::TYPE_DEFAULT instead of TYPE_MOZILLA_CHILD, otherwise, the process would crash suddenly.

We still need to do:
 - Integrate it with VR module.
 - Handle the process shutdown.
 - Launch the process when entering VR content
Hi Randell,

I have a question about creating a GeckoChildProcessHost. We are working on launching the VR process in the GPU process when users are first coming to a WebVR website in the content process. The way I would like to do is when accessing a WebVR content, we will send an ipc message via PContent and PGPU to notify the GPU process that we can spawn our VR process. I am doing some experiment in gfx/vr/ipc/VRProcessParent.cpp at attachment 8984611 [details].

I notice if I use MessageLoop::TYPE_MOZILLA_CHILD like the content and GPU process did. The new process we create would be a separate process as a main process, and it will be killed suddenly. Thus, I changed it and use MessageLoop::TYPE_DEFAULT, the process can be created successfully. However, I can't receive the OnChannelConnected() event and base::GetProcId(GetChildProcessHandle()) is always 0.

Could you give me some hints about the creation of a child process? Thanks.
Flags: needinfo?(rjesup)
(In reply to Daosheng Mu[:daoshengmu] from comment #3)
> Hi Randell,
> 
> I have a question about creating a GeckoChildProcessHost. We are working on
> launching the VR process in the GPU process when users are first coming to a
> WebVR website in the content process. The way I would like to do is when
> accessing a WebVR content, we will send an ipc message via PContent and PGPU
> to notify the GPU process that we can spawn our VR process. I am doing some
> experiment in gfx/vr/ipc/VRProcessParent.cpp at attachment 8984611 [details].
> 
> I notice if I use MessageLoop::TYPE_MOZILLA_CHILD like the content and GPU
> process did. The new process we create would be a separate process as a main
> process, and it will be killed suddenly. Thus, I changed it and use
> MessageLoop::TYPE_DEFAULT, the process can be created successfully. However,
> I can't receive the OnChannelConnected() event and
> base::GetProcId(GetChildProcessHandle()) is always 0.
> 
> Could you give me some hints about the creation of a child process? Thanks.


It seems like I didn't make an IPC connection once the new process is spawned. I have figured out the problem by myself. I still need to use MessageLoop::TYPE_DEFAULT and can receive the OnChannelConnected() event now.

Thanks.
Flags: needinfo?(rjesup)
(In reply to Daosheng Mu[:daoshengmu] from comment #9)
> Comment on attachment 8987324 [details]
> Bug 1430038 - Part 2: launch VR process and manage its shutdown when
> exisitng;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/252578/diff/1-2/

Updating the current work for launching VR process. Currently, we choose to spawn the VR process in the main process like other types of process' behavior in Firefox. Then, we make a connection, PVR.ipdl, between the main process and VR process. When any content process be created, we will construct a IPC connection (PVRContent.ipdl) for the new content process and VR process in the main process, and forward the parent/child pipe to VR process and the new content process. After that, these two processes can start to communicate with each other.
Attached image VR process architecture.jpg (obsolete) — 

    
    

    
  

      
      
      
      

        Attached image
          
        VR process architecture.jpg (obsolete)
        — 
      

    


  

        Attachment #8987975 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Daosheng Mu[:daoshengmu] from comment #12)
> Created attachment 8988017 [details]
> VR process architecture.jpg

AS attachment 8987975 [details] mentioned, we spawn the VR process in the main process, like GPU process did. We can let Content process or GPU process to notify the main process to launch the VR process. We use PVR.ipdl to construct the message channel for VRProcess child/parent that we are hosted in VR/main process. When a content process is spawned, it notifies the main process and make message channels with other processes. So, in the main process, we make the pipe (PVRContent) for VR/Content process, and forward the parent pipe to VR process, the child pipe to Content process. Then, we can communicate VR/content process via PVRContent.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Hi Kip,

I have done the implementation for spawning VR process. My next step is integrating it with our VR module to make gfx/vr be executed in this process.

I know you are working on VRExternal interface and making communicate cross processes with shared memory. Do you think I should wait for your patches landed? Ideally, after your patches landed, we don't need most of IPC messages, it should be more easier to make VR process communicate with the content process.
Flags: needinfo?(kgilbert)

    
    

    
  
Hi Daosheng!

Thanks for pulling all this together.  I think we can use it together with my patch in Bug 1466699 with minimal changes.

In particular, I would like to keep a little bit of VRManager's functionality running in the main or GPU process.  This will include the part that detects when to start the VR process (instead of running it right away when the browser starts).  Also, we wish to keep the part outside of the VR process to run timers that stop it from trying to re-launch the VR process too soon after the VR process fails to enumerate hardware.  Perhaps we can move the implementation inside "gfx/vr/service" to the new VR process first, while keeping the old classes around temporarily so we can turn it on and off by pref.  Once we can turn off the old path, we can remove lots of the gfxVR... files and combine gfxVRExternal with VRDisplayHost.  Also, some of VRManager could be simplified as it is only needed then for coordinating things between content processes and launching the VR process when needed.

Perhaps we should land Bug 1466699 first, then update the protocols in this patch a bit to match.  I think we can simplify the PVRManager and PVRLayer protocols by introducing the content process <-> VR process protocol that simply establishes a connection with the Shmem to the VR service.
Flags: needinfo?(kgilbert)
Blocks: 1454204

    
    

    
  
I have added dependencies to Bug 1473399 to break down the work needed to implement the VR Service thread that will run in the VR process.  These dependencies create the IPC mechanisms required to implement the out-of-process VR implementation and include all functionality needed before we can remove the then-redundant implementations.

We should first land the implementation of the VRService thread (see gfx/vr/service/VRService.cpp) then land this bug to move this thread to its own process.  VRManager would initially remain in the same process as the compositor.  After landing, we can refactor to simplify the implementation and remove much of the PVRManager and PVRLayer protocol functions. (See Bug 1473401 and Bug 1473402 for initial clean-up tasks)
See Also:  → 1473399

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
The part 4th patch works for making Vsync thread to communicate with VR process and notify events at the vsync time. We need to consider to launch an individual thread to observe/dispatch events from Vsync thread at the main process (VRChild.cpp).

Besides, VR process can't reuse gfxPrefs::xxx() because that only works in GPU process. We need to make an another one, gfxPrefs, to be run in VR process.

    
    

    
      
  


  

    
    

    
  
I would like to move the work of sharing memory between different process to Bug 1481327. It seems like we need to do more investigations for using shmem.

    
    

    
  


  
Update the architecture because we only want the VR process to communicate with GPU process for now instead of talking to the content process directly.

        Attachment #8988017 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8995586 -
        Attachment is obsolete: true

    
  

        Attachment #8996462 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8984611 [details]
Bug 1430038 - Part 1: Add VR process to the process list;

https://reviewboard.mozilla.org/r/250506/#review268886

r+ from me on the VR specific parts.  Please add an review reqest for the GFX owner (@jgilbert) and peers for other modules.

        Attachment #8984611 -
        Flags: review?(kgilbert) → review+

    
    

    
  
Comment on attachment 8987324 [details]
Bug 1430038 - Part 2: launch VR process and communicate with GPU process;

https://reviewboard.mozilla.org/r/252578/#review268888

r+ from me on the VR specific parts.  Please add an review reqest for the GFX owner (@jgilbert) and peers for other modules.

        Attachment #8987324 -
        Flags: review?(kgilbert) → review+

    
    

    
  
Comment on attachment 8987691 [details]
Bug 1430038 - Part 3: Construct IPC connection for VR/GPU process;

https://reviewboard.mozilla.org/r/252922/#review268890

r+ from me on the VR specific parts.  Please add an review reqest for the GFX owner (@jgilbert) and peers for other modules.

        Attachment #8987691 -
        Flags: review?(kgilbert) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
@jimm please help review the process creation & *.ipdl parts.
@jgilbert, please help review the gfx part.

Thanks.

    
    

    
  
Comment on attachment 8984611 [details]
Bug 1430038 - Part 1: Add VR process to the process list;

https://reviewboard.mozilla.org/r/250506/#review269286

        Attachment #8984611 -
        Flags: review?(jmathies) → review+

    
    

    
  
working my way through these but it'll be into the weekend before I'm done.

    
    

    
  
Thanks, we really need to land these before the merge date (8/23) of FF 63.

        Attachment #8987691 -
        Flags: review?(jgilbert) → review?(matt.woodrow)

    
    

    
  
Comment on attachment 8987691 [details]
Bug 1430038 - Part 3: Construct IPC connection for VR/GPU process;

https://reviewboard.mozilla.org/r/252922/#review269550

These parts of gfx were really more dvander's thing, but mattwoodrow reviewed at least some of that, so I'm passing the review to mattwoodrow.

    
  
Blocks: 1484076

    
    

    
  
Comment on attachment 8987691 [details]
Bug 1430038 - Part 3: Construct IPC connection for VR/GPU process;

https://reviewboard.mozilla.org/r/252922/#review269586

        Attachment #8987691 -
        Flags: review?(matt.woodrow) → review+

    
    

    
  
Comment on attachment 8987324 [details]
Bug 1430038 - Part 2: launch VR process and communicate with GPU process;

https://reviewboard.mozilla.org/r/252578/#review269582

Generally this looked like solid code. I didn't do an exhaustive review so that I can unblock.

::: gfx/vr/ipc/PVR.ipdl:19
(Diff revision 7)
> +async protocol PVR
> +{
> +parent:
> +  async NewGPUVRManager(Endpoint<PVRGPUParent> endpoint);
> +  async Init(GfxPrefSetting[] prefs, GfxVarUpdate[] vars, DevicePrefs devicePrefs);
> +  async NotifyVsync(TimeStamp aVsyncTimestamp);

nit - if you're not using the aVar convention here, don't use it at all. Makes the code look sloppy.

::: gfx/vr/ipc/VRGPUParent.cpp:30
(Diff revision 7)
> +
> +void
> +VRGPUParent::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  MessageLoop::current()->PostTask(
> +  NewRunnableMethod("gfx::VRGPUParent::DeferredDestroy",

nit - indentation

::: gfx/vr/ipc/VRParent.h:30
(Diff revision 7)
> +            MessageLoop* aIOLoop,
> +            IPC::Channel* aChannel);
> +  virtual void ActorDestroy(ActorDestroyReason aWhy) override;
> +
> +protected:
> +  virtual mozilla::ipc::IPCResult RecvNewGPUVRManager(Endpoint<PVRGPUParent>&& aEndpoint) override;

nit - consider typcasting IPCResult within the class to shorten this.

        Attachment #8987324 -
        Flags: review?(jmathies) → review+

    
    

    
  
Comment on attachment 8987691 [details]
Bug 1430038 - Part 3: Construct IPC connection for VR/GPU process;

https://reviewboard.mozilla.org/r/252922/#review269614

        Attachment #8987691 -
        Flags: review?(jmathies) → review+

    
    

    
  
As Daosheng away, I will fix up the nits and land.

    
    

    
  
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a61001aaee6
Part 1: Add VR process to the process list; r=kip, jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/57c32dd4f6e6
Part 2: launch VR process and communicate with GPU process; r=kip, jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e16db6bbb21
Part 3: Construct IPC connection for VR/GPU process; r=kip, jimm, jgilbert

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/1a61001aaee6
https://hg.mozilla.org/mozilla-central/rev/57c32dd4f6e6
https://hg.mozilla.org/mozilla-central/rev/2e16db6bbb21
Status: NEW → RESOLVED
Closed: 6 years ago

          status-firefox63:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

    
  
Blocks: 1485095

    
  
Blocks: 1481327

    
  
Flags: needinfo?(bobowencode)

    
    

    
  
Jim, asked me to take a retrospective glance over this.
There's a fair bit I'm not familiar with here, but the parts of the process definition and launch of which I am, look good to me.
Flags: needinfo?(bobowencode)

    
    

    
  
In therms of manual testing, is there something we should check for this item?
Flags: needinfo?(dmu)

    
    

    
  
Hi Cristi,

Currently, we only support VR process in Windows Nightly. You can enable it at about:config, set "dom.vr.external.enabled;true, dom.vr.process.enabled;true, dom.vr.service.enabled;true, dom.vr.enabled;true". Then, go to the WebVR websites you tested before, you should see a vr process from Windows Process Explorer. After playing WebVR for a few minutes, closing Firefox, you should see the VR process will be closed as well.

Thanks
Flags: needinfo?(dmu)

    
  
No longer blocks: 1362578

    
  
No longer blocks: 1484076

    
  
No longer blocks: 1454204
Depends on: 1512429

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: