Closed Bug 1299928 Opened 7 years ago Closed 7 years ago

[webvr] Enumerate HTC Vive Controllers through the Gamepad API

Categories

(Core :: Graphics, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: kip, Assigned: daoshengmu)

References

Details

Attachments

(6 files)

Bug 1299928 - Part 1: Make GamepadManager knows the gamepad is from VRController when adding gamepads;
58 bytes, text/x-review-board-request
kip
: review+
cleu
: review+
Details
Bug 1299928 - Part 2: Rename VRDisplayType to VRDeviceType;
58 bytes, text/x-review-board-request
kip
: review+
Details
Bug 1299928 - Part 3: Construct IPC channel between Gamepad and VRManager;
58 bytes, text/x-review-board-request
kip
: review+
Details
Bug 1299928 - Part 4: Implement VRControllerManager;
58 bytes, text/x-review-board-request
kip
: review+
Details
Bug 1299928 - Part 5: Handle gamepad events in Vsync time;
58 bytes, text/x-review-board-request
kip
: review+
Details
Bug 1299928 - Part 6: Making GamepadManager is only run at the same process at VRMangerChild;
58 bytes, text/x-review-board-request
kip
: review+
Details 
      No description provided.
Depends on: 1299929
No longer depends on: 1299929
Blocks: 1299926
No longer depends on: 1299926
Depends on: 1299975
Assignee: nobody → dmu
In my current plan, I will divide it into three parts:
1. Refactor GamepadPlatformService to make it can assign GamepadChangeEvent to be sent on which channel.
2. Implement a VirtualControllerManager that Oculus, Vive, other VR controllers can inherit it to implement their own and handle their own events.
3. Load openvr_api.dll and check the runtime. If it success, it will collect how many controllers are using in VirtualControllerManagerOpenVR, and then sending AddGamepad event to GamepadManager.
(In reply to Daosheng Mu[:daoshengmu] from comment #2)
> Created attachment 8791114 [details]
> Bug 1299928 - Part 1: Refactor GamepadPlatformService to assign
> GamepadChangeEvent to be sent in the specific channel;
> 
> Review commit: https://reviewboard.mozilla.org/r/78646/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/78646/

Although we can add the channelId at GamepadChangeEvent as well, in the current usecase, we are just interested in what channel we are using at GamepadplatformService. So, I add aChannel parameter at GamepadplatformService::NotifyGamepadChange would be a good choice.
Just remain a few works need to do.
- Part 1: I need to do some modification to make other platforms (OSX, Linux, Android) support this new design.
- Part 3: Get the path of openvr_api.dll from the preference, it probably has to use promise to get it from the main thread. Moreover, I have to make OpenVR related functions can be shared between gfx and gamepad modules to avoid duplicate work.
Comment on attachment 8791116 [details]
Bug 1299928 - Part 3: Construct IPC channel between Gamepad and VRManager;

https://reviewboard.mozilla.org/r/78650/#review78482

::: dom/gamepad/vr/VRControllerManagerOpenVR.cpp:32
(Diff revision 3)
> +
> +namespace mozilla {
> +namespace dom {
> +
> +bool
> +LoadOpenVRRuntime(const char* aPath)

Probably, we should think about sharing the same code from gfxVROpenVR.cpp. I did some experiment, but I found if I put them into a header file that can be included by gfxVROpenVR.cpp and VRControllerManagerOpenVR.cpp. vr_IsRuntimeInstalled will be a null function pointer at VRControllerManagerOpenVR at runtime. I am wondering whether I should use wrong namespace to call it...
(In reply to Daosheng Mu[:daoshengmu] from comment #9)
> Just remain a few works need to do.
> - Part 1: I need to do some modification to make other platforms (OSX,
> Linux, Android) support this new design.
> - Part 3: Get the path of openvr_api.dll from the preference, it probably
> has to use promise to get it from the main thread. Moreover, I have to make
> OpenVR related functions can be shared between gfx and gamepad modules to
> avoid duplicate work.

I choose to get the dll path from the preference at the main thread and send it via IPC to the background thread that is more simple approach, although I think we should get this patch info from the system in the future.

Regarding to avoiding duplicate work, as Comment 13, I have tried some ways but I still didn't get the idea. Currently, it is a workable version.
Comment on attachment 8791116 [details]
Bug 1299928 - Part 3: Construct IPC channel between Gamepad and VRManager;

https://reviewboard.mozilla.org/r/78650/#review78516

::: dom/gamepad/ipc/GamepadEventChannelParent.cpp:89
(Diff revision 3)
> +  mHasGamepadListener = true;
>    return true;
>  }
>  
>  bool
>  GamepadEventChannelParent::RecvGamepadListenerRemoved(const uint32_t& mappingType)

I have to replace mappingType with channelType.

::: dom/gamepad/ipc/GamepadEventChannelParent.cpp:92
(Diff revision 3)
>  
>  bool
>  GamepadEventChannelParent::RecvGamepadListenerRemoved(const uint32_t& mappingType)
>  {
>    AssertIsOnBackgroundThread();
>    MOZ_ASSERT(mHasGamepadListener);

For some cases, we would not have mHasGamepadListener because it could create VRContollerManager fail. We only can make sure it must has a gamepadListener while GamepadChannel is GamepadChannel::eStandard.
Comment on attachment 8791114 [details]
Bug 1299928 - Part 1: Make GamepadManager knows the gamepad is from VRController when adding gamepads;

https://reviewboard.mozilla.org/r/78646/#review78902

This looks good to me, but someone who has worked on the Gamepad API before may have some feedback as well.

::: dom/gamepad/ipc/GamepadEventChannelParent.h:18
(Diff revision 4)
>  {
>   public:
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(GamepadEventChannelParent)
>    GamepadEventChannelParent();
>    virtual void ActorDestroy(ActorDestroyReason aWhy) override;
> -  virtual bool RecvGamepadListenerAdded() override;
> +  virtual bool RecvGamepadListenerAdded(const uint32_t& aChannelType) override;

Is there a reason we can't pass aChannelType as a GamepadChannel?

::: dom/gamepad/ipc/GamepadEventChannelParent.cpp:53
(Diff revision 4)
>    service->AddChannelParent(this);
>    mBackgroundThread = NS_GetCurrentThread();
>  }
>  
>  bool
> -GamepadEventChannelParent::RecvGamepadListenerAdded()
> +GamepadEventChannelParent::RecvGamepadListenerAdded(const uint32_t& aChannelType)

Is there a reason we can't pass aChannelType as a GamepadChannel?

::: dom/gamepad/ipc/PGamepadEventChannel.ipdl:14
(Diff revision 4)
>  
>  async protocol PGamepadEventChannel {
>    manager PBackground;
> +
>    parent:
> -    async GamepadListenerAdded();
> +    async GamepadListenerAdded(uint32_t aChannelType);

Same here..  Perhaps we could use the GamepadChannel enum?
Comment on attachment 8791115 [details]
Bug 1299928 - Part 2: Rename VRDisplayType to VRDeviceType;

https://reviewboard.mozilla.org/r/78648/#review78904

This looks good
Attachment #8791115 - Flags: review?(kgilbert) → review+
This is coming together very quickly, thanks Daosheng!

I should have mentioned one issue in the bug description that could affect the implementation; however...

The WebVR rendering code in gfx/vr will be running in the GPU process which I think may differ from the process that VRControllerManagerOpenVR will be running in.

The problem arises when the session created for enumerating VR gamepads takes focus away from the separate session created for rendering to the VR display.

We will need to access the VR api's only from one process with Oculus as well once we implement the touch controllers.

There are some ways to work around this.  Perhaps VRControllerManagerOpenVR's code could be moved into gfxVROpenVR and the code in the gamepad API's process could access this through the PVRManager procotol?  Perhaps it would be possible to move the abstraction of different VR API's to VRDisplayHost as well.

If you'd like to explore some ideas on what the protocol might look like or have some other ideas on how to work around this, I can also meet up with you on Vidyo or IRC.

Excellent work so far!
Flags: needinfo?(dmu)
(In reply to :kip (Kearwood Gilbert) from comment #21)
> This is coming together very quickly, thanks Daosheng!
> 
> I should have mentioned one issue in the bug description that could affect
> the implementation; however...
> 
> The WebVR rendering code in gfx/vr will be running in the GPU process which
> I think may differ from the process that VRControllerManagerOpenVR will be
> running in.
> 
> The problem arises when the session created for enumerating VR gamepads
> takes focus away from the separate session created for rendering to the VR
> display.
> 
> We will need to access the VR api's only from one process with Oculus as
> well once we implement the touch controllers.
> 
> There are some ways to work around this.  Perhaps
> VRControllerManagerOpenVR's code could be moved into gfxVROpenVR and the
> code in the gamepad API's process could access this through the PVRManager
> procotol?  Perhaps it would be possible to move the abstraction of different
> VR API's to VRDisplayHost as well.
>

I do some study on the current gamepad architecture. GamepadPlatformService that manages all Gamepad services cross all platforms is running at the main process. It helps us manage gamepad id and notify events between PBackground thread to the Content process. And our VRManager is at Compositor process.

I agree that we should move VRControllerManagerOpenVR to our Compositor process, but PVRManager is designed to serve messages between Content & Compositor processes / threads. It would not help me a lot. Because in the case of Gamepad, it needs a protocol to serve me send/recv messages between PBackground thread and Compositor processes.

So, my next goal is to implement a new protocol to help me send/recv messages between PBackground thread and Compositor processes. I will refer PVRManager and do some modification.

> If you'd like to explore some ideas on what the protocol might look like or
> have some other ideas on how to work around this, I can also meet up with
> you on Vidyo or IRC.
> 
> Excellent work so far!
Flags: needinfo?(dmu)
(In reply to Daosheng Mu[:daoshengmu] from comment #22)
> (In reply to :kip (Kearwood Gilbert) from comment #21)
> > This is coming together very quickly, thanks Daosheng!
> > 
> > I should have mentioned one issue in the bug description that could affect
> > the implementation; however...
> > 
> > The WebVR rendering code in gfx/vr will be running in the GPU process which
> > I think may differ from the process that VRControllerManagerOpenVR will be
> > running in.
> > 
> > The problem arises when the session created for enumerating VR gamepads
> > takes focus away from the separate session created for rendering to the VR
> > display.
> > 
> > We will need to access the VR api's only from one process with Oculus as
> > well once we implement the touch controllers.
> > 
> > There are some ways to work around this.  Perhaps
> > VRControllerManagerOpenVR's code could be moved into gfxVROpenVR and the
> > code in the gamepad API's process could access this through the PVRManager
> > procotol?  Perhaps it would be possible to move the abstraction of different
> > VR API's to VRDisplayHost as well.
> >
> 
> I do some study on the current gamepad architecture. GamepadPlatformService
> that manages all Gamepad services cross all platforms is running at the main
> process. It helps us manage gamepad id and notify events between PBackground
> thread to the Content process. And our VRManager is at Compositor process.
> 
> I agree that we should move VRControllerManagerOpenVR to our Compositor
> process, but PVRManager is designed to serve messages between Content &
> Compositor processes / threads. It would not help me a lot. Because in the
> case of Gamepad, it needs a protocol to serve me send/recv messages between
> PBackground thread and Compositor processes.
> 
> So, my next goal is to implement a new protocol to help me send/recv
> messages between PBackground thread and Compositor processes. I will refer
> PVRManager and do some modification.
> 
> > If you'd like to explore some ideas on what the protocol might look like or
> > have some other ideas on how to work around this, I can also meet up with
> > you on Vidyo or IRC.
> > 
> > Excellent work so far!

Another possible implementation is moving some gamepad management from the main process to the Content process and communicate GPU process / Content process via PVRManager. In this way, we can reduce the effort of implementing a new protocol for helping us send messages between Background thread and GPU process.

How do you think about this? Kip
Flags: needinfo?(kgilbert)
(In reply to Daosheng Mu[:daoshengmu] from comment #23)
> (In reply to Daosheng Mu[:daoshengmu] from comment #22)
> > (In reply to :kip (Kearwood Gilbert) from comment #21)
> > > This is coming together very quickly, thanks Daosheng!
> > > 
> > > I should have mentioned one issue in the bug description that could affect
> > > the implementation; however...
> > > 
> > > The WebVR rendering code in gfx/vr will be running in the GPU process which
> > > I think may differ from the process that VRControllerManagerOpenVR will be
> > > running in.
> > > 
> > > The problem arises when the session created for enumerating VR gamepads
> > > takes focus away from the separate session created for rendering to the VR
> > > display.
> > > 
> > > We will need to access the VR api's only from one process with Oculus as
> > > well once we implement the touch controllers.
> > > 
> > > There are some ways to work around this.  Perhaps
> > > VRControllerManagerOpenVR's code could be moved into gfxVROpenVR and the
> > > code in the gamepad API's process could access this through the PVRManager
> > > procotol?  Perhaps it would be possible to move the abstraction of different
> > > VR API's to VRDisplayHost as well.
> > >
> > 
> > I do some study on the current gamepad architecture. GamepadPlatformService
> > that manages all Gamepad services cross all platforms is running at the main
> > process. It helps us manage gamepad id and notify events between PBackground
> > thread to the Content process. And our VRManager is at Compositor process.
> > 
> > I agree that we should move VRControllerManagerOpenVR to our Compositor
> > process, but PVRManager is designed to serve messages between Content &
> > Compositor processes / threads. It would not help me a lot. Because in the
> > case of Gamepad, it needs a protocol to serve me send/recv messages between
> > PBackground thread and Compositor processes.
> > 
> > So, my next goal is to implement a new protocol to help me send/recv
> > messages between PBackground thread and Compositor processes. I will refer
> > PVRManager and do some modification.
> > 
> > > If you'd like to explore some ideas on what the protocol might look like or
> > > have some other ideas on how to work around this, I can also meet up with
> > > you on Vidyo or IRC.
> > > 
> > > Excellent work so far!
> 
> Another possible implementation is moving some gamepad management from the
> main process to the Content process and communicate GPU process / Content
> process via PVRManager. In this way, we can reduce the effort of
> implementing a new protocol for helping us send messages between Background
> thread and GPU process.
> 
> How do you think about this? Kip

I like the idea of using the existing PVRManager interface, as it already provides some useful bits for things such as hot swapping.

Moving management to the content process should be okay, but may need to check with others that work in Gamepad code to make sure they like it as well.  This might be a good option if that is easier for you.

My recommendation would be to open an additional channel using the PVRManager protocol from the main thread.  Calling VRManagerChild::Get() should open the channel for you.  I believe our code will clean up this channel for you in the main thread as well (if not, I can help out there).  The VRManagerParent / VRManagerChild classes should make no assumptions about what process or thread the child is created on and dispatch updates so all VRManagerChild's get updates on the status of VR displays.

If there are any asserts() that fail using VRManagerChild in the main thread, we can adjust them to be more correct for general usage.  Let me know if you run into anything like that -- I'd be glad to help.
Flags: needinfo?(kgilbert)
Currently, I am working on using PVRManager protocol to connect the main thread and GPU process, and it already can get OpenVR controller events and is sending to GamepadManager which is at the main thread. 

But, it happens an assertion at AssertWorkerThread() when I am sending an message from VRManager to PVRManagerParent. Probably, this is because my controller event handler is running on a runnable thread.
Kip, could you help me take a look at if PVRManager does not allow dispatch events from a runnable thread? I have tried to send events from a non-runnable functions at VRManagerParent that can be received by VRManagerChild. But no matter I dispatch them from the current thread or the main thread at VRManagerParent, both of them would hit AssertWorkerThread() at (https://dxr.mozilla.org/mozilla-central/rev/7c576fe3279d87543f0a03b844eba7bc215e17f1/ipc/glue/MessageChannel.h#445)
Flags: needinfo?(kgilbert)
(In reply to Daosheng Mu[:daoshengmu] from comment #26)
> Kip, could you help me take a look at if PVRManager does not allow dispatch
> events from a runnable thread? I have tried to send events from a
> non-runnable functions at VRManagerParent that can be received by
> VRManagerChild. But no matter I dispatch them from the current thread or the
> main thread at VRManagerParent, both of them would hit AssertWorkerThread()
> at
> (https://dxr.mozilla.org/mozilla-central/rev/
> 7c576fe3279d87543f0a03b844eba7bc215e17f1/ipc/glue/MessageChannel.h#445)

Hi Daosheng,

Could you share a patch where you are hitting the asserts?

The VRManagerParent side is (currently) only intended to be accessed from a single thread, communicating with VRManager/VRDisplayHost/VRLayerParent's...  Currently, this is the compositor thread; however, this will be changing soon to be a dedicated VR thread in the GPU process.  The VRManagerChild should be accessible from any other thread.  If you are in the compositor thread, then you should just use VRManager directly.

If you are trying to create a VRManagerChild and failing to open an IPC channel, maybe the assertion could be incorrect.  If you share a patch, I can take a look.
Flags: needinfo?(kgilbert)
If you are wanting to send a message to all VRManagerChild's from outside the VR Thread (compositor currently), I'd suggest creating a VRManagerChild in the thread reporting the event and bouncing the message from the VRManagerChild->VRManagerParent->VRManager->many VRManagerParents->many VRManagerChilds.
Attachment #8791116 - Flags: review?(kgilbert)
Attachment #8791115 - Flags: review+
Attachment #8791114 - Flags: review?(kgilbert)
Attachment #8791116 - Flags: review?(kgilbert)
Comment on attachment 8797536 [details]
Bug 1299928 - Part 5: Handle gamepad events in Vsync time;

https://reviewboard.mozilla.org/r/83220/#review81762

Hi Kip, can you help me check why I hit the AssertWorkerThread() when I send messages to VRManagerChild? Is it an expected result? If it true, could you give me a suggestion on how to fix it. Thanks.

::: gfx/vr/gfxVROpenVR.cpp:469
(Diff revision 2)
> +  NS_IMETHOD Run() override
> +  {
> +    MOZ_ASSERT(NS_GetCurrentThread() == mControllerManager->mMonitorThread);
> +
> +    mControllerManager->HandleInput();
> +    NS_DispatchToCurrentThread(new VRControllerServiceRunnable(mControllerManager));

Dispatch gamepadEvents to the current thread. But it wil hit AssertWorkerThread() at MessagetChannel.

::: gfx/vr/gfxVROpenVR.cpp:528
(Diff revision 2)
>    ScanForDevices();
> +
> +  // Start watching gamepad event in loop
> +  NS_NewThread(getter_AddRefs(mMonitorThread));
> +  mMonitorThread->Dispatch(new VRControllerServiceRunnable(this), NS_DISPATCH_NORMAL);
>   

I launch a runnable thread for monitoring controller events.
(In reply to Daosheng Mu[:daoshengmu] from comment #37)
> Comment on attachment 8797536 [details]
> Bug 1299928 - Part 5: Dispatch VRController events through a runnableThread;
> 
> https://reviewboard.mozilla.org/r/83220/#review81762
> 
> Hi Kip, can you help me check why I hit the AssertWorkerThread() when I send
> messages to VRManagerChild? Is it an expected result? If it true, could you
> give me a suggestion on how to fix it. Thanks.
> 
> ::: gfx/vr/gfxVROpenVR.cpp:469
> (Diff revision 2)
> > +  NS_IMETHOD Run() override
> > +  {
> > +    MOZ_ASSERT(NS_GetCurrentThread() == mControllerManager->mMonitorThread);
> > +
> > +    mControllerManager->HandleInput();
> > +    NS_DispatchToCurrentThread(new VRControllerServiceRunnable(mControllerManager));
> 
> Dispatch gamepadEvents to the current thread. But it wil hit
> AssertWorkerThread() at MessagetChannel.
> 
> ::: gfx/vr/gfxVROpenVR.cpp:528
> (Diff revision 2)
> >    ScanForDevices();
> > +
> > +  // Start watching gamepad event in loop
> > +  NS_NewThread(getter_AddRefs(mMonitorThread));
> > +  mMonitorThread->Dispatch(new VRControllerServiceRunnable(this), NS_DISPATCH_NORMAL);
> >   
> 
> I launch a runnable thread for monitoring controller events.

Kip, please help me check if it is a correct usage on PVRManager. Thanks.
Flags: needinfo?(kgilbert)
(In reply to Daosheng Mu[:daoshengmu] from comment #38)
> (In reply to Daosheng Mu[:daoshengmu] from comment #37)
> > Comment on attachment 8797536 [details]
> > Bug 1299928 - Part 5: Dispatch VRController events through a runnableThread;
> > 
> > https://reviewboard.mozilla.org/r/83220/#review81762
> > 
> > Hi Kip, can you help me check why I hit the AssertWorkerThread() when I send
> > messages to VRManagerChild? Is it an expected result? If it true, could you
> > give me a suggestion on how to fix it. Thanks.
> > 
> > ::: gfx/vr/gfxVROpenVR.cpp:469
> > (Diff revision 2)
> > > +  NS_IMETHOD Run() override
> > > +  {
> > > +    MOZ_ASSERT(NS_GetCurrentThread() == mControllerManager->mMonitorThread);
> > > +
> > > +    mControllerManager->HandleInput();
> > > +    NS_DispatchToCurrentThread(new VRControllerServiceRunnable(mControllerManager));
> > 
> > Dispatch gamepadEvents to the current thread. But it wil hit
> > AssertWorkerThread() at MessagetChannel.
> > 
> > ::: gfx/vr/gfxVROpenVR.cpp:528
> > (Diff revision 2)
> > >    ScanForDevices();
> > > +
> > > +  // Start watching gamepad event in loop
> > > +  NS_NewThread(getter_AddRefs(mMonitorThread));
> > > +  mMonitorThread->Dispatch(new VRControllerServiceRunnable(this), NS_DISPATCH_NORMAL);
> > >   
> > 
> > I launch a runnable thread for monitoring controller events.
> 
> Kip, please help me check if it is a correct usage on PVRManager. Thanks.

I'm reviewing now, thanks!
Attachment #8791114 - Flags: review?(kgilbert)
Attachment #8791116 - Flags: review?(kgilbert)
In this update, I fix some bugs to make our gamepad tests can be passed. Try looks great, https://treeherder.mozilla.org/#/jobs?repo=try&revision=382e5bf0fd11.
Comment on attachment 8791116 [details]
Bug 1299928 - Part 3: Construct IPC channel between Gamepad and VRManager;

https://reviewboard.mozilla.org/r/78650/#review82288

::: gfx/vr/ipc/PVRManager.ipdl:64
(Diff revision 7)
>    sync SetHaveEventListener(bool aHaveEventListener);
>  
> +  async ControllerListenerAdded();
> +  async ControllerListenerRemoved();
> +  // GetControllers synchronously returns the VR controllers that have already been
> +  // enumerated by xxx() but does not enumerate new ones.

Perhaps either remove xxx() from the comment or describe what xxx is..  Perhaps you mean VRControllerManager?
Attachment #8791116 - Flags: review?(kgilbert)
Comment on attachment 8791116 [details]
Bug 1299928 - Part 3: Construct IPC channel between Gamepad and VRManager;

https://reviewboard.mozilla.org/r/78650/#review82290
Attachment #8791116 - Flags: review?(kgilbert) → review+
Comment on attachment 8797535 [details]
Bug 1299928 - Part 4: Implement VRControllerManager;

https://reviewboard.mozilla.org/r/83218/#review82296

Looks good, r+ from me with the couple of small formatting nit's fixed.

::: gfx/vr/gfxVROpenVR.cpp:503
(Diff revision 3)
> +    vr_ShutdownInternal();
> +    return false;
> +  }
> +
> +  ScanForDevices();
> + 

nit: Whitespace on empty line

::: gfx/vr/gfxVROpenVR.cpp:515
(Diff revision 3)
> +{
> +  mMonitorThread->Shutdown();
> +
> +  mOpenVRInstalled = false;
> +  mOpenVRController.Clear();
> +  mOpenVRInstalled = false;

Remove duplicate line:
mOpenVRInstalled = false;

::: gfx/vr/gfxVROpenVR.cpp:533
(Diff revision 3)
> +      continue;
> +    }
> +
> +    if (mVRSystem->GetControllerState(trackedDevice, &state)) {
> +      if (state.ulButtonPressed) {
> +        //TODO: Convert the button mask to an ID button

It's okay to just handle the single button like this for now, then fill this out for Bug 1299929 after landing.

::: gfx/vr/gfxVROpenVR.cpp:554
(Diff revision 3)
> +    return;
> +  }
> +
> +  aControllerResult.Clear();
> +  for (uint32_t i = 0; i < mOpenVRController.Length(); ++i)
> +  {

nit: Move { to end of for() line
Attachment #8797535 - Flags: review+
Comment on attachment 8791114 [details]
Bug 1299928 - Part 1: Make GamepadManager knows the gamepad is from VRController when adding gamepads;

https://reviewboard.mozilla.org/r/78646/#review82298

r+ from me if you either switch to the GamepadChannel enum in the 3 places, or add a comment why they need to be an uint32_t instead.
Attachment #8791114 - Flags: review+
Comment on attachment 8797535 [details]
Bug 1299928 - Part 4: Implement VRControllerManager;

https://reviewboard.mozilla.org/r/83218/#review82300

r+ from me if you fix the small marked formatting nits.
Comment on attachment 8797536 [details]
Bug 1299928 - Part 5: Handle gamepad events in Vsync time;

https://reviewboard.mozilla.org/r/83220/#review82304

::: gfx/vr/gfxVROpenVR.cpp:468
(Diff revision 3)
> +
> +  NS_IMETHOD Run() override
> +  {
> +    MOZ_ASSERT(NS_GetCurrentThread() == mControllerManager->mMonitorThread);
> +
> +    mControllerManager->HandleInput();

If you are encountering assert's due to sending messages from VRManagerParent to VRManagerChild in this runnable, it would be acceptable to simply call HandleInput() at the bottom of VRManager::RefreshVRDisplays.  This will ensure that it is called once per vsync of the main 2d display when not in VR, and at the full VR refresh rate when in VR (ie. 90hz for HTC Vive).

Later, we intend to move VRManager to its own thread.  There won't be non-vr events handled on this new thread and we could call HandleInput() multiple times per frame for reduced latency.
Comment on attachment 8791114 [details]
Bug 1299928 - Part 1: Make GamepadManager knows the gamepad is from VRController when adding gamepads;

https://reviewboard.mozilla.org/r/78646/#review82306
(In reply to :kip (Kearwood Gilbert) from comment #49)
> Comment on attachment 8791114 [details]
> Bug 1299928 - Part 1: Make GamepadManager knows the gamepad is from
> VRController when adding gamepads;
> 
> https://reviewboard.mozilla.org/r/78646/#review82298
> 
> r+ from me if you either switch to the GamepadChannel enum in the 3 places,
> or add a comment why they need to be an uint32_t instead.

In order to avoid to use this kind of converting, I put GamepadMappingType and GamepadChannel enum into GamepadMessageUtil.h for IPC Serializer.
Comment on attachment 8797536 [details]
Bug 1299928 - Part 5: Handle gamepad events in Vsync time;

https://reviewboard.mozilla.org/r/83220/#review82774

::: gfx/vr/VRManager.cpp:189
(Diff revision 4)
>      // that is looking for VR displays.
>      if (mLastRefreshTime.IsNull()) {
>        // This is the first vsync, must refresh VR displays
>        RefreshVRDisplays();
> +      RefreshVRControllers();
> +      mLastRefreshTime = TimeStamp::Now();

In Vsync time, we refresh out displays and controllers.

::: gfx/vr/VRManager.cpp:364
(Diff revision 4)
> +                         controller);
> +    }
> +  }
> +
> +  for (uint32_t i = 0; i < mControllerManagers.Length(); ++i) {
> +    mControllerManagers[i]->HandleInput();

I put HandleInput() at RefreshVRContollers at Vsync time.
Comment on attachment 8791116 [details]
Bug 1299928 - Part 3: Construct IPC channel between Gamepad and VRManager;

https://reviewboard.mozilla.org/r/78650/#review82772

::: gfx/vr/ipc/VRManagerChild.cpp:489
(Diff revision 8)
> +bool
> +VRManagerChild::RecvGamepadUpdate(const GamepadChangeEvent& aGamepadEvent)
> +{
> +  // VRManagerChild could be at other processes,
> +  // but GamepadManager is only allowed to be run at Content process.
> +  if (XRE_IsContentProcess()) {

I find there would be an in-process VRManager channel, but GamepadManager has to be run at Content process. So, I add this line for sure this is working on Content process.
Attachment #8791115 - Flags: review?(kgilbert)
(In reply to Daosheng Mu[:daoshengmu] from comment #60)
> Comment on attachment 8791116 [details]
> Bug 1299928 - Part 3: Construct IPC channel between Gamepad and VRManager;
> 
> https://reviewboard.mozilla.org/r/78650/#review82772
> 
> ::: gfx/vr/ipc/VRManagerChild.cpp:489
> (Diff revision 8)
> > +bool
> > +VRManagerChild::RecvGamepadUpdate(const GamepadChangeEvent& aGamepadEvent)
> > +{
> > +  // VRManagerChild could be at other processes,
> > +  // but GamepadManager is only allowed to be run at Content process.
> > +  if (XRE_IsContentProcess()) {
> 
> I find there would be an in-process VRManager channel, but GamepadManager
> has to be run at Content process. So, I add this line for sure this is
> working on Content process.

Should I add ( || XRE_IsParentProcess()) for non-e10s mode?
Attachment #8791114 - Flags: review?(kyle) → review?(cleu)
I'm forwarding this review to :lenzak800 since he's in the same office as :daoshengmu, has worked on gamepad before, and could help with further gamepad work after this possibly.

:lenzak800, if you're too busy, just forward the review back to me and I can take care of it. :)

I do have a couple of quick remarks about the patch though. 

- GamepadChannel is a somewhat ambiguous name, since it's actually a type which is used to figure out the index by which you can retrieve the channel actor. Maybe GamepadChannelType? Throwing some quick comments about Gamepad vs. VR channels above the enum definition would help too.

- It looks like this limits us to 16 regular gamepads before we start VR gamepads (index offset is 1 << 4). That should probably be more like 1 << 8 or 1 << 16. I realize the chance of us needing to service >= 16 gamepads is near zero, but I'd rather be safe than sorry, and we have 32 bits to work with in the gamepad indexer.
Comment on attachment 8797536 [details]
Bug 1299928 - Part 5: Handle gamepad events in Vsync time;

https://reviewboard.mozilla.org/r/83220/#review82956

This looks good, thanks!
Attachment #8797536 - Flags: review?(kgilbert) → review+
Comment on attachment 8791115 [details]
Bug 1299928 - Part 2: Rename VRDisplayType to VRDeviceType;

https://reviewboard.mozilla.org/r/78648/#review82958
Attachment #8791115 - Flags: review?(kgilbert) → review+
(In reply to Daosheng Mu[:daoshengmu] from comment #61)
> (In reply to Daosheng Mu[:daoshengmu] from comment #60)
> > Comment on attachment 8791116 [details]
> > Bug 1299928 - Part 3: Construct IPC channel between Gamepad and VRManager;
> > 
> > https://reviewboard.mozilla.org/r/78650/#review82772
> > 
> > ::: gfx/vr/ipc/VRManagerChild.cpp:489
> > (Diff revision 8)
> > > +bool
> > > +VRManagerChild::RecvGamepadUpdate(const GamepadChangeEvent& aGamepadEvent)
> > > +{
> > > +  // VRManagerChild could be at other processes,
> > > +  // but GamepadManager is only allowed to be run at Content process.
> > > +  if (XRE_IsContentProcess()) {
> > 
> > I find there would be an in-process VRManager channel, but GamepadManager
> > has to be run at Content process. So, I add this line for sure this is
> > working on Content process.
> 
> Should I add ( || XRE_IsParentProcess()) for non-e10s mode?
Yes, that would be a good idea.  Even without e10s, there may be other cases where the parent process may want to access all of WebVR including the controllers.  For example, we expect to use the controllers within the browser chrome code and for VR window management.
Flags: needinfo?(kgilbert)
(In reply to Kyle Machulis [:qdot] from comment #62)
> I'm forwarding this review to :lenzak800 since he's in the same office as
> :daoshengmu, has worked on gamepad before, and could help with further
> gamepad work after this possibly.
> 
> :lenzak800, if you're too busy, just forward the review back to me and I can
> take care of it. :)
> 
> I do have a couple of quick remarks about the patch though. 
> 
> - GamepadChannel is a somewhat ambiguous name, since it's actually a type
> which is used to figure out the index by which you can retrieve the channel
> actor. Maybe GamepadChannelType? Throwing some quick comments about Gamepad
> vs. VR channels above the enum definition would help too.
> 
> - It looks like this limits us to 16 regular gamepads before we start VR
> gamepads (index offset is 1 << 4). That should probably be more like 1 << 8
> or 1 << 16. I realize the chance of us needing to service >= 16 gamepads is
> near zero, but I'd rather be safe than sorry, and we have 32 bits to work
> with in the gamepad indexer.

Kyle, thanks for comments. I will continue to discuss this implementation with lenzak800.
(In reply to Daosheng Mu[:daoshengmu] from comment #66)
> Created attachment 8799697 [details]
> Bug 1299928 - Part 6: Making GamepadManager is only run at the same process
> at VRMangerChild;
> 
> Review commit: https://reviewboard.mozilla.org/r/84832/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/84832/

In this update, I have updated some fixes according to the comment 62. Moreover, I realize there is no good way to determine if the VRManagerChild is in e10s mode or not. Therefore, I decide to set GamepadManager to VRManagerChild to make sure it uses the GamepadManager singleton that is used by the content side.
Comment on attachment 8799697 [details]
Bug 1299928 - Part 6: Making GamepadManager is only run at the same process at VRMangerChild;

https://reviewboard.mozilla.org/r/84832/#review83562

This LGTM
Attachment #8799697 - Flags: review?(kgilbert) → review+
Comment on attachment 8791114 [details]
Bug 1299928 - Part 1: Make GamepadManager knows the gamepad is from VRController when adding gamepads;

https://reviewboard.mozilla.org/r/78646/#review84386

LGTM with some naming issue.

::: dom/gamepad/Gamepad.h:39
(Diff revision 8)
>  const int kRightStickYAxis = 3;
>  
> +// Standard channel is used for managing gamepads that
> +// are from GamepadPlatformService. VR channel
> +// is for gamepads that are from VRManagerChild.
> +enum class GamepadChannelType : uint16_t {

GamepadChannelType is a confusing name since its real use is to specify different backend services.

Maybe GamepadServiceType is better.

::: dom/gamepad/GamepadManager.h:50
(Diff revision 8)
>    // Indicate that |aWindow| should no longer receive gamepad events.
>    void RemoveListener(nsGlobalWindow* aWindow);
>  
>    // Add a gamepad to the list of known gamepads.
>    void AddGamepad(uint32_t aIndex, const nsAString& aID, GamepadMappingType aMapping,
> -                  uint32_t aNumButtons, uint32_t aNumAxes);
> +                  GamepadChannelType aChannel, uint32_t aNumButtons, uint32_t aNumAxes);

aChannel -> aServiceType

::: dom/gamepad/GamepadManager.h:53
(Diff revision 8)
>    // Add a gamepad to the list of known gamepads.
>    void AddGamepad(uint32_t aIndex, const nsAString& aID, GamepadMappingType aMapping,
> -                  uint32_t aNumButtons, uint32_t aNumAxes);
> +                  GamepadChannelType aChannel, uint32_t aNumButtons, uint32_t aNumAxes);
>  
>    // Remove the gamepad at |aIndex| from the list of known gamepads.
> -  void RemoveGamepad(uint32_t aIndex);
> +  void RemoveGamepad(uint32_t aIndex, GamepadChannelType aChannel);

ditto.

::: dom/gamepad/GamepadManager.h:132
(Diff revision 8)
>    // Indicate that a window has recieved data from a gamepad.
>    void SetWindowHasSeenGamepad(nsGlobalWindow* aWindow, uint32_t aIndex,
>                                 bool aHasSeen = true);
> +  // Our gamepad index has VR_GAMEPAD_IDX_OFFSET while GamepadChannelType
> +  // is from VRManager.
> +  uint32_t GetGamepadIndexWithChannel(uint32_t aIndex, GamepadChannelType aChannel);

Maybe GetGamepadIndexWithServiceType is better?

And aChannel -> aServiceType

::: dom/gamepad/GamepadManager.cpp:192
(Diff revision 8)
>    }
>  
>    return nullptr;
>  }
>  
> +uint32_t GamepadManager::GetGamepadIndexWithChannel(uint32_t aIndex,

GetGamepadIndexWithChannel -> GetGamepadIndexWithServiceType

aChannel -> aServiceType

::: dom/gamepad/GamepadManager.cpp:221
(Diff revision 8)
> +
>  void
>  GamepadManager::AddGamepad(uint32_t aIndex,
>                             const nsAString& aId,
>                             GamepadMappingType aMapping,
> +                           GamepadChannelType aChannel,

aChannel -> aServiceType

::: dom/gamepad/ipc/GamepadEventTypes.ipdlh:16
(Diff revision 8)
>  
>  struct GamepadAdded {
>    nsString id;
>    uint32_t index;
> -  uint32_t mapping;
> +  GamepadMappingType mapping;
> +  GamepadChannelType channel;

Rename this attribute from channel to service_type

::: dom/gamepad/ipc/GamepadEventTypes.ipdlh:23
(Diff revision 8)
>    uint32_t num_axes;
>  };
>  
>  struct GamepadRemoved {
>    uint32_t index;
> +  GamepadChannelType channel;

ditto.
Attachment #8791114 - Flags: review?(cleu) → review+
Keywords: checkin-needed
Please mark all pending issues as resolved in MozReview so that this can be autolanded.
Keywords: checkin-needed
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #83)
> Please mark all pending issues as resolved in MozReview so that this can be
> autolanded.

Ok. I have marked all pending issues. Thanks
Autoland couldn't rebase the commits for landing :(
Keywords: checkin-needed
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #85)
> Autoland couldn't rebase the commits for landing :(

I have rebased. Please help me try again. Thanks.
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cf68c36df030
Part 1: Make GamepadManager knows the gamepad is from VRController when adding gamepads; r=kip,lenzak800
https://hg.mozilla.org/integration/autoland/rev/2ad2da73d53a
Part 2: Rename VRDisplayType to VRDeviceType; r=kip
https://hg.mozilla.org/integration/autoland/rev/16414aeacf43
Part 3: Construct IPC channel between Gamepad and VRManager; r=kip
https://hg.mozilla.org/integration/autoland/rev/c56ec883e2f3
Part 4: Implement VRControllerManager; r=kip
https://hg.mozilla.org/integration/autoland/rev/298aafb4c28c
Part 5: Handle gamepad events in Vsync time; r=kip
https://hg.mozilla.org/integration/autoland/rev/99eb47ffccb9
Part 6: Making GamepadManager is only run at the same process at VRMangerChild; r=kip
Keywords: checkin-needed
Depends on: 1310904
Depends on: 1310862
You need to log in before you can comment on or make changes to this bug.