Gamepad: Improve gamepad mapping code and unknown gamepad heuristics

Gamepad: Improve gamepad mapping code and unknown gamepad heuristics

This CL modifies the GamepadMappings class to use subclasses
for mappings rather than static methods. The advantage is that
the unknown gamepad mapping can keep state around that allows
it to create heuristic mappings at GamepadDevice constrution
time. This allows stable mappings that don't depend on input
to decide correctly. This also means we aren't doing string
parsing on each gamepad event anymore.

With this change, Moga Pro, Nexus Player (ASUS) Gamepad, and Razer
Serval controllers are working correctly without explict mappings.

Before the change, the D-Pad on Moga, Serval, and ASUS Gamepad were
non-functional. The triggers on the Moga didn't work at all, and the
Serval and ASUS Gamepad's triggers were swapped with the shoulder
buttons.

TEST=http://html5gamepad.com/
BUG=615656
Committed: https://crrev.com/2ec0b0c09fc2cfd30d2ef3b1a7e292c00d0369a8
Cr-Commit-Position: refs/heads/master@{#419372}

[cgutman, 2016-06-17 04:14:53]

aicommander@gmail.com changed reviewers:
+ bajones@chromium.org

[cgutman, 2016-06-17 04:14:54]

Let me know if you'd rather I pursue this implementation instead. The changes
are more extensive, but I think the overall design is better.

[bajones, 2016-06-17 20:37:58]

On 2016/06/17 04:14:54, cgutman wrote:
> Let me know if you'd rather I pursue this implementation instead. The changes
> are more extensive, but I think the overall design is better.

I'm not an owner of the files in question, but LGTM regardless. This definitely
strikes me as a better, more flexible design than the previous CL. Thanks!

[cgutman, 2016-06-17 21:38:41]

aicommander@gmail.com changed reviewers:
+ aelias@chromium.org, changwan@chromium.org, scottmg@chromium.org

[cgutman, 2016-06-17 21:38:42]

Adding reviewers from the old CL.

[aelias_OOO_until_Jul13, 2016-09-15 03:20:30]

Thanks for looking into this, sorry I let this codereview fall off my radar.

https://codereview.chromium.org/2071223002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java
(right):

https://codereview.chromium.org/2071223002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:280:
case MotionEvent.AXIS_LTRIGGER:
These heuristics raise the possibility that we'll drop some axes on some other
unexpected type of gamepad, and it isn't according to spec, which says in
https://w3c.github.io/gamepad/#remapping : "Devices that are not recognized
should still be exposed in their raw form."

So I think we should keep the unknown mapping logic as simple and dumb as
possible (like Firefox does in
https://github.com/mozilla/gecko-dev/blob/master/mobile/android/geckoview/src...
).  Let's map each one of these MotionEvent enum values to one particular axis
index in
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebGa...,
and likewise for the buttons.  And we should make it such that  the same unknown
DirectInput gamepad plugged into Chrome for Windows and Android will produce the
same axes and button information via the gamepad API (which would make it easier
for Javascript name-string-based mapping databases to work).

[cgutman, 2016-09-16 20:01:38]

On 2016/09/15 03:20:30, aelias wrote:
> Thanks for looking into this, sorry I let this codereview fall off my radar.
> 
No problem. Thanks for giving it a look.

>
https://codereview.chromium.org/2071223002/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java
> (right):
> 
>
https://codereview.chromium.org/2071223002/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:280:
> case MotionEvent.AXIS_LTRIGGER:
> These heuristics raise the possibility that we'll drop some axes on some other
> unexpected type of gamepad, and it isn't according to spec, which says in
> https://w3c.github.io/gamepad/#remapping : "Devices that are not recognized
> should still be exposed in their raw form."
> 
> So I think we should keep the unknown mapping logic as simple and dumb as
> possible (like Firefox does in
>
https://github.com/mozilla/gecko-dev/blob/master/mobile/android/geckoview/src...
> ).  Let's map each one of these MotionEvent enum values to one particular axis
> index in
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebGa...,
> and likewise for the buttons.  And we should make it such that  the same
unknown
> DirectInput gamepad plugged into Chrome for Windows and Android will produce
the
> same axes and button information via the gamepad API (which would make it
easier
> for Javascript name-string-based mapping databases to work).

Hmm, I think that's going to be challenging on Android, since different devices
may ship with different .kl files for various gamepads. Those may skew our view
of the same gamepads when attached to different Android devices or different
versions of Android.

It looks like the code you linked does seem to have some hardcoded logic for
certain axes to mean certain things. For example, they assume that triggers
are either AXIS_LTRIGGER and AXIS_RTRIGGER or AXIS_BRAKE and AXIS_GAS on line
119. Likewise, they're assuming the sticks are on X, Y, Z, and RZ (line 31) and
the d-pad is on AXIS_HAT_X and AXIS_HAT_Y (line 61).

[aelias_OOO_until_Jul13, 2016-09-16 20:17:21]

https://codereview.chromium.org/2071223002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java
(right):

https://codereview.chromium.org/2071223002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:280:
case MotionEvent.AXIS_LTRIGGER:
> Hmm, I think that's going to be challenging on Android, since different
devices
> may ship with different .kl files for various gamepads. Those may skew our
view
> of the same gamepads when attached to different Android devices or different
> versions of Android.

Hmm, OK, I suppose attempting to hold the line on any manner of uniformity in
the wild west of gamepads is hopeless.  This patch is mostly good to go as is,
then.

https://codereview.chromium.org/2071223002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:320:
// These are correct on all standard Android gamepads
Is "standard Android gamepad" really a thing?  If not, you could say "These are
shared among all gamepads intended for use with Android that we tested so far."

https://codereview.chromium.org/2071223002/diff/1/content/public/android/juni...
File
content/public/android/junit/src/org/chromium/content/browser/input/GamepadMappingsTest.java
(right):

https://codereview.chromium.org/2071223002/diff/1/content/public/android/juni...
content/public/android/junit/src/org/chromium/content/browser/input/GamepadMappingsTest.java:150:
public void testUnknownGamepadMappingA() throws Exception {
These tests seem to be based on certain reference gamepads.  Could you name each
test after them?  For example, testUnknownMogaPro().

[cgutman, 2016-09-17 01:16:09]

On 2016/09/16 20:17:21, aelias wrote:
>
https://codereview.chromium.org/2071223002/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java
> (right):
> 
>
https://codereview.chromium.org/2071223002/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:280:
> case MotionEvent.AXIS_LTRIGGER:
> > Hmm, I think that's going to be challenging on Android, since different
> devices
> > may ship with different .kl files for various gamepads. Those may skew our
> view
> > of the same gamepads when attached to different Android devices or different
> > versions of Android.
> 
> Hmm, OK, I suppose attempting to hold the line on any manner of uniformity in
> the wild west of gamepads is hopeless.  This patch is mostly good to go as is,
> then.
> 
>
https://codereview.chromium.org/2071223002/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:320:
> // These are correct on all standard Android gamepads
> Is "standard Android gamepad" really a thing?  If not, you could say "These
are
> shared among all gamepads intended for use with Android that we tested so
far."
> 

Not really, I changed it to your wording instead.

>
https://codereview.chromium.org/2071223002/diff/1/content/public/android/juni...
> File
>
content/public/android/junit/src/org/chromium/content/browser/input/GamepadMappingsTest.java
> (right):
> 
>
https://codereview.chromium.org/2071223002/diff/1/content/public/android/juni...
>
content/public/android/junit/src/org/chromium/content/browser/input/GamepadMappingsTest.java:150:
> public void testUnknownGamepadMappingA() throws Exception {
> These tests seem to be based on certain reference gamepads.  Could you name
each
> test after them?  For example, testUnknownMogaPro().

They were loosely based on real gamepads. I retested and made some slight
changes
to the test cases to match real hardware more closely.

[aelias_OOO_until_Jul13, 2016-09-17 01:17:12]

The CQ bit was checked by aelias@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-17 01:17:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aelias_OOO_until_Jul13, 2016-09-17 01:24:58]

lgtm, thanks!

[commit-bot: I haz the power, 2016-09-17 02:17:13]

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

[commit-bot: I haz the power, 2016-09-17 02:17:14]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[aelias_OOO_until_Jul13, 2016-09-17 02:22:35]

There is a minor issue found by the trybot, you will have to fix before landing:

FindBugs reported the following issues:
SF_SWITCH_NO_DEFAULT: Switch statement found where default case is missing
  In class org.chromium.device.gamepad.GamepadMappings$UnknownGamepadMappings
  In method new
org.chromium.device.gamepad.GamepadMappings$UnknownGamepadMappings(int[])
  At GamepadMappings.java:[lines 278-300]

[cgutman, 2016-09-17 02:34:12]

On 2016/09/17 02:22:35, aelias wrote:
> There is a minor issue found by the trybot, you will have to fix before
landing:
> 
> FindBugs reported the following issues:
> SF_SWITCH_NO_DEFAULT: Switch statement found where default case is missing
>   In class org.chromium.device.gamepad.GamepadMappings$UnknownGamepadMappings
>   In method new
> org.chromium.device.gamepad.GamepadMappings$UnknownGamepadMappings(int[])
>   At GamepadMappings.java:[lines 278-300]

Uploaded a new patch set to fix this.

[aelias_OOO_until_Jul13, 2016-09-17 02:41:09]

The CQ bit was checked by aelias@chromium.org

[aelias_OOO_until_Jul13, 2016-09-17 02:41:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
bajones@chromium.org, aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/2071223002/#ps60001
(title: "Add missing switch default case")

[commit-bot: I haz the power, 2016-09-17 02:41:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aelias_OOO_until_Jul13, 2016-09-17 17:12:30]

The CQ bit was unchecked by aelias@chromium.org

[aelias_OOO_until_Jul13, 2016-09-17 17:12:59]

The CQ bit was checked by aelias@chromium.org

[commit-bot: I haz the power, 2016-09-17 17:13:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-17 19:36:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-17 19:38:08]

Description was changed from

==========
Gamepad: Improve gamepad mapping code and unknown gamepad heuristics

This CL modifies the GamepadMappings class to use subclasses
for mappings rather than static methods. The advantage is that
the unknown gamepad mapping can keep state around that allows
it to create heuristic mappings at GamepadDevice constrution
time. This allows stable mappings that don't depend on input
to decide correctly. This also means we aren't doing string
parsing on each gamepad event anymore.

With this change, Moga Pro, Nexus Player (ASUS) Gamepad, and Razer
Serval controllers are working correctly without explict mappings.

Before the change, the D-Pad on Moga, Serval, and ASUS Gamepad were
non-functional. The triggers on the Moga didn't work at all, and the
Serval and ASUS Gamepad's triggers were swapped with the shoulder
buttons.

TEST=http://html5gamepad.com/
BUG=615656
==========

to

==========
Gamepad: Improve gamepad mapping code and unknown gamepad heuristics

This CL modifies the GamepadMappings class to use subclasses
for mappings rather than static methods. The advantage is that
the unknown gamepad mapping can keep state around that allows
it to create heuristic mappings at GamepadDevice constrution
time. This allows stable mappings that don't depend on input
to decide correctly. This also means we aren't doing string
parsing on each gamepad event anymore.

With this change, Moga Pro, Nexus Player (ASUS) Gamepad, and Razer
Serval controllers are working correctly without explict mappings.

Before the change, the D-Pad on Moga, Serval, and ASUS Gamepad were
non-functional. The triggers on the Moga didn't work at all, and the
Serval and ASUS Gamepad's triggers were swapped with the shoulder
buttons.

TEST=http://html5gamepad.com/
BUG=615656

Committed: https://crrev.com/2ec0b0c09fc2cfd30d2ef3b1a7e292c00d0369a8
Cr-Commit-Position: refs/heads/master@{#419372}
==========

[commit-bot: I haz the power, 2016-09-17 19:38:09]

Patchset 4 (id:??) landed as
https://crrev.com/2ec0b0c09fc2cfd30d2ef3b1a7e292c00d0369a8
Cr-Commit-Position: refs/heads/master@{#419372}