Implement our own GLThread for VR Shell.

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

Committed: https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e
Cr-Commit-Position: refs/heads/master@{#438256}

[mthiesse, 2016-12-08 16:39:41]

Description was changed from

==========
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
==========

to

==========
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
==========

[mthiesse, 2016-12-08 16:41:35]

mthiesse@chromium.org changed reviewers:
+ bajones@chromium.org, bshe@chromium.org, cjgrant@chromium.org,
klausw@chromium.org

[mthiesse, 2016-12-08 16:41:36]

PTAL. Sorry for the size of this CL, it's not easy to split up.

Most of the deletions are double-counted by the diff tool, and most of the
changes are just moving functions from vr_shell to vr_shell_gl.

[cjgrant, 2016-12-08 17:02:49]

Nit-grade comments only from a first pass.

https://codereview.chromium.org/2562733002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.h (right):

https://codereview.chromium.org/2562733002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.h:70: };
Blank line?

https://codereview.chromium.org/2562733002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2562733002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:229: LOG(ERROR) << "No VSync
Provider.";
Typically no periods in logging sentences.

https://codereview.chromium.org/2562733002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:369: // TODO(mthiesse): Fix this
properly.
Should elaborate.

https://codereview.chromium.org/2562733002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:379: //    if
(html_interface_->GetMode() == UiInterface::Mode::WEB_VR) {
Remove this block?

https://codereview.chromium.org/2562733002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell_gl.h (right):

https://codereview.chromium.org/2562733002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.h:6: #define
CHROME_BROWSER_ANDROID_VR_SHELL_VR_SHELL_GL_H_
Should vr_gl_util's contents move into these new files, or do you prefer them
separate?

[mthiesse, 2016-12-08 17:07:00]

mthiesse@chromium.org changed reviewers:
+ brettw@chromium.org, piman@chromium.org

[mthiesse, 2016-12-08 17:07:00]

brettw@chromium.org: Please review content/ and ui/ changes.

+piman for a sanity check on our GL init/teardown in vr_shell_gl.cc

[piman, 2016-12-08 21:12:19]

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

https://codereview.chromium.org/2562733002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:212:
gl::init::InitializeGLOneOff());
This can fail. I don't think CHECK is appropriate.

https://codereview.chromium.org/2562733002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:214: CHECK(surface_.get()) <<
"gl::init::CreateOffscreenGLSurface failed";
This can fail too.

https://codereview.chromium.org/2562733002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:217: CHECK(context_.get()) <<
"gl::init::CreateGLContext failed";
This too.

https://codereview.chromium.org/2562733002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:220: <<
"gl::GLContext::MakeCurrent() failed";
And this.

[mthiesse, 2016-12-09 01:27:19]

Patchset #3 (id:40001) has been deleted

[mthiesse, 2016-12-09 01:28:43]

brettw, PTAL at the use of ScopedAllowIO in ~VrShell. I tried long and hard to
find better options, but this seems to be the only reasonable solution. Note
that we're not actually doing IO, just stopping a thread.

https://codereview.chromium.org/2562733002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.h (right):

https://codereview.chromium.org/2562733002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.h:70: };
On 2016/12/08 17:02:49, cjgrant wrote:
> Blank line?

Done.

https://codereview.chromium.org/2562733002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2562733002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:229: LOG(ERROR) << "No VSync
Provider.";
On 2016/12/08 17:02:49, cjgrant wrote:
> Typically no periods in logging sentences.

Done

https://codereview.chromium.org/2562733002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:369: // TODO(mthiesse): Fix this
properly.
On 2016/12/08 17:02:49, cjgrant wrote:
> Should elaborate.

Done.

https://codereview.chromium.org/2562733002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:379: //    if
(html_interface_->GetMode() == UiInterface::Mode::WEB_VR) {
On 2016/12/08 17:02:49, cjgrant wrote:
> Remove this block?

For clarity, I'd like to leave it here as a sign that it's broken, and when I
fix it you'll see this code removed and added where it should end up.

If you prefer I can remove it altogether.

https://codereview.chromium.org/2562733002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell_gl.h (right):

https://codereview.chromium.org/2562733002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.h:6: #define
CHROME_BROWSER_ANDROID_VR_SHELL_VR_SHELL_GL_H_
On 2016/12/08 17:02:49, cjgrant wrote:
> Should vr_gl_util's contents move into these new files, or do you prefer them
> separate?

No preference. Feel free to move address this in a followup CL.

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

https://codereview.chromium.org/2562733002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:212:
gl::init::InitializeGLOneOff());
On 2016/12/08 21:12:19, piman - On leave - No reviews wrote:
> This can fail. I don't think CHECK is appropriate.

Done.

https://codereview.chromium.org/2562733002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:214: CHECK(surface_.get()) <<
"gl::init::CreateOffscreenGLSurface failed";
On 2016/12/08 21:12:19, piman - On leave - No reviews wrote:
> This can fail too.

Done.

https://codereview.chromium.org/2562733002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:217: CHECK(context_.get()) <<
"gl::init::CreateGLContext failed";
On 2016/12/08 21:12:19, piman - On leave - No reviews wrote:
> This too.

Done.

https://codereview.chromium.org/2562733002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:220: <<
"gl::GLContext::MakeCurrent() failed";
On 2016/12/08 21:12:19, piman - On leave - No reviews wrote:
> And this.

Done.

[piman, 2016-12-09 01:32:13]

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

https://codereview.chromium.org/2562733002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:214: ForceExitVR();
You would need to return here, if initialization fails, it's not ok to continue.

Same on all of the other places below.

[mthiesse, 2016-12-09 15:47:59]

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

https://codereview.chromium.org/2562733002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:214: ForceExitVR();
On 2016/12/09 01:32:13, piman - On leave - No reviews wrote:
> You would need to return here, if initialization fails, it's not ok to
continue.
> 
> Same on all of the other places below.

whoops. Done.

[bshe, 2016-12-09 19:36:25]

https://codereview.chromium.org/2562733002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2562733002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:112:
mPresentationView = new SurfaceView(mActivity);
Do you need a SurfaceView or just  a View is enough? It sounds you just need it
to get UI controls into view's hierarchy?

https://codereview.chromium.org/2562733002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2562733002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:116: gl_thread_.reset(new
GLThread(this, weak_ptr_factory_.GetWeakPtr(),
why do you need a weak ptr of VrShell? It looks like gl_thread_ is owned by
VrShell?

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

https://codereview.chromium.org/2562733002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:192: draw_task_.Cancel();
Do you need to delete textures and shutdown GL here? I think it was handled by
GlSurfaceView perviously. But I am not sure if we need to manually delete the
generated textures in the new path.

https://codereview.chromium.org/2562733002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:925: gvr_api_->PauseTracking();
do you need pause DrawFrame too?

[mthiesse, 2016-12-09 20:23:18]

https://codereview.chromium.org/2562733002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2562733002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:112:
mPresentationView = new SurfaceView(mActivity);
On 2016/12/09 19:36:25, bshe wrote:
> Do you need a SurfaceView or just  a View is enough? It sounds you just need
it
> to get UI controls into view's hierarchy?

I had previously tried a FrameLayout and it didn't work, but there must have
been some other problem because it's working now.

Done.

https://codereview.chromium.org/2562733002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2562733002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:116: gl_thread_.reset(new
GLThread(this, weak_ptr_factory_.GetWeakPtr(),
On 2016/12/09 19:36:25, bshe wrote:
> why do you need a weak ptr of VrShell? It looks like gl_thread_ is owned by
> VrShell?

Assuming you're not asking why vrShellGl needs the WeakPtr, the reason I pass a
weak pointer to thread creation is because I want to remove the raw pointer to
VrShell from GLThread's constructor and make thread safety violations even
harder to mistakenly add. We still pass the raw pointer because we haven't
cleaned up all of the existing violations yet.

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

https://codereview.chromium.org/2562733002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:192: draw_task_.Cancel();
On 2016/12/09 19:36:25, bshe wrote:
> Do you need to delete textures and shutdown GL here? I think it was handled by
> GlSurfaceView perviously. But I am not sure if we need to manually delete the
> generated textures in the new path.

No, calling ShutdownGL here will lead to segfaults (even if we manually clear
references to all of our GL-related objects). I'm pretty sure GL automagically
gets cleaned up when the thread dies. media/gpu/rendering_helper.cc was my main
reference point when wiring this up, and they also don't ShutdownGL after
initializing it.

However, when we have a more stable build (ToT crashes too frequently for me to
get any good data), we should check for any leaks.

https://codereview.chromium.org/2562733002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:925: gvr_api_->PauseTracking();
On 2016/12/09 19:36:25, bshe wrote:
> do you need pause DrawFrame too?

Good question, I'm not sure. To be on the safe side, I'll stop the render loop
too.

[brettw, 2016-12-09 20:38:33]

It's hard for me to judge without understanding this service. is there a design
doc I can skim?

[mthiesse, 2016-12-12 16:01:31]

On 2016/12/09 20:38:33, brettw (ping after 24h) wrote:
> It's hard for me to judge without understanding this service. is there a
design
> doc I can skim?

This change specifically has no design doc, as it was an uncontroversial
refactoring, without any externally facing changes (with respect to the
service).
The original design regarding the glSurfaceView/java-manged-GL-thread didn't
have a design doc, as we were just following the sample apps for how to render
in an NDK daydream app (see the ndk-treasurehunt sample in the google source
tree if curious).

So I guess I'll try to summarize how this all works:
When the user enters VR (usually through webVR), we add a GvrLayout to the
Android view hierarchy, which essentially takes over the display and creates an
async-reprojected surface we can submit frames to. Prior to this change in order
to submit frames, we would create a GL thread by creating an Android
GLSurfaceView, and render/submit frames inside of its DrawFrame callback. In
this change, we use a base::Thread as our GL thread, so that we have better
control over it and can post tasks to it without having to call up to Java,
allowing for a bunch of refactoring. The GVRLayout is, and must always be, used
only on the UI thread, and the GvrApi used for rendering should only be used
from the GL thread as it's not thread safe. However, the GvrLayout owns the
GvrApi instance, and when it gets shut down it deletes the GvrApi instance with
it. Therefore, we need to block shutting down the GvrLayout on stopping our GL
thread from using the GvrApi instance. Prior to this patch we used the blocking
GLSurfaceView::onPause to stop the thread, and in this patch we use the blocking
base::Thread::stop to stop the thread. There shouldn't be any concerns regarding
jank here, because at the time we stop the GL thread, we're switching from 3D to
2D, adding/removing a bunch of Java View, and probably changing device
orientation.

The gpu/ipc and content/browser changes are only necessary because with this
change we're now passing a ScopedJavaGlobalRef, where previously only
ScopedJavaLocalRef was used in that codepath. I don't think that code did/should
care whether the JavaRef for the surface is local or global.

If I've failed to adequately explain everything here, feel free to follow up
here or on hangouts with questions.

[mthiesse, 2016-12-12 16:05:35]

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

[mthiesse, 2016-12-12 16:05:36]

dcheng: Please review gpu/ipc changes.

[brettw, 2016-12-12 19:12:40]

LGTM with comment change:

https://codereview.chromium.org/2562733002/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2562733002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:167: // Our options are:
Can you replace this "options" section with the information about object
ownership and lifetimes you put in the review comments?

[mthiesse, 2016-12-12 19:49:31]

https://codereview.chromium.org/2562733002/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2562733002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:167: // Our options are:
On 2016/12/12 19:12:40, brettw (ping after 24h) wrote:
> Can you replace this "options" section with the information about object
> ownership and lifetimes you put in the review comments?

Done.

[bshe, 2016-12-12 20:30:20]

lgtm with a few nits

https://codereview.chromium.org/2562733002/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2562733002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:187: if
(thread->GetVrShellGlUnsafe()) {
why do you use Unsafe here instead of post to GL thread? Perhaps add comment
about why?

https://codereview.chromium.org/2562733002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:435: void VrShell::ForceExitVR() {
nit: VR/Vr to be consistent

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

https://codereview.chromium.org/2562733002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:611: // nothing but a black
screen.
nit: Is there currently a bug because of the following lines been comment out?
If so, can you create a crbug and assign to klausw?

https://codereview.chromium.org/2562733002/diff/140001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2562733002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:169: // base::Thread::Stop asserts
that IO is allowed to prevent jank, but there
nit: perhaps move everything into dctor of GLThread since you mentioned Stop
here.

[dcheng, 2016-12-12 20:52:35]

https://codereview.chromium.org/2562733002/diff/140001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2562733002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:47: base::WeakPtr<VrInputManager>
ui_input_manager,
Nit: convention is to pass WeakPtr by const ref. That being said, if you want to
pass by value, that's OK, but we should std::move() as well then.

https://codereview.chromium.org/2562733002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:61: Stop();
Perhaps put a comment here, base::Unretained(this) looks far too interesting.

(Especially since ~Thread also calls Stop(), but we probably don't want to use
that, because GLThread's members would already be destroyed at some point.
Normally, I'd say that this seems like a good place to use DeleteSoon, since
that's essentially what StopRenderLoop is doing.)

https://codereview.chromium.org/2562733002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:64: VrShellGl* GetVrShellGlUnsafe()
{ return vr_shell_gl_.get(); }
Nit: add a comment what "Unsafe" means.

https://codereview.chromium.org/2562733002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:89: base::WeakPtr<VrShell>
weak_vr_shell_;
I think it's pretty confusing to have a class named VrShell and another one
named VrShellGl. Can we add some class-level comments to help differentiate
between the two?

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

https://codereview.chromium.org/2562733002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.h:66: enum InputTarget {
Nit: consider enum class in new code (the enumerators, as defined, aren't very
descriptive names)

[mthiesse, 2016-12-12 23:26:55]

https://codereview.chromium.org/2562733002/diff/140001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2562733002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:47: base::WeakPtr<VrInputManager>
ui_input_manager,
On 2016/12/12 20:52:34, dcheng wrote:
> Nit: convention is to pass WeakPtr by const ref. That being said, if you want
to
> pass by value, that's OK, but we should std::move() as well then.

Done.

https://codereview.chromium.org/2562733002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:61: Stop();
On 2016/12/12 20:52:34, dcheng wrote:
> Perhaps put a comment here, base::Unretained(this) looks far too interesting.
> 
> (Especially since ~Thread also calls Stop(), but we probably don't want to use
> that, because GLThread's members would already be destroyed at some point.
> Normally, I'd say that this seems like a good place to use DeleteSoon, since
> that's essentially what StopRenderLoop is doing.)

Oh, whoops that was a big oversight. I had originally written this using
StopSoon/DeleteSoon but ran into the issues mentioned in ~VrShell. When I
switched to using Stop and base::ScopedAllowIO I forgot to remove the
StopRenderLoop call.

So yes, I wish I could use stop/deletesoon but I would have to wait on stopping
the render loop anyways, so it seems best to just use Stop(). I've removed the
posted task and overridden Thread::CleanUp().

https://codereview.chromium.org/2562733002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:64: VrShellGl* GetVrShellGlUnsafe()
{ return vr_shell_gl_.get(); }
On 2016/12/12 20:52:34, dcheng wrote:
> Nit: add a comment what "Unsafe" means.

Done.

https://codereview.chromium.org/2562733002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:89: base::WeakPtr<VrShell>
weak_vr_shell_;
On 2016/12/12 20:52:34, dcheng wrote:
> I think it's pretty confusing to have a class named VrShell and another one
> named VrShellGl. Can we add some class-level comments to help differentiate
> between the two?

Done.

https://codereview.chromium.org/2562733002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:169: // base::Thread::Stop asserts
that IO is allowed to prevent jank, but there
On 2016/12/12 20:30:20, bshe wrote:
> nit: perhaps move everything into dctor of GLThread since you mentioned Stop
> here.

I clarified the comment a bit.

I think it should stay here, as this isn't specific to our derived GLThread.
Deleting any thread calls Stop() (derived classes are required to call Stop() in
the destructor). Also it's VrShell that decides the GLThread is owned and
cleaned up by the UI thread.

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

https://codereview.chromium.org/2562733002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.h:66: enum InputTarget {
On 2016/12/12 20:52:34, dcheng wrote:
> Nit: consider enum class in new code (the enumerators, as defined, aren't very
> descriptive names)

Done. (Fixed in vr_shell_gl.h, I missed deleting this from vr_shell.h)

[dcheng, 2016-12-13 00:59:11]

LGTM with the scoped_refptr nit addressed (again, sorry for the confusion. We
should make these rules consistent, which I'll send out an email about)

https://codereview.chromium.org/2562733002/diff/160001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2562733002/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:48: const
scoped_refptr<base::SingleThreadTaskRunner>&
Sorry for the confusion, but it's actually the explicit recommendation to pass
scoped_refpt by value and move =(

I think we should probably make a similar rule for WeakPtr for consistency
(that's why I explicitly only mentioned WeakPtr in my original comment)

[mthiesse, 2016-12-13 15:13:53]

https://codereview.chromium.org/2562733002/diff/160001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2562733002/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:48: const
scoped_refptr<base::SingleThreadTaskRunner>&
On 2016/12/13 00:59:11, dcheng wrote:
> Sorry for the confusion, but it's actually the explicit recommendation to pass
> scoped_refpt by value and move =(
> 
> I think we should probably make a similar rule for WeakPtr for consistency
> (that's why I explicitly only mentioned WeakPtr in my original comment)

Done.

[mthiesse, 2016-12-13 15:14:03]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2016-12-13 15:14:03]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
bshe@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2562733002/#ps180001
(title: "Address comments")

[commit-bot: I haz the power, 2016-12-13 15:14:19]

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-12-13 15:16:06]

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

[commit-bot: I haz the power, 2016-12-13 15:16:08]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mthiesse, 2016-12-13 15:22:58]

Description was changed from

==========
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
==========

to

==========
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
==========

[mthiesse, 2016-12-13 15:26:16]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2016-12-13 15:26:16]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
brettw@chromium.org, bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2562733002/#ps200001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-13 15:26:34]

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

[klausw, 2016-12-13 18:05:17]

LGTM

Thanks for making this change, I think this looks a whole lot
cleaner. I'm dreading merging this with my pending changes,
but it's well worth the temporary pain.

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

https://codereview.chromium.org/2562733002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:611: // nothing but a black
screen.
On 2016/12/12 20:30:20, bshe wrote:
> nit: Is there currently a bug because of the following lines been comment out?
> If so, can you create a crbug and assign to klausw?

I'm well aware of the issue :-)

I'll look into fixing it once this is merged, but we can live without
resizing for now. It'll work, just with some additional blur from
resampling and/or a small performance hit for the known-slow WebVR
mode.

I'm currently working on a change where WebVR manages its own
(resizeable) surface for WebGL content so that it doesn't need
to go through Chrome's compositor, and I think that would make this 
issue moot.

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

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

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

Failed to apply patch for
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:
While running git apply --index -p1;
  error: patch failed:
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:72
  error:
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:
patch does not apply

Patch:      
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
Index:
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
b/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
index
b8326cd43ebea2ab893bfd03172f28dda9872e31..a077c8a36650bdbc912a175c6b0cd29c7117f0d6
100644
---
a/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
+++
b/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
@@ -4,23 +4,11 @@
 
 package org.chromium.chrome.browser.vr_shell;
 
-import static android.opengl.GLES20.GL_NEAREST;
-import static android.opengl.GLES20.GL_TEXTURE_MAG_FILTER;
-import static android.opengl.GLES20.GL_TEXTURE_MIN_FILTER;
-import static android.opengl.GLES20.glBindTexture;
-import static android.opengl.GLES20.glGenTextures;
-import static android.opengl.GLES20.glTexParameteri;
-
 import android.annotation.SuppressLint;
 import android.app.Activity;
 import android.graphics.Point;
-import android.graphics.SurfaceTexture;
-import android.graphics.SurfaceTexture.OnFrameAvailableListener;
-import android.opengl.GLES11Ext;
-import android.opengl.GLSurfaceView;
 import android.os.StrictMode;
 import android.view.MotionEvent;
-import android.view.Surface;
 import android.view.View;
 import android.view.ViewGroup;
 import android.widget.FrameLayout;
@@ -46,14 +34,11 @@ import org.chromium.ui.base.WindowAndroid;
 import org.chromium.ui.display.DisplayAndroid;
 import org.chromium.ui.display.VirtualDisplayAndroid;
 
-import javax.microedition.khronos.egl.EGLConfig;
-import javax.microedition.khronos.opengles.GL10;
-
 /**
  * This view extends from GvrLayout which wraps a GLSurfaceView that renders VR
shell.
  */
 @JNINamespace("vr_shell")
-public class VrShellImpl extends GvrLayout implements GLSurfaceView.Renderer,
VrShell {
+public class VrShellImpl extends GvrLayout implements VrShell {
     private static final String TAG = "VrShellImpl";
 
     // TODO(mthiesse): These values work well for Pixel/Pixel XL in VR, but we
need to come up with
@@ -72,20 +57,15 @@ public class VrShellImpl extends GvrLayout implements
GLSurfaceView.Renderer, Vr
     private static final float DEFAULT_UI_WIDTH = 1920f;
     private static final float DEFAULT_UI_HEIGHT = 1080f;
 
-    private final GLSurfaceView mGlSurfaceView;
     private final Activity mActivity;
     private final VirtualDisplayAndroid mContentVirtualDisplay;
     private final VirtualDisplayAndroid mUiVirtualDisplay;
 
     private long mNativeVrShell = 0;
 
-    private int mContentTextureHandle;
-    private int mUiTextureHandle;
-    private FrameListener mContentFrameListener;
-    private FrameListener mUiFrameListener;
-
     private FrameLayout mContentCVCContainer;
     private FrameLayout mUiCVCContainer;
+    private FrameLayout mPresentationView;
 
     // The tab that holds the main ContentViewCore.
     private Tab mTab;
@@ -106,12 +86,6 @@ public class VrShellImpl extends GvrLayout implements
GLSurfaceView.Renderer, Vr
     private ContentViewCore mUiCVC;
     private VrWindowAndroid mUiVrWindowAndroid;
 
-    private boolean mSurfacesInitialized;
-    private int mContentSurfaceWidth = -1;
-    private int mContentSurfaceHeight = -1;
-    private int mUiSurfaceWidth = -1;
-    private int mUiSurfaceHeight = -1;
-
     public VrShellImpl(Activity activity) {
         super(activity);
         mActivity = activity;
@@ -130,12 +104,8 @@ public class VrShellImpl extends GvrLayout implements
GLSurfaceView.Renderer, Vr
         addView(mContentCVCContainer, 0, new FrameLayout.LayoutParams(0, 0));
         addView(mUiCVCContainer, 0, new FrameLayout.LayoutParams(0, 0));
 
-        mGlSurfaceView = new GLSurfaceView(getContext());
-        mGlSurfaceView.setEGLContextClientVersion(2);
-        mGlSurfaceView.setEGLConfigChooser(8, 8, 8, 8, 0, 0);
-        mGlSurfaceView.setPreserveEGLContextOnPause(true);
-        mGlSurfaceView.setRenderer(this);
-        setPresentationView(mGlSurfaceView);
+        mPresentationView = new FrameLayout(mActivity);
+        setPresentationView(mPresentationView);
 
         if (setAsyncReprojectionEnabled(true)) {
             AndroidCompat.setSustainedPerformanceMode(mActivity, true);
@@ -148,7 +118,7 @@ public class VrShellImpl extends GvrLayout implements
GLSurfaceView.Renderer, Vr
     }
 
     @Override
-    public void initializeNativeOnUI(Tab currentTab, VrShellDelegate delegate,
boolean forWebVR) {
+    public void initializeNative(Tab currentTab, VrShellDelegate delegate,
boolean forWebVR) {
         assert currentTab.getContentViewCore() != null;
         mTab = currentTab;
         mContentCVC = mTab.getContentViewCore();
@@ -161,35 +131,30 @@ public class VrShellImpl extends GvrLayout implements
GLSurfaceView.Renderer, Vr
        
mUiCVC.initialize(ViewAndroidDelegate.createBasicDelegate(uiContentView),
                 uiContentView, mUiContents, mUiVrWindowAndroid);
 
-        // Set the UI and content sizes before we initialize VR Shell and load
the UI.
-        setUiCssSizeOnUI(DEFAULT_UI_WIDTH, DEFAULT_UI_HEIGHT, DEFAULT_DPR);
-        float dpr;
+        mNativeVrShell = nativeInit(mContentCVC.getWebContents(),
+                mContentVrWindowAndroid.getNativePointer(), mUiContents,
+                mUiVrWindowAndroid.getNativePointer(), forWebVR, delegate,
+                getGvrApi().getNativeGvrContext());
+
+        // Set the UI and content sizes before we load the UI.
+        setUiCssSize(DEFAULT_UI_WIDTH, DEFAULT_UI_HEIGHT, DEFAULT_DPR);
         if (forWebVR) {
             DisplayAndroid primaryDisplay =
DisplayAndroid.getNonMultiDisplay(mActivity);
-            dpr = WEBVR_DPR;
-            setContentCssSizeOnUI(primaryDisplay.getPhysicalDisplayWidth(),
-                    primaryDisplay.getPhysicalDisplayHeight(), dpr);
+            setContentCssSize(primaryDisplay.getPhysicalDisplayWidth(),
+                    primaryDisplay.getPhysicalDisplayHeight(), WEBVR_DPR);
         } else {
-            dpr = DEFAULT_DPR;
-            setContentCssSizeOnUI(DEFAULT_CONTENT_WIDTH,
DEFAULT_CONTENT_HEIGHT, dpr);
+            setContentCssSize(DEFAULT_CONTENT_WIDTH, DEFAULT_CONTENT_HEIGHT,
DEFAULT_DPR);
         }
 
-        mNativeVrShell = nativeInitOnUI(mContentCVC.getWebContents(),
-                mContentVrWindowAndroid.getNativePointer(), mUiContents,
-                mUiVrWindowAndroid.getNativePointer(), forWebVR);
-
-        // Set the initial Content and UI bounds, as they won't have been
propagated yet.
-        nativeContentBoundsChangedOnUI(mNativeVrShell, mContentSurfaceWidth,
mContentSurfaceHeight,
-                dpr);
-        nativeUIBoundsChangedOnUI(mNativeVrShell, mUiSurfaceWidth,
mUiSurfaceHeight, DEFAULT_DPR);
+        nativeLoadUIContent(mNativeVrShell);
 
-        mGlSurfaceView.setOnTouchListener(new View.OnTouchListener() {
+        mPresentationView.setOnTouchListener(new View.OnTouchListener() {
             @Override
             @SuppressLint("ClickableViewAccessibility")
             public boolean onTouch(View v, MotionEvent event) {
                 if
(!CommandLine.getInstance().hasSwitch(ChromeSwitches.ENABLE_VR_SHELL_DEV)
                         && event.getActionMasked() == MotionEvent.ACTION_DOWN)
{
-                    nativeOnTriggerEventOnUI(mNativeVrShell);
+                    nativeOnTriggerEvent(mNativeVrShell);
                     return true;
                 }
                 return false;
@@ -205,14 +170,10 @@ public class VrShellImpl extends GvrLayout implements
GLSurfaceView.Renderer, Vr
         mUiCVC.setTopControlsHeight(0, false);
         mUiVrWindowAndroid.onVisibilityChanged(true);
 
-        nativeSetDelegateOnUI(mNativeVrShell, delegate);
-
-        reparentContentWindowOnUI();
-
-        nativeUpdateCompositorLayersOnUI(mNativeVrShell);
+        reparentContentWindow();
     }
 
-    private void reparentContentWindowOnUI() {
+    private void reparentContentWindow() {
         mOriginalWindowAndroid = mContentCVC.getWindowAndroid();
 
         mTab.updateWindowAndroid(mContentVrWindowAndroid);
@@ -227,7 +188,7 @@ public class VrShellImpl extends GvrLayout implements
GLSurfaceView.Renderer, Vr
                 FrameLayout.LayoutParams.MATCH_PARENT,
FrameLayout.LayoutParams.MATCH_PARENT));
     }
 
-    private void restoreContentWindowOnUI() {
+    private void restoreContentWindow() {
         mTab.updateWindowAndroid(mOriginalWindowAndroid);
 
         // If the tab's view has changed, the necessary view reparenting has
already been done.
@@ -239,143 +200,37 @@ public class VrShellImpl extends GvrLayout implements
GLSurfaceView.Renderer, Vr
         mTabParent = null;
     }
 
-    private final Runnable mUpdateContentBufferSize = new Runnable() {
-        @Override
-        public void run() {
-            if (mSurfacesInitialized) {
-                mContentFrameListener.mSurfaceTexture.setDefaultBufferSize(
-                        mContentSurfaceWidth, mContentSurfaceHeight);
-            }
-        }
-    };
-
-    private final Runnable mUpdateUiBufferSize = new Runnable() {
-        @Override
-        public void run() {
-            if (mSurfacesInitialized) {
-                mUiFrameListener.mSurfaceTexture.setDefaultBufferSize(
-                        mUiSurfaceWidth, mUiSurfaceHeight);
-            }
-        }
-    };
-
     @CalledByNative
-    public void setUiCssSizeOnUI(float width, float height, float dpr) {
+    public void setUiCssSize…
(message too large)

[mthiesse, 2016-12-13 18:16:08]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2016-12-13 18:16:09]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
klausw@chromium.org, brettw@chromium.org, bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2562733002/#ps220001
(title: "sigh")

[commit-bot: I haz the power, 2016-12-13 18:16:27]

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-12-13 19:35:20]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1481652968822260,
"parent_rev": "146871090e016b81eb10cd8408244985aee9a7f5", "commit_rev":
"6bf3ab621cedec4384212729194ff45bee626865"}

[commit-bot: I haz the power, 2016-12-13 19:35:46]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-12-13 19:35:48]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2016-12-13 19:37:45]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e
Cr-Commit-Position: refs/heads/master@{#438256}
==========

[commit-bot: I haz the power, 2016-12-13 19:37:47]

Patchset 11 (id:??) landed as
https://crrev.com/0be372f1b41a7e7ad897743abd6b265da979a82e
Cr-Commit-Position: refs/heads/master@{#438256}