Implemented Gamepad support via Raw Input on Windows

Implemented Gamepad support via Raw Input on Windows

This will hopefully be less unstable than the DirectX method.

BUG=305267
R=piman@chromium.org, scottmg@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248741

[bajones, 2014-01-31 19:10:43]

I feel like this is in a good state to go in now. In the process of gathering
some more gamepads so I can test/map them, but this still provides better
support than what we had with DirectInput disabled. Several more follow-up
patches that I want to do in this space as well, such as removing the
DirectInput code and updating the Web-facing API to match the current spec.

[scottmg, 2014-01-31 20:51:10]

Still trying to wrap my head around it, but some early comments.

Also, could you remove DirectInput as a pre-cursor to this patch instead of
afterwards? I think it would simplify some of the changes here. It's all
disabled right now anyway, so no benefit to keeping it regardless of whether Raw
proves more reliable or not.

https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right):

https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:398: pads->items[i]
= data_.items[i];
this seems kind of silly for xinput

https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
File content/browser/gamepad/gamepad_platform_data_fetcher_win.h (right):

https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
content/browser/gamepad/gamepad_platform_data_fetcher_win.h:107:
blink::WebGamepads data_;
i don't like storing this state here. does Raw need to maintain state between
updates? if so, can it go in the fetcher instead?

https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
File content/browser/gamepad/gamepad_provider.cc (right):

https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
content/browser/gamepad/gamepad_provider.cc:122: // message loop needs to be a
UI-type loop. On Windows it doesn't matter what the
Is there some reason this needed to change? If so, comment for Windows (and line
length).

https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
File content/browser/gamepad/raw_input_data_fetcher_win.cc (right):

https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
content/browser/gamepad/raw_input_data_fetcher_win.cc:34: :
hid_dll_(base::FilePath(FILE_PATH_LITERAL("hid.dll")))
can you run this through "git cl format"? there's some "for(" and "if(" spacing
nits below too.

