Gamepad API for Android

content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java

Index: content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java b/content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java
new file mode 100644
index 0000000000000000000000000000000000000000..fcf63a692fcf56ccd8fa4a2a1a834871ba59ea07
--- /dev/null
+++ b/content/public/android/java/src/org/chromium/content/browser/input/GamepadAdapter.java
@@ -0,0 +1,305 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.content.browser.input;
+
+import android.content.Context;
+import android.hardware.input.InputManager;
+import android.util.SparseArray;
+import android.view.InputDevice;
+import android.view.InputDevice.MotionRange;
+import android.view.KeyEvent;
+import android.view.MotionEvent;
+
+import org.chromium.base.CalledByNative;
+import org.chromium.base.JNINamespace;
+import org.chromium.base.ThreadUtils;
+
+import java.util.List;
+
+@JNINamespace("content")
+class WebGamepadData {

[Ted C, 2014/03/07 20:18:09]

I'd prefer this to be a separate class as it is referred to by other files in
the package.

[kbalazs, 2014/03/12 00:17:36]

You meant a separate file, right? :) Done.

+    public String id;
+    public String mapping;
+    public float[] axes;
+    public float[] buttons;
+}
+
+/**
+ * Java counterpart of GamepadPlatformDataFetcherAndroid.
+ * Manages game input devices and feed Gamepad API with input data.
+ * GamepadPlatformDataFetcherAndroid is merely a wrepper around this.
+ * Native callable methods called by GamepadPlatformDataFetcherAndroid on the poller thread
+ * which is a native thread without a java looper. Events are processed on the UI thread.
+ */
+@JNINamespace("content")
+public class GamepadAdapter implements InputManager.InputDeviceListener {
+
+    private static final int NUM_WEB_GAMEPADS = 4;

[Ted C, 2014/03/07 20:18:09]

where did this number come from?

[kbalazs, 2014/03/07 22:40:22]

From the spec :) Also from blink::WebGamepads.

+
+    private static GamepadAdapter instance;
+
+    private final InputManager mInputManager;
+    private InputDeviceHandler[] mDeviceHandlers;
+    private final Object mDeviceHandlersLock = new Object();
+    private boolean mIsFetched;
+    private boolean mIsPaused;
+    private long mNativeDataFetcher;
+
+    public static void initialize(Context context) {
+        if (instance == null)
+            instance = new GamepadAdapter(context);
+    }
+
+    public static GamepadAdapter getInstance() {

[Ted C, 2014/03/07 20:18:09]

Any reason this has to be an instance?

We are trying to remove the expectations that only a single content view/web
contents will be visible at any given time.  I'm just wondering if this can be
tied 1-1 to a content view.  It also makes sure that we don't leak events from
one content view to another.

[kbalazs, 2014/03/07 22:40:22]

I'm not sure a 1-1 relation is a really good idea. Afaik there is certainly one
view that can be focused and generate input events at a time. The native side of
the implementation is already organized around singletons, i.e. there is one
GamepadService, one GamepadProvider and one GamepadPlatformDataFetcher. The
desktop implementations read the device directly and provide the data to every
renderer that started reading it. I think leaking events between views is not
something the current design tries to avoid and it would require a complete
redesign (not just in the platform specific part).

[Ted C, 2014/03/08 01:06:00]

You are definitely correct that only a single view can have focus and
generate events.

And that is why I find it quite odd (and in my opinion very much the wrong
and incorrect behavior) that you could have two desktop browser windows
open that were registered for gamepad input and that we would be sending
it to both even though only one had window focus (or maybe the renderer
only polls for active windows).

Personally, that feels incorrect, but I agree that it is better to match
desktop behavior then gate this on a larger refactor.

+        assert instance != null;
+        return instance;
+    }
+
+    private GamepadAdapter(Context context) {
+        assert ThreadUtils.runningOnUiThread();
+        mInputManager = (InputManager) context.getSystemService(Context.INPUT_SERVICE);

[boliu, 2014/03/10 18:27:04]

Can this ever be null (say if webview is in a service)? What if there is no
keyboard active (I got my device into that situation before...)

Probably safer to not assume getSystemService a real object.

[kbalazs, 2014/03/12 00:17:36]

Done, added checks.

+        mIsFetched = false;
+        mIsPaused = false;

[Ted C, 2014/03/08 01:06:00]

I just noticed that this is the same calls as resume(), so you can probably
just call that here.

+        enumerateDevices();

[Ted C, 2014/03/07 20:18:09]

I would probably call this initializeDevices

[kbalazs, 2014/03/12 00:17:36]

Done.

+        mInputManager.registerInputDeviceListener(this, null);
+    }
+
+    public void pause() {

[Ted C, 2014/03/07 20:18:09]

For all non-private methods, you should have javadoc.

Here too, I'd really like to see pause and resume tied to the ContentView
instead of singleton.

[kbalazs, 2014/03/07 22:40:22]

You are right in that if there is more than one active ContentView they can
conflict here. Considering to keep the singleton design, I think it can be
addressed by adding a counter for pause/resume or calling them by ContentView
when it gains/loses focus.

[Ted C, 2014/03/08 01:06:00]

Relying on pause and resume is somewhat dangerous as onHide might never be
called on a ContentView if it is being destroyed.  And at the same time,
onDestroy is also not reliably called in cases like WebView (where
applications can just just rely on garbage collection).

I believe you can rely on onAttachedToWindow and onDetachedFromWindow in
WebView as well as Chrome, so maybe that is ok for now.  Granted, having
to keep a counter seems very fragile and the same logic has been the
source of many of bugs for us in the past.

I guess if you did:

private boolean mAttachedToWindow;

ContentViewCore#onAttachedToWindow() {
    if (mAttachedToWindow) return;
    ....
    GameAdapter.getInstance().registerListener();
    mAttachedToWindow = true;
}

ContentViewCore#onDetachedFromWindow() {
    if (!mAttachedToWindow) return;
    GameAdapter.getInstance().unregisterListener();
    mAttachedToWindow = false;
}

That is probably the safest way to to ensure the counters are never
out of sync.  Conversely, if you want to be move overzealous of
unregistering, you can track a separate boolean for mGamepadRegistered
and allow unregistering in onHide(), onDetachedFromWindow() and 
destroy().

Granted, I think if you only register in onAttachedToWindow(), you should
remove the registration in the constructor and only do it in these methods,
which I think is cleaner anyway.

[boliu, 2014/03/10 18:27:04]

What Ted said.

Webview is a view, so Activity lifetime events does not apply (webview can be
used in an android service where there is no activity). onHide is never called
in webview either because there is no such thing as a hidden webview in chrome's
sense.

The only pair of calls webview can guarantee to match is onAttachedToWindow and
onDetachedFromWindow. It's not perfect either since webview can be used without
ever being attached, but there is no better alternative to support refcounting a
global resource. But I guess gamepad support makes no sense for an unattached
webview anyway, so might be ok.

[kbalazs, 2014/03/12 00:17:36]

javadoc added.

[kbalazs, 2014/03/12 00:17:36]

Relying on onAttachedToWindow and onDetachedFromWindow now.

+        if (mIsPaused)

[Ted C, 2014/03/07 20:18:09]

in java, braces are always required if the statement and conditional don't all
fit on the same line.

For this (and many places), you can just do:

if (mIsPaused) return;

[kbalazs, 2014/03/12 00:17:36]

Done.

+            return;
+        mIsPaused = true;
+        synchronized (mDeviceHandlersLock) {
+            for (int i = 0; i < NUM_WEB_GAMEPADS; ++i) {

[Ted C, 2014/03/07 20:18:09]

in java land, we use i++ (applies many places)

[kbalazs, 2014/03/12 00:17:36]

Done.

+                mDeviceHandlers[i] = null;
+            }
+        }
+        mInputManager.unregisterInputDeviceListener(this);
+    }
+
+    public void resume() {
+        if (!mIsPaused)
+            return;
+        mIsPaused = false;
+        enumerateDevices();
+        mInputManager.registerInputDeviceListener(this, null);
+    }
+
+    private void enumerateDevices() {
+        InputDeviceHandler[] handlers = new InputDeviceHandler[NUM_WEB_GAMEPADS];

[Ted C, 2014/03/07 20:18:09]

I would add the assert runningOnUiThread to here to make sure all mutators of
the mDeviceHandlers happens on the UI thread.

[kbalazs, 2014/03/12 00:17:36]

Done.

+        int[] ids = mInputManager.getInputDeviceIds();

[Ted C, 2014/03/07 20:18:09]

the api doesn't clarify if null is a valid return for this.  might want to check
null just to be cautious

[kbalazs, 2014/03/12 00:17:36]

Done.

+        int activeDevices = 0;
+        for (int i = 0; i < ids.length && activeDevices < NUM_WEB_GAMEPADS; ++i) {
+            InputDevice device = mInputManager.getInputDevice(ids[i]);
+            if (isGameDevice(device)) {
+                handlers[activeDevices++] = new InputDeviceHandler(device);
+            }
+        }
+        synchronized (mDeviceHandlersLock) {
+            mDeviceHandlers = handlers;
+        }
+    }
+
+    @Override
+    public void onInputDeviceAdded(int deviceID) {

[Ted C, 2014/03/07 20:18:09]

I think you're supposed to use a lower d in ID.

https://source.android.com/source/code-style.html#treat-acronyms-as-words

So, "deviceId" and elsewhere

[kbalazs, 2014/03/12 00:17:36]

Done.

+        ThreadUtils.assertOnUiThread();
+        InputDevice device = mInputManager.getInputDevice(deviceID);
+        if (!isGameDevice(device))
+            return;
+        int index = nextAvailableIndex();
+        if (index == -1)
+            return;
+        synchronized (mDeviceHandlersLock) {
+            mDeviceHandlers[index] = new InputDeviceHandler(device);
+        }
+    }
+
+    @Override
+    public void onInputDeviceRemoved(int deviceID) {
+        ThreadUtils.assertOnUiThread();
+        int index = indexForDeviceID(deviceID);
+        if (index == -1)
+            return;
+        synchronized (mDeviceHandlersLock) {
+            mDeviceHandlers[index] = null;
+        }
+    }
+
+    @Override
+    public void onInputDeviceChanged(int deviceID) {
+        ThreadUtils.assertOnUiThread();
+        int index = indexForDeviceID(deviceID);
+        if (index == -1) {
+            index = nextAvailableIndex();
+            if (index == -1) return;
+        }
+        InputDevice device = mInputManager.getInputDevice(deviceID);
+        synchronized (mDeviceHandlersLock) {
+            mDeviceHandlers[index] = null;
+            if (isGameDevice(device)) {
+                mDeviceHandlers[index] = new InputDeviceHandler(device);
+            }
+        }
+    }
+
+    public boolean handleMotionEvent(MotionEvent event) {
+        if (!mIsFetched)
+            return false;
+        InputDeviceHandler handler = handlerForDeviceId(event.getDeviceId());
+        if (handler == null)
+            return false;
+        if (!isGameEvent(event))
+            return false;
+        handler.handleMotionEvent(event);
+        return true;
+    }
+
+    public boolean handleKeyEvent(KeyEvent event) {
+        if (!mIsFetched)
+            return false;
+        if (event.getAction() != KeyEvent.ACTION_DOWN &&
+            event.getAction() != KeyEvent.ACTION_UP) {

[Ted C, 2014/03/07 20:18:09]

+4 indent

[kbalazs, 2014/03/07 22:40:22]

This is apparently 4, could you elaborate on how this should be written?

[Ted C, 2014/03/08 01:06:00]

+4 indent was meant to say it needs to be indented 4 more.

Second lines of conditionals require two levels of indent (4 spaces per level):

if (event.getAction() != KeyEvent.ACTION_DOWN
        && event.getAction() != KeyEvent.ACTION_UP) {
...

[kbalazs, 2014/03/11 20:38:34]

I see. For the record I don't find this rule neither in the Android style guide
nor at http://www.chromium.org/developers/coding-style/java so probably this
should be written somewhere.

[Ted C, 2014/03/11 20:42:47]

It's in the referenced android style guide:
http://source.android.com/source/code-style.html#use-spaces-for-indentation

"""
We use 8 space indents for line wraps, including function calls and assignments.
"""

[kbalazs, 2014/03/12 00:17:36]

Ah, ok, I missed it. Done.

+            return false;
+        }
+        InputDeviceHandler handler = handlerForDeviceId(event.getDeviceId());
+        if (handler == null)
+            return false;
+        int keyCode = event.getKeyCode();
+        if (!isGameKey(keyCode))
+            return false;
+        boolean isDown = event.getAction() == KeyEvent.ACTION_DOWN;
+        handler.handleKeyEvent(keyCode, isDown, event.getEventTime());
+        return true;
+    }
+
+    @CalledByNative
+    public static GamepadAdapter attach(long nativeDataFetcher) {

[Ted C, 2014/03/07 20:18:09]

why public?

[kbalazs, 2014/03/07 22:40:22]

If I remember correctly the jni generator only allows it to be called by native
if it's public. I can double check.

[Ted C, 2014/03/08 01:06:00]

Ah, yeah the jni generator definitely allows you to call private methods.

[kbalazs, 2014/03/12 00:17:36]

Now I remember, the presubmit complained about private + @CalledByNative. Now I
used package private.

+        assert instance != null;
+        instance.mNativeDataFetcher = nativeDataFetcher;
+        instance.mIsFetched = true;
+        return instance;

[jdduke (slow), 2014/03/07 23:33:27]

Could we rename mIsFetched to something like |mHasListener| or |mDataRequested|.

[kbalazs, 2014/03/12 00:17:36]

Done.

+    }
+
+    // Called on polling thread.
+    @CalledByNative
+    private void getGamepadData() {
+        synchronized (mDeviceHandlersLock) {
+            for (int i = 0; i < NUM_WEB_GAMEPADS; ++i) {
+                if (mDeviceHandlers[i] == null) {
+                    nativeRefreshDevice(mNativeDataFetcher, i, false, null, null, 0, null, null);
+                } else {
+                    InputDeviceHandler handler = mDeviceHandlers[i];
+                    WebGamepadData data = handler.produceWebData();
+                    nativeRefreshDevice(mNativeDataFetcher, i, true,
+                            data.id, data.mapping, handler.getTimestamp(),
+                            data.axes, data.buttons);
+                }
+            }
+        }
+    }
+
+    @CalledByNative
+    public void setFetched(boolean fetched) {
+        mIsFetched = fetched;
+    }
+
+    int nextAvailableIndex() {
+        for (int i = 0; i < mDeviceHandlers.length; ++i) {
+            if (mDeviceHandlers[i] == null)
+                return i;
+        }
+        return -1;
+    }
+
+    int indexForDeviceID(int deviceID) {

[Ted C, 2014/03/07 20:18:09]

indexForDeviceId

+        for (int i = 0; i < mDeviceHandlers.length; ++i) {
+            if (mDeviceHandlers[i] != null &&
+                mDeviceHandlers[i].getInputDevice().getId() == deviceID)

[Ted C, 2014/03/07 20:18:09]

+4 indent and needs braces

+                return i;
+        }
+        return -1;
+    }
+
+    InputDeviceHandler handlerForDeviceId(int deviceID) {
+        int index = indexForDeviceID(deviceID);
+        return index == -1 ? null : mDeviceHandlers[index];
+    }
+
+    private static boolean isGameDevice(InputDevice device) {
+        return (device.getSources() & InputDevice.SOURCE_JOYSTICK) != 0;
+    }
+
+    private static boolean isGameEvent(MotionEvent event) {
+        return (event.getSource() & InputDevice.SOURCE_JOYSTICK) != 0 &&
+                event.getAction() == MotionEvent.ACTION_MOVE;
+    }
+
+    private static boolean isGameKey(int keyCode) {
+        switch (keyCode) {
+            case KeyEvent.KEYCODE_DPAD_UP:
+            case KeyEvent.KEYCODE_DPAD_DOWN:
+            case KeyEvent.KEYCODE_DPAD_LEFT:
+            case KeyEvent.KEYCODE_DPAD_RIGHT:
+            case KeyEvent.KEYCODE_DPAD_CENTER:
+                return true;
+            default:
+                return KeyEvent.isGamepadButton(keyCode);
+        }
+    }
+
+    private static class InputDeviceHandler {
+        private final InputDevice mDevice;
+        private final SparseArray<Float> mAxes;
+        private final SparseArray<Boolean> mButtons = new SparseArray<Boolean>();

[Ted C, 2014/03/07 20:18:09]

Should be able to use SparseBooleanArray here.

I would also probably call this mButtonPressedStates or something like that as
mButtons sounds like a collection of buttons.

And along those lines, maybe mAxisValues above.

[kbalazs, 2014/03/12 00:17:36]

I still wanted something like an associative array that can be indexed with the
keycode and axis id constants. But I realized that all the axis id's and the key
codes are small constants, less than 256. So now I just use two 256 size
primitive arrays.

+        private final GamepadDataMapper mMapper;
+        private long mTimestamp;
+        private final Object mLock = new Object();
+
+        InputDevice getInputDevice() { return mDevice; }
+        long getTimestamp() { return mTimestamp; }
+
+        public InputDeviceHandler(InputDevice device) {
+            assert isGameDevice(device);
+            mDevice = device;
+            mMapper = GamepadDataMapper.createDataMapper(device.getName());
+            List<MotionRange> ranges = device.getMotionRanges();
+            mAxes = new SparseArray<Float>(ranges.size());
+            for (MotionRange range : ranges) {
+                if ((range.getSource() & InputDevice.SOURCE_CLASS_JOYSTICK) != 0) {
+                    mAxes.put(range.getAxis(), new Float(0));
+                }
+            }
+        }
+
+        // Called on UI thread.
+        void handleMotionEvent(MotionEvent event) {
+            synchronized (mLock) {
+                mTimestamp = event.getEventTime();
+                for (int i = 0; i < mAxes.size(); ++i) {
+                    final float value = event.getAxisValue(mAxes.keyAt(i));
+                    mAxes.setValueAt(i, value);

[Ted C, 2014/03/07 20:18:09]

Hmm...this will result in a new capital Float object for each motion event. 
That can result in more GC churn than you want for something that happens
potentially for every frame.

Although it's more manual, I think you'll want to use just a float[] to avoid
these objects.

[kbalazs, 2014/03/07 22:40:22]

IMHO an associative array is quite convenient here. I want to be able to
represent the data in a device agnostic way. I'm not saying that this
representation is the best but that's what I found the best so far. This will
create a few object (<10) per frame, do you think it's really a notable cost?
Anyways, I can think about how to avoid that.

[jdduke (slow), 2014/03/07 23:33:27]

Any amount of garbage created per frame is undesirable, unfortunately.  It's not
the memory cost, but the garbage collection trigger that hurts.

+                }
+            }
+        }
+
+        // Called on UI thread.
+        void handleKeyEvent(int keyCode, boolean isDown, long timestamp) {
+            synchronized (mLock) {
+                mTimestamp = timestamp;
+                mButtons.put(keyCode, isDown);
+            }
+        }
+
+        // Called on polling thread.
+        WebGamepadData produceWebData() {
+            synchronized (mLock) {
+                return mMapper.map(mAxes, mButtons);

[Ted C, 2014/03/07 20:18:09]

Is this also called every often?  It looks like there are a bunch of object
creations here as well.  You probably want to look at getting the number of
allocations to 0 if at all possible.

+            }
+        }
+    }
+
+    private native void nativeRefreshDevice(long nativeGamepadPlatformDataFetcherAndroid,
+            int index, boolean connected, String id, String mapping, long timestamp,
+            float[] axes, float[] buttons);
+}