Threading in VrShell is a bit of a mess right now, with various object, including the GVR API being used from multiple threads. This is potentially leading to bugs like issue 667327 , and should be resolved sooner, rather than later.
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0964056106e231da2363132eeb3d0d2358c06e7c commit 0964056106e231da2363132eeb3d0d2358c06e7c Author: mthiesse <mthiesse@chromium.org> Date: Tue Dec 06 02:42:29 2016 Suffix VrShell calls with which thread they're expected to be run on. This change is only intended to improve readability and spot threading issues. In the future we may replace the suffixes with CHECKs once the issues are fixed. BUG= 671302 Review-Url: https://codereview.chromium.org/2551983002 Cr-Commit-Position: refs/heads/master@{#436493} [modify] https://crrev.com/0964056106e231da2363132eeb3d0d2358c06e7c/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java [modify] https://crrev.com/0964056106e231da2363132eeb3d0d2358c06e7c/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java [modify] https://crrev.com/0964056106e231da2363132eeb3d0d2358c06e7c/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java [modify] https://crrev.com/0964056106e231da2363132eeb3d0d2358c06e7c/chrome/browser/android/vr_shell/vr_shell.cc [modify] https://crrev.com/0964056106e231da2363132eeb3d0d2358c06e7c/chrome/browser/android/vr_shell/vr_shell.h [modify] https://crrev.com/0964056106e231da2363132eeb3d0d2358c06e7c/chrome/browser/android/vr_shell/vr_web_contents_observer.cc [modify] https://crrev.com/0964056106e231da2363132eeb3d0d2358c06e7c/chrome/browser/ui/webui/vr_shell/vr_shell_ui_message_handler.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0be372f1b41a7e7ad897743abd6b265da979a82e commit 0be372f1b41a7e7ad897743abd6b265da979a82e Author: mthiesse <mthiesse@chromium.org> Date: Tue Dec 13 19:35:13 2016 Implement our own GLThread for VR Shell. aka most satisfying refactor of the week. This CL gets rid of our glSurfaceView managed GL thread, and replaces it with a thread we control and can post tasks to. It also refactors all of our code that runs on the GL thread into its own class, making our existing thread safety violations painfully obvious. This breaks the unshipped menu mode - to be fixed in a followup CL, and disables some webVR buffer size manipulation, which might possibly regress some webVR experiences. This will also be fixed in a followup CL. BUG= 671302 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel # Skipping presubmit due to ScopedAllowIO use. NOPRESUBMIT=true Review-Url: https://codereview.chromium.org/2562733002 Cr-Commit-Position: refs/heads/master@{#438256} [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/chrome/browser/android/vr_shell/BUILD.gn [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/chrome/browser/android/vr_shell/vr_compositor.cc [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/chrome/browser/android/vr_shell/vr_compositor.h [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/chrome/browser/android/vr_shell/vr_shell.cc [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/chrome/browser/android/vr_shell/vr_shell.h [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/chrome/browser/android/vr_shell/vr_shell_delegate.cc [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/chrome/browser/android/vr_shell/vr_shell_delegate.h [add] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/chrome/browser/android/vr_shell/vr_shell_gl.cc [add] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/chrome/browser/android/vr_shell/vr_shell_gl.h [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/chrome/browser/android/vr_shell/vr_web_contents_observer.cc [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/chrome/browser/android/vr_shell/vr_web_contents_observer.h [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/chrome/browser/ui/webui/vr_shell/vr_shell_ui_message_handler.cc [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/content/browser/media/android/browser_surface_view_manager.cc [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/content/browser/renderer_host/compositor_impl_android.cc [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/gpu/ipc/common/gpu_surface_tracker.cc [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/gpu/ipc/common/gpu_surface_tracker.h [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/ui/android/java/src/org/chromium/ui/gl/SurfaceTexturePlatformWrapper.java [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/ui/gl/android/surface_texture.cc [modify] https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e/ui/gl/android/surface_texture.h
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/64ebaec4a09a6d214d566ce5fc59d61e55540253 commit 64ebaec4a09a6d214d566ce5fc59d61e55540253 Author: mthiesse <mthiesse@chromium.org> Date: Wed Dec 14 18:39:38 2016 Remove unnecessary threading concerns from VrMetricsHelper. (Includes some unrelated minor cleanup) BUG= 671302 Review-Url: https://codereview.chromium.org/2572013003 Cr-Commit-Position: refs/heads/master@{#438546} [modify] https://crrev.com/64ebaec4a09a6d214d566ce5fc59d61e55540253/chrome/browser/android/vr_shell/vr_shell.cc [modify] https://crrev.com/64ebaec4a09a6d214d566ce5fc59d61e55540253/chrome/browser/android/vr_shell/vr_shell.h [modify] https://crrev.com/64ebaec4a09a6d214d566ce5fc59d61e55540253/chrome/browser/android/vr_shell/vr_usage_monitor.cc [modify] https://crrev.com/64ebaec4a09a6d214d566ce5fc59d61e55540253/chrome/browser/android/vr_shell/vr_usage_monitor.h
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/25adbad06941a56f6ffe5a45ab8b50e9194ea487 commit 25adbad06941a56f6ffe5a45ab8b50e9194ea487 Author: mthiesse <mthiesse@chromium.org> Date: Wed Dec 14 19:26:44 2016 Fix some thread safety violations in device::GvrDelegate implementation. Also fixes some missed nits in https://codereview.chromium.org/2562733002/ BUG= 671302 Review-Url: https://codereview.chromium.org/2571713006 Cr-Commit-Position: refs/heads/master@{#438587} [modify] https://crrev.com/25adbad06941a56f6ffe5a45ab8b50e9194ea487/chrome/browser/android/vr_shell/vr_compositor.cc [modify] https://crrev.com/25adbad06941a56f6ffe5a45ab8b50e9194ea487/chrome/browser/android/vr_shell/vr_compositor.h [modify] https://crrev.com/25adbad06941a56f6ffe5a45ab8b50e9194ea487/chrome/browser/android/vr_shell/vr_shell.cc [modify] https://crrev.com/25adbad06941a56f6ffe5a45ab8b50e9194ea487/chrome/browser/android/vr_shell/vr_shell.h [modify] https://crrev.com/25adbad06941a56f6ffe5a45ab8b50e9194ea487/chrome/browser/android/vr_shell/vr_shell_gl.cc [modify] https://crrev.com/25adbad06941a56f6ffe5a45ab8b50e9194ea487/chrome/browser/android/vr_shell/vr_shell_gl.h
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ae6174ec3aa5ef0db0b68a40bb087e5ab8243b81 commit ae6174ec3aa5ef0db0b68a40bb087e5ab8243b81 Author: mthiesse <mthiesse@chromium.org> Date: Thu Dec 15 16:17:51 2016 Clean up threading around VR Scene updates. BUG= 671302 Review-Url: https://codereview.chromium.org/2574313002 Cr-Commit-Position: refs/heads/master@{#438843} [modify] https://crrev.com/ae6174ec3aa5ef0db0b68a40bb087e5ab8243b81/chrome/browser/android/vr_shell/ui_scene.cc [modify] https://crrev.com/ae6174ec3aa5ef0db0b68a40bb087e5ab8243b81/chrome/browser/android/vr_shell/ui_scene.h [modify] https://crrev.com/ae6174ec3aa5ef0db0b68a40bb087e5ab8243b81/chrome/browser/android/vr_shell/vr_shell.cc [modify] https://crrev.com/ae6174ec3aa5ef0db0b68a40bb087e5ab8243b81/chrome/browser/android/vr_shell/vr_shell.h [modify] https://crrev.com/ae6174ec3aa5ef0db0b68a40bb087e5ab8243b81/chrome/browser/android/vr_shell/vr_shell_gl.cc [modify] https://crrev.com/ae6174ec3aa5ef0db0b68a40bb087e5ab8243b81/chrome/browser/android/vr_shell/vr_shell_gl.h [modify] https://crrev.com/ae6174ec3aa5ef0db0b68a40bb087e5ab8243b81/chrome/browser/ui/webui/vr_shell/vr_shell_ui_message_handler.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2bcae0c61cb693232a4cf83dd4dcd175463e9bd4 commit 2bcae0c61cb693232a4cf83dd4dcd175463e9bd4 Author: mthiesse <mthiesse@chromium.org> Date: Thu Dec 15 17:50:36 2016 Clean up some VrShell threading issues and remove unnecessary WeakPtr types. I originally added the WeakPtrs all over the place so that we could post to the device provider from the GL thread. This CL moves the calls to the UI thread, and removes the WeakPtrs with it. BUG= 671302 Review-Url: https://codereview.chromium.org/2570553004 Cr-Commit-Position: refs/heads/master@{#438870} [modify] https://crrev.com/2bcae0c61cb693232a4cf83dd4dcd175463e9bd4/chrome/browser/android/vr_shell/vr_shell.cc [modify] https://crrev.com/2bcae0c61cb693232a4cf83dd4dcd175463e9bd4/chrome/browser/android/vr_shell/vr_shell.h [modify] https://crrev.com/2bcae0c61cb693232a4cf83dd4dcd175463e9bd4/chrome/browser/android/vr_shell/vr_shell_delegate.cc [modify] https://crrev.com/2bcae0c61cb693232a4cf83dd4dcd175463e9bd4/chrome/browser/android/vr_shell/vr_shell_delegate.h [modify] https://crrev.com/2bcae0c61cb693232a4cf83dd4dcd175463e9bd4/chrome/browser/android/vr_shell/vr_shell_gl.cc [modify] https://crrev.com/2bcae0c61cb693232a4cf83dd4dcd175463e9bd4/chrome/browser/android/vr_shell/vr_shell_gl.h [modify] https://crrev.com/2bcae0c61cb693232a4cf83dd4dcd175463e9bd4/device/vr/android/gvr/gvr_delegate.h [modify] https://crrev.com/2bcae0c61cb693232a4cf83dd4dcd175463e9bd4/device/vr/android/gvr/gvr_device.cc [modify] https://crrev.com/2bcae0c61cb693232a4cf83dd4dcd175463e9bd4/device/vr/android/gvr/gvr_device.h [modify] https://crrev.com/2bcae0c61cb693232a4cf83dd4dcd175463e9bd4/device/vr/android/gvr/gvr_device_provider.cc [modify] https://crrev.com/2bcae0c61cb693232a4cf83dd4dcd175463e9bd4/device/vr/android/gvr/gvr_device_provider.h [modify] https://crrev.com/2bcae0c61cb693232a4cf83dd4dcd175463e9bd4/device/vr/android/gvr/gvr_gamepad_data_fetcher.cc [modify] https://crrev.com/2bcae0c61cb693232a4cf83dd4dcd175463e9bd4/device/vr/android/gvr/gvr_gamepad_data_fetcher.h
I'm going to consider this fixed in general. issue 674594 covers the remaining issues around GVR API usage.
Comment 1 by ddorwin@chromium.org
, Dec 5 2016