New issue
Advanced search Search tips

Issue 655297 link

Starred by 1 user
Status: Fixed
Owner:
mthiesse@chromium.org
Closed: Mar 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug
Proj-VR
Proj-XR
Proj-XR-VR

Blocking:
issue 670166
issue 688611



Sign in to add a comment

Clean up GVR Delegate lifecycle

Project Member Reported by tedc...@chromium.org, Oct 12 2016 
GvrDelegateProvider::SetInstance has a DCHECK to ensure only one instance is set at a time.

Right now, it is not being cleared because there are no callers of:
VrShellDelegate::~VrShellDelegate() {
  GvrDelegateProvider::SetInstance(nullptr);
}

There are other cases to consider where multiple activities can be alive at any given point, so there is a chance of multiple VrShellDelegate[s].  Need to figure out that more clearly.

Comment 1 by mthiesse@chromium.org, Oct 13 2016 

Actually, multiple VrShellDelegates will exist in multi-window mode, because ChromeTabbedActivity2 is an instance of ChromeTabbedActivity.

Comment 2 by mthiesse@chromium.org, Oct 24 2016

Components: UI>Browser>VR
Labels: Proj-VR M-56

Comment 3 by mthiesse@chromium.org, Oct 24 2016

Labels: VR-Demo

Comment 4 by sko...@chromium.org, Dec 1 2016

Labels: -M-56

Comment 5 by ddorwin@chromium.org, Dec 2 2016

Labels: -Proj-VR M-57 Proj-VR-Daydream Type-Bug
Owner: mthiesse@chromium.org
mthiesse@, what do we need to do here? This sounds like something we should fix soon.

Comment 6 by mthiesse@chromium.org, Dec 2 2016 

Well, the native code assumes VrShell/Delegate is a singleton, and it currently isn't. We load a second one in multiwindow mode, and strange things happen.

The solution might be to have a single shared VrShellDelegate instance that interacts with whichever CTA instance is active, but I haven't put much thought into the problem yet.

As a workaround we currently prevent entering VR in multiwindow mode, though magic window mode can still do strange things.

Comment 7 by sko...@chromium.org, Dec 5 2016

Labels: -VR-Demo

Comment 8 by ddorwin@chromium.org, Dec 5 2016

Labels: VR-FF

Comment 9 by ddorwin@chromium.org, Dec 5 2016

Labels: -Pri-2 Pri-1
We should address this soon to avoid chasing various bugs that might be caused by lifecycle issues.

Comment 10 by mthiesse@chromium.org, Dec 8 2016

Owner: ----
Un-owning this as I'm not working on this now.

Comment 11 by sko...@chromium.org, Dec 8 2016

Status: Available (was: Assigned)

Comment 12 by ddorwin@chromium.org, Jan 7 2017

Labels: -M-57

Comment 13 by mthiesse@chromium.org, Jan 26 2017

Owner: mthiesse@chromium.org
Project Member

Comment 14 by bugdroid1@chromium.org, Feb 1 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4c6465c3b85e664ca700e1c04bebf637c7846243

commit 4c6465c3b85e664ca700e1c04bebf637c7846243
Author: mthiesse <mthiesse@chromium.org>
Date: Wed Feb 01 23:40:17 2017

Refactor GvrDelegate ownership into GvrDelegateProvider and fix more threading violations.

This CL does two main things: It refactors controlling when the GvrDelegate is created and destroyed into the GvrDelegateProvider, and it refactors out some instances of using the GVR api in a non-threadsafe way.

Some minor functional changes may be visible with this patch - namely that we no longer (possibly) send a DisplayConnected event for a display that isn't ready yet due to the gvr api not being available. This should be an improvement in all cases.

BUG= 674594 ,  655297 

