Refactor GVR controller gamepad API integration

Refactor GVR controller gamepad API integration

The GVR device code had its own gvr_api instance and controller
state in the UI thread, this was causing conflicts with using
the same APIs from the GL thread in vr_shell_gl. Symptoms included
not getting window click events for touchpad clicks after a WebVR
application used the gamepad API to read a GVR controller, this
broke transient event handling such as button up/down detection.

Instead, poll controllers only once from within VrShellGl, and post
the data to VrShell which acts as a GvrGamepadDataProvider. That
also handles registering/unregistering the gamepad device factory
as needed.

This only works while presenting, but that restriction already existed,
the controller was not functional in magic window mode.

BUG=687992
Review-Url: https://codereview.chromium.org/2762003002
Cr-Commit-Position: refs/heads/master@{#459017}
Committed: https://chromium.googlesource.com/chromium/src/+/dee8e0a41990efbaec2435faff764baee5b894d9

[klausw, 2017-03-21 00:04:09]

Description was changed from

==========
Refactor GVR controller gamepad API integration

The GVR device code had its own gvr_api instance and controller
state in the UI thread, this was causing conflicts with using
the same APIs from the GL thread in vr_shell_gl. Symptoms included
not getting window click events for touchpad clicks after a WebVR
application used the gamepad API to read a GVR controller, this
broke transient event handling such as button up/down detection.

Instead, poll controllers only once from within VrShellGl, and post
the data to VrShell which acts as a GvrGamepadDataProvider. That
also handles registering/unregistering the gamepad device factory
as needed.

This only works while presenting, but that restriction already existed,
the controller was not functional in magic window mode.

BUG=687992
==========

to

==========
Refactor GVR controller gamepad API integration

The GVR device code had its own gvr_api instance and controller
state in the UI thread, this was causing conflicts with using
the same APIs from the GL thread in vr_shell_gl. Symptoms included
not getting window click events for touchpad clicks after a WebVR
application used the gamepad API to read a GVR controller, this
broke transient event handling such as button up/down detection.

Instead, poll controllers only once from within VrShellGl, and post
the data to VrShell which acts as a GvrGamepadDataProvider. That
also handles registering/unregistering the gamepad device factory
as needed.

This only works while presenting, but that restriction already existed,
the controller was not functional in magic window mode.

BUG=687992
==========

[klausw, 2017-03-21 00:04:09]

klausw@chromium.org changed reviewers:
+ mthiesse@chromium.org

[klausw, 2017-03-21 00:04:50]

klausw@chromium.org changed reviewers:
+ bajones@chromium.org

[mthiesse, 2017-03-21 14:56:24]

missing gvr_gamepad_data_provider.h?

[klausw, 2017-03-21 15:37:10]

On 2017/03/21 14:56:24, mthiesse wrote:
> missing gvr_gamepad_data_provider.h?

Added.

[cjgrant, 2017-03-21 15:55:37]

cjgrant@chromium.org changed reviewers:
+ cjgrant@chromium.org

[cjgrant, 2017-03-21 15:55:39]

Thanks Klaus.  I'll patch this onto Menu Mode and re-test.

https://codereview.chromium.org/2762003002/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2762003002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1212: // Get controller data now
so that it's ready both for WebVR's
s/ready/ready for/

https://codereview.chromium.org/2762003002/diff/60001/device/vr/android/gvr/g...
File device/vr/android/gvr/gvr_gamepad_data_provider.h (right):

https://codereview.chromium.org/2762003002/diff/60001/device/vr/android/gvr/g...
device/vr/android/gvr/gvr_gamepad_data_provider.h:8: #include
"third_party/gvr-android-sdk/src/libraries/headers/vr/gvr/capi/include/gvr.h"
Is it only gvr_types.h needed here?

[bajones, 2017-03-21 16:01:08]

LGTM with outstanding nits fixed.

[klausw, 2017-03-21 16:55:57]

https://codereview.chromium.org/2762003002/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2762003002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1212: // Get controller data now
so that it's ready both for WebVR's
On 2017/03/21 15:55:38, cjgrant wrote:
> s/ready/ready for/

Done.

https://codereview.chromium.org/2762003002/diff/60001/device/vr/android/gvr/g...
File device/vr/android/gvr/gvr_gamepad_data_provider.h (right):

https://codereview.chromium.org/2762003002/diff/60001/device/vr/android/gvr/g...
device/vr/android/gvr/gvr_gamepad_data_provider.h:8: #include
"third_party/gvr-android-sdk/src/libraries/headers/vr/gvr/capi/include/gvr.h"
On 2017/03/21 15:55:38, cjgrant wrote:
> Is it only gvr_types.h needed here?

Indeed, done.

[mthiesse, 2017-03-21 17:35:13]

lgtm

https://codereview.chromium.org/2762003002/diff/100001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_gamepad_data_provider.h (right):

https://codereview.chromium.org/2762003002/diff/100001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_gamepad_data_provider.h:34: virtual void
GamepadDataUpdate(GvrGamepadData data) = 0;
nit: Update/GetGamepadData? GamepadDataGet is strange phrasing...

Alternatively OnGamepadDataUpdate(GvrGamepadData data) = 0;

[klausw, 2017-03-21 18:28:56]

https://codereview.chromium.org/2762003002/diff/100001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_gamepad_data_provider.h (right):

https://codereview.chromium.org/2762003002/diff/100001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_gamepad_data_provider.h:34: virtual void
GamepadDataUpdate(GvrGamepadData data) = 0;
On 2017/03/21 17:35:13, mthiesse wrote:
> nit: Update/GetGamepadData? GamepadDataGet is strange phrasing...
> 
> Alternatively OnGamepadDataUpdate(GvrGamepadData data) = 0;

Renamed both to put the verb first. I don't like OnGamepadDataUpdate as much
since it's more of an active state change than an event handler, but let me know
if you'd strongly prefer that.

[klausw, 2017-03-21 18:29:09]

The CQ bit was checked by klausw@chromium.org

[klausw, 2017-03-21 18:29:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
bajones@chromium.org, mthiesse@chromium.org
Link to the patchset: https://codereview.chromium.org/2762003002/#ps120001
(title: "Rename GamepadData{Update,Get} to {Update,Get}GamepadData")

[commit-bot: I haz the power, 2017-03-21 18:30:04]

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, 2017-03-21 18:34:35]

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

[commit-bot: I haz the power, 2017-03-21 18:34:36]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[klausw, 2017-03-21 19:35:58]

The CQ bit was checked by klausw@chromium.org

[klausw, 2017-03-21 19:35:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2762003002/#ps140001
(title: "Rebase, fix merge conflict")

[commit-bot: I haz the power, 2017-03-21 19:36:29]

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, 2017-03-21 20:23:45]

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

[commit-bot: I haz the power, 2017-03-21 20:23:46]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[klausw, 2017-03-21 22:59:13]

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

[commit-bot: I haz the power, 2017-03-21 23:00:06]

Dry run: 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, 2017-03-21 23:08:49]

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

