Enable creation of offscreen contexts which own their backing surface.

Enable creation of offscreen contexts which own their backing surface.

Uses this flag with WebGL contexts when WebVR is enabled, to allow the surface
to be swapped out with a compatible one if used for VR presentation.

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

[bajones, 2016-10-28 21:14:35]

Description was changed from

==========
Enable creation of offscreen contexts which own their backing surface.

Uses this flag with WebGL contexts when WebVR is enabled, to allow the surface
to be swapped out with a compatible one if used for VR presentation.

BUG=389343
==========

to

==========
Enable creation of offscreen contexts which own their backing surface.

Uses this flag with WebGL contexts when WebVR is enabled, to allow the surface
to be swapped out with a compatible one if used for VR presentation.

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

[bajones, 2016-11-10 22:50:14]

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

[commit-bot: I haz the power, 2016-11-10 22:51: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, 2016-11-10 23:05:49]

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

[commit-bot: I haz the power, 2016-11-10 23:05:49]

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...)

[bajones, 2016-11-15 20:49:51]

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

[commit-bot: I haz the power, 2016-11-15 20:50:11]

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

[commit-bot: I haz the power, 2016-11-15 21:10:37]

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

[commit-bot: I haz the power, 2016-11-15 21:10:38]

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...)

[bajones, 2016-11-15 21:20:33]

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

[commit-bot: I haz the power, 2016-11-15 21:21:05]

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

[commit-bot: I haz the power, 2016-11-15 21:40:10]

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

[commit-bot: I haz the power, 2016-11-15 21:40:11]

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

[bajones, 2016-11-16 00:07:59]

bajones@chromium.org changed reviewers:
+ dcheng@chromium.org, kbr@chromium.org, mkwst@chromium.org, piman@chromium.org

[bajones, 2016-11-16 00:07:59]

dcheng@ or mkwst@: PTAL at gpu_command_buffer_traits_multi.h and Platform.h

kbr@chromium.org: Please review WebGL changes

piman@chromium.org: Please review ui/gl and command_buffer changes.

This change is one I've talked about with piman@ breifly in the past. For WebVR
we want to swap out the default backbuffer for a WebGL context with one of our
own construction when presenting VR content, but that's not practical if the
context is using the default shared offscreen surface (which does not have the
necessary depth buffer, and thus is an incompatible surface.) This change allows
us to create WebGL contexts with it's own 1x1 offscreen surface so that we can
have greater control over to format and ensure it's compatible with the target
we want to swap in later. May also benefit fullscreen WebGL content down the
line.

[piman, 2016-11-16 00:44:03]

https://codereview.chromium.org/2461803002/diff/80001/gpu/ipc/service/gpu_com...
File gpu/ipc/service/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/2461803002/diff/80001/gpu/ipc/service/gpu_com...
gpu/ipc/service/gpu_command_buffer_stub.cc:580: << depth << " to base format "
<< surface_format;
Should we fail initialization here, then? I.e. return false.

https://codereview.chromium.org/2461803002/diff/80001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/2461803002/diff/80001/ui/gl/gl_surface.h#newc...
ui/gl/gl_surface.h:47: SURFACE_RGB565_DEPTH24,
It seems like this won't scale to more depth bits or more surface formats.
We could either:
- explicitly add depth bits to GLSurface::Initialize
- change GLSurface::Initialize to take a GLSurfaceAttribs struct instead of
GLSurface::Format that can specify the color format, but also the depth and
stencil bits, MSAA samples, etc. over time as we need them.

https://codereview.chromium.org/2461803002/diff/80001/ui/gl/init/gl_factory.h
File ui/gl/init/gl_factory.h (right):

https://codereview.chromium.org/2461803002/diff/80001/ui/gl/init/gl_factory.h...
ui/gl/init/gl_factory.h:75: const gfx::Size& size);
So, it sounds like the only reason you want to not initialize is so that you can
specify the format. Maybe a better API would be
CreateOffscreenGLSurfaceWithFormat(const gfx::Size& size, GLSurface::Format
format).

Separately, it would be nice to refactor the logic a bit so that
CreateOffscreenGLSurface(size) is equivalent to
CreateOffscreenGLSurfaceWithFormat(size, SURFACE_DEFAULT). This could be a
separate change:
- make SURFACE_DEFAULT its own value (as opposed to SURFACE_ARGB8888)
- make GLSurface::Initialize() (the 0 parameter version) non-virtual and remove
overloads
- in each of the GLSurface::Initialize(GLSurface::Format), explicitly handle
SURFACE_DEFAULT and convert to the right format (i.e. SURFACE_ARGB8888 almost
everywhere, except PbufferGLSurfaceEGL and SurfacelessEGL which have a special
Initialize() overload - that logic would move to Initialize(GLSurface::Format))

