Re-land^2 WebVR compositor bypass via BrowserMain context + mailbox

Re-land^2 WebVR compositor bypass via BrowserMain context + mailbox

[Cause for the layout test failures was likely a missing export that
causes mailbox_holder.mojom.js to get lost when packaging/extracting
in swarming builds.

bsheedy@'s https://chromium-review.googlesource.com/c/453099/ should
fix that, trying again.]

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's
  backed by a native Surface. This is encapsulated inside the new 
  MailboxToSurfaceBridge class.

- WebKit's VRDisplay sends a MailboxHolder for each frame (as drawn
  via WebGL on the GPU process) as argument to the SubmitFrame mojo
  call.

- MailboxToSurfaceLayerBridge uses its GPU command buffer GL context
  to draw the mailbox content to a Surface.

- VrShellGl picks up the frame from the corresponding SurfaceTexture
  and  proceeds with rendering as usual with its native GL context
  as needed for the low-level GVR API.

Also implements on-the-fly canvas resizing, the transfer surface and
GVR renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing
analysis.

It includes two separate modes for frame scheduling, the more
aggressive one is conditional on
chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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

Review-Url: https://codereview.chromium.org/2729523002
Cr-Original-Original-Commit-Position: refs/heads/master@{#455592}
Committed: https://chromium.googlesource.com/chromium/src/+/6280ec5b6e3b4c307926eb1bf0167acc2a0d4a74
Review-Url: https://codereview.chromium.org/2729523002
Cr-Original-Commit-Position: refs/heads/master@{#456130}
Committed: https://chromium.googlesource.com/chromium/src/+/9f86464794416e81abfe5921e9fac892716c610b
Review-Url: https://codereview.chromium.org/2729523002
Cr-Commit-Position: refs/heads/master@{#456272}
Committed: https://chromium.googlesource.com/chromium/src/+/362c621af1b1c9ad8da5bac607a082108b4aa213

[mthiesse, 2017-03-01 16:38:20]

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

[mthiesse, 2017-03-01 16:38:21]

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

https://codereview.chromium.org/2729523002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:113: void
VrShell::OnWebVrGpuChannelEstablished(
Should this code all live in vr_shell_gl?

[bajones, 2017-03-01 18:43:49]

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

[bajones, 2017-03-01 18:43:50]

This is really great! Seems more elegant to me than some of the previously
suggested methods. I really like the ability to pass around the frame contents
via mailbox. Also seems like about half of this would also potentially apply to
desktop? Obviously the surface bits don't translate, but we could still pass
textures via mailbox and then use that within the WebVR command buffer to get
the right cross-process synchronization primitives.

> The lifetime handling of the mailbox is probably wrong, the returned texture
> ID from glCreateAndConsumeTextureCHROMIUM increments on each call which
> smells like a leak.

I seem to recall that this is just how mailboxes are expected to work? Worth
noting that the actual mailbox is, more or less, just a string. Once you've
consumed the mailbox into a texture it's doesn't have any other content that
sticks around. As for the texture ID you could always re-use an existing texture
with glConsumeTextureCHROMIUM (consumes into the bound texture) but that's
probably not as efficient as you'd think, and in most cases creating a "new"
texture (really just a new texture name) is the right way to go.

A couple of other silly nits that don't really have an effect of the overall
patch:

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

https://codereview.chromium.org/2729523002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gpu_renderer.cc:132: GLint oldBuffer =
0;
Shouldn't need to do this here. I figured it out and fixed it in the WebVR
renderer (it needed to be reset to 0 so that the other renderers use of client
side arrays worked.)

https://codereview.chromium.org/2729523002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gpu_renderer.cc:141:
gl->BindBuffer(GL_ARRAY_BUFFER, oldBuffer);
Remove. See above comment.

https://codereview.chromium.org/2729523002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gpu_renderer.cc:165:
gl->GetIntegerv(GL_ARRAY_BUFFER_BINDING, &oldBuffer);
Remove. See above comment.

https://codereview.chromium.org/2729523002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gpu_renderer.cc:198:
gl->BindBuffer(GL_ARRAY_BUFFER, oldBuffer);
Remove. See above comment.

[klausw, 2017-03-02 08:14:31]

Description was changed from

==========
WIP: WebVR compositor bypass via BrowserMain context + mailbox

This is a proof of concept to do very basic compositor bypass via mailbox copy,
without any GL extensions or other invasive Chrome changes, so the changes
are effectively just within the VR code.

- Create a GPU command buffer GL context in VrShell's UI thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it via Mojo

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

I've been able to remove the fullscreen hack and pixel readback, and
reprojection seems to be working right which would indicate the poses are being
matched up correctly.

This attempt took a bunch of shortcuts to see if the approach can work at all,
these would of course need to be addressed:

- Very obviously, need to remove dead code, super verbose logging, and lots
  of style violations.

- Generating the mailbox is clunky, took me a while to find non-protected APIs.
  Seems to work, but I'm unsure if this is efficient or if it causes extra
  invisible copies.

- VRDisplay uses a sync token internally. Pass the token via mojo and do fence
  sync separately?

- The lifetime handling of the mailbox is probably wrong, the returned texture
  ID from glCreateAndConsumeTextureCHROMIUM increments on each call which
  smells like a leak.

- I added very crude VSync throttling to avoid overstuffing the Surface -
  calling SwapBuffers on it twice without an intervening updateTexImage loses
  frames. Result is that it doesn't get above 30fps even on simple scenes.

- Exiting/re-entering presentation confuses it, looks like things don't get
  reinitialized right.

- Resolution is hardcoded to 2560x1440 for now, not sure if it works on a Pixel
  not-XL model.

- I disabled VrShell UI in WebVR mode (so no security warning) since this
  somehow messed up GL state. Haven't investigated further.

- Some changes are likely to need to be #ifdef ANDROID, and there's probably
  dependency violations. If there are, I hope that these are solvable without
  having to do major refactoring elsewhere.

BUG=
==========

to

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Known issues (to be addressed before submitting):

- Very verbose VLOG(1) logging, I'm planning to remove most of this but it's
  useful while making changes.

- There's occasional flickering that seems related to mailbox / texture lifetime
  issues.

- Exiting/re-entering presentation confuses it, looks like things don't get
  reinitialized right.

- Resolution is hardcoded to 2560x1440 for now, the compositor surface size
  is currently not being initialized.

- I disabled VrShell UI in WebVR mode (so no security warning) since this
  results in bad GL state - the WebVR content gets clipped.

BUG=655722
==========

[klausw, 2017-03-02 08:16:14]

PTAL, I've continued with cleanups. Still some known issues, but overall I think
it's working.

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

https://codereview.chromium.org/2729523002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:113: void
VrShell::OnWebVrGpuChannelEstablished(
On 2017/03/01 16:38:21, mthiesse wrote:
> Should this code all live in vr_shell_gl?

Done.

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

https://codereview.chromium.org/2729523002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gpu_renderer.cc:132: GLint oldBuffer =
0;
On 2017/03/01 18:43:50, bajones wrote:
> Shouldn't need to do this here. I figured it out and fixed it in the WebVR
> renderer (it needed to be reset to 0 so that the other renderers use of client
> side arrays worked.)

Done.

https://codereview.chromium.org/2729523002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gpu_renderer.cc:141:
gl->BindBuffer(GL_ARRAY_BUFFER, oldBuffer);
On 2017/03/01 18:43:50, bajones wrote:
> Remove. See above comment.

Done.

https://codereview.chromium.org/2729523002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gpu_renderer.cc:198:
gl->BindBuffer(GL_ARRAY_BUFFER, oldBuffer);
On 2017/03/01 18:43:50, bajones wrote:
> Remove. See above comment.

Done.

[mthiesse, 2017-03-02 15:55:17]

Seems like a good approach to me, Brandon can give a better review of
gpu-related stuff. Lots of cleanup needed still.

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

https://codereview.chromium.org/2729523002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:328: void VrShell::SubmitWebVRFrame(
In a future patch we should consider binding SubmitFrame directly to the GL
thread to skip this unnecessary hop.

https://codereview.chromium.org/2729523002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc (right):

https://codereview.chromium.org/2729523002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc:15: #if
defined(OS_ANDROID)
nit: Remove this for now? We're still in the Android directory.

https://codereview.chromium.org/2729523002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc:31: // Don't retry
if we are shutting down.
Also don't retry if initialization fails?

https://codereview.chromium.org/2729523002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc:41: const size_t
full_screen_texture_size_in_bytes = 4000 * 3000 * 4;
Lots of magic numbers, where do these come from?

https://codereview.chromium.org/2729523002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc:100: // When
should this be released? Does registering it keep it alive?
Is this relevant?
https://codereview.chromium.org/2584343002/diff/60001/chrome/browser/android/...

[klausw, 2017-03-03 02:58:47]

Description was changed from

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Known issues (to be addressed before submitting):

- Very verbose VLOG(1) logging, I'm planning to remove most of this but it's
  useful while making changes.

- There's occasional flickering that seems related to mailbox / texture lifetime
  issues.

- Exiting/re-entering presentation confuses it, looks like things don't get
  reinitialized right.

- Resolution is hardcoded to 2560x1440 for now, the compositor surface size
  is currently not being initialized.

- I disabled VrShell UI in WebVR mode (so no security warning) since this
  results in bad GL state - the WebVR content gets clipped.

BUG=655722
==========

to

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Known issues (to be addressed before submitting):

- Very verbose VLOG(1) logging, I'm planning to remove most of this but it's
  useful while making changes.

- There's occasional flickering that seems related to mailbox / texture lifetime
  issues.

- Exiting/re-entering presentation confuses it, looks like things don't get
  reinitialized right.

- Resolution is hardcoded to 2560x1440 for now, the compositor surface size
  is currently not being initialized.

- I disabled VrShell UI in WebVR mode (so no security warning) since this
  results in bad GL state - the WebVR content gets clipped.

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

[klausw, 2017-03-03 03:01:58]

Description was changed from

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Known issues (to be addressed before submitting):

- Very verbose VLOG(1) logging, I'm planning to remove most of this but it's
  useful while making changes.

- There's occasional flickering that seems related to mailbox / texture lifetime
  issues.

- Exiting/re-entering presentation confuses it, looks like things don't get
  reinitialized right.

- Resolution is hardcoded to 2560x1440 for now, the compositor surface size
  is currently not being initialized.

- I disabled VrShell UI in WebVR mode (so no security warning) since this
  results in bad GL state - the WebVR content gets clipped.

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

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Known issues (to be addressed before submitting):

- Very verbose VLOG(1) logging, I'm planning to remove most of this but it's
  useful while making changes.

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

[klausw, 2017-03-03 03:05:28]

New snapshot, I've fixed most of the known issues:

- mailbox sync seems working now, thanks to Justin Novosad for the invaluable
help!

- use content size (screen size) as the WebVR rendering resolution. Resizing it
  is intended for a followup patch.

- fixed broken rendering on re-entering presentation.

The debug logs are still in it, I'll take another pass at removing those.
@mthiesse, I haven't addressed your comments yet.

New known issue: added very ugly code to copy mailbox and sync token element
by element for mojo transport. I was unable to get automatic typemap based
conversion working and gave up on it for now, help would be appreciated.

[klausw, 2017-03-03 07:01:48]

Description was changed from

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Known issues (to be addressed before submitting):

- Very verbose VLOG(1) logging, I'm planning to remove most of this but it's
  useful while making changes.

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

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Known issues (to be addressed before submitting):

- Very verbose VLOG(1) logging, I'm planning to remove most of this but it's
  useful while making changes.

BUG=655722
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation;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
==========

[klausw, 2017-03-03 07:02:18]

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

[commit-bot: I haz the power, 2017-03-03 07:02:30]

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

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

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

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

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
  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...)

[klausw, 2017-03-04 08:21:47]

Reimplemented as a vr_compositor using cc::TextureLayerClient to feed mailboxes
to a surface.

Thanks to Khushal Sagar for patiently working with me and answering lots of
questions.

This is in very rough shape, just wanting to snapshot my current work for
reference.

Known issues:

- Rendering is slower, ~45fps for 03-vr-presentation. This is due to poor
sequencing, I need to find a "DidSwapBuffers" hook so that I can notify
VRDisplay that it can release the mailbox, or use a member to keep the mailbox
texture alive to make this async.

- Lots of dead code that I haven't removed yet, the vr_shell_gpu_renderer and
vr_shell_command_buffer_gl files are all essentially obsolete now. Good
riddance.

I've uploaded a build (20170303b) at the usual place.

[klausw, 2017-03-05 07:07:02]

Using CompositorImpl turned out to be a bad idea, I've reverted it, but we're
back in business.

It's now using Khushal's shiny new crrev.com/2731833002 to provide a
GpuChannelEstablishFactory, this lets me get a GL context and I'm seeing steady
60fps again: http://i.imgur.com/tCNWXQV.png

Thank you once more for Khushal for lots of advice and explanations. Looks like
I should be able to replace the ad-hoc quad drawing code with
cc/output/gl_renderer, though I think it could also work as-is if that turns out
to be difficult.

Problems with CompositorImpl drawing included:

- Rendering is delayed due to buffering and scheduler heuristics, including
LayerTree / LayerTreeHost complications.

- the VrWindow is still hooked up to the original vsync, the synthetic ones I
added were in addition. Solvable but tedious.

- the DidSwapBuffers notification arrives very late, causing a lot of extra
delay waiting for notification.

[klausw, 2017-03-05 07:18:54]

Here's a timing diagram: http://i.imgur.com/rWn3XyQ.png

Binary 20170304c at the usual place, branch point is
298bf2e2d0ccfc87571bae60b2c0259d58f0d18e, and this patch includes Khushal's
proposed http://crrev.com/2731833002.

[klausw, 2017-03-06 05:41:41]

Description was changed from

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Known issues (to be addressed before submitting):

- Very verbose VLOG(1) logging, I'm planning to remove most of this but it's
  useful while making changes.

BUG=655722
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation;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

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Also implements on-the-fly canvas resizing, the transfer surface and GVR
renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing analysis.

It includes two separate modes for frame scheduling, the more aggressive one is
conditional on chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=655722
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation;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
==========

[klausw, 2017-03-06 05:41:41]

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

[klausw, 2017-03-06 05:46:53]

Description was changed from

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Also implements on-the-fly canvas resizing, the transfer surface and GVR
renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing analysis.

It includes two separate modes for frame scheduling, the more aggressive one is
conditional on chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=655722
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation;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

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Also implements on-the-fly canvas resizing, the transfer surface and GVR
renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing analysis.

It includes two separate modes for frame scheduling, the more aggressive one
is conditional on chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=655722
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation;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
==========

[klausw, 2017-03-06 06:22:56]

dcheng@, please review the mojom changes and the minor edit to FrameView.cpp.
We're running in a slightly strange mode where the view is marked hidden to
suppress normal compositor updates, but still active to process the canvas
contents and rAF handling for rendering. That seemed to violate a visibility
assumption.

mthiesse and bajones, PTAL. I've cleaned up the VLOGs and dead code, and
implemented full canvas resize support.

khushal: this is largely FYI, I've set your http://crrev.com/2731833002 as the
"Depends on Patchset".

Rebased to LKGR 1504af7fee3fcc0532208cfe3042571d89d131ed as of Sun Mar 5
18:50:39 2017 -0800, and uploaded APK 20170305d to the usual location.

[klausw, 2017-03-06 06:23:43]

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

[klausw, 2017-03-06 06:27:39]

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

[commit-bot: I haz the power, 2017-03-06 06:27:55]

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

[klausw, 2017-03-06 06:37:04]

Description was changed from

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Also implements on-the-fly canvas resizing, the transfer surface and GVR
renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing analysis.

It includes two separate modes for frame scheduling, the more aggressive one
is conditional on chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=655722
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation;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

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Also implements on-the-fly canvas resizing, the transfer surface and GVR
renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing analysis.

It includes two separate modes for frame scheduling, the more aggressive one
is conditional on chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation;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
==========

[commit-bot: I haz the power, 2017-03-06 06:40:17]

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

[commit-bot: I haz the power, 2017-03-06 06:40:18]

Dry run: Try jobs failed on following builders:
  linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)

[klausw, 2017-03-06 23:52:12]

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

[commit-bot: I haz the power, 2017-03-06 23:52:53]

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

[klausw, 2017-03-06 23:58:03]

On 2017/03/06 06:22:56, klausw wrote:
> dcheng@, please review the mojom changes and the minor edit to FrameView.cpp.
> We're running in a slightly strange mode where the view is marked hidden to
> suppress normal compositor updates, but still active to process the canvas
> contents and rAF handling for rendering. That seemed to violate a visibility
> assumption.

I've reverted that change, I found a public interface:
 doc->layoutView()->compositor()->setCompositingModeEnabled(false);

Thanks to mthiesse@'s help and more experimentation, I think we mostly have the
typemaps sorted out. It's fully working on the device/vr side, and the
sync_token works on the blink side. However, the Mailbox-to-mojo conversion on
the blink side apparently requires a change to blink_typemaps.gni, and the
resulting code fails to link due to a duplicate definition. See the comment I
added there. I've included a workaround to do without the typemap for now.

[klausw, 2017-03-07 00:15:01]

Description was changed from

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Also implements on-the-fly canvas resizing, the transfer surface and GVR
renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing analysis.

It includes two separate modes for frame scheduling, the more aggressive one
is conditional on chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation;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

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Also implements on-the-fly canvas resizing, the transfer surface and GVR
renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing analysis.

It includes two separate modes for frame scheduling, the more aggressive one
is conditional on chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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
==========

[bajones, 2017-03-07 00:48:07]

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc (right):

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc:33:
gpu::SharedMemoryLimits limits;
Looks like this could probably use gpu::SharedMemoryLimits::ForMailboxContext()
instead of the magic numbers here?

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc:63:
GURL("chrome://gpu/MusContextFactory"), automatic_flushes,
I believe we want to use a different URL here? Maybe
"chrome://gpu/VRContextFactory"?

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc:65:
ui::command_buffer_metrics::MUS_CLIENT_CONTEXT));
I doubt Mus wants our command buffer metrics mixed in with theirs. Ideally this
should be a custom command_buffer_metrics, but for now it's probably better to
use CONTEXT_TYPE_UNKNOWN than MUS_CLIENT_CONTEXT.

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

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:92: static constexpr unsigned
kWebVrRecommendedResolutionPercent = 50;
Nit: I'd recommend making this float kWebVrRecommendedResolutionScale = 0.5,
since it's still pretty readable and avoids the / 100 below.

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:252: // Don't create the WebVR
surface yet, wait until we need it.
Comment seems to imply WebVR surface will be created on demand at another time,
but we're calling CreateOrResizeWebVRSurface() 30 lines down in the same
function?

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:282: size.width &= ~0x1f;
Is this supposed to be 0xf instead of 0x1f? Also, comment above ends with a "?"
as though it's something to be done in a future CL, but you're doing the
rounding anyway right here. Maybe replace the "?" with a "."

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_gpu_renderer.cc (right):

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gpu_renderer.cc:184: // GPUs.
TODO(klausw): compare performance with and without it?
Nit: discardFramebufferEXT is the "right" thing to use in this case, I believe.
On non-tilers it'll be a no-op, and on tilers it'll do the efficient thing and
prevent the tile memory from being written out. See
https://cs.chromium.org/chromium/src/cc/output/gl_renderer.cc?type=cs&q=disca...
for an example use.

https://codereview.chromium.org/2729523002/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2729523002/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:260: if (firstPresent) {
if (firstPresent) { /* Do nothing. I just like braces! */ }

;)

