Closed Bug 690937 Opened 13 years ago Closed 10 years ago

Add an XInput backend for Windows gamepad support

Categories

(Core :: Hardware Abstraction Layer (HAL), defect)

All
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: ted, Assigned: ted)

References

(Blocks 1 open bug)

Details

(Whiteboard: [paladin])

Attachments

(1 file, 1 obsolete file)

XInput is the shiny new API that replaces DirectInput. It has much better support for XBox 360 controllers and other new gamepads. We should be able to use both XI and DI to support the widest range of devices, but we'll get much better results out of 360 controllers using XI.
Keywords: dev-doc-needed
Assignee: nobody → jon
I'm not sure why you think this is dev-doc-needed? Doesn't seem to fit the criteria to me. In any event, it hasn't been implemented yet, so it's a bit early for that.
(In reply to Ted Mielczarek [:ted, :luser] from comment #1)
> Doesn't seem to fit the criteria to me. 
True, sorry. This was a quick scan thrugh bugs which might need that keyword.

> In any event, it hasn't been implemented yet, so it's a bit early for that.
It's never to early (it doesn't hurt) but somtimes to late (if forgotten). IMHO it should be added when the bug is filed.

http://www.bitstampede.com/?s=dev-doc-needed
"First off, my traditional plea: if you’ve got code you’re working on that even might impact documentation, please add the “dev-doc-needed” keyword to the relevant bug"

"First, if there’s a bug related to your work, make sure the “dev-doc-needed” keyword is added to the bug in Bugzilla. It doesn’t matter if you’ve actually made the change or not. The writers only apply changes to the documentation once the bug is both tagged as “dev-doc-needed” and the bug is marked as fixed."
Whiteboard: [paladin]
Component: DOM → Hardware Abstraction Layer (HAL)
Assignee: jon → nobody
Jesse, do you have any interest in working on this for your final releases, since your layout stuff for webvtt is done?  I know Ted's looking for some help.
Attached patch add XInput gamepad backend (obsolete) — Splinter Review
This is a work in progress. With this patch applied my demos work with an xbox 360 controller on Windows. The patch disables scanning for DirectInput devices, since I haven't added code to check if a DI device is handled using XInput.

As a bonus, gamepads exposed via XInput get mapped to the same buttons/axes as they do in Chrome.
Assignee: nobody → ted
Depends on: 860409
Depends on: 860413
Depends on: 996078
This is written on top of the patch in bug 996078, and adds support for XInput controllers.

The only downside here is that XInput is a polling API, so this adds a repeating timer to do the polling. We could probably be a little smarter about disabling the timer when no visible page is using the gamepad.
Attachment #731140 - Attachment is obsolete: true
Comment on attachment 8406260 [details] [diff] [review]
Add XInput support to the Windows gamepad backend

Jim: since you so graciously reviewed my Raw Input patch, this patch goes on top of that one.

The XInput docs are here:
http://msdn.microsoft.com/en-us/library/windows/desktop/ee417001%28v=vs.85%29.aspx

It's a pretty simple API.
Attachment #8406260 - Flags: review?(jmathies)
rats, I only have wireless controllers. I'll order one on amazon but won't have it to test this patch.
Comment on attachment 8406260 [details] [diff] [review]
Add XInput support to the Windows gamepad backend

Review of attachment 8406260 [details] [diff] [review]:
-----------------------------------------------------------------

::: hal/windows/WindowsGamepad.cpp
@@ +45,5 @@
>  // Therefore, we wait a bit after receiving one before looking for
>  // device changes.
>  const uint32_t kDevicesChangedStableDelay = 200;
> +// Arbitrary.
> +const uint32_t kXInputPollInterval = 50;

nit - maybe be a bit more descriptive here. :) What's this poll value for?

@@ +122,5 @@
>    // Used during rescan to find devices that were disconnected.
>    bool present;
>  };
>  
> +// If we dropped support for VC++ < 2010 I would use decltype in the

I'd suggest rewording this - "when we drop support for.." "..switch to decltype for GetProcAddress calls below."

@@ +369,5 @@
> +  return false;
> +}
> +
> +bool
> +WindowsGamepadService::ScanForXInputDevices()

nit - add a debug assert that checks for a valid mXInput here.

@@ +448,5 @@
> +  self->PollXInput();
> +}
> +
> +void
> +WindowsGamepadService::PollXInput()

nit - debug assert on mXInput.

@@ +496,5 @@
> +  }
> +
> +  // Finally deal with analog sticks
> +  if (state.Gamepad.sThumbLX != gamepad.state.Gamepad.sThumbLX) {
> +    //TODO: deadzone

nit - how bout we skip adding the todos and just file a bug?

@@ +792,5 @@
>  
>  void
>  WindowsGamepadService::Cleanup()
>  {
> +  mXInputPollTimer->Cancel();

any chance this could be null here? If so please wrap in a null check.
Attachment #8406260 - Flags: review?(jmathies) → review+
(In reply to Jim Mathies [:jimm] from comment #7)
> rats, I only have wireless controllers. I'll order one on amazon but won't
> have it to test this patch.

FYI you can use one of these to use wireless controllers on a PC:
http://www.amazon.com/Xbox-360-Wireless-Gaming-Receiver-Windows/dp/B000HZFCT2
(In reply to Jim Mathies [:jimm] from comment #8)
> > +// If we dropped support for VC++ < 2010 I would use decltype in the
> 
> I'd suggest rewording this - "when we drop support for.." "..switch to
> decltype for GetProcAddress calls below."

As it turns out we did already drop support for VC < 2010, so I can do this. Unfortunately the definition of XInputEnable isn't in the SDK I have, so I think I need to leave the hacky bit in for that. I'll fix the one declaration and the comment, at least.
Blocks: 1001955
Backed out for B2G desktop Windows build failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f99cf3aee13f
https://hg.mozilla.org/mozilla-central/rev/2462b87f45bb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.