https://codereview.chromium.org/2461803002/diff/80001/ui/gl/init/gl_factory_a...
File ui/gl/init/gl_factory_android.cc (right):

https://codereview.chromium.org/2461803002/diff/80001/ui/gl/init/gl_factory_a...
ui/gl/init/gl_factory_android.cc:138: if (surface && GetGLImplementation() !=
kGLImplementationMockGL)
Here and other places: no need to check against kGLImplementationMockGL - the
reason we didn't call InitializeGLSurface on GLSurfaceStub is not so much that
it's wrong, but that it's a noop. But it would simplify to always call
Initialize on it.

https://codereview.chromium.org/2461803002/diff/80001/ui/gl/init/gl_factory_o...
File ui/gl/init/gl_factory_ozone.cc (right):

https://codereview.chromium.org/2461803002/diff/80001/ui/gl/init/gl_factory_o...
ui/gl/init/gl_factory_ozone.cc:151: return CreateOffscreenGLSurface(size);
That will return an initialized surface, and it will be incorrect to call
Initialize on it again.

[dcheng, 2016-11-17 01:17:00]

https://codereview.chromium.org/2461803002/diff/80001/gpu/ipc/service/gpu_com...
File gpu/ipc/service/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/2461803002/diff/80001/gpu/ipc/service/gpu_com...
gpu/ipc/service/gpu_command_buffer_stub.cc:580: << depth << " to base format "
<< surface_format;
On 2016/11/16 00:44:02, piman wrote:
> Should we fail initialization here, then? I.e. return false.

+1, this is coming from the renderer right? We need to gracefully handle invalid
or otherwise impossible combinations here.

[Ken Russell (switch to Gerrit), 2016-11-29 05:09:07]

https://codereview.chromium.org/2461803002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2461803002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:609:
contextAttributes.ownOffscreenSurface = true;
I haven't reviewed the rest of the patch in depth, but how does this interact
with WebGL's DrawingBuffer and in particular its allocation of its backing
store? I'm concerned about interactions with the code path gated by
DrawingBuffer::shouldUseChromiumImage().

Is the offscreen canvas's storage actually used? Or is the surface's format the
only significant aspect? WebGL's been using the DrawingBuffer for its back
buffer for a long time and it would be a step backward to use the offscreen
context's default framebuffer for anything significant.

Can DrawingBuffer abstract the VR-specific surface, similarly to its
GL_CHROMIUM_image path?

[klausw, 2017-01-05 02:17:42]

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

[klausw, 2017-01-05 02:17:44]

I'm revisiting this patch in two parts:

https://crrev.com/2616723002 "Refactor GL surface format handling" should
functionally be a no-op, but changes
the GLSurface::Format handling to prepare for more flexible
formats.

https://crrev.com/2586803003 "Enable creation of offscreen contexts which own
their backing surface" is an updated
version of this patch based on the refactored Format handling.

Can we move future discussion there?

https://codereview.chromium.org/2461803002/diff/80001/gpu/ipc/service/gpu_com...
File gpu/ipc/service/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/2461803002/diff/80001/gpu/ipc/service/gpu_com...
gpu/ipc/service/gpu_command_buffer_stub.cc:580: << depth << " to base format "
<< surface_format;
On 2016/11/17 01:17:00, dcheng wrote:
> On 2016/11/16 00:44:02, piman wrote:
> > Should we fail initialization here, then? I.e. return false.
> 
> +1, this is coming from the renderer right? We need to gracefully handle
invalid
> or otherwise impossible combinations here.

Please see the new logic, I think it's much cleaner now that it's
using a GLSurfaceFormat structure with Get/SetDepthBits() and related
accessors.

https://codereview.chromium.org/2461803002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2461803002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:609:
contextAttributes.ownOffscreenSurface = true;
On 2016/11/29 05:09:06, Ken Russell wrote:
> I haven't reviewed the rest of the patch in depth, but how does this interact
> with WebGL's DrawingBuffer and in particular its allocation of its backing
> store? I'm concerned about interactions with the code path gated by
> DrawingBuffer::shouldUseChromiumImage().
> 
> Is the offscreen canvas's storage actually used? Or is the surface's format
the
> only significant aspect? WebGL's been using the DrawingBuffer for its back
> buffer for a long time and it would be a step backward to use the offscreen
> context's default framebuffer for anything significant.
> 
> Can DrawingBuffer abstract the VR-specific surface, similarly to its
> GL_CHROMIUM_image path?