Review-Url: https://codereview.chromium.org/2658643003
Cr-Commit-Position: refs/heads/master@{#447643}

[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/chrome/browser/android/vr_shell/BUILD.gn
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/chrome/browser/android/vr_shell/non_presenting_gvr_delegate.h
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/chrome/browser/android/vr_shell/vr_compositor.cc
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/chrome/browser/android/vr_shell/vr_compositor.h
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/chrome/browser/android/vr_shell/vr_gl_thread.h
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/chrome/browser/android/vr_shell/vr_shell.cc
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/chrome/browser/android/vr_shell/vr_shell.h
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/chrome/browser/android/vr_shell/vr_shell_delegate.cc
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/chrome/browser/android/vr_shell/vr_shell_delegate.h
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/chrome/browser/android/vr_shell/vr_shell_gl.cc
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/chrome/browser/android/vr_shell/vr_shell_gl.h
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/device/vr/android/gvr/gvr_delegate.h
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/device/vr/android/gvr/gvr_device.cc
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/device/vr/android/gvr/gvr_device.h
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/device/vr/android/gvr/gvr_device_provider.cc
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/device/vr/android/gvr/gvr_device_provider.h
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/device/vr/android/gvr/gvr_gamepad_data_fetcher.cc
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/device/vr/android/gvr/gvr_gamepad_data_fetcher.h
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/device/vr/test/fake_vr_device.cc
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/device/vr/test/fake_vr_device.h
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/device/vr/vr_device.cc
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/device/vr/vr_device.h
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/device/vr/vr_display_impl.cc
[modify] https://crrev.com/4c6465c3b85e664ca700e1c04bebf637c7846243/device/vr/vr_display_impl.h

Comment 15 by sko...@chromium.org, Feb 2 2017

Status: Assigned (was: Available)

Comment 16 by sko...@chromium.org, Feb 2 2017

Labels: -VR-FF VR-DF

Comment 17 by mthiesse@chromium.org, Feb 3 2017 

What I think we need to do here for the remainder of this is make VrShellDelegate a lazily-initialized singleton that can be dynamically 'attached' to whichever the focused Activity, not necessarily CTA, when webVR or VR Shell are active. (Some features like VR Shell would probably only be available when attached to CTA)

This would fix multiwindow issues, fix issues around webVR in CCT, and get rid of our (small) memory overhead in non-VR clank.

Comment 18 by ddorwin@chromium.org, Feb 4 2017 

Does this affect WebVR code paths too? If so, we need to make this M-58 and move back to VR-FF.

Comment 19 by mthiesse@chromium.org, Feb 6 2017 

What part of this makes you think we need to get this in for M-58?

Comment 20 by mthiesse@chromium.org, Feb 6 2017 

My previous comments were incorrect, this would not be sufficient to fix issues around webVR in CCT.

Comment 21 by mthiesse@chromium.org, Feb 6 2017

Blocking: 688611

Comment 22 by ddorwin@chromium.org, Feb 23 2017

Blocking: 670166
Labels: M-58

Comment 23 by ddorwin@chromium.org, Feb 24 2017

Summary: Clean up GVR Delegate lifecycle (was: Clean up GVR Delegate lifecyle)
Project Member

Comment 24 by bugdroid1@chromium.org, Mar 3 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/182cf17def973f5d7a981025afb65ebd6f022ca9

commit 182cf17def973f5d7a981025afb65ebd6f022ca9
Author: mthiesse <mthiesse@chromium.org>
Date: Fri Mar 03 02:31:26 2017

Implement lazy initialization for VrShellDelegate

This allows VrShellDelegate to be created when it's needed, removing memory overhead for VR support when VR is not used.

This CL also allows for VrShellDelegate to be destroyed once it's no longer needed as well, but that will be implemented in a follow up CL.

Also also, this paves the way for allowing VrShellDelegate to attach to arbitrary ChromeActivities in the future (though for now only CTA is supported)

Also also also, this fixes VrShellDelegate being accessible from CCT (and webview?) so that they now correctly return no displays available.

BUG= 655297 ,  689293 

Review-Url: https://codereview.chromium.org/2727873002
Cr-Commit-Position: refs/heads/master@{#454474}

[modify] https://crrev.com/182cf17def973f5d7a981025afb65ebd6f022ca9/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
[modify] https://crrev.com/182cf17def973f5d7a981025afb65ebd6f022ca9/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
[modify] https://crrev.com/182cf17def973f5d7a981025afb65ebd6f022ca9/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
[modify] https://crrev.com/182cf17def973f5d7a981025afb65ebd6f022ca9/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrClassesWrapper.java
[modify] https://crrev.com/182cf17def973f5d7a981025afb65ebd6f022ca9/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrClassesWrapperImpl.java
[modify] https://crrev.com/182cf17def973f5d7a981025afb65ebd6f022ca9/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDaydreamApiImpl.java
[modify] https://crrev.com/182cf17def973f5d7a981025afb65ebd6f022ca9/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
[modify] https://crrev.com/182cf17def973f5d7a981025afb65ebd6f022ca9/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
[modify] https://crrev.com/182cf17def973f5d7a981025afb65ebd6f022ca9/chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellTest.java
[modify] https://crrev.com/182cf17def973f5d7a981025afb65ebd6f022ca9/chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java
[modify] https://crrev.com/182cf17def973f5d7a981025afb65ebd6f022ca9/chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java
[modify] https://crrev.com/182cf17def973f5d7a981025afb65ebd6f022ca9/chrome/browser/android/vr_shell/vr_shell_delegate.cc
[modify] https://crrev.com/182cf17def973f5d7a981025afb65ebd6f022ca9/chrome/browser/android/vr_shell/vr_shell_delegate.h
[modify] https://crrev.com/182cf17def973f5d7a981025afb65ebd6f022ca9/chrome/browser/android/vr_shell/vr_shell_gl.h
[modify] https://crrev.com/182cf17def973f5d7a981025afb65ebd6f022ca9/chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java
[modify] https://crrev.com/182cf17def973f5d7a981025afb65ebd6f022ca9/device/vr/android/gvr/gvr_delegate.cc
[modify] https://crrev.com/182cf17def973f5d7a981025afb65ebd6f022ca9/device/vr/android/gvr/gvr_delegate.h
Project Member

Comment 25 by bugdroid1@chromium.org, Mar 3 2017

Labels: merge-merged-3029
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d

commit 7cc487a97758ef4bdb217f80bf3eee82c13c9f6d
Author: Michael Thiessen <mthiesse@chromium.org>
Date: Fri Mar 03 18:13:28 2017

Implement lazy initialization for VrShellDelegate

This allows VrShellDelegate to be created when it's needed, removing memory overhead for VR support when VR is not used.

This CL also allows for VrShellDelegate to be destroyed once it's no longer needed as well, but that will be implemented in a follow up CL.

Also also, this paves the way for allowing VrShellDelegate to attach to arbitrary ChromeActivities in the future (though for now only CTA is supported)

Also also also, this fixes VrShellDelegate being accessible from CCT (and webview?) so that they now correctly return no displays available.

BUG= 655297 ,  689293 

Review-Url: https://codereview.chromium.org/2727873002
Cr-Commit-Position: refs/heads/master@{#454474}
(cherry picked from commit 182cf17def973f5d7a981025afb65ebd6f022ca9)

Review-Url: https://codereview.chromium.org/2730903007 .
Cr-Commit-Position: refs/branch-heads/3029@{#3}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
[modify] https://crrev.com/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
[modify] https://crrev.com/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
[modify] https://crrev.com/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrClassesWrapper.java
[modify] https://crrev.com/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrClassesWrapperImpl.java
[modify] https://crrev.com/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDaydreamApiImpl.java
[modify] https://crrev.com/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
[modify] https://crrev.com/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
[modify] https://crrev.com/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d/chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellTest.java
[modify] https://crrev.com/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d/chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrUtils.java
[modify] https://crrev.com/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d/chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java
[modify] https://crrev.com/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d/chrome/browser/android/vr_shell/vr_shell_delegate.cc
[modify] https://crrev.com/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d/chrome/browser/android/vr_shell/vr_shell_delegate.h
[modify] https://crrev.com/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d/chrome/browser/android/vr_shell/vr_shell_gl.h
[modify] https://crrev.com/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d/chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java
[modify] https://crrev.com/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d/device/vr/android/gvr/gvr_delegate.cc
[modify] https://crrev.com/7cc487a97758ef4bdb217f80bf3eee82c13c9f6d/device/vr/android/gvr/gvr_delegate.h

Comment 26 by mthiesse@chromium.org, Mar 3 2017 

Remaining work: Delete the VrShellDelegate when it's no longer needed, and dynamically attach it to whichever CTA is active (in multi-window).

Comment 27 by mthiesse@chromium.org, Mar 6 2017

Status: Fixed (was: Assigned)
Closing this in favor of more fine-grained bugs.

Filed  issue 698755  and  issue 698754 .
►

Sign in to add a comment