Introduce ChromeVR to Chrome on Android (behind a build flag)

Introduce ChromeVR to Chrome on Android (behind a build flag)

This CL implements the following:
1. Adds a new activity called VrActivity. It extends from AsyncInitializationActivity
and can only be launched from Daydream app.
2. Adds a VrShell and its native part. VrShell has a GLSurfaceView as presentation
view for drawing left and right images and an inheriated(from GvrLayout) scanline
racing view(another GLSurfaceView).
3. Created a texture quad that is reserved for compositor to use to output content
frames. The quad is positioned at (0.0, 0.0, -1.0)

Known issue:
1. The quad is black as we haven't hookup a compositor to draw frames
2. tabs are not shared with ChromeTabbedActivity

To test this, add "enable_vr_shell=true" in your gn args.

BUG=638642
Committed: https://crrev.com/16e132a1b213a07254d25b50a42ba9e9304d3f75
Cr-Commit-Position: refs/heads/master@{#414890}

[bshe, 2016-08-17 17:57:07]

bshe@chromium.org changed reviewers:
+ sievers@chromium.org

[bshe, 2016-08-17 17:57:07]

Hi Daniel.

Do you mind to take a look at this CL? It is a stripped version of our prototype
with only bare minimal functions. It only support launching from daydream home
app right now. 

There are a lot of known issues and even more unknown issues with this patch. So
everything is currently hided under a build time flag to make the feature less
visible and don't pollute normal Chrome build. Hopefully this is easier to land
and iterate on too. :)