[klausw, 2017-03-07 02:55:55]

PTAL, I think patchset 21 covers the open comments.

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

https://codereview.chromium.org/2729523002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gpu_renderer.cc:165:
gl->GetIntegerv(GL_ARRAY_BUFFER_BINDING, &oldBuffer);
On 2017/03/01 18:43:50, bajones wrote:
> Remove. See above comment.

Done.

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

https://codereview.chromium.org/2729523002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:328: void VrShell::SubmitWebVRFrame(
On 2017/03/02 15:55:16, mthiesse wrote:
> In a future patch we should consider binding SubmitFrame directly to the GL
> thread to skip this unnecessary hop.

Acknowledged.

https://codereview.chromium.org/2729523002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc (right):

https://codereview.chromium.org/2729523002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc:15: #if
defined(OS_ANDROID)
On 2017/03/02 15:55:16, mthiesse wrote:
> nit: Remove this for now? We're still in the Android directory.

Done.

https://codereview.chromium.org/2729523002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc:31: // Don't retry
if we are shutting down.
On 2017/03/02 15:55:16, mthiesse wrote:
> Also don't retry if initialization fails?

Moot, the switch no longer exists.

https://codereview.chromium.org/2729523002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc:41: const size_t
full_screen_texture_size_in_bytes = 4000 * 3000 * 4;
On 2017/03/02 15:55:16, mthiesse wrote:
> Lots of magic numbers, where do these come from?

Replaced with gpu::SharedMemoryLimits::ForMailboxContext() at Brandon's
suggestion.

https://codereview.chromium.org/2729523002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc:100: // When
should this be released? Does registering it keep it alive?
On 2017/03/02 15:55:16, mthiesse wrote:
> Is this relevant?
>
https://codereview.chromium.org/2584343002/diff/60001/chrome/browser/android/...

It is indeed, added unregistration to destructor.

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc (right):

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc:33:
gpu::SharedMemoryLimits limits;
On 2017/03/07 00:48:07, bajones wrote:
> Looks like this could probably use
gpu::SharedMemoryLimits::ForMailboxContext()
> instead of the magic numbers here?

Done.

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc:63:
GURL("chrome://gpu/MusContextFactory"), automatic_flushes,
On 2017/03/07 00:48:07, bajones wrote:
> I believe we want to use a different URL here? Maybe
> "chrome://gpu/VRContextFactory"?

Done, using chrome://gpu/WebVRContextFactory

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_command_buffer_gl.cc:65:
ui::command_buffer_metrics::MUS_CLIENT_CONTEXT));
On 2017/03/07 00:48:07, bajones wrote:
> I doubt Mus wants our command buffer metrics mixed in with theirs. Ideally
this
> should be a custom command_buffer_metrics, but for now it's probably better to
> use CONTEXT_TYPE_UNKNOWN than MUS_CLIENT_CONTEXT.

Done.

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

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:92: static constexpr unsigned
kWebVrRecommendedResolutionPercent = 50;
On 2017/03/07 00:48:07, bajones wrote:
> Nit: I'd recommend making this float kWebVrRecommendedResolutionScale = 0.5,
> since it's still pretty readable and avoids the / 100 below.