https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
content/browser/gamepad/raw_input_data_fetcher_win.cc:129: if (result == -1) {
UINT == -1? :/

https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
content/browser/gamepad/raw_input_data_fetcher_win.cc:402: hidp_get_usage_value_
= NULL;
is hid.dll always available? add if (!hid_dll_.is_valid()) return false; here?

[bajones, 2014-02-03 22:57:31]

Reasonably big update, got my hands on several new controllers that exposed a
couple of issues with the previous patch. Fortunately the code is now more
robust because of it!

On 2014/01/31 20:51:10, scottmg wrote:
>
https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
> File content/browser/gamepad/gamepad_platform_data_fetcher_win.cc (right):
> 
>
https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
> content/browser/gamepad/gamepad_platform_data_fetcher_win.cc:398:
pads->items[i]
> = data_.items[i];
> this seems kind of silly for xinput

Right, and now that the DI path is gone it makes no sense at all. Refactored.
 
>
https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
> File content/browser/gamepad/gamepad_platform_data_fetcher_win.h (right):
> 
>
https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
> content/browser/gamepad/gamepad_platform_data_fetcher_win.h:107:
> blink::WebGamepads data_;
> i don't like storing this state here. does Raw need to maintain state between
> updates? if so, can it go in the fetcher instead?

We were losing the controller ID on the DI and Raw Input paths. I looked at the
mac and linux code paths, and this was how they preserved that kind of
information, so I tried for an approach that was closer to theirs. After some
more fiddling, however, I got everything working again without it. Agreed that
less copies of state == better.
 
>
https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
> File content/browser/gamepad/gamepad_provider.cc (right):
> 
>
https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
> content/browser/gamepad/gamepad_provider.cc:122: // message loop needs to be a
> UI-type loop. On Windows it doesn't matter what the
> Is there some reason this needed to change? If so, comment for Windows (and
line
> length).

I changed this once, must have been the victim of a bad rebase. Updated! (We
need a UI thread to pump the MessageWindow)

>
https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
> File content/browser/gamepad/raw_input_data_fetcher_win.cc (right):
> 
>
https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
> content/browser/gamepad/raw_input_data_fetcher_win.cc:34: :
> hid_dll_(base::FilePath(FILE_PATH_LITERAL("hid.dll")))
> can you run this through "git cl format"? there's some "for(" and "if("
spacing
> nits below too.
> 
>
https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
> content/browser/gamepad/raw_input_data_fetcher_win.cc:129: if (result == -1) {
> UINT == -1? :/

Yay Microsoft...
(http://msdn.microsoft.com/en-us/library/windows/desktop/ms645598(v=vs.85).aspx)
:P
Added a static cast so that at the very least it _looks_ intentional. 

>
https://codereview.chromium.org/135523006/diff/60001/content/browser/gamepad/...
> content/browser/gamepad/raw_input_data_fetcher_win.cc:402:
hidp_get_usage_value_
> = NULL;
> is hid.dll always available? add if (!hid_dll_.is_valid()) return false; here?

I think hid.dll is always available (I doubt anything USB works without it) but
since it's not directly under our control the added check is probably wise.
Added.

[scottmg, 2014-02-03 23:58:15]

This mostly lg (modulo my complete infamiliarty with raw input). Can you confirm
with manual testing that:

- there's no significant part of this code being run when you're on a page
that's *not* using gamepad (that's where the DirectInput path bit us by tickling
old broken drivers on the main path)

- adding/removing XInput controllers with Raw Input doesn't mix the player
index? (it looks right to me, but...) i.e. insert xinput (index 0), insert raw
input (index 1), remove xinput, raw input stays as index 1 (and variations on
that).

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
File content/browser/gamepad/gamepad_platform_data_fetcher_win.h (right):

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/gamepad_platform_data_fetcher_win.h:89: HANDLE handle; 
// RawInput-only fields.
handle -> raw_input_handle for consistency

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
File content/browser/gamepad/raw_input_data_fetcher_win.cc (right):

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
nit; just Copyright 2014 since it's new.

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:8: //#include
"base/win/windows_version.h"
nit; remove

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:138: if (result == -1) {
another static_cast here

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:175: if (result == -1) {
another static_cast here

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:186: if (result == -1) {
another static_cast here

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:202: return NULL;
nit; indent wrong (git cl format?)

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:205: gamepad_info->handle
= hDevice;
please reorder the ones near the top here so they match the header declaration
order so it's  a bit clearer they're all getting initialized.

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:219: if (result == -1) {
and here

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:228: if (result == -1) {
and here :/

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:252:
swprintf(gamepad_info->id, WebGamepad::idLengthCap, L"Unknown Gamepad");
is "Gamepad" correct? i.e. DeviceUsages name instead? i guess this doesn't
really happen anyway, and calling it a "Multi Axis" might be more confusing
anyway. soo... feel free to ignore if you don't think it's important.

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:257: if (result == -1) {
and here

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:268: if (result == -1) {
and here

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:289: min_index =
gamepad_info->button_caps[i].Range.UsageMin - 1;
can UsageMin be 0?

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:336: scoped_ptr<USAGE[]>
usages(new USAGE[buttons_length]);
These new's make me (pointlessly) cringe in the Update loop, but I guess we
don't know an upper bound other than 65536. :(

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:368: // If the min in < 0
we have to query the scaled value, otherwise we need
in -> is

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:399: if (result == -1) {
and here

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:410: if (result == -1) {
and here

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:423: return
DefRawInputProc(&input, 1, sizeof(RAWINPUTHEADER));
do we want to pass these on to the DefProc? it seems like we did process them,
so wouldn't want them to passed on, for example if they were to cause navigation
or similar.

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
File content/browser/gamepad/raw_input_data_fetcher_win.h (right):

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
nit; 2014

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.h:11: #define
WIN32_LEAN_AND_MEAN
is this necessary to avoid a conflict? there's other places where we need to
*not* have it, so could induce confusion.

[bajones, 2014-02-04 01:00:35]

Addressed feedback, with a couple of specific comments below. 

Seems well behaved in regards to preserving gamepad indicies, as demonstrated by
my totally scientific random flurry of connects and disconnects. I've also
verified that we don't start listening to WM_INPUT messages until you have
visited a gamepad-enabled page.

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
File content/browser/gamepad/raw_input_data_fetcher_win.cc (right):

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:202: return NULL;
On 2014/02/03 23:58:15, scottmg wrote:
> nit; indent wrong (git cl format?)
git cl format is broken for me right now for unknown reasons. Sorry! I've done
another manual style pass, hopefully I've caught remaining style nits (and
hopefully I can fix the tool soon)

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:252:
swprintf(gamepad_info->id, WebGamepad::idLengthCap, L"Unknown Gamepad");
On 2014/02/03 23:58:15, scottmg wrote:
> is "Gamepad" correct? i.e. DeviceUsages name instead? i guess this doesn't
> really happen anyway, and calling it a "Multi Axis" might be more confusing
> anyway. soo... feel free to ignore if you don't think it's important.
Well then, ignored it is! :)
(Might squeeze this into a future CL)

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:289: min_index =
gamepad_info->button_caps[i].Range.UsageMin - 1;
On 2014/02/03 23:58:15, scottmg wrote:
> can UsageMin be 0?
If the device is well behaved? No. Should we rely on that? Probably not. I've
refactored the related code to ensure it's a non-issue.

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:423: return
DefRawInputProc(&input, 1, sizeof(RAWINPUTHEADER));
On 2014/02/03 23:58:15, scottmg wrote:
> do we want to pass these on to the DefProc? it seems like we did process them,
> so wouldn't want them to passed on, for example if they were to cause
navigation
> or similar.

MSDN says that we should be calling it every time to ensure that internal
resources are cleaned up:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms645590(v=vs.85).aspx

[scottmg, 2014-02-04 01:15:06]

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
File content/browser/gamepad/raw_input_data_fetcher_win.cc (right):

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:423: return
DefRawInputProc(&input, 1, sizeof(RAWINPUTHEADER));
On 2014/02/04 01:00:35, bajones wrote:
> On 2014/02/03 23:58:15, scottmg wrote:
> > do we want to pass these on to the DefProc? it seems like we did process
them,
> > so wouldn't want them to passed on, for example if they were to cause
> navigation
> > or similar.
> 
> MSDN says that we should be calling it every time to ensure that internal
> resources are cleaned up:
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms645590%28v=vs.85%29...


Sorry, I don't see that there? I see DefWindowProc which MessageWindow will call
when HandleMessage returns false, but for WM_INPUT, we're not calling
DefWindowProc, only DefRawInputProc, right?

So my questions:
- should we DefWindowProc per your link?
- should we *not* DefRawInputProc per
http://msdn.microsoft.com/en-us/library/windows/desktop/ms645594(v=vs.85).aspx ?

https://codereview.chromium.org/135523006/diff/220001/content/browser/gamepad...
File content/browser/gamepad/raw_input_data_fetcher_win.cc (right):

https://codereview.chromium.org/135523006/diff/220001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:286: if
(button_caps->Range.UsageMin - 1 < WebGamepad::buttonsLengthCap) {
i don't think this works if UsageMin == 0 still? (rather + 1 on the other side)
but if UsageMin is documented to be > 0 then ok.

[bajones, 2014-02-04 03:53:45]

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
File content/browser/gamepad/raw_input_data_fetcher_win.cc (right):

https://codereview.chromium.org/135523006/diff/140001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:423: return
DefRawInputProc(&input, 1, sizeof(RAWINPUTHEADER));
On 2014/02/04 01:15:06, scottmg wrote:
> Sorry, I don't see that there? [...]

Sorry, I misread DefWindowProc as DefRawInputProc on that page. Now I'm fairly
confused. :( Microsoft's documentation regarding this leaves a lot to be
desired.

What I do know is that the two other locations in Chrome that we use Raw Input
follow the same pattern as this code:
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/user_in...
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/loca...

And all other code examples that I've found do the same. I've been looking
around for something more definitive, but haven't had much luck so far.

https://codereview.chromium.org/135523006/diff/220001/content/browser/gamepad...
File content/browser/gamepad/raw_input_data_fetcher_win.cc (right):

https://codereview.chromium.org/135523006/diff/220001/content/browser/gamepad...
content/browser/gamepad/raw_input_data_fetcher_win.cc:286: if
(button_caps->Range.UsageMin - 1 < WebGamepad::buttonsLengthCap) {
On 2014/02/04 01:15:06, scottmg wrote:
> i don't think this works if UsageMin == 0 still? (rather + 1 on the other
side)
> but if UsageMin is documented to be > 0 then ok.

This DOES work, but it occurred to me that the correct logic is easily achieved
without the -1. I've updated it to leave out the subtraction and use <= instead,
which reads cleaner.

[scottmg, 2014-02-04 04:04:12]

LGTM enough to land and iterate on the parts that aren't well-documented. :)

Thanks for doing this!

[bajones, 2014-02-04 04:23:30]

On 2014/02/04 04:04:12, scottmg wrote:
> LGTM enough to land and iterate on the parts that aren't well-documented. :)
> 
> Thanks for doing this!

My pleasure!

jam@, or piman@ can I get a quick review on the gypi? (Just two new files)

[scottmg, 2014-02-04 04:42:34]

I think it's OK to TBR non-structural changes to the gypi files like these
minor file additions.


On Mon, Feb 3, 2014 at 8:23 PM, <bajones@chromium.org> wrote:

> On 2014/02/04 04:04:12, scottmg wrote:
>
>> LGTM enough to land and iterate on the parts that aren't well-documented.
>> :)
>>
>
>  Thanks for doing this!
>>
>
> My pleasure!
>
> jam@, or piman@ can I get a quick review on the gypi? (Just two new files)
>
> https://codereview.chromium.org/135523006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-02-04 04:51:31]

lgtm

[bajones, 2014-02-04 04:53:12]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-02-04 04:53:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/135523006/260001

[commit-bot: I haz the power, 2014-02-04 05:33:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-04 05:33:14]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[bajones, 2014-02-04 17:21:47]

Committed patchset #6 manually as r248741 (presubmit successful).