For context, the point of this change is to prepare for a WebVR
optimization to let a WebGL DrawingBuffer render directly to a
separately provided Surface, this can then be consumed cross-process
(without copying) as a SurfaceTexture for use with the VR device.

The followup CL https://codereview.chromium.org/2456213 internally
uses SetSurface and requires an output surface that's compatible 
with WebGL drawing, and this generally requires depth buffer and
other features that aren't currently available. This does not work
with offscreen drawing unfortunately, Android's SurfaceTexture 
needs a real surface to draw on.

https://codereview.chromium.org/2461803002/diff/80001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/2461803002/diff/80001/ui/gl/gl_surface.h#newc...
ui/gl/gl_surface.h:47: SURFACE_RGB565_DEPTH24,
On 2016/11/16 00:44:02, piman - On leave - No reviews wrote:
> It seems like this won't scale to more depth bits or more surface formats.
> We could either:
> - explicitly add depth bits to GLSurface::Initialize
> - change GLSurface::Initialize to take a GLSurfaceAttribs struct instead of
> GLSurface::Format that can specify the color format, but also the depth and
> stencil bits, MSAA samples, etc. over time as we need them.

See https://codereview.chromium.org/2616723002 for a new approach,
now using a GLSurfaceFormat class which replaces the custom format
types.

https://codereview.chromium.org/2461803002/diff/80001/ui/gl/init/gl_factory.h
File ui/gl/init/gl_factory.h (right):

https://codereview.chromium.org/2461803002/diff/80001/ui/gl/init/gl_factory.h...
ui/gl/init/gl_factory.h:75: const gfx::Size& size);
On 2016/11/16 00:44:02, piman - On leave - No reviews wrote:
> So, it sounds like the only reason you want to not initialize is so that you
can
> specify the format. Maybe a better API would be
> CreateOffscreenGLSurfaceWithFormat(const gfx::Size& size, GLSurface::Format
> format).
> 
> Separately, it would be nice to refactor the logic a bit so that
> CreateOffscreenGLSurface(size) is equivalent to
> CreateOffscreenGLSurfaceWithFormat(size, SURFACE_DEFAULT). This could be a
> separate change:
> - make SURFACE_DEFAULT its own value (as opposed to SURFACE_ARGB8888)

I briefly tried this but it caused a bunch of complications,
it would need careful conversion in GetFormat and other places
to ensure that comparisons would work as expected. The new
approach uses a GLSurfaceFormat class and moves non-default
properties to appropriate subtype initialization.

> - make GLSurface::Initialize() (the 0 parameter version) non-virtual and
remove
> overloads

Done.

> - in each of the GLSurface::Initialize(GLSurface::Format), explicitly handle
> SURFACE_DEFAULT and convert to the right format (i.e. SURFACE_ARGB8888 almost
> everywhere, except PbufferGLSurfaceEGL and SurfacelessEGL which have a special
> Initialize() overload - that logic would move to
Initialize(GLSurface::Format))

Handled a bit differently, using merging of properties on the
GLSurfaceFormat class instead of directly overwriting.

https://codereview.chromium.org/2461803002/diff/80001/ui/gl/init/gl_factory_a...
File ui/gl/init/gl_factory_android.cc (right):

https://codereview.chromium.org/2461803002/diff/80001/ui/gl/init/gl_factory_a...
ui/gl/init/gl_factory_android.cc:138: if (surface && GetGLImplementation() !=
kGLImplementationMockGL)
On 2016/11/16 00:44:03, piman - On leave - No reviews wrote:
> Here and other places: no need to check against kGLImplementationMockGL - the
> reason we didn't call InitializeGLSurface on GLSurfaceStub is not so much that
> it's wrong, but that it's a noop. But it would simplify to always call
> Initialize on it.

Moot, no longer applicable in the new refactor.

https://codereview.chromium.org/2461803002/diff/80001/ui/gl/init/gl_factory_o...
File ui/gl/init/gl_factory_ozone.cc (right):

https://codereview.chromium.org/2461803002/diff/80001/ui/gl/init/gl_factory_o...
ui/gl/init/gl_factory_ozone.cc:151: return CreateOffscreenGLSurface(size);
On 2016/11/16 00:44:03, piman - On leave - No reviews wrote:
> That will return an initialized surface, and it will be incorrect to call
> Initialize on it again.

Moot, now using CreateOffscreenGLSurfaceWithFormat.