Done.

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:252: // Don't create the WebVR
surface yet, wait until we need it.
On 2017/03/07 00:48:07, bajones wrote:
> Comment seems to imply WebVR surface will be created on demand at another
time,
> but we're calling CreateOrResizeWebVRSurface() 30 lines down in the same
> function?

Changed to: WebVR surface is created below.

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:282: size.width &= ~0x1f;
On 2017/03/07 00:48:07, bajones wrote:
> Is this supposed to be 0xf instead of 0x1f? Also, comment above ends with a
"?"
> as though it's something to be done in a future CL, but you're doing the
> rounding anyway right here. Maybe replace the "?" with a "."

I've removed the rounding and replaced with a TODO comment to try this. It
didn't seem to help with the artifacts. FWIW, 0x1f was intentional since I
wanted the overall width to be a multiple of 32 so that each eye is a multiple
of 16.

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_gpu_renderer.cc (right):

https://codereview.chromium.org/2729523002/diff/330001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gpu_renderer.cc:184: // GPUs.
TODO(klausw): compare performance with and without it?
On 2017/03/07 00:48:07, bajones wrote:
> Nit: discardFramebufferEXT is the "right" thing to use in this case, I
believe.
> On non-tilers it'll be a no-op, and on tilers it'll do the efficient thing and
> prevent the tile memory from being written out. See
>
https://cs.chromium.org/chromium/src/cc/output/gl_renderer.cc?type=cs&q=disca...
> for an example use.

Done, though simplified to just use GL_COLOR_ATTACHMENT0 since it's not using an
FBO. Is that right?

https://codereview.chromium.org/2729523002/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2729523002/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:260: if (firstPresent) {
On 2017/03/07 00:48:07, bajones wrote:
> if (firstPresent) { /* Do nothing. I just like braces! */ }
> 
> ;)

Oops, removed.

[klausw (use chromium instead), 2017-03-07 20:42:49]

The CQ bit was checked by klausw@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 20:43:52]

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

[klausw, 2017-03-07 22:13:29]

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

[commit-bot: I haz the power, 2017-03-07 22:14:28]

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

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

Description was changed from

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Also implements on-the-fly canvas resizing, the transfer surface and GVR
renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing analysis.

It includes two separate modes for frame scheduling, the more aggressive one
is conditional on chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Also implements on-the-fly canvas resizing, the transfer surface and GVR
renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing analysis.

It includes two separate modes for frame scheduling, the more aggressive one
is conditional on chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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
==========

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

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

[klausw, 2017-03-07 22:30:44]

Description was changed from

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's backed
  by a native Surface

- SubmitFrame creates a mailbox and sends it as part of the mojo call

- On SubmitFrame, the GPU command buffer GL context converts the mailbox to a
  texture, and draws the content to the native Surface

- VrShellGl picks up the frame from the corresponding SurfaceTexture and
  proceeds with rendering as usual

Also implements on-the-fly canvas resizing, the transfer surface and GVR
renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing analysis.

It includes two separate modes for frame scheduling, the more aggressive one
is conditional on chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's
  backed by a native Surface. This is encapsulated inside the new 
  MailboxToSurfaceBridge class.

- WebKit's VRDisplay sends a MailboxHolder for each frame (as drawn
  via WebGL on the GPU process) as argument to the SubmitFrame mojo
  call.

- MailboxToSurfaceLayerBridge uses its GPU command buffer GL context
  to draw the mailbox content to a Surface.

- VrShellGl picks up the frame from the corresponding SurfaceTexture
  and  proceeds with rendering as usual with its native GL context
  as needed for the low-level GVR API.

Also implements on-the-fly canvas resizing, the transfer surface and
GVR renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing
analysis.

It includes two separate modes for frame scheduling, the more
aggressive one is conditional on
chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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
==========

[klausw, 2017-03-07 22:42:46]

jbauman@, please review the DEPS change in device/vr/ which adds 
+gpu/command_buffer/common/mailbox_holder.h, and the use of khushal's
proposed API to access a GpuChannelEstablishFactory. 

I've updated the description to match the latest changes, please feel free
to ask if you have questions about what it's doing and why.

[klausw, 2017-03-07 22:46:30]

klausw@chromium.org changed reviewers:
- klausw@google.com

[klausw, 2017-03-07 23:30:33]

On 2017/03/07 22:42:46, klausw wrote:
> jbauman@, please review the DEPS change in device/vr/ which adds 
> +gpu/command_buffer/common/mailbox_holder.h, and the use of khushal's
> proposed API to access a GpuChannelEstablishFactory. 
> 
> I've updated the description to match the latest changes, please feel free
> to ask if you have questions about what it's doing and why.

As per piman@'s request in https://codereview.chromium.org/2731833002#msg10,
we'll refactor the API there to return a ContextProvider instead, I'll
update this CL to match once that's done. The overall logic would remain
the same, just moving some code that's currently in MailboxToSurfaceBridge
to that interface.

[klausw, 2017-03-08 00:40:47]

On 2017/03/07 23:30:33, klausw wrote:
> On 2017/03/07 22:42:46, klausw wrote:
> > jbauman@, please review the DEPS change in device/vr/ which adds 
> > +gpu/command_buffer/common/mailbox_holder.h, and the use of khushal's
> > proposed API to access a GpuChannelEstablishFactory. 
> > 
> > I've updated the description to match the latest changes, please feel free
> > to ask if you have questions about what it's doing and why.
> 
> As per piman@'s request in https://codereview.chromium.org/2731833002#msg10,
> we'll refactor the API there to return a ContextProvider instead, I'll
> update this CL to match once that's done. The overall logic would remain
> the same, just moving some code that's currently in MailboxToSurfaceBridge
> to that interface.

Done, now using the new ContextProvider callback API as per Khushal's
updated crrev.com/2731833002 patchset 3.

I also removed the webvr_surface_ reference in VrShellGl since it's not
needed outside MailboxToSurfaceBridge.

PTAL for the other reviewers also, I don't currently have any known open
issues or feedback requests. Let me know if I missed something. 

(Still investigating rendering artifacts in the right eye, that seems to be
a GVR issue and is tracked in b/36032514.)

[klausw, 2017-03-08 00:41:18]

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

[commit-bot: I haz the power, 2017-03-08 00:42:07]

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