[commit-bot: I haz the power, 2017-03-21 23:08:50]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[klausw, 2017-03-22 00:18:52]

PTAL. I moved the gamepad data provider to vr_shell_delegate to fix a
pre-existing crash due to bad lifecycle:

[VERBOSE1:gvr_gamepad_data_fetcher.cc(35)] Factory
[VERBOSE1:gvr_gamepad_data_fetcher.cc(55)] GvrGamepadDataFetcher
[VERBOSE1:gvr_gamepad_data_fetcher.cc(39)] ~Factory
[VERBOSE1:gvr_gamepad_data_fetcher.cc(59)] ~GvrGamepadDataFetcher
[FATAL:GamepadDispatcher.cpp(45)] Check failed: connected == gamepad.connected.

Apparently the renderer side is still doing stuff after the data fetcher was
removed, triggering an assertion failure in debug builds.

The new lifecycle fixes the crash, but there's a new issue that it's sticking
around too long, the controller remains in magic window mode but is no longer
being updated.

Is there an appropriate lifecycle event that fires when the renderer is
definitely done? Or some other way to cleanly shut down the controller?

[mthiesse, 2017-03-22 14:57:30]

On 2017/03/22 00:18:52, klausw wrote:
> PTAL. I moved the gamepad data provider to vr_shell_delegate to fix a
> pre-existing crash due to bad lifecycle:
> 
> [VERBOSE1:gvr_gamepad_data_fetcher.cc(35)] Factory
> [VERBOSE1:gvr_gamepad_data_fetcher.cc(55)] GvrGamepadDataFetcher
> [VERBOSE1:gvr_gamepad_data_fetcher.cc(39)] ~Factory
> [VERBOSE1:gvr_gamepad_data_fetcher.cc(59)] ~GvrGamepadDataFetcher
> [FATAL:GamepadDispatcher.cpp(45)] Check failed: connected ==
gamepad.connected.
> 
> Apparently the renderer side is still doing stuff after the data fetcher was
> removed, triggering an assertion failure in debug builds.
> 
> The new lifecycle fixes the crash, but there's a new issue that it's sticking
> around too long, the controller remains in magic window mode but is no longer
> being updated.
> 
> Is there an appropriate lifecycle event that fires when the renderer is
> definitely done? Or some other way to cleanly shut down the controller?