Also, you can safely ignore the changes in devices/vr/*. It is here to make my
patch compile. After https://codereview.chromium.org/2219203002/ landed, I
shouldn't need these changes.

Thanks!

[bshe, 2016-08-17 18:12:50]

Description was changed from

==========
Introduce ChromeVR to Chrome on Android (behind a build flag)

This CL implements the following:
1. Adds a new activity called ChromeVrActivity. It extends from
ChromeTabbedActivity
and can only be launched from Daydream app.
2. Adds a VrShell and its native part. VrShell has a GLSurfaceView as
presentation
view for drawing left and right images and an inheriated(from GvrLayout)
scanline
racing view(another GLSurfaceView).
3. Replaces the Surface that compositor uses with a new Surface which backed by
a SurfaceView. We can then access the output stream of compositor as a texture
in
our VR drawing context which exist in the above presentation view.
Currently, the output of compositor is displayed in a floating square quad in 3D
space.
The quad is positioned at (0.0, 0.0, -1.0)

Known issue:
1. Normal browser UI will appear shortly when launching ChromeVR.
2. If Chrome was opened in the background before launching ChromeVR, it will
open a new tab instead of showing the last used tab.

To test this, add "enable_vr_shell=true" in your gn args.

BUG=638642
==========

to

==========
Introduce ChromeVR to Chrome on Android (behind a build flag)

This CL implements the following:
1. Adds a new activity called ChromeVrActivity. It extends from
ChromeTabbedActivity
and can only be launched from Daydream app.
2. Adds a VrShell and its native part. VrShell has a GLSurfaceView as
presentation
view for drawing left and right images and an inheriated(from GvrLayout)
scanline
racing view(another GLSurfaceView).
3. Replaces the Surface that compositor uses with a new Surface which backed by
a SurfaceTexture. We can then access the output stream of compositor as a
texture in
our VR drawing context which exist in the above presentation view.
Currently, the output of compositor is displayed in a floating square quad in 3D
space.
The quad is positioned at (0.0, 0.0, -1.0)

Known issue:
1. Normal browser UI will appear shortly when launching ChromeVR.
2. If Chrome was opened in the background before launching ChromeVR, it will
open a new tab instead of showing the last used tab.

To test this, add "enable_vr_shell=true" in your gn args.

BUG=638642
==========

[mthiesse1, 2016-08-18 16:11:45]

mthiesse@google.com changed reviewers:
+ mthiesse@google.com

[mthiesse1, 2016-08-18 16:11:46]

device/vr/ changes look like the should probably go in a separate CL.

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

https://codereview.chromium.org/2252103002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:35: transfrom_to_world.m[0][0] = 1;
move this function to vr_util.h?

[bajones, 2016-08-18 16:42:17]

On 2016/08/18 16:11:46, mthiesse1 wrote:
> device/vr/ changes look like the should probably go in a separate CL.

bshe@ already mentioned that the device/vr/ changes are temporary and won't be
necessary once https://codereview.chromium.org/2219203002/ (permanently) lands.

[mthiesse1, 2016-08-18 16:57:50]

On 2016/08/18 16:42:17, bajones wrote:
> On 2016/08/18 16:11:46, mthiesse1 wrote:
> > device/vr/ changes look like the should probably go in a separate CL.
> 
> bshe@ already mentioned that the device/vr/ changes are temporary and won't be
> necessary once https://codereview.chromium.org/2219203002/ (permanently)
lands.

Ah, I see.

lgtm

[bshe, 2016-08-18 17:15:43]

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

https://codereview.chromium.org/2252103002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:35: transfrom_to_world.m[0][0] = 1;
On 2016/08/18 16:11:46, mthiesse1 wrote:
> move this function to vr_util.h?

I think we will need to replace these functions with functions provided by
gfx/skia library. And I plan to file a bug after I land this patch. So I would
prefer to keep it here for now unless you strongly disagree. :)

[mthiesse1, 2016-08-18 17:18:35]

On 2016/08/18 17:15:43, bshe wrote:
>
https://codereview.chromium.org/2252103002/diff/1/chrome/browser/android/vr_s...
> File chrome/browser/android/vr_shell/vr_shell.cc (right):
> 
>
https://codereview.chromium.org/2252103002/diff/1/chrome/browser/android/vr_s...
> chrome/browser/android/vr_shell/vr_shell.cc:35: transfrom_to_world.m[0][0] =
1;
> On 2016/08/18 16:11:46, mthiesse1 wrote:
> > move this function to vr_util.h?
> 
> I think we will need to replace these functions with functions provided by
> gfx/skia library. And I plan to file a bug after I land this patch. So I would
> prefer to keep it here for now unless you strongly disagree. :)

Fine by me

[bshe, 2016-08-18 21:17:22]

bshe@chromium.org changed reviewers:
+ piman@chromium.org

[bshe, 2016-08-18 21:17:22]

looks like Daniel is OOO.
Add piman@ for review.
Antoine, do you mind to take a look? We use a SurfaceTexture backed Surface to
replace the Surface that is used by compositor in normal Clank. Thanks!

[piman, 2016-08-18 21:47:36]

On 2016/08/18 21:17:22, bshe wrote:
> looks like Daniel is OOO.
> Add piman@ for review.
> Antoine, do you mind to take a look? We use a SurfaceTexture backed Surface to
> replace the Surface that is used by compositor in normal Clank. Thanks!

sorry - I don't think I'm a good reviewer for this, I don't know any of this
code, and my knowledge of SurfaceTexture is superficial at best...

[piman, 2016-08-18 21:51:14]

https://codereview.chromium.org/2252103002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java
(right):

https://codereview.chromium.org/2252103002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:77:
mSurfaceTexture = new SurfaceTexture(textureDataHandle);
IIUC, createExternalTextureHandle generates a new texture. 2 questions:
- in what context does it do that?
- what destroys that texture?

[bshe, 2016-08-19 16:48:08]

bshe@chromium.org changed reviewers:
+ changwan@chromium.org, yfriedman@chromium.org

[bshe, 2016-08-19 16:48:09]

Thanks for review. Add owners for some of the folder that I modified for review.
I am not too familiar with SurfaceTexture either. I will keep digging details of
it and make sure that I am doing the release correctly. Thanks!

At the same time, adding owners:
changwan@chromium.org: Please review changes in 
compositor/*

yfriedman@chromium.org: Please review changes in 
everything else

https://codereview.chromium.org/2252103002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java
(right):

https://codereview.chromium.org/2252103002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:77:
mSurfaceTexture = new SurfaceTexture(textureDataHandle);
On 2016/08/18 21:51:14, piman wrote:
> IIUC, createExternalTextureHandle generates a new texture. 2 questions:
> - in what context does it do that?
This is created on the GL context in the rendering thread of mGlSurfaceView.
> - what destroys that texture?
I dont have a full understanding of SurfaceTexture either.
The documentation of SurfaceTexture seems to suggest to call release() once
SurfaceTexture is no longer needed. So I added a release call in the shutdown
function.

[bajones, 2016-08-19 17:29:42]

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

[bajones, 2016-08-19 17:29:43]

https://codereview.chromium.org/2252103002/diff/20001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/2252103002/diff/20001/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:564: if (enable_vr_shell) {
This block won't be necessary once https://codereview.chromium.org/2219203002/
(re)lands

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

https://codereview.chromium.org/2252103002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_renderer.cc:30: const GLchar*
kTexturedQuadVertexShader =
Minor suggestion, not blocking: There's a reasonably nice pattern for declaring
shader code that you can see at
https://cs.chromium.org/chromium/src/gpu/tools/compositor_model_bench/shaders...
It allows you to avoid all the "\n" gunk and makes the actual shader text more
readable. I suggest using it here.

https://codereview.chromium.org/2252103002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_util.h (right):

https://codereview.chromium.org/2252103002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_util.h:17: // Util functions that are copied
from the trasure_hunt NDK demo in
Nit: "trasure_hunt" -> "treasure_hunt"

[bshe, 2016-08-19 21:05:10]

https://codereview.chromium.org/2252103002/diff/20001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/2252103002/diff/20001/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:564: if (enable_vr_shell) {
On 2016/08/19 17:29:42, bajones wrote:
> This block won't be necessary once https://codereview.chromium.org/2219203002/
> (re)lands

yeap. Will rebase once your cl landed and thanks for pave the way for using Gvr
library in chrome!!

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

https://codereview.chromium.org/2252103002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_renderer.cc:30: const GLchar*
kTexturedQuadVertexShader =
On 2016/08/19 17:29:43, bajones wrote:
> Minor suggestion, not blocking: There's a reasonably nice pattern for
declaring
> shader code that you can see at
>
https://cs.chromium.org/chromium/src/gpu/tools/compositor_model_bench/shaders...
> It allows you to avoid all the "\n" gunk and makes the actual shader text more
> readable. I suggest using it here.

Done.

https://codereview.chromium.org/2252103002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_util.h (right):

https://codereview.chromium.org/2252103002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_util.h:17: // Util functions that are copied
from the trasure_hunt NDK demo in
On 2016/08/19 17:29:43, bajones wrote:
> Nit: "trasure_hunt" -> "treasure_hunt"

Done.

[bshe, 2016-08-22 13:51:00]

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

[commit-bot: I haz the power, 2016-08-22 13:51:16]

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, 2016-08-22 14:10:16]

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

[commit-bot: I haz the power, 2016-08-22 14:10:17]

Dry run: 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...)

[bshe, 2016-08-22 14:59:37]

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

[commit-bot: I haz the power, 2016-08-22 14:59:50]

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, 2016-08-22 16:10:06]

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

[commit-bot: I haz the power, 2016-08-22 16:10:07]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[bshe, 2016-08-22 21:28:26]

bshe@chromium.org changed reviewers:
- yfriedman@chromium.org

[bshe, 2016-08-22 22:10:36]

bshe@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[bshe, 2016-08-22 22:10:37]

Looks like Yaron is OOO too. add another owner:
mariakhomenko@chromium.org: Please review changes in 
chrome/browser/android/*
and
chrome/android/*

Thanks!

[no sievers, 2016-08-22 22:55:02]

https://codereview.chromium.org/2252103002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java
(right):

https://codereview.chromium.org/2252103002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:77:
mSurfaceTexture = new SurfaceTexture(textureDataHandle);
On 2016/08/19 16:48:09, bshe wrote:
> On 2016/08/18 21:51:14, piman wrote:
> > IIUC, createExternalTextureHandle generates a new texture. 2 questions:
> > - in what context does it do that?
> This is created on the GL context in the rendering thread of mGlSurfaceView.
> > - what destroys that texture?
> I dont have a full understanding of SurfaceTexture either.
> The documentation of SurfaceTexture seems to suggest to call release() once
> SurfaceTexture is no longer needed. So I added a release call in the shutdown
> function.
> 

The call to release() will cause all the gralloc buffers to be freed (instead of
having to wait for GC).

You still probably want to call glDeleteTextures() somewhere for the handles
esp. since keeping the texture handle alive will probably keep the last buffer
referenced and alive.
Furthermore, you also need to unbind the texture for it to go away. SO maybe
TexturedQuadRenderer could deal with all that?

https://codereview.chromium.org/2252103002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2252103002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:214:
public void replaceCompositorSurface(SurfaceTexture surfaceTexture) {
I feel like this doesn't belong here since this class implements a SurfaceView
that has a compositor.

What you want is a compositor with a different Surface.

Also the other code in here probably mostly doesn't apply to your activity.
It reminds me a bit more of entering fullscreen. The other question is whether
you need to / should reuse the browser compositor here. It probably doesn't have
any interesting state since you transition away from the UI and to a new output
surface. And you have a different activity. In other use cases where that
happens (document mode, custom tabs) we transition to a new browser compositor.

So one thing you could do is have a ActivityWindowAndroid for your activity and
create a new compositor with it.

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

https://codereview.chromium.org/2252103002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/TextureHelper.java:24:
public static int createExternalTextureHandle() {
Why have this class? There is only one call site. Maybe just inline the code
there?

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

https://codereview.chromium.org/2252103002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:62:
mSurfaceTexture.releaseTexImage();
Why are you releasing the current buffer back to the producer? It hurts
performance and should be unnecessary here. You'd only want to do this in
single-buffered mode, but it looks like you are not creating the SurfaceTexture
in that mode.

Also, note that for single-buffered mode you'd have to synchronize this
differently, it's actually quite subtle.

[bshe, 2016-08-23 19:11:04]

Patchset #6 (id:100001) has been deleted

[Maria, 2016-08-24 00:06:07]

A few questions

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:357: <activity-alias
android:name="com.google.android.apps.chrome.VrMain"
why do you need an activity alias for this? Why not just export
ChromeVrActivity?

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeVrActivity.java
(right):

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeVrActivity.java:18:
public class ChromeVrActivity extends ChromeTabbedActivity {
We actually try not to put Chrome before the activity name, I would rename this
to "VrActivity". Some of the ChromeBlah things are that way because of legacy
that cannot be changed.

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeVrActivity.java:18:
public class ChromeVrActivity extends ChromeTabbedActivity {
It would help if there is a design doc you could share with me for this? Why do
you need all the stuff in ChromeTabbedActivity?

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:64:
private Surface mSurface = null;
no need to do = null here

[bshe, 2016-08-24 13:47:41]

Thanks for the review!
Had some discussion with sievers@ offline yesterday. And I am still working on
creating a new compositor instead of reusing the compositor in compositor_view
right now. Other than that, I think I have addressed most of the reviews.
Maria@ For question about ChromeTabbedActivity, I will create a bug and upload a
follow up patch to refer to the bug if you agree(see inline reply). Thanks.

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

https://codereview.chromium.org/2252103002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/TextureHelper.java:24:
public static int createExternalTextureHandle() {
On 2016/08/22 22:55:02, sievers wrote:
> Why have this class? There is only one call site. Maybe just inline the code
> there?

Moved the function to VrShell.

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

https://codereview.chromium.org/2252103002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:62:
mSurfaceTexture.releaseTexImage();
On 2016/08/22 22:55:02, sievers wrote:
> Why are you releasing the current buffer back to the producer? It hurts
> performance and should be unnecessary here. You'd only want to do this in
> single-buffered mode, but it looks like you are not creating the
SurfaceTexture
> in that mode.
> 
> Also, note that for single-buffered mode you'd have to synchronize this
> differently, it's actually quite subtle.

You are right. We don't need to do that.

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:357: <activity-alias
android:name="com.google.android.apps.chrome.VrMain"
On 2016/08/24 00:06:07, Maria wrote:
> why do you need an activity alias for this? Why not just export
> ChromeVrActivity?

huh. I remember had problem with launching ChromeVrActivity directly. But
I may just forget to export it. Done.

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeVrActivity.java
(right):

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeVrActivity.java:18:
public class ChromeVrActivity extends ChromeTabbedActivity {
On 2016/08/24 00:06:07, Maria wrote:
> We actually try not to put Chrome before the activity name, I would rename
this
> to "VrActivity". Some of the ChromeBlah things are that way because of legacy
> that cannot be changed.

Done.

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeVrActivity.java:18:
public class ChromeVrActivity extends ChromeTabbedActivity {
On 2016/08/24 00:06:07, Maria wrote:
> It would help if there is a design doc you could share with me for this? Why
do
> you need all the stuff in ChromeTabbedActivity?

Good point. I dont have a design doc for it. We don't need all the things in
ChromeTabbedActivity. The reason I use it here is that it is easier to start.
To do it correctly, I imagin that we will need to make changes in ChromeActivity
and then
extend from it. Or we may even need to modify ChromeTabbedActivity itself to
support the new VR mode. This is not going to be trivial and I don't want this
CL get much larger.
It is not perfect. But this code is behind a build time flag and wont affect
clank
in anyway. Given that, if you are fine with it, I will leave a TODO here and
create
a bug to track the process. The bug will block enable the build time flag.

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:64:
private Surface mSurface = null;
On 2016/08/24 00:06:07, Maria wrote:
> no need to do = null here

[Maria, 2016-08-24 17:35:02]

lgtm from perspective of activity setup / manifest. I haven't reviewed the
compositor / native code and will leave it to sievers@ and others who know more
about that.

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeVrActivity.java
(right):

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeVrActivity.java:18:
public class ChromeVrActivity extends ChromeTabbedActivity {
On 2016/08/24 13:47:40, bshe wrote:
> On 2016/08/24 00:06:07, Maria wrote:
> > It would help if there is a design doc you could share with me for this? Why
> do
> > you need all the stuff in ChromeTabbedActivity?
> 
> Good point. I dont have a design doc for it. We don't need all the things in
> ChromeTabbedActivity. The reason I use it here is that it is easier to start.
> To do it correctly, I imagin that we will need to make changes in
ChromeActivity
> and then
> extend from it. Or we may even need to modify ChromeTabbedActivity itself to
> support the new VR mode. This is not going to be trivial and I don't want this
> CL get much larger.
> It is not perfect. But this code is behind a build time flag and wont affect
> clank
> in anyway. Given that, if you are fine with it, I will leave a TODO here and
> create
> a bug to track the process. The bug will block enable the build time flag.

Plan SGTM

https://codereview.chromium.org/2252103002/diff/140001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2252103002/diff/140001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:350:
android:theme="@style/TabbedModeTheme"
I think the reason you have flashing chrome at startup is because you are using
TabbedModeTheme (which pre-paints chrome screenshot as a background). I suggest
using @style/MainTheme instead.

[bshe, 2016-08-26 17:59:42]

Description was changed from

==========
Introduce ChromeVR to Chrome on Android (behind a build flag)

This CL implements the following:
1. Adds a new activity called ChromeVrActivity. It extends from
ChromeTabbedActivity
and can only be launched from Daydream app.
2. Adds a VrShell and its native part. VrShell has a GLSurfaceView as
presentation
view for drawing left and right images and an inheriated(from GvrLayout)
scanline
racing view(another GLSurfaceView).
3. Replaces the Surface that compositor uses with a new Surface which backed by
a SurfaceTexture. We can then access the output stream of compositor as a
texture in
our VR drawing context which exist in the above presentation view.
Currently, the output of compositor is displayed in a floating square quad in 3D
space.
The quad is positioned at (0.0, 0.0, -1.0)

Known issue:
1. Normal browser UI will appear shortly when launching ChromeVR.
2. If Chrome was opened in the background before launching ChromeVR, it will
open a new tab instead of showing the last used tab.

To test this, add "enable_vr_shell=true" in your gn args.

BUG=638642
==========

to

==========
Introduce ChromeVR to Chrome on Android (behind a build flag)

This CL implements the following:
1. Adds a new activity called VrActivity. It extends from
AsyncInitializationActivity
and can only be launched from Daydream app.
2. Adds a VrShell and its native part. VrShell has a GLSurfaceView as
presentation
view for drawing left and right images and an inheriated(from GvrLayout)
scanline
racing view(another GLSurfaceView).
3. Created a texture quad that is reserved for compositor to use to output
content
frames. The quad is positioned at (0.0, 0.0, -1.0)

Known issue:
1. The quad is black as we haven't hookup a compositor to draw frames
2. tabs are not shared with ChromeTabbedActivity

To test this, add "enable_vr_shell=true" in your gn args.

BUG=638642
==========

[bshe, 2016-08-26 18:20:23]

bshe@chromium.org changed reviewers:
+ sky@chromium.org

[bshe, 2016-08-26 18:20:24]

Thanks for review.
Had offline discussion with Daniel and decided to separate the code that hook up
compositor's frame to SurfaceTexture in a follow up CL. I have changed the issue
description to reflect the change.
Daniel,  do you mind to take another look?

+ sky@chromium.org: for owner:
chrome/browser/BUILD.gn

https://codereview.chromium.org/2252103002/diff/140001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2252103002/diff/140001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:350:
android:theme="@style/TabbedModeTheme"
On 2016/08/24 17:35:02, Maria wrote:
> I think the reason you have flashing chrome at startup is because you are
using
> TabbedModeTheme (which pre-paints chrome screenshot as a background). I
suggest
> using @style/MainTheme instead.

hah. That explains. Thanks!

[mthiesse, 2016-08-26 18:36:06]

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

[mthiesse, 2016-08-26 18:36:07]

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java
(right):

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:77: if
(mNativeVrShell == 0) return;
This should either be an assert, or go away, as it shouldn't be possible for the
native vr shell to be uninitialized here.

[bshe, 2016-08-26 19:02:09]

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java
(right):

https://codereview.chromium.org/2252103002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:77: if
(mNativeVrShell == 0) return;
On 2016/08/26 18:36:07, mthiesse wrote:
> This should either be an assert, or go away, as it shouldn't be possible for
the
> native vr shell to be uninitialized here.

Done.

[sky, 2016-08-26 20:13:02]

LGTM

[bshe, 2016-08-26 22:11:22]

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

[commit-bot: I haz the power, 2016-08-26 22:12:00]

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

[no sievers, 2016-08-26 22:35:54]

lgtm

https://codereview.chromium.org/2252103002/diff/180001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java
(right):

https://codereview.chromium.org/2252103002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:76:
int textureDataHandle = createExternalTextureHandle();
per offline chat, i'd put a comment here that this handle doesn't get deleted
anywhere because we create it in onSurfaceCreated() and rely on
onSurfaceDestroyed() according to the GLSurfaceView documentation to delete the
context and clean up resources.

[bshe, 2016-08-26 22:50:23]

Thanks!

https://codereview.chromium.org/2252103002/diff/180001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java
(right):

https://codereview.chromium.org/2252103002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:76:
int textureDataHandle = createExternalTextureHandle();
On 2016/08/26 22:35:54, sievers wrote:
> per offline chat, i'd put a comment here that this handle doesn't get deleted
> anywhere because we create it in onSurfaceCreated() and rely on
> onSurfaceDestroyed() according to the GLSurfaceView documentation to delete
the
> context and clean up resources.

Done.

[bshe, 2016-08-26 22:50:54]

The CQ bit was checked by bshe@chromium.org

[bshe, 2016-08-26 22:50:54]

The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@google.com,
mariakhomenko@chromium.org, sky@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/2252103002/#ps200001
(title: "add comment as suggested by Daniel")

[commit-bot: I haz the power, 2016-08-26 22:51: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, 2016-08-27 10:38:35]

Description was changed from

==========
Introduce ChromeVR to Chrome on Android (behind a build flag)

This CL implements the following:
1. Adds a new activity called VrActivity. It extends from
AsyncInitializationActivity
and can only be launched from Daydream app.
2. Adds a VrShell and its native part. VrShell has a GLSurfaceView as
presentation
view for drawing left and right images and an inheriated(from GvrLayout)
scanline
racing view(another GLSurfaceView).
3. Created a texture quad that is reserved for compositor to use to output
content
frames. The quad is positioned at (0.0, 0.0, -1.0)

Known issue:
1. The quad is black as we haven't hookup a compositor to draw frames
2. tabs are not shared with ChromeTabbedActivity

To test this, add "enable_vr_shell=true" in your gn args.

BUG=638642
==========

to

==========
Introduce ChromeVR to Chrome on Android (behind a build flag)

This CL implements the following:
1. Adds a new activity called VrActivity. It extends from
AsyncInitializationActivity
and can only be launched from Daydream app.
2. Adds a VrShell and its native part. VrShell has a GLSurfaceView as
presentation
view for drawing left and right images and an inheriated(from GvrLayout)
scanline
racing view(another GLSurfaceView).
3. Created a texture quad that is reserved for compositor to use to output
content
frames. The quad is positioned at (0.0, 0.0, -1.0)

Known issue:
1. The quad is black as we haven't hookup a compositor to draw frames
2. tabs are not shared with ChromeTabbedActivity

To test this, add "enable_vr_shell=true" in your gn args.

BUG=638642
==========

[commit-bot: I haz the power, 2016-08-27 10:38:36]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-08-27 10:40:37]

Description was changed from

==========
Introduce ChromeVR to Chrome on Android (behind a build flag)

This CL implements the following:
1. Adds a new activity called VrActivity. It extends from
AsyncInitializationActivity
and can only be launched from Daydream app.
2. Adds a VrShell and its native part. VrShell has a GLSurfaceView as
presentation
view for drawing left and right images and an inheriated(from GvrLayout)
scanline
racing view(another GLSurfaceView).
3. Created a texture quad that is reserved for compositor to use to output
content
frames. The quad is positioned at (0.0, 0.0, -1.0)

Known issue:
1. The quad is black as we haven't hookup a compositor to draw frames
2. tabs are not shared with ChromeTabbedActivity

To test this, add "enable_vr_shell=true" in your gn args.

BUG=638642
==========

to

==========
Introduce ChromeVR to Chrome on Android (behind a build flag)

This CL implements the following:
1. Adds a new activity called VrActivity. It extends from
AsyncInitializationActivity
and can only be launched from Daydream app.
2. Adds a VrShell and its native part. VrShell has a GLSurfaceView as
presentation
view for drawing left and right images and an inheriated(from GvrLayout)
scanline
racing view(another GLSurfaceView).
3. Created a texture quad that is reserved for compositor to use to output
content
frames. The quad is positioned at (0.0, 0.0, -1.0)

Known issue:
1. The quad is black as we haven't hookup a compositor to draw frames
2. tabs are not shared with ChromeTabbedActivity

To test this, add "enable_vr_shell=true" in your gn args.

BUG=638642

Committed: https://crrev.com/16e132a1b213a07254d25b50a42ba9e9304d3f75
Cr-Commit-Position: refs/heads/master@{#414890}
==========

[commit-bot: I haz the power, 2016-08-27 10:40:38]

Patchset 10 (id:??) landed as
https://crrev.com/16e132a1b213a07254d25b50a42ba9e9304d3f75
Cr-Commit-Position: refs/heads/master@{#414890}