[mthiesse, 2017-03-08 01:00:09]

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
File chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc (right):

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:48: GLuint
CompileShader(gpu::gles2::GLES2Interface* gl,
Move this into vr_gl_util.h/cc?

We could share code by calling "glShaderSource" or "gl->glShaderSource"
depending on whether gl is null.

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:64:
gl->GetShaderiv(shaderHandle, GL_INFO_LOG_LENGTH, &infoLogLength);
nit: I know you're copying this from elsewhere, but we should move the logging -
especially the string - behind "#if DCHECK_IS_ON()".

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:132: // Our
attributes must be compatible with the shared offscreen
Add a comment to wherever the code lives that these values have to be compatible
with?

Also, file a bug?

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:161: LOG(ERROR) <<
__FUNCTION__ << ";;; failed to init ContextProvider";
Remove this? (Or DLOG)

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:202: bool
MailboxToSurfaceBridge::CopyFrameToSurface(
nit: This function is somewhat confusing. Maybe split it into two?
ConsumeFrame(frame_index, mailbox)->GLuint, and DrawFrame(GLUint).

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:226: // yet.
(Swapping twice on a Surfacewithout consuming one in
nit: s/Surfacewithout/Surface without/

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
File chrome/browser/android/vr_shell/mailbox_to_surface_bridge.h (right):

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.h:9: #include
"services/ui/public/cpp/gpu/context_provider_command_buffer.h"
nit: forward declare this

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

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:92: static constexpr float
kWebVrRecommendedResolutionScale = 0.5;
Why does gvr choose a size so large that we have to half it?

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:247: // WebVR surface is created
below.
nit: useless comment

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:260: // Pick a size for the WebVR
transfer surface based on a downscaled
Can you comment on why it's okay to do this?

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:266: 
nit: remove blank line

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:274: // TODO(klausw): should the
size be rounded to a multiple of N pixels
File a bug?

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:303: LOG(ERROR) << "No WebVR
surface texture available";
nit: DLOG?

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:379: // It is be legal for the
WebVR client to submit a new frame now,
nit: s/It is be/It is/

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_gl.h (right):

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.h:54: class WebVrBounds {
nit: struct

https://codereview.chromium.org/2729523002/diff/470001/gpu/ipc/common/mailbox...
File gpu/ipc/common/mailbox_holder_for_blink.typemap (right):

https://codereview.chromium.org/2729523002/diff/470001/gpu/ipc/common/mailbox...
gpu/ipc/common/mailbox_holder_for_blink.typemap:1: # Copyright 2017 The Chromium
Authors. All rights reserved.
nit: don't update copyright year?

https://codereview.chromium.org/2729523002/diff/470001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2729523002/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:550: TRACE_EVENT0("gpu",
"waitPrevXfer");
nit: more descriptive names for trace events?

[commit-bot: I haz the power, 2017-03-08 01:12:34]

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

[commit-bot: I haz the power, 2017-03-08 01:12:35]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[piman, 2017-03-08 01:20:35]

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

[piman, 2017-03-08 01:20:37]

I mainly looked at mailbox_to_surface_bridge.* and DEPS, and mostly LGTM + a
bunch of nits

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
File chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc (right):

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:50: GLenum
shaderType,
nit: here and other places in this file, should be using chromium style
(shader_type, shader_source, shader_handle, etc...)

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:66: GLchar*
strInfoLog = new GLchar[infoLogLength + 1];
nit: use std::unique_ptr<GLchar[]> instead of explicit delete[]

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:104: GLchar*
strInfoLog = new GLchar[infoLogLength + 1];
nit: ditto unique_ptr

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:144: VLOG(1) <<
__FUNCTION__ << ";;; gl=" << gl_;
nit: DVLOG

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:147: LOG(ERROR) <<
"Did not get a GL context";
nit: DLOG ?

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:191: LOG(ERROR) <<
"Cannot resize, not initialized";
nit: DLOG?

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:238:
CHECK(vertexShaderHandle) << error;
This should either be a DCHECK, or if the error can happen, should be handled
gracefully.

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:242:
CHECK(fragmentShaderHandle) << error;
nit: ditto

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:246:
CHECK(programHandle) << error;
nit: ditto

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:316: gl_->Flush();
nit: this may be unnecessary / redundant with the SwapBuffers that follows the
DrawQuad in CopyFrameToSurface (SwapBuffers implicitly flushes).

[klausw, 2017-03-08 02:59:20]

klausw@chromium.org changed reviewers:
- jbauman@chromium.org

[klausw, 2017-03-08 02:59:23]

-jbauman@ as required reviewer, piman@ volunteered to look at it.

PTAL, changes are addressed as of ps27.

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
File chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc (right):

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:48: GLuint
CompileShader(gpu::gles2::GLES2Interface* gl,
On 2017/03/08 01:00:00, mthiesse wrote:
> Move this into vr_gl_util.h/cc?
> 
> We could share code by calling "glShaderSource" or "gl->glShaderSource"
> depending on whether gl is null.

I'd strongly prefer to keep this independent since at boliu@'s request
I'll be looking into moving this class elsewhere, and we're unlikely
to be able to use vr_gl_util from the new location.

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:64:
gl->GetShaderiv(shaderHandle, GL_INFO_LOG_LENGTH, &infoLogLength);
On 2017/03/08 01:00:01, mthiesse wrote:
> nit: I know you're copying this from elsewhere, but we should move the logging
-
> especially the string - behind "#if DCHECK_IS_ON()".

This code only gets executed if shader compilation failed, and in that case I
think it's very helpful to retrieve the driver's log information. Unless I'm
misunderstanding things, it's not logging or allocating any strings in the
success case. Unless you feel strongly about it, I'd prefer to keep it.

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:132: // Our
attributes must be compatible with the shared offscreen
On 2017/03/08 01:00:00, mthiesse wrote:
> Add a comment to wherever the code lives that these values have to be
compatible
> with?
> 
> Also, file a bug?

Done. I filed 699330 and also added an IsLowEndDevice() conditional to be
compatible with CompositorImplAndroid's GetCompositorContextAttributes, though I
doubt that people will have a good experience on low end devices.

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:161: LOG(ERROR) <<
__FUNCTION__ << ";;; failed to init ContextProvider";
On 2017/03/08 01:00:00, mthiesse wrote:
> Remove this? (Or DLOG)

Changed to DLOG

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:202: bool
MailboxToSurfaceBridge::CopyFrameToSurface(
On 2017/03/08 01:00:01, mthiesse wrote:
> nit: This function is somewhat confusing. Maybe split it into two?
> ConsumeFrame(frame_index, mailbox)->GLuint, and DrawFrame(GLUint).

I've created a separate ConsumeMailbox function, simplified the
logic by moving the "discard" logic to the caller, and renamed
it to CopyMailboxToSurfaceAndSwap(mailbox). I'd like to keep
the consume + draw + swap in one place since that's the basic
abstraction the class provides, I don't want the underlying
texture available outside. It wouldn't work outside the private
GL context in this class anyway. 

If I understand it right, I don't have to do anything if I don't 
intend to consume the mailbox, it'll just expire when the VRDisplay 
side context drops its reference. (I had this happen unintentionally
in earlier versions of this CL when my synchronization was wrong.)

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:226: // yet.
(Swapping twice on a Surfacewithout consuming one in
On 2017/03/08 01:00:00, mthiesse wrote:
> nit: s/Surfacewithout/Surface without/

Done.

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

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:92: static constexpr float
kWebVrRecommendedResolutionScale = 0.5;
On 2017/03/08 01:00:05, mthiesse wrote:
> Why does gvr choose a size so large that we have to half it?

GVR reports a maximum recommended render resolution at which
the central region would have a 1:1 pixel mapping after distortion.

A Pixel XL has native 2560x1440 resolution and a 1:1
render resolution of 3604x2268 or so. For comparison, an HTC Vive
has native 2160x1200 resolution and a 1:1 render resolution of
3024x1680, and obviously a far more powerful GPU.

While it's typical to do supersampling with even higher 
than recommended resolutions on desktop, we need to go
the other way on mobile, it's not realistic to render any
moderately complex content at the 1:1 pixel ratio. Native GVR
apps often render at 0.7, but due to the additional overhead
in current WebVR we need to downscale it even more.

Now that we support canvas resizing, applications can always
opt to use larger sizes if they think they can handle it.

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:247: // WebVR surface is created
below.
On 2017/03/08 01:00:02, mthiesse wrote:
> nit: useless comment

Done.

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:260: // Pick a size for the WebVR
transfer surface based on a downscaled
On 2017/03/08 01:00:05, mthiesse wrote:
> Can you comment on why it's okay to do this?

Expanded the comment, it's arbitrary and just needs to be consistent for the
three places where the size gets used. We keep the two sizes within this class
consistent, and will resize to match the client if they don't
use our recommended size.

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:266: 
On 2017/03/08 01:00:04, mthiesse wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:274: // TODO(klausw): should the
size be rounded to a multiple of N pixels
On 2017/03/08 01:00:03, mthiesse wrote:
> File a bug?

Done, crbug.com/699350

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:303: LOG(ERROR) << "No WebVR
surface texture available";
On 2017/03/08 01:00:05, mthiesse wrote:
> nit: DLOG?

Done.

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:379: // It is be legal for the
WebVR client to submit a new frame now,
On 2017/03/08 01:00:02, mthiesse wrote:
> nit: s/It is be/It is/

Done.

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_gl.h (right):

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.h:54: class WebVrBounds {
On 2017/03/08 01:00:07, mthiesse wrote:
> nit: struct

Done.

https://codereview.chromium.org/2729523002/diff/470001/gpu/ipc/common/mailbox...
File gpu/ipc/common/mailbox_holder_for_blink.typemap (right):

https://codereview.chromium.org/2729523002/diff/470001/gpu/ipc/common/mailbox...
gpu/ipc/common/mailbox_holder_for_blink.typemap:1: # Copyright 2017 The Chromium
Authors. All rights reserved.
On 2017/03/08 01:00:07, mthiesse wrote:
> nit: don't update copyright year?

I didn't change it, this is a new file. crrev is just showing a 
diff based on its copy detection from mailbox_holder.typemap
which is confusing.

https://codereview.chromium.org/2729523002/diff/470001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2729523002/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:550: TRACE_EVENT0("gpu",
"waitPrevXfer");
On 2017/03/08 01:00:08, mthiesse wrote:
> nit: more descriptive names for trace events?

Done.

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
File chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc (right):

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:50: GLenum
shaderType,
On 2017/03/08 01:20:37, piman - On leave - No reviews wrote:
> nit: here and other places in this file, should be using chromium style
> (shader_type, shader_source, shader_handle, etc...)

Oops, done. This code had done a round trip through WebKit and its coding
conventions in a previous unsubmitted CL.

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:66: GLchar*
strInfoLog = new GLchar[infoLogLength + 1];
On 2017/03/08 01:20:37, piman - On leave - No reviews wrote:
> nit: use std::unique_ptr<GLchar[]> instead of explicit delete[]

Done.

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:104: GLchar*
strInfoLog = new GLchar[infoLogLength + 1];
On 2017/03/08 01:20:36, piman - On leave - No reviews wrote:
> nit: ditto unique_ptr

Done.

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:147: LOG(ERROR) <<
"Did not get a GL context";
On 2017/03/08 01:20:37, piman - On leave - No reviews wrote:
> nit: DLOG ?

Done.

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:191: LOG(ERROR) <<
"Cannot resize, not initialized";
On 2017/03/08 01:20:37, piman - On leave - No reviews wrote:
> nit: DLOG?

Moot, I've changed it to save the size for later so that it can resize once the
context has been created, it's no longer treated as an error.

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:238:
CHECK(vertexShaderHandle) << error;
On 2017/03/08 01:20:37, piman - On leave - No reviews wrote:
> This should either be a DCHECK, or if the error can happen, should be handled
> gracefully.

Replaced with this:

  if (!vertex_shader_handle) {
    DestroyContext();
    return;
  }

DestroyContext just nulls gl_ and context_provider_ which suppresses further
attempts to operate on the context, and the failing methods now use DLOG(ERROR)
for diagnostics.

I've confirmed in a debug build that it reports the error and keeps running 
(with blank eye views), and recovers as expected when exiting presentation.

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:242:
CHECK(fragmentShaderHandle) << error;
On 2017/03/08 01:20:37, piman - On leave - No reviews wrote:
> nit: ditto

Done.

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:246:
CHECK(programHandle) << error;
On 2017/03/08 01:20:37, piman - On leave - No reviews wrote:
> nit: ditto

Done.

https://codereview.chromium.org/2729523002/diff/490001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:316: gl_->Flush();
On 2017/03/08 01:20:37, piman - On leave - No reviews wrote:
> nit: this may be unnecessary / redundant with the SwapBuffers that follows the
> DrawQuad in CopyFrameToSurface (SwapBuffers implicitly flushes).

Removed.

[klausw, 2017-03-08 03:08:06]

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

[commit-bot: I haz the power, 2017-03-08 03:08:53]

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

[commit-bot: I haz the power, 2017-03-08 03:36:27]

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

[commit-bot: I haz the power, 2017-03-08 03:36:28]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mthiesse, 2017-03-08 04:15:43]

vr_shell/ lgtm.

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
File chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc (right):

https://codereview.chromium.org/2729523002/diff/470001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:64:
gl->GetShaderiv(shaderHandle, GL_INFO_LOG_LENGTH, &infoLogLength);
On 2017/03/08 02:59:22, klausw wrote:
> On 2017/03/08 01:00:01, mthiesse wrote:
> > nit: I know you're copying this from elsewhere, but we should move the
logging
> -
> > especially the string - behind "#if DCHECK_IS_ON()".
> 
> This code only gets executed if shader compilation failed, and in that case I
> think it's very helpful to retrieve the driver's log information. Unless I'm
> misunderstanding things, it's not logging or allocating any strings in the
> success case. Unless you feel strongly about it, I'd prefer to keep it.

This is good now. Generally we avoid unnecessary strings in source code for
binary size, so I just wanted the 'error = "Error compiling shader: ";' line
gone.

Can you make this change in vr_gl_util too please?

[bajones, 2017-03-08 06:18:05]

device/vr/ and modules/vr/ LGTM

[klausw, 2017-03-08 06:34:15]

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

[commit-bot: I haz the power, 2017-03-08 06:34:38]

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

[klausw, 2017-03-08 06:40:42]

mthiesse@, I got compilation errors from vr_display_impl_unittest.cc since its
RequestPresent call didn't match the new API. I admittedly don't entirely
understand how to set up interface clients for a test, but if I understand it
right this isn't used at all by FakeVRDevice, so I passed a nullptr to get it
compiling. Let me know if something fancier would be more appropriate. I'll see
what the trybots say, I can't currently run the unittest locally.

I'll make the error handling change for vr_gl_util in a separate change to avoid
entangling it with this one. Thank you for explaining, I assume this relies on
the LAZY_STREAM magic to not emit code or strings for debug-only expressions?

[klausw, 2017-03-08 06:45:39]

dcheng@, PTAL at the mojo and typemap-related changes. Some refactorings since
you last looked at it, but same overall logic. And thank you again for your help
getting the very mysterious compilation issue sorted out!

[commit-bot: I haz the power, 2017-03-08 07:08:27]

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

[commit-bot: I haz the power, 2017-03-08 07:08:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[klausw, 2017-03-08 07:17:40]

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

[commit-bot: I haz the power, 2017-03-08 07:18:05]

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

[dcheng, 2017-03-08 07:23:40]

mainly nits, mostly lg overall

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
File chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc (right):

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:31: const char*
kQuadCopyVertex =
const char kQuadCopyVertex[], as this is a mutable pointer otherwise.

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:45: //   x     y   
u,   v
I'd suggest disabling clang-format if you want the comments to line up

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:48: static
constexpr int kQuadVerticesSize = sizeof(float) * 16;
Use arraysize() from base/macros.h?

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:53: GLuint
shader_handle = gl->CreateShader(shader_type);
I'm not sure if it's possible, but perhaps we should consider some scoping
primitives for GL things as a followup. It seems quite easy to miss
DeleteShader() or DeleteProgram() on a branch.

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:56: int len =
strlen(shader_source);
size_t

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:142:
context_provider_ = provider;
Nit: std::move(provider)

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:159:
scoped_refptr<gl::SurfaceTexture> surface_texture) {
Nit: pass as a raw pointer since this function doesn't appear to take any
references to it.

(Maybe: I'm not familiar with ScopedJavaSurface...)

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
File chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc (right):

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:23: if (context)
Just for my understanding: why can context be null

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

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:179: binding_.Close();
Please add a comment why it's necessary to explicitly close.

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:263: auto render_target_size =
gvr_api_->GetMaximumEffectiveRenderTargetSize();
Can this be explicitly typed?

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:707: void
VrShellGl::DrawFrame(int frame_index) {
int16_t to match the other frame_index type?

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1125:
pending_bounds_.emplace(std::make_pair(
std::make_pair should be unnecessary with emplace

https://codereview.chromium.org/2729523002/diff/550001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_device_provider.cc (right):

https://codereview.chromium.org/2729523002/diff/550001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device_provider.cc:42:
Initialize(device::GvrDelegateProvider::GetInstance());
Nit: pass provider here

https://codereview.chromium.org/2729523002/diff/550001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2729523002/diff/550001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:553: LOG(ERROR) << "Failed to
receive SubmitFrame response";
DLOG, here and elsewhere

https://codereview.chromium.org/2729523002/diff/550001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:559: auto image_ref =
m_renderingContext->getImage(
Nit: I'd prefer to see the auto type written out here as well.

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

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

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

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[klausw, 2017-03-08 08:11:09]

Comments addressed as of ps31.

mthiesse@, one open question marked for you below.

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
File chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc (right):

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:31: const char*
kQuadCopyVertex =
On 2017/03/08 07:23:39, dcheng wrote:
> const char kQuadCopyVertex[], as this is a mutable pointer otherwise.

Done.

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:45: //   x     y   
u,   v
On 2017/03/08 07:23:39, dcheng wrote:
> I'd suggest disabling clang-format if you want the comments to line up

Done.

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:48: static
constexpr int kQuadVerticesSize = sizeof(float) * 16;
On 2017/03/08 07:23:39, dcheng wrote:
> Use arraysize() from base/macros.h?

Replaced with a simple sizeof(kQuadVertices) since we want the overall byte size
and not the count. (Took me embarrassingly long to figure out why using just
arraysize() broke rendering.)

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:53: GLuint
shader_handle = gl->CreateShader(shader_type);
On 2017/03/08 07:23:39, dcheng wrote:
> I'm not sure if it's possible, but perhaps we should consider some scoping
> primitives for GL things as a followup. It seems quite easy to miss
> DeleteShader() or DeleteProgram() on a branch.

I'd agree in general, but this code is mainly intended to be self-contained and
reasonably simple for this class, not for general use in more complex GL
applications. The error handling in this class is basically to null the context
and ignore further drawing commands, and nulling the context + provider should
clean up leftover shaders etc., right?

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:56: int len =
strlen(shader_source);
On 2017/03/08 07:23:39, dcheng wrote:
> size_t

Changed to GLint since that's what glShaderSource requires.

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:142:
context_provider_ = provider;
On 2017/03/08 07:23:39, dcheng wrote:
> Nit: std::move(provider)

Done.

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:159:
scoped_refptr<gl::SurfaceTexture> surface_texture) {
On 2017/03/08 07:23:39, dcheng wrote:
> Nit: pass as a raw pointer since this function doesn't appear to take any
> references to it.
> 
> (Maybe: I'm not familiar with ScopedJavaSurface...)

Done. It just needs the SurfaceTexture briefly to create a Surface based on it,
this surface gets registered in the tracker. Only the client needs to continue
using the SurfaceTexture.

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
File chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc (right):

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:23: if (context)
On 2017/03/08 07:23:39, dcheng wrote:
> Just for my understanding: why can context be null

mthiesse@, can you clarify? You had suggested this change as part of the
lifecycle refactor. Was this to prevent crashes in error conditions?

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

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:179: binding_.Close();
On 2017/03/08 07:23:39, dcheng wrote:
> Please add a comment why it's necessary to explicitly close.

Does the comment below the next line make it clearer? The VRDisplay side of the
connection outlives VrShellGl due to how it switches between the presenting
delegate (which is VrShellGl) and the non-presenting delegate (for "magic
window" non-headset mode) on entering/exiting presentation.

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:263: auto render_target_size =
gvr_api_->GetMaximumEffectiveRenderTargetSize();
On 2017/03/08 07:23:39, dcheng wrote:
> Can this be explicitly typed?

Done.

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:707: void
VrShellGl::DrawFrame(int frame_index) {
On 2017/03/08 07:23:39, dcheng wrote:
> int16_t to match the other frame_index type?

Done.

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1125:
pending_bounds_.emplace(std::make_pair(
On 2017/03/08 07:23:40, dcheng wrote:
> std::make_pair should be unnecessary with emplace

Done.

https://codereview.chromium.org/2729523002/diff/550001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_device_provider.cc (right):

https://codereview.chromium.org/2729523002/diff/550001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device_provider.cc:42:
Initialize(device::GvrDelegateProvider::GetInstance());
On 2017/03/08 07:23:40, dcheng wrote:
> Nit: pass provider here

Done.

https://codereview.chromium.org/2729523002/diff/550001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2729523002/diff/550001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:553: LOG(ERROR) << "Failed to
receive SubmitFrame response";
On 2017/03/08 07:23:40, dcheng wrote:
> DLOG, here and elsewhere

Done.

https://codereview.chromium.org/2729523002/diff/550001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:559: auto image_ref =
m_renderingContext->getImage(
On 2017/03/08 07:23:40, dcheng wrote:
> Nit: I'd prefer to see the auto type written out here as well.

Done.

[klausw, 2017-03-08 08:11:31]

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

[commit-bot: I haz the power, 2017-03-08 08:12:03]

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

[commit-bot: I haz the power, 2017-03-08 10:12:44]

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

[commit-bot: I haz the power, 2017-03-08 10:12:45]

Dry run: Try jobs failed on following builders:
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_TIMED_OUT, no
build URL)

[mthiesse, 2017-03-08 13:44:49]

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
File chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc (right):

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:23: if (context)
On 2017/03/08 08:11:09, klausw wrote:
> On 2017/03/08 07:23:39, dcheng wrote:
> > Just for my understanding: why can context be null
> 
> mthiesse@, can you clarify? You had suggested this change as part of the
> lifecycle refactor. Was this to prevent crashes in error conditions?

Yeah, this is to prevent crashes in strange error conditions. See
VrShellDelegate#createNonPresentingNativeContext for the possible reasons a
context would fail to be created.

Basically, vr support can change at runtime in response to the user uninstalling
apps/clearing data/etc. so we could fail to create a context even after we
thought we had VR support and created a VR Display.

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

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:179: binding_.Close();
On 2017/03/08 08:11:09, klausw wrote:
> On 2017/03/08 07:23:39, dcheng wrote:
> > Please add a comment why it's necessary to explicitly close.
> 
> Does the comment below the next line make it clearer? The VRDisplay side of
the
> connection outlives VrShellGl due to how it switches between the presenting
> delegate (which is VrShellGl) and the non-presenting delegate (for "magic
> window" non-headset mode) on entering/exiting presentation.

I wrote this - my concern was that it might be possible, if both ends of the
binding ever live in the same process for some reason, that we could receive
another VSync request in response to the closing message while in the
destructor, then fail to respond to the new callback. If this isn't a reasonable
concern, then we don't need to explicitly close the binding.

[mthiesse, 2017-03-08 15:40:56]

On 2017/03/08 06:40:42, klausw wrote:
> mthiesse@, I got compilation errors from vr_display_impl_unittest.cc since its
> RequestPresent call didn't match the new API. I admittedly don't entirely
> understand how to set up interface clients for a test, but if I understand it
> right this isn't used at all by FakeVRDevice, so I passed a nullptr to get it
> compiling. Let me know if something fancier would be more appropriate. I'll
see
> what the trybots say, I can't currently run the unittest locally.
> 
> I'll make the error handling change for vr_gl_util in a separate change to
avoid
> entangling it with this one. Thank you for explaining, I assume this relies on
> the LAZY_STREAM magic to not emit code or strings for debug-only expressions?

null submitclient for testing sgtm for now.

Yes, the DLOG does magic to strip out the strings for release builds.

[klausw, 2017-03-08 16:54:07]

On 2017/03/08 13:44:49, mthiesse wrote:
>
https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
> File chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc (right):
> 
>
https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
> chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:23: if
(context)
> On 2017/03/08 08:11:09, klausw wrote:
> > On 2017/03/08 07:23:39, dcheng wrote:
> > > Just for my understanding: why can context be null
> > 
> > mthiesse@, can you clarify? You had suggested this change as part of the
> > lifecycle refactor. Was this to prevent crashes in error conditions?
> 
> Yeah, this is to prevent crashes in strange error conditions. See
> VrShellDelegate#createNonPresentingNativeContext for the possible reasons a
> context would fail to be created.
> 
> Basically, vr support can change at runtime in response to the user
uninstalling
> apps/clearing data/etc. so we could fail to create a context even after we
> thought we had VR support and created a VR Display.
> 
>
https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
> File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):
> 
>
https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
> chrome/browser/android/vr_shell/vr_shell_gl.cc:179: binding_.Close();
> On 2017/03/08 08:11:09, klausw wrote:
> > On 2017/03/08 07:23:39, dcheng wrote:
> > > Please add a comment why it's necessary to explicitly close.
> > 
> > Does the comment below the next line make it clearer? The VRDisplay side of
> the
> > connection outlives VrShellGl due to how it switches between the presenting
> > delegate (which is VrShellGl) and the non-presenting delegate (for "magic
> > window" non-headset mode) on entering/exiting presentation.
> 
> I wrote this - my concern was that it might be possible, if both ends of the
> binding ever live in the same process for some reason, that we could receive
> another VSync request in response to the closing message while in the
> destructor, then fail to respond to the new callback. If this isn't a
reasonable
> concern, then we don't need to explicitly close the binding.

Added this as a TODO(mthiesse) comment. And just to clarify, mthiesse@
contributed many fixes and enhancements for this patch, see the ps history. I
wasn't meaning to imply otherwise. Thank you for doing so!

[klausw, 2017-03-08 16:56:26]

No code changes, just added a few comments with details from the review thread.
The unittest passed on the bots.

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
File chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc (right):

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:23: if (context)
On 2017/03/08 13:44:48, mthiesse wrote:
> On 2017/03/08 08:11:09, klausw wrote:
> > On 2017/03/08 07:23:39, dcheng wrote:
> > > Just for my understanding: why can context be null
> > 
> > mthiesse@, can you clarify? You had suggested this change as part of the
> > lifecycle refactor. Was this to prevent crashes in error conditions?
> 
> Yeah, this is to prevent crashes in strange error conditions. See
> VrShellDelegate#createNonPresentingNativeContext for the possible reasons a
> context would fail to be created.
> 
> Basically, vr support can change at runtime in response to the user
uninstalling
> apps/clearing data/etc. so we could fail to create a context even after we
> thought we had VR support and created a VR Display.

Added this as a comment. That is rather devious.

[klausw, 2017-03-08 16:58:51]

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

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:179: binding_.Close();
On 2017/03/08 13:44:48, mthiesse wrote:
> On 2017/03/08 08:11:09, klausw wrote:
> > On 2017/03/08 07:23:39, dcheng wrote:
> > > Please add a comment why it's necessary to explicitly close.
> > 
> > Does the comment below the next line make it clearer? The VRDisplay side of
> the
> > connection outlives VrShellGl due to how it switches between the presenting
> > delegate (which is VrShellGl) and the non-presenting delegate (for "magic
> > window" non-headset mode) on entering/exiting presentation.
> 
> I wrote this - my concern was that it might be possible, if both ends of the
> binding ever live in the same process for some reason, that we could receive
> another VSync request in response to the closing message while in the
> destructor, then fail to respond to the new callback. If this isn't a
reasonable
> concern, then we don't need to explicitly close the binding.

Added as a comment. Just to clarify, mthiesse@ contributed many fixes and
enhancements as noted in the ps history, I wasn't meaning to imply otherwise.
Thank you again for your help with mysterious mojo magic!

[klausw, 2017-03-08 17:02:09]

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

[commit-bot: I haz the power, 2017-03-08 17:02:54]

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

[commit-bot: I haz the power, 2017-03-08 19:37:25]

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

[commit-bot: I haz the power, 2017-03-08 19:37:27]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-03-08 23:08:06]

LGTM

Thanks for the explanations!

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
File chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc (right):

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:48: static
constexpr int kQuadVerticesSize = sizeof(float) * 16;
On 2017/03/08 08:11:08, klausw wrote:
> On 2017/03/08 07:23:39, dcheng wrote:
> > Use arraysize() from base/macros.h?
> 
> Replaced with a simple sizeof(kQuadVertices) since we want the overall byte
size
> and not the count. (Took me embarrassingly long to figure out why using just
> arraysize() broke rendering.)

Yikes, oops.

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:53: GLuint
shader_handle = gl->CreateShader(shader_type);
On 2017/03/08 08:11:08, klausw wrote:
> On 2017/03/08 07:23:39, dcheng wrote:
> > I'm not sure if it's possible, but perhaps we should consider some scoping
> > primitives for GL things as a followup. It seems quite easy to miss
> > DeleteShader() or DeleteProgram() on a branch.
> 
> I'd agree in general, but this code is mainly intended to be self-contained
and
> reasonably simple for this class, not for general use in more complex GL
> applications. The error handling in this class is basically to null the
context
> and ignore further drawing commands, and nulling the context + provider should
> clean up leftover shaders etc., right?

Again, doesn't have to be this CL, but I think it doesn't help to throw some
simple scopers in.

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

https://codereview.chromium.org/2729523002/diff/550001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:179: binding_.Close();
On 2017/03/08 16:58:51, klausw wrote:
> On 2017/03/08 13:44:48, mthiesse wrote:
> > On 2017/03/08 08:11:09, klausw wrote:
> > > On 2017/03/08 07:23:39, dcheng wrote:
> > > > Please add a comment why it's necessary to explicitly close.
> > > 
> > > Does the comment below the next line make it clearer? The VRDisplay side
of
> > the
> > > connection outlives VrShellGl due to how it switches between the
presenting
> > > delegate (which is VrShellGl) and the non-presenting delegate (for "magic
> > > window" non-headset mode) on entering/exiting presentation.
> > 
> > I wrote this - my concern was that it might be possible, if both ends of the
> > binding ever live in the same process for some reason, that we could receive
> > another VSync request in response to the closing message while in the
> > destructor, then fail to respond to the new callback. If this isn't a
> reasonable
> > concern, then we don't need to explicitly close the binding.
> 
> Added as a comment. Just to clarify, mthiesse@ contributed many fixes and
> enhancements as noted in the ps history, I wasn't meaning to imply otherwise.
> Thank you again for your help with mysterious mojo magic!

OK, thanks. In the case of non-sync calls (which is the default), there
shouldn't be anything to worry about, so I would say this can probably be
removed. But investigating this in a followup is fine.

[bajones, 2017-03-08 23:16:12]

The CQ bit was checked by bajones@chromium.org

[bajones, 2017-03-08 23:16:13]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
mthiesse@chromium.org, bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2729523002/#ps610001
(title: "Comments only, add explanations from review thread.")

[commit-bot: I haz the power, 2017-03-08 23:16:52]

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

[commit-bot: I haz the power, 2017-03-08 23:26:10]

CQ is committing da patch.

Bot data: {"patchset_id": 610001, "attempt_start_ts": 1489014972634370,
"parent_rev": "87cc44e2eb708c276f8fb2b6644588753ed75c6a", "commit_rev":
"6280ec5b6e3b4c307926eb1bf0167acc2a0d4a74"}

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

Description was changed from

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's
  backed by a native Surface. This is encapsulated inside the new 
  MailboxToSurfaceBridge class.

- WebKit's VRDisplay sends a MailboxHolder for each frame (as drawn
  via WebGL on the GPU process) as argument to the SubmitFrame mojo
  call.

- MailboxToSurfaceLayerBridge uses its GPU command buffer GL context
  to draw the mailbox content to a Surface.

- VrShellGl picks up the frame from the corresponding SurfaceTexture
  and  proceeds with rendering as usual with its native GL context
  as needed for the low-level GVR API.

Also implements on-the-fly canvas resizing, the transfer surface and
GVR renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing
analysis.

It includes two separate modes for frame scheduling, the more
aggressive one is conditional on
chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's
  backed by a native Surface. This is encapsulated inside the new 
  MailboxToSurfaceBridge class.

- WebKit's VRDisplay sends a MailboxHolder for each frame (as drawn
  via WebGL on the GPU process) as argument to the SubmitFrame mojo
  call.

- MailboxToSurfaceLayerBridge uses its GPU command buffer GL context
  to draw the mailbox content to a Surface.

- VrShellGl picks up the frame from the corresponding SurfaceTexture
  and  proceeds with rendering as usual with its native GL context
  as needed for the low-level GVR API.

Also implements on-the-fly canvas resizing, the transfer surface and
GVR renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing
analysis.

It includes two separate modes for frame scheduling, the more
aggressive one is conditional on
chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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

Review-Url: https://codereview.chromium.org/2729523002
Cr-Commit-Position: refs/heads/master@{#455592}
Committed:
https://chromium.googlesource.com/chromium/src/+/6280ec5b6e3b4c307926eb1bf016...
==========

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

Committed patchset #32 (id:610001) as
https://chromium.googlesource.com/chromium/src/+/6280ec5b6e3b4c307926eb1bf016...

[Sam McNally, 2017-03-09 03:07:56]

A revert of this CL (patchset #32 id:610001) has been created in
https://codereview.chromium.org/2739973002/ by sammc@chromium.org.

The reason for reverting is: Speculative revert for several failing vr layout
tests:
https://luci-milo.appspot.com/buildbot/chromium.webkit/WebKit%20Mac10.11/17274
https://luci-milo.appspot.com/buildbot/chromium.webkit/WebKit%20Mac10.11%20%2...
https://luci-milo.appspot.com/buildbot/chromium.webkit/WebKit%20Mac10.10/31124
https://luci-milo.appspot.com/buildbot/chromium.webkit/WebKit%20Mac10.9/43536
https://luci-milo.appspot.com/buildbot/chromium.webkit/WebKit%20Win7/50908
https://luci-milo.appspot.com/buildbot/chromium.webkit/WebKit%20Win10/20452.

[klausw, 2017-03-10 02:23:22]

Description was changed from

==========
WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's
  backed by a native Surface. This is encapsulated inside the new 
  MailboxToSurfaceBridge class.

- WebKit's VRDisplay sends a MailboxHolder for each frame (as drawn
  via WebGL on the GPU process) as argument to the SubmitFrame mojo
  call.

- MailboxToSurfaceLayerBridge uses its GPU command buffer GL context
  to draw the mailbox content to a Surface.

- VrShellGl picks up the frame from the corresponding SurfaceTexture
  and  proceeds with rendering as usual with its native GL context
  as needed for the low-level GVR API.

Also implements on-the-fly canvas resizing, the transfer surface and
GVR renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing
analysis.

It includes two separate modes for frame scheduling, the more
aggressive one is conditional on
chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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

Review-Url: https://codereview.chromium.org/2729523002
Cr-Commit-Position: refs/heads/master@{#455592}
Committed:
https://chromium.googlesource.com/chromium/src/+/6280ec5b6e3b4c307926eb1bf016...
==========

to

==========
Re-land WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's
  backed by a native Surface. This is encapsulated inside the new 
  MailboxToSurfaceBridge class.

- WebKit's VRDisplay sends a MailboxHolder for each frame (as drawn
  via WebGL on the GPU process) as argument to the SubmitFrame mojo
  call.

- MailboxToSurfaceLayerBridge uses its GPU command buffer GL context
  to draw the mailbox content to a Surface.

- VrShellGl picks up the frame from the corresponding SurfaceTexture
  and  proceeds with rendering as usual with its native GL context
  as needed for the low-level GVR API.

Also implements on-the-fly canvas resizing, the transfer surface and
GVR renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing
analysis.

It includes two separate modes for frame scheduling, the more
aggressive one is conditional on
chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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

Review-Url: https://codereview.chromium.org/2729523002
Cr-Commit-Position: refs/heads/master@{#455592}
Committed:
https://chromium.googlesource.com/chromium/src/+/6280ec5b6e3b4c307926eb1bf016...
==========

[klausw, 2017-03-10 02:24:42]

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

[commit-bot: I haz the power, 2017-03-10 02:25:12]

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

[klausw, 2017-03-10 02:49:33]

Attempting to re-land this change. It got reverted due to a layout test failure
that was blamed on this CL. We've been unable to reproduce the test failure on
any of the allegedly affected platforms. I've added the relevant trybots to
confirm as suggested by billorr@ and bsheedy@.

I've added a minor bugfix related to resizes and removed the old hack of
reporting a (0, 0) recommended render resolution while in magic window mode, it
now consistently exposes a plausible size. (I did this while investigating a
separate problem that turned out to be unrelated.)

mthiesse@, please review the changes between ps32 and ps33. I've annotated the
changes below (as suggested by ddorwin@), they include some spurious changes
from rebasing. (Note to self: correct sequence is rebase, upload, make needed
changes, upload. Sorry about that.) This affects only the VR directories, the
mojom change was someone else's CL imported from the rebase.

Please let me know in case of comments or concerns about re-landing this.

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

https://codereview.chromium.org/2729523002/diff/610001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:89: static constexpr float
kWebVrRecommendedResolutionScale = 0.5;
Moved to vr_shell.gl

https://codereview.chromium.org/2729523002/diff/610001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_delegate.h (right):

https://codereview.chromium.org/2729523002/diff/610001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_delegate.h:15: constexpr gvr::Sizei
kInvalidRenderTargetSize = {0, 0};
No longer necessary, we're always reporting valid recommended render resolutions
now even while not presenting.

https://codereview.chromium.org/2729523002/diff/610001/device/vr/vr_service.m...
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2729523002/diff/610001/device/vr/vr_service.m...
device/vr/vr_service.mojom:22: double timestamp;
Not my change, this is from rebase:

commit f4b1ca9d2d930fad5daaf6291a7a41c2eb141465
Author: shaobo.yan <shaobo.yan@intel.com>
Date:   Wed Mar 8 17:13:32 2017 -0800

    Update API to latest spec : Remove VRFrameData#timestamp
    According to latest WebVR Spec, there are several changes. Remove
VRFrameData#timestamp is one of them.

    BUG=697167
    
    Review-Url: https://codereview.chromium.org/2736593002

https://codereview.chromium.org/2729523002/diff/610001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2729523002/diff/610001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:522: auto elem =
m_layer.source();
Remove unused variable.

https://codereview.chromium.org/2729523002/diff/630001/chrome/browser/android...
File chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc (right):

https://codereview.chromium.org/2729523002/diff/630001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:32: /* clang-format
off */
No shader changes, just added clang-format off to keep it from reformatting
them.

https://codereview.chromium.org/2729523002/diff/630001/chrome/browser/android...
File chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc (right):

https://codereview.chromium.org/2729523002/diff/630001/chrome/browser/android...
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:179: gvr::Sizei
webvr_size = VrShell::GetRecommendedWebVrSize(gvr_api_.get());
Calling the static GetRecommendedWebVrSize here instead of sending Size(0, 0).
That used to be necessary since we didn't know the size we need while not
presenting, but we do now.

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

https://codereview.chromium.org/2729523002/diff/630001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:65: static constexpr float
kWebVrRecommendedResolutionScale = 0.5;
Moved here from vr_shell_gl.cc

https://codereview.chromium.org/2729523002/diff/630001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:386: void
VrShell::SetHistoryButtonsEnabled(JNIEnv* env,
Not my change, from rebase.

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

https://codereview.chromium.org/2729523002/diff/630001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.h:123: void
SetHistoryButtonsEnabled(JNIEnv* env,
Not my change, from rebase.

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

https://codereview.chromium.org/2729523002/diff/630001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:254: gvr::Sizei webvr_size =
VrShell::GetRecommendedWebVrSize(gvr_api_.get());
Also use the same static GetRecommendedWebVrSize here.

https://codereview.chromium.org/2729523002/diff/630001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:703: if
(gvr_api_->GetAsyncReprojectionEnabled()) {
This code isn't new, it's just moved here from below to ensure that it gets run
before acquiring a frame. The order wouldn't matter for the UV bounds, but GVR
can't resize the current frame if it's already bound, this could cause a 1-frame
glitch while resizing.

https://codereview.chromium.org/2729523002/diff/630001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:902: const gvr::Mat4f
eye_view_matrix =
Not my change, from rebase.

https://codereview.chromium.org/2729523002/diff/630001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:916: DrawElements(render_matrix,
eye_view_matrix, elementsInDrawOrder);
Not my change, from rebase.

https://codereview.chromium.org/2729523002/diff/630001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1126:
CreateOrResizeWebVRSurface(source_size);
This is a bugfix, the initial size wasn't being set consistently if this bounds
update happened with the special "-1" frame index and the client had already
chosen a canvas size different from the recommended render resolution.

[commit-bot: I haz the power, 2017-03-10 04:03:50]

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

[commit-bot: I haz the power, 2017-03-10 04:03:52]

Dry run: This issue passed the CQ dry run.

[klausw, 2017-03-10 07:17:56]

Another update in ps34:

There's no longer a need to differentiate reprojection mode from
non-reprojection (classic Cardboard) mode. The original reason for doing so was
to avoid a performance penalty for pixel readback in non-reprojection mode, and
we're no longer doing this in either case. Unifying the branches simplifies the
overall logic and removes duplication. Also, I'm not sure if the original
behavior was entirely correct for non-reprojection mode, for example we do need
synchronized bounds changes as requested by the client.

Reverted a suggested change, older devices such as Nexus 5X don't support
glDiscardFramebufferEXT. Replace with plain glClear for non-Daydream
compatibility, and added a TODO to revisit this for desktop GPUs, though I doubt
that matters since we wouldn't be using this MailboxToSurfaceBridge there.

I confirmed that the dynamic resolution sample works on both Pixel XL and Nexus
5X.

[klausw, 2017-03-10 07:18:13]

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

[commit-bot: I haz the power, 2017-03-10 07:18:27]

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

[commit-bot: I haz the power, 2017-03-10 08:50:16]

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

[commit-bot: I haz the power, 2017-03-10 08:50:18]

Dry run: This issue passed the CQ dry run.

[mthiesse, 2017-03-10 15:40:27]

https://codereview.chromium.org/2729523002/diff/650001/chrome/browser/android...
File chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc (right):

https://codereview.chromium.org/2729523002/diff/650001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:330: //
TODO(klausw): investigate adding a capability check for use on
File bug?

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

https://codereview.chromium.org/2729523002/diff/650001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:747:
swap_chain_->ResizeBuffer(kFramePrimaryBuffer, render_size_primary_);
You're no longer restoring the buffer size when not in WebVR mode. Does the
buffer size just not matter outside of WebVR? If so, rename webvr_surface_size_
to something more general?

https://codereview.chromium.org/2729523002/diff/650001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_gl.h (right):

https://codereview.chromium.org/2729523002/diff/650001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.h:189: gvr::Sizei
render_size_vrshell_;
This is unused now, remove it?

[klausw, 2017-03-10 16:33:08]

PTAL

https://codereview.chromium.org/2729523002/diff/650001/chrome/browser/android...
File chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc (right):

https://codereview.chromium.org/2729523002/diff/650001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:330: //
TODO(klausw): investigate adding a capability check for use on
On 2017/03/10 15:40:27, mthiesse wrote:
> File bug?

Done, filed crbug.com/700389. I also added the same comment in
VrShellGl::DrawWebVr and added a glClear there, it's the same scenario and that
code path is relevant for desktop also in the future.

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

https://codereview.chromium.org/2729523002/diff/650001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:747:
swap_chain_->ResizeBuffer(kFramePrimaryBuffer, render_size_primary_);
On 2017/03/10 15:40:27, mthiesse wrote:
> You're no longer restoring the buffer size when not in WebVR mode. Does the
> buffer size just not matter outside of WebVR? If so, rename
webvr_surface_size_
> to something more general?

This was unintentional, I think I got confused when removing the reprojection
conditional layer. We should still restore the VrShell size on exiting WebVR
mode to ensure this mode switch will then render at the resolution intended for
VrShell. I restored the branch for that, the "if (render_size_primary_ !=
webvr_surface_size_)" branch was supposed to be inside if(web_vr_mode_). Thank
you for catching that!

https://codereview.chromium.org/2729523002/diff/650001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_gl.h (right):

https://codereview.chromium.org/2729523002/diff/650001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.h:189: gvr::Sizei
render_size_vrshell_;
On 2017/03/10 15:40:27, mthiesse wrote:
> This is unused now, remove it?

We still need it, see next comment.

[klausw, 2017-03-10 16:34:04]

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

[commit-bot: I haz the power, 2017-03-10 16:34:29]

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

[mthiesse, 2017-03-10 16:38:41]

lgtm. Have we figured out the actual cause of the webvr tests timing out?

[klausw, 2017-03-10 17:23:43]

On 2017/03/10 16:38:41, mthiesse wrote:
> lgtm. Have we figured out the actual cause of the webvr tests timing out?

We haven't been able to reproduce it in local testing or on trybots. Out of
paranoia, I moved the //gpu/ipc/common:interfaces dependency in device/vr from
the Android-specific branch to the shared section to help ensure that it's
explicitly declared for tests even on platforms that don't enable the GVR code.
dcheng@, let me know if you have any concerns about that.

[klausw, 2017-03-10 17:24:26]

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

[commit-bot: I haz the power, 2017-03-10 17:24:54]

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

[mthiesse, 2017-03-10 18:17:26]

On 2017/03/10 17:23:43, klausw wrote:
> On 2017/03/10 16:38:41, mthiesse wrote:
> > lgtm. Have we figured out the actual cause of the webvr tests timing out?
> 
> We haven't been able to reproduce it in local testing or on trybots. Out of
> paranoia, I moved the //gpu/ipc/common:interfaces dependency in device/vr from
> the Android-specific branch to the shared section to help ensure that it's
> explicitly declared for tests even on platforms that don't enable the GVR
code.
> dcheng@, let me know if you have any concerns about that.

Build file change lgtm, but that dependency should be redundant anyways, so I
doubt it makes a difference.

[commit-bot: I haz the power, 2017-03-10 19:10:55]

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

[commit-bot: I haz the power, 2017-03-10 19:10:56]

Dry run: This issue passed the CQ dry run.

[klausw, 2017-03-10 19:14:35]

The CQ bit was checked by klausw@chromium.org

[klausw, 2017-03-10 19:14:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
bajones@chromium.org, dcheng@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2729523002/#ps710001
(title: "move //gpu/ipc/common:interfaces deps from android-specifc to all")

[commit-bot: I haz the power, 2017-03-10 19:15:14]

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

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

CQ is committing da patch.

Bot data: {"patchset_id": 710001, "attempt_start_ts": 1489173275499860,
"parent_rev": "9168fba24e7585305b61b67f426e025c358b3c61", "commit_rev":
"9f86464794416e81abfe5921e9fac892716c610b"}

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

Description was changed from

==========
Re-land WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's
  backed by a native Surface. This is encapsulated inside the new 
  MailboxToSurfaceBridge class.

- WebKit's VRDisplay sends a MailboxHolder for each frame (as drawn
  via WebGL on the GPU process) as argument to the SubmitFrame mojo
  call.

- MailboxToSurfaceLayerBridge uses its GPU command buffer GL context
  to draw the mailbox content to a Surface.

- VrShellGl picks up the frame from the corresponding SurfaceTexture
  and  proceeds with rendering as usual with its native GL context
  as needed for the low-level GVR API.

Also implements on-the-fly canvas resizing, the transfer surface and
GVR renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing
analysis.

It includes two separate modes for frame scheduling, the more
aggressive one is conditional on
chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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

Review-Url: https://codereview.chromium.org/2729523002
Cr-Commit-Position: refs/heads/master@{#455592}
Committed:
https://chromium.googlesource.com/chromium/src/+/6280ec5b6e3b4c307926eb1bf016...
==========

to

==========
Re-land WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's
  backed by a native Surface. This is encapsulated inside the new 
  MailboxToSurfaceBridge class.

- WebKit's VRDisplay sends a MailboxHolder for each frame (as drawn
  via WebGL on the GPU process) as argument to the SubmitFrame mojo
  call.

- MailboxToSurfaceLayerBridge uses its GPU command buffer GL context
  to draw the mailbox content to a Surface.

- VrShellGl picks up the frame from the corresponding SurfaceTexture
  and  proceeds with rendering as usual with its native GL context
  as needed for the low-level GVR API.

Also implements on-the-fly canvas resizing, the transfer surface and
GVR renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing
analysis.

It includes two separate modes for frame scheduling, the more
aggressive one is conditional on
chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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

Review-Url: https://codereview.chromium.org/2729523002
Cr-Original-Commit-Position: refs/heads/master@{#455592}
Committed:
https://chromium.googlesource.com/chromium/src/+/6280ec5b6e3b4c307926eb1bf016...
Review-Url: https://codereview.chromium.org/2729523002
Cr-Commit-Position: refs/heads/master@{#456130}
Committed:
https://chromium.googlesource.com/chromium/src/+/9f86464794416e81abfe5921e9fa...
==========

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

Committed patchset #37 (id:710001) as
https://chromium.googlesource.com/chromium/src/+/9f86464794416e81abfe5921e9fa...

[mthiesse, 2017-03-10 20:29:23]

https://codereview.chromium.org/2729523002/diff/710001/device/vr/vr_service.m...
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2729523002/diff/710001/device/vr/vr_service.m...
device/vr/vr_service.mojom:9: import "gpu/ipc/common/sync_token.mojom";
This is an unused import. Could this be causing the waterfall problems?

[klausw, 2017-03-10 20:46:41]

A revert of this CL (patchset #37 id:710001) has been created in
https://codereview.chromium.org/2742083002/ by klausw@chromium.org.

The reason for reverting is: Layout tests still failing for mac and windows
despite passing on the trybots. Example:
https://build.chromium.org/p/chromium.webkit/buildstatus?builder=WebKit%20Mac....

[klausw, 2017-03-10 22:22:15]

bsheedy@ found what looks like the root cause for the test issue.
scripts/slave/zip_build.py has a hardcoded list of directories it scans for
mojom files to package, and the one we need isn't on that list yet.

https://chromium-review.googlesource.com/c/453099/ should fix that, I'll try
another re-land once that change has propagated.

[klausw, 2017-03-10 23:39:05]

Description was changed from

==========
Re-land WebVR compositor bypass via BrowserMain context + mailbox

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's
  backed by a native Surface. This is encapsulated inside the new 
  MailboxToSurfaceBridge class.

- WebKit's VRDisplay sends a MailboxHolder for each frame (as drawn
  via WebGL on the GPU process) as argument to the SubmitFrame mojo
  call.

- MailboxToSurfaceLayerBridge uses its GPU command buffer GL context
  to draw the mailbox content to a Surface.

- VrShellGl picks up the frame from the corresponding SurfaceTexture
  and  proceeds with rendering as usual with its native GL context
  as needed for the low-level GVR API.

Also implements on-the-fly canvas resizing, the transfer surface and
GVR renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing
analysis.

It includes two separate modes for frame scheduling, the more
aggressive one is conditional on
chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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

Review-Url: https://codereview.chromium.org/2729523002
Cr-Original-Commit-Position: refs/heads/master@{#455592}
Committed:
https://chromium.googlesource.com/chromium/src/+/6280ec5b6e3b4c307926eb1bf016...
Review-Url: https://codereview.chromium.org/2729523002
Cr-Commit-Position: refs/heads/master@{#456130}
Committed:
https://chromium.googlesource.com/chromium/src/+/9f86464794416e81abfe5921e9fa...
==========

to

==========
Re-land^2 WebVR compositor bypass via BrowserMain context + mailbox

Cause for the layout test failures was likely a missing export that causes
mailbox_holder.mojom.js to get lost when packaging/extracting in swarming
builds. bsheedy@'s https://chromium-review.googlesource.com/c/453099/ should fix
that, trying again.

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's
  backed by a native Surface. This is encapsulated inside the new 
  MailboxToSurfaceBridge class.

- WebKit's VRDisplay sends a MailboxHolder for each frame (as drawn
  via WebGL on the GPU process) as argument to the SubmitFrame mojo
  call.

- MailboxToSurfaceLayerBridge uses its GPU command buffer GL context
  to draw the mailbox content to a Surface.

- VrShellGl picks up the frame from the corresponding SurfaceTexture
  and  proceeds with rendering as usual with its native GL context
  as needed for the low-level GVR API.

Also implements on-the-fly canvas resizing, the transfer surface and
GVR renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing
analysis.

It includes two separate modes for frame scheduling, the more
aggressive one is conditional on
chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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

Review-Url: https://codereview.chromium.org/2729523002
Cr-Original-Commit-Position: refs/heads/master@{#455592}
Committed:
https://chromium.googlesource.com/chromium/src/+/6280ec5b6e3b4c307926eb1bf016...
Review-Url: https://codereview.chromium.org/2729523002
Cr-Commit-Position: refs/heads/master@{#456130}
Committed:
https://chromium.googlesource.com/chromium/src/+/9f86464794416e81abfe5921e9fa...
==========

[klausw, 2017-03-10 23:40:07]

Description was changed from

==========
Re-land^2 WebVR compositor bypass via BrowserMain context + mailbox

Cause for the layout test failures was likely a missing export that causes
mailbox_holder.mojom.js to get lost when packaging/extracting in swarming
builds. bsheedy@'s https://chromium-review.googlesource.com/c/453099/ should fix
that, trying again.

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's
  backed by a native Surface. This is encapsulated inside the new 
  MailboxToSurfaceBridge class.

- WebKit's VRDisplay sends a MailboxHolder for each frame (as drawn
  via WebGL on the GPU process) as argument to the SubmitFrame mojo
  call.

- MailboxToSurfaceLayerBridge uses its GPU command buffer GL context
  to draw the mailbox content to a Surface.

- VrShellGl picks up the frame from the corresponding SurfaceTexture
  and  proceeds with rendering as usual with its native GL context
  as needed for the low-level GVR API.

Also implements on-the-fly canvas resizing, the transfer surface and
GVR renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing
analysis.

It includes two separate modes for frame scheduling, the more
aggressive one is conditional on
chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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

Review-Url: https://codereview.chromium.org/2729523002
Cr-Original-Commit-Position: refs/heads/master@{#455592}
Committed:
https://chromium.googlesource.com/chromium/src/+/6280ec5b6e3b4c307926eb1bf016...
Review-Url: https://codereview.chromium.org/2729523002
Cr-Commit-Position: refs/heads/master@{#456130}
Committed:
https://chromium.googlesource.com/chromium/src/+/9f86464794416e81abfe5921e9fa...
==========

to

==========
Re-land^2 WebVR compositor bypass via BrowserMain context + mailbox

[Cause for the layout test failures was likely a missing export that
causes mailbox_holder.mojom.js to get lost when packaging/extracting
in swarming builds.

bsheedy@'s https://chromium-review.googlesource.com/c/453099/ should
fix that, trying again.]

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's
  backed by a native Surface. This is encapsulated inside the new 
  MailboxToSurfaceBridge class.

- WebKit's VRDisplay sends a MailboxHolder for each frame (as drawn
  via WebGL on the GPU process) as argument to the SubmitFrame mojo
  call.

- MailboxToSurfaceLayerBridge uses its GPU command buffer GL context
  to draw the mailbox content to a Surface.

- VrShellGl picks up the frame from the corresponding SurfaceTexture
  and  proceeds with rendering as usual with its native GL context
  as needed for the low-level GVR API.

Also implements on-the-fly canvas resizing, the transfer surface and
GVR renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing
analysis.

It includes two separate modes for frame scheduling, the more
aggressive one is conditional on
chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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

Review-Url: https://codereview.chromium.org/2729523002
Cr-Original-Commit-Position: refs/heads/master@{#455592}
Committed:
https://chromium.googlesource.com/chromium/src/+/6280ec5b6e3b4c307926eb1bf016...
Review-Url: https://codereview.chromium.org/2729523002
Cr-Commit-Position: refs/heads/master@{#456130}
Committed:
https://chromium.googlesource.com/chromium/src/+/9f86464794416e81abfe5921e9fa...
==========

[klausw, 2017-03-11 01:44:43]

The CQ bit was checked by klausw@chromium.org

[klausw, 2017-03-11 01:44:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, bajones@chromium.org, dcheng@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2729523002/#ps730001
(title: "Rebase to 11e28fd6b9380b77273db51ef0b6ccc7ea341944")

[commit-bot: I haz the power, 2017-03-11 01:45:04]

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

[commit-bot: I haz the power, 2017-03-11 03:37:17]

CQ is committing da patch.

Bot data: {"patchset_id": 730001, "attempt_start_ts": 1489196683743190,
"parent_rev": "8683b364d376b33bed4cbfe6b46d2c39411b05cd", "commit_rev":
"362c621af1b1c9ad8da5bac607a082108b4aa213"}

[commit-bot: I haz the power, 2017-03-11 03:38:01]

Description was changed from

==========
Re-land^2 WebVR compositor bypass via BrowserMain context + mailbox

[Cause for the layout test failures was likely a missing export that
causes mailbox_holder.mojom.js to get lost when packaging/extracting
in swarming builds.

bsheedy@'s https://chromium-review.googlesource.com/c/453099/ should
fix that, trying again.]

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's
  backed by a native Surface. This is encapsulated inside the new 
  MailboxToSurfaceBridge class.

- WebKit's VRDisplay sends a MailboxHolder for each frame (as drawn
  via WebGL on the GPU process) as argument to the SubmitFrame mojo
  call.

- MailboxToSurfaceLayerBridge uses its GPU command buffer GL context
  to draw the mailbox content to a Surface.

- VrShellGl picks up the frame from the corresponding SurfaceTexture
  and  proceeds with rendering as usual with its native GL context
  as needed for the low-level GVR API.

Also implements on-the-fly canvas resizing, the transfer surface and
GVR renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing
analysis.

It includes two separate modes for frame scheduling, the more
aggressive one is conditional on
chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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

Review-Url: https://codereview.chromium.org/2729523002
Cr-Original-Commit-Position: refs/heads/master@{#455592}
Committed:
https://chromium.googlesource.com/chromium/src/+/6280ec5b6e3b4c307926eb1bf016...
Review-Url: https://codereview.chromium.org/2729523002
Cr-Commit-Position: refs/heads/master@{#456130}
Committed:
https://chromium.googlesource.com/chromium/src/+/9f86464794416e81abfe5921e9fa...
==========

to

==========
Re-land^2 WebVR compositor bypass via BrowserMain context + mailbox

[Cause for the layout test failures was likely a missing export that
causes mailbox_holder.mojom.js to get lost when packaging/extracting
in swarming builds.

bsheedy@'s https://chromium-review.googlesource.com/c/453099/ should
fix that, trying again.]

Implement compositor bypass for WebVR via mailbox copy:

- Create a GPU command buffer GL context in the VrShellGl thread that's
  backed by a native Surface. This is encapsulated inside the new 
  MailboxToSurfaceBridge class.

- WebKit's VRDisplay sends a MailboxHolder for each frame (as drawn
  via WebGL on the GPU process) as argument to the SubmitFrame mojo
  call.

- MailboxToSurfaceLayerBridge uses its GPU command buffer GL context
  to draw the mailbox content to a Surface.

- VrShellGl picks up the frame from the corresponding SurfaceTexture
  and  proceeds with rendering as usual with its native GL context
  as needed for the low-level GVR API.

Also implements on-the-fly canvas resizing, the transfer surface and
GVR renderbuffer are resized as needed to match the source canvas.

Added frame number as an argument to various TRACE_EVENTs for timing
analysis.

It includes two separate modes for frame scheduling, the more
aggressive one is conditional on
chrome:://flags#enable-webvr-experimental-rendering since the
framerate seems a bit less stable.

BUG=698643
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:closure_compilation;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

Review-Url: https://codereview.chromium.org/2729523002
Cr-Original-Original-Commit-Position: refs/heads/master@{#455592}
Committed:
https://chromium.googlesource.com/chromium/src/+/6280ec5b6e3b4c307926eb1bf016...
Review-Url: https://codereview.chromium.org/2729523002
Cr-Original-Commit-Position: refs/heads/master@{#456130}
Committed:
https://chromium.googlesource.com/chromium/src/+/9f86464794416e81abfe5921e9fa...
Review-Url: https://codereview.chromium.org/2729523002
Cr-Commit-Position: refs/heads/master@{#456272}
Committed:
https://chromium.googlesource.com/chromium/src/+/362c621af1b1c9ad8da5bac607a0...
==========

[commit-bot: I haz the power, 2017-03-11 03:38:04]

Committed patchset #38 (id:730001) as
https://chromium.googlesource.com/chromium/src/+/362c621af1b1c9ad8da5bac607a0...