I think you should move the gamepad data provider back into vr_shell, and only
pass a weakPtr to it into the GvrGamepadDataFetcher. If the weakPtr returns
null, you return a disconnected gamepad.

[klausw, 2017-03-22 22:21:07]

On 2017/03/22 14:57:30, mthiesse wrote:
> On 2017/03/22 00:18:52, klausw wrote:
> > PTAL. I moved the gamepad data provider to vr_shell_delegate to fix a
> > pre-existing crash due to bad lifecycle:
> > 
> > [VERBOSE1:gvr_gamepad_data_fetcher.cc(35)] Factory
> > [VERBOSE1:gvr_gamepad_data_fetcher.cc(55)] GvrGamepadDataFetcher
> > [VERBOSE1:gvr_gamepad_data_fetcher.cc(39)] ~Factory
> > [VERBOSE1:gvr_gamepad_data_fetcher.cc(59)] ~GvrGamepadDataFetcher
> > [FATAL:GamepadDispatcher.cpp(45)] Check failed: connected ==
> gamepad.connected.
> > 
> > Apparently the renderer side is still doing stuff after the data fetcher was
> > removed, triggering an assertion failure in debug builds.
> > 
> > The new lifecycle fixes the crash, but there's a new issue that it's
sticking
> > around too long, the controller remains in magic window mode but is no
longer
> > being updated.
> > 
> > Is there an appropriate lifecycle event that fires when the renderer is
> > definitely done? Or some other way to cleanly shut down the controller?
> 
> I think you should move the gamepad data provider back into vr_shell, and only
> pass a weakPtr to it into the GvrGamepadDataFetcher. If the weakPtr returns
> null, you return a disconnected gamepad.

bajones@: please review new changes in device/gamepad/gamepad_provider.cc as
suggested by mthiesse@ to prevent the assertion failure on exiting presentation.

Switched to a new approach as discussed in IM. Need to be careful since the
fetcher's GetGamepadData runs in a separate polling thread, while objects are
constructed/destructed on the UI thread.

Now, the sequence is:
VrShell creates factory
factory creates fetcher
fetcher registers itself with VrShell
VrShell uses the registered fetcher pointer to push poses to it

VrShell starts destructor
destructor removes factory
factory removes fetcher

[klausw, 2017-03-22 22:21:14]

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

[commit-bot: I haz the power, 2017-03-22 22:22:05]

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

[mthiesse, 2017-03-22 22:30:48]

lgtm, though those linker errors are going to give me bad dreams tonight.

https://codereview.chromium.org/2762003002/diff/200001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell.h (right):

https://codereview.chromium.org/2762003002/diff/200001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.h:183: void
RegisterGamepadDataFetcher(device::GvrGamepadDataFetcher*);
Add a comment that this DataFetcher* must outlive VrShell.

[klausw, 2017-03-22 22:53:25]

https://codereview.chromium.org/2762003002/diff/200001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell.h (right):

https://codereview.chromium.org/2762003002/diff/200001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.h:183: void
RegisterGamepadDataFetcher(device::GvrGamepadDataFetcher*);
On 2017/03/22 22:30:47, mthiesse wrote:
> Add a comment that this DataFetcher* must outlive VrShell.

Done, added some more detailed lifecycle comments to gvr_gamepad_data_provider.h
too.

Strictly speaking it doesn't have to outlive VrShell, just don't call
SetGamepadData on it after it's destroyed. The precise order how things get shut
down in VrShell's destructor doesn't matter as long as that's true.

[klausw, 2017-03-22 22:53:35]

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

[commit-bot: I haz the power, 2017-03-22 22:54:04]

Dry run: 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, 2017-03-23 00:19:07]

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

[commit-bot: I haz the power, 2017-03-23 00:19:08]

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...)

[bajones, 2017-03-23 01:37:55]

On 2017/03/22 22:53:25, klausw wrote:
>
https://codereview.chromium.org/2762003002/diff/200001/chrome/browser/android...
> File chrome/browser/android/vr_shell/vr_shell.h (right):
> 
>
https://codereview.chromium.org/2762003002/diff/200001/chrome/browser/android...
> chrome/browser/android/vr_shell/vr_shell.h:183: void
> RegisterGamepadDataFetcher(device::GvrGamepadDataFetcher*);
> On 2017/03/22 22:30:47, mthiesse wrote:
> > Add a comment that this DataFetcher* must outlive VrShell.
> 
> Done, added some more detailed lifecycle comments to
gvr_gamepad_data_provider.h
> too.
> 
> Strictly speaking it doesn't have to outlive VrShell, just don't call
> SetGamepadData on it after it's destroyed. The precise order how things get
shut
> down in VrShell's destructor doesn't matter as long as that's true.

LGTM again.

[klausw, 2017-03-23 03:03:25]

The CQ bit was checked by klausw@chromium.org

[klausw, 2017-03-23 03:03:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2762003002/#ps240001
(title: "Add missing "override"")

[commit-bot: I haz the power, 2017-03-23 03:03:46]

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, 2017-03-23 03:19:18]

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

[commit-bot: I haz the power, 2017-03-23 03:19:20]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[klausw, 2017-03-23 03:47:37]

The CQ bit was checked by klausw@chromium.org

[commit-bot: I haz the power, 2017-03-23 03:47:55]

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

[klausw, 2017-03-23 03:48:04]

The CQ bit was unchecked by klausw@chromium.org

[klausw, 2017-03-23 03:51:33]

The CQ bit was checked by klausw@chromium.org

[klausw, 2017-03-23 03:51:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2762003002/#ps260001
(title: "Rebase, no changes")

[commit-bot: I haz the power, 2017-03-23 03:51:49]

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, 2017-03-23 04:09:24]

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

[commit-bot: I haz the power, 2017-03-23 04:09:25]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[klausw, 2017-03-23 07:09:48]

The CQ bit was checked by klausw@chromium.org

[commit-bot: I haz the power, 2017-03-23 07:10:05]

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, 2017-03-23 07:45:16]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1490252988073600,
"parent_rev": "059d59531760005f8bfa5a03eab31fb9e3203ff5", "commit_rev":
"dee8e0a41990efbaec2435faff764baee5b894d9"}

[commit-bot: I haz the power, 2017-03-23 07:45:58]

Description was changed from

==========
Refactor GVR controller gamepad API integration

The GVR device code had its own gvr_api instance and controller
state in the UI thread, this was causing conflicts with using
the same APIs from the GL thread in vr_shell_gl. Symptoms included
not getting window click events for touchpad clicks after a WebVR
application used the gamepad API to read a GVR controller, this
broke transient event handling such as button up/down detection.

Instead, poll controllers only once from within VrShellGl, and post
the data to VrShell which acts as a GvrGamepadDataProvider. That
also handles registering/unregistering the gamepad device factory
as needed.

This only works while presenting, but that restriction already existed,
the controller was not functional in magic window mode.

BUG=687992
==========

to

==========
Refactor GVR controller gamepad API integration

The GVR device code had its own gvr_api instance and controller
state in the UI thread, this was causing conflicts with using
the same APIs from the GL thread in vr_shell_gl. Symptoms included
not getting window click events for touchpad clicks after a WebVR
application used the gamepad API to read a GVR controller, this
broke transient event handling such as button up/down detection.

Instead, poll controllers only once from within VrShellGl, and post
the data to VrShell which acts as a GvrGamepadDataProvider. That
also handles registering/unregistering the gamepad device factory
as needed.

This only works while presenting, but that restriction already existed,
the controller was not functional in magic window mode.

BUG=687992

Review-Url: https://codereview.chromium.org/2762003002
Cr-Commit-Position: refs/heads/master@{#459017}
Committed:
https://chromium.googlesource.com/chromium/src/+/dee8e0a41990efbaec2435faff76...
==========

[commit-bot: I haz the power, 2017-03-23 07:46:00]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/dee8e0a41990efbaec2435faff76...