WebVR: implement SetSurfaceHandleCHROMIUM extension for gvr_device.

WebVR: implement SetSurfaceHandleCHROMIUM extension for gvr_device.

This extension supports attaching an offscreen GL rendering context
to a native Android Surface for drawing, and restoring it to the
original offscreen context again later.

This is based on bajones@'s "Temp plumbing for the surface swap" patch.

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, 2016-10-28 00:20:07]

Description was changed from

==========
WebVR: implement SetSurfaceHandleCHROMIUM extension for gvr_device.

This extension supports attaching an offscreen GL rendering context
to a native Android Surface for drawing, and restoring it to the
original offscreen context again later.

This is based on bajones@'s "Temp plumbing for the surface swap" patch.

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

to

==========
WebVR: implement SetSurfaceHandleCHROMIUM extension for gvr_device.

This extension supports attaching an offscreen GL rendering context
to a native Android Surface for drawing, and restoring it to the
original offscreen context again later.

This is based on bajones@'s "Temp plumbing for the surface swap" patch.

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

[klausw, 2016-10-28 00:21:06]

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

[klausw, 2016-10-28 00:21:06]

Here's the updated extension code, I've slimmed it down to just the extension
itself. The code using it will follow separately.

[klausw, 2016-10-28 00:21:31]

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

[commit-bot: I haz the power, 2016-10-28 00:22:02]

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-10-28 00:25:31]

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

[commit-bot: I haz the power, 2016-10-28 00:25:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[bajones, 2016-10-28 00:41:22]

https://codereview.chromium.org/2456213002/diff/1/gpu/command_buffer/build_gl...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/2456213002/diff/1/gpu/command_buffer/build_gl...
gpu/command_buffer/build_gles2_cmd_buffer.py:2390: },
This is totally my fault, and it's a nit, but this probably doesn't belong a the
top of this list. Lets either throw it down at the bottom or, if it makes sense,
stick it somewhere alphabetical.

https://codereview.chromium.org/2456213002/diff/1/gpu/command_buffer/cmd_buff...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/2456213002/diff/1/gpu/command_buffer/cmd_buff...
gpu/command_buffer/cmd_buffer_functions.txt:367: // WebVR temporary plumbing
Comment doesn't make much sense if we're landing it now. I'd recommend moving
this function up to around line 317, under the "Non-GL commands" section.

https://codereview.chromium.org/2456213002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2456213002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16820: void
GLES2DecoderImpl::DoSetSurfaceHandleCHROMIUM(GLint surface_handle) {
Looking at this implementation the function name is very generic but the
internals are very WebVR specific. Maybe we should change the name to reflect
that? Also the fact that it actually changes the framebuffer binding maybe
deserves a callout. BindWebVRSurfaceHandleCHROMIUM?

[bajones, 2016-10-28 16:47:46]

https://codereview.chromium.org/2456213002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2456213002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16820: void
GLES2DecoderImpl::DoSetSurfaceHandleCHROMIUM(GLint surface_handle) {
Oh, and I also realized last night that you're going to need to wrap the
majority of the code in this function in

#if defined(OS_ANDROID)

Since the file is compiled on all platforms but the code here uses
android-specific calls and types.

[klausw, 2016-11-08 02:18:30]

https://codereview.chromium.org/2456213002/diff/1/gpu/command_buffer/build_gl...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/2456213002/diff/1/gpu/command_buffer/build_gl...
gpu/command_buffer/build_gles2_cmd_buffer.py:2390: },
On 2016/10/28 00:41:21, bajones wrote:
> This is totally my fault, and it's a nit, but this probably doesn't belong a
the
> top of this list. Lets either throw it down at the bottom or, if it makes
sense,
> stick it somewhere alphabetical.

I've put it i a mostly-alphabetical section, the order is unclear to me.

https://codereview.chromium.org/2456213002/diff/1/gpu/command_buffer/cmd_buff...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/2456213002/diff/1/gpu/command_buffer/cmd_buff...
gpu/command_buffer/cmd_buffer_functions.txt:367: // WebVR temporary plumbing
On 2016/10/28 00:41:22, bajones wrote:
> Comment doesn't make much sense if we're landing it now. I'd recommend moving
> this function up to around line 317, under the "Non-GL commands" section.

Done.

https://codereview.chromium.org/2456213002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2456213002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16820: void
GLES2DecoderImpl::DoSetSurfaceHandleCHROMIUM(GLint surface_handle) {
On 2016/10/28 16:47:46, bajones wrote:
> Oh, and I also realized last night that you're going to need to wrap the
> majority of the code in this function in
> 
> #if defined(OS_ANDROID)
> 
> Since the file is compiled on all platforms but the code here uses
> android-specific calls and types.

Done, and added an #else to raise a GL_INVALID_OPERATION on other platforms. Is
that the right way to do it?

https://codereview.chromium.org/2456213002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16820: void
GLES2DecoderImpl::DoSetSurfaceHandleCHROMIUM(GLint surface_handle) {
On 2016/10/28 00:41:22, bajones wrote:
> Looking at this implementation the function name is very generic but the
> internals are very WebVR specific. Maybe we should change the name to reflect
> that? Also the fact that it actually changes the framebuffer binding maybe
> deserves a callout. BindWebVRSurfaceHandleCHROMIUM?

It no longer changes the framebuffer binding in this version.

Actually it's not really WebVR specific, it's functionality to rebind the
current context to draw directly to a surface. I've replaced "WebVR" with 
"direct render" in comments and member variables.

What do you think?

[klausw, 2016-11-08 02:24:44]

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

[commit-bot: I haz the power, 2016-11-08 02:25:00]

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-08 02:29:20]

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

[commit-bot: I haz the power, 2016-11-08 02:29:21]

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...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[klausw, 2016-11-08 02:31:49]

FYI, the trybot fails due to forbidden include. What would be the cleanest way
to get this sorted out?

ERROR at //gpu/command_buffer/service/gles2_cmd_decoder.cc:68:11: Include not
allowed.
#include "gpu/ipc/common/gpu_surface_lookup.h"
          ^----------------------------------
It is not in any dependency of
  //gpu/command_buffer/service:service_sources
The include file is in the target(s):
  //gpu/ipc/common:ipc_common_sources
which should somehow be reachable.
GN gen failed: 1

[bajones, 2016-11-08 05:12:19]

On 2016/11/08 02:31:49, klausw wrote:
> FYI, the trybot fails due to forbidden include. What would be the cleanest way
> to get this sorted out?
> 
> ERROR at //gpu/command_buffer/service/gles2_cmd_decoder.cc:68:11: Include not
> allowed.
> #include "gpu/ipc/common/gpu_surface_lookup.h"
>           ^----------------------------------
> It is not in any dependency of
>   //gpu/command_buffer/service:service_sources
> The include file is in the target(s):
>   //gpu/ipc/common:ipc_common_sources
> which should somehow be reachable.
> GN gen failed: 1

It seems that you need to add "//gpu/ipc/common" (not
"//gpu/ipc/common:ipc_common_sources", as the file explicitly says not to
include that one directly) to the deps in "//gpu/command_buffer/service/BUILD.gn
under the target(link_target_type, "service_sources") section. Not sure if that
would break any dependency rules, but given that
"//gpu/ipc/common:surface_handle_type" is already there it may not be a problem?
Suggest adding piman@ as a reviewer, since he'll know the details better than I.

[bajones, 2016-11-08 05:13:48]

On 2016/11/08 02:18:30, klausw wrote:
> Done, and added an #else to raise a GL_INVALID_OPERATION on other platforms.
Is
> that the right way to do it?

Looks fine to me.

> It no longer changes the framebuffer binding in this version.
> 
> Actually it's not really WebVR specific, it's functionality to rebind the
> current context to draw directly to a surface. I've replaced "WebVR" with 
> "direct render" in comments and member variables.
> 
> What do you think?

New names are great, as far as I'm concerned!

[bajones, 2016-11-08 05:17:00]

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

[bajones, 2016-11-08 05:17:01]

Pulling in piman@, as I think he's got a far better grasp on the care and
feeding of surfaces than I do, but aside from the basic question of "Are we
managing the surfaces correctly?" I think this looks good.

https://codereview.chromium.org/2456213002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2456213002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16961:
direct_render_surface_->Initialize(gl::GLSurface::SURFACE_ARGB8888);
The format of this surface should be the same as the format of the existing
surface, right? Should we pass surface_->GetFormat() instead of the static
ARGB8888?

https://codereview.chromium.org/2456213002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16998: // clearing the smart
pointer calls the appropriate destructor.
Correct. The smart pointers will call the destructor.

[klausw, 2016-11-08 08:21:10]

BTW, is the size increase for the command decoder impl an issue? It's just
a few fields, but if needed I could replace those with a pointer to a
nested struct that's only allocated when needed.

On Nov 7, 2016 21:17, <bajones@chromium.org> wrote:

> Pulling in piman@, as I think he's got a far better grasp on the care and
> feeding of surfaces than I do, but aside from the basic question of "Are we
> managing the surfaces correctly?" I think this looks good.
>
>
> https://codereview.chromium.org/2456213002/diff/20001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
>
> https://codereview.chromium.org/2456213002/diff/20001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc#newcode16961
> gpu/command_buffer/service/gles2_cmd_decoder.cc:16961:
> direct_render_surface_->Initialize(gl::GLSurface::SURFACE_ARGB8888);
> The format of this surface should be the same as the format of the
> existing surface, right? Should we pass surface_->GetFormat() instead of
> the static ARGB8888?
>
> https://codereview.chromium.org/2456213002/diff/20001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc#newcode16998
> gpu/command_buffer/service/gles2_cmd_decoder.cc:16998: // clearing the
> smart pointer calls the appropriate destructor.
> Correct. The smart pointers will call the destructor.
>
> https://codereview.chromium.org/2456213002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[klausw, 2016-11-08 18:00:13]

On 2016/11/08 05:12:19, bajones wrote:
> On 2016/11/08 02:31:49, klausw wrote:
> > FYI, the trybot fails due to forbidden include. What would be the cleanest
way
> > to get this sorted out?
> > 
> > ERROR at //gpu/command_buffer/service/gles2_cmd_decoder.cc:68:11: Include
not
> > allowed.
> > #include "gpu/ipc/common/gpu_surface_lookup.h"
> >           ^----------------------------------
> > It is not in any dependency of
> >   //gpu/command_buffer/service:service_sources
> > The include file is in the target(s):
> >   //gpu/ipc/common:ipc_common_sources
> > which should somehow be reachable.
> > GN gen failed: 1
> 
> It seems that you need to add "//gpu/ipc/common" (not
> "//gpu/ipc/common:ipc_common_sources", as the file explicitly says not to
> include that one directly) to the deps in
"//gpu/command_buffer/service/BUILD.gn
> under the target(link_target_type, "service_sources") section. Not sure if
that
> would break any dependency rules, but given that
> "//gpu/ipc/common:surface_handle_type" is already there it may not be a
problem?
> Suggest adding piman@ as a reviewer, since he'll know the details better than
I.

Adding //gpu/ipc/common doesn't work:

ERROR Dependency cycle:
  //gpu:gpu ->
  //gpu/ipc/service:ipc_service_sources ->
  //gpu/command_buffer/service:service_sources ->
  //gpu/ipc/common:common ->
  //gpu:gpu

I've changed it to use "//gpu/ipc/common:ipc_common_sources" which does
appear to work.

The comment you're referring to is directly above a "visibility"
restriction to "//gpu/*". I would assume "external code" in this context
would be non-//gpu/ code in which case this appears ok, but I'd appreciate
a clarification:

source_set("ipc_common_sources") {
  # External code should depend on this via //gpu/ipc/common above rather than
  # depending on this directly or the component build will break.
  visibility = [ "//gpu/*" ]

[klausw, 2016-11-08 18:45:06]

PTAL. I've switched it to using a direct_render_state_ struct which looks
cleaner. Please sanity check that I'm using the
smart pointers right, I'm assuming that the struct's destructor via unique_ptr
.reset() will call appropriate destructors to ensure correct ref counts for the
surfaces.

https://codereview.chromium.org/2456213002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2456213002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16961:
direct_render_surface_->Initialize(gl::GLSurface::SURFACE_ARGB8888);
On 2016/11/08 05:17:01, bajones wrote:
> The format of this surface should be the same as the format of the existing
> surface, right? Should we pass surface_->GetFormat() instead of the static
> ARGB8888?

Yes, done, and added a comment explaining why we can't change the format here.

https://codereview.chromium.org/2456213002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16998: // clearing the smart
pointer calls the appropriate destructor.
On 2016/11/08 05:17:01, bajones wrote:
> Correct. The smart pointers will call the destructor.

OK, removed the comment.

[piman, 2016-11-09 17:50:07]

Sorry for the delay - a bunch of comments. Many are somewhat cosmetic, but the
security issue I mention below is fairly critical to get right.
Separately, could we add tests for this feature?

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:3607: 'extension': True,
A new extension should have a name and documentation in
gpu/GLES2/extensions/CHROMIUM/
We also need to expose it in the extension list, using AddExtensionString in
FeatureInfo::InitializeFeatures, so that the client can know whether it's
available or not. Additionally, you may consider adding a bool to
gpu::Capabilities, so that the client can save a round-trip to check for
availability. Lastly, you can consider adding a bool to
FeatureInfo::FeatureFlags, and referencing its name here as 'extension_flag' so
that the auto-generator can generate checking for availability of that
extension.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/cmd_...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/cmd_...
gpu/command_buffer/cmd_buffer_functions.txt:317: GL_APICALL void        
GL_APIENTRY glSetSurfaceHandleCHROMIUM (GLint surfaceHandle);
Could you add this at the end of the file? Odd request, but it avoids
renumbering commands, which helps keeping fuzzer coverage.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:68: #include
"gpu/ipc/common/gpu_surface_lookup.h"
So, including gpu/ipc is a layering violation. Indeed, the file needs to work in
a context where we don't use IPCs (e.g. for in-process tests).

One option is to move gpu_surface_lookup to gpu/command_buffer/common, because
it doesn't really need anything from IPC. That being said, the command buffer
code will be used in a context where GpuSurfaceLookup::GetInstance is NULL -
tests in particular - because the production implementation of GpuSurfaceLookup
needs things from outside of gpu/. When that is the case, we should make sure to
avoid exposing the extension.

One other option is to provide this functionality as an IPC instead of a
command. As an example to follow, you can take a look at how TakeFrontBuffer is
routed. It may be easier to provide the security guarantee (see below). A side
benefit for that is that we could then feed a generic GLSurface to the decoder,
avoiding making this an android-specific feature.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:86: #include
"ui/gl/gl_surface_egl.h"
I believe we can't include this on Mac (EGL is not present there). Maybe
consider including only on Android.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2145: bool
orig_back_buffer_has_depth;
saving/restoring the old values seems like additional complexity. I think it
would be preferable to have SetSurface compute the right values for the surface
we use, so that we know it's always accurate and we don't need to deal with
save/restore state.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16963: // surface_handle=0 in
between.
This is a bit distasteful as an API... It doesn't seem very hard to handle the
case. But if not, please raise an error instead of failing silently.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16973: surface_handle);
This could be NULL? How should we handle this?

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16975: new
gl::NativeViewGLSurfaceEGL(window);
So, this is an extremely powerful capability. If this extension is exposed to a
generic renderer, and that renderer gets compromized, it can use this API to
take over the main window and fake the browser UI, which is a security issue. We
need a mechanism to make sure the renderer can only access surfaces it should
have access to. There's essentially 2 ways of doing that. Either:
1- provide a mechanism where the browser gives to the GPU process a reference to
the window as associated with a given client. Then this would check that the
client does have access to that window. This would be similar to how we handle
binding GLImages to GpuMemoryBuffers (see e.g.
GpuMemoryBufferFactoryIOSurface::CreateImageForGpuMemoryBuffer)
2- make the surface handle unguessable. It would need to be a crypto-secure
identifier that's large enough - base::UnguessableToken is a good primitive for
that.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16992: // SurfaceTexture gets
updated.
FYI, "async" swap doesn't remove any "control", it just lets the implementation
free the main thread (using a callback) instead of blocking.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16994: supports_async_swap_ =
false;
what about other properties of the surface, like supports_post_sub_buffer_,
supports_swap_buffers_with_damage_, supports_commit_overlay_planes_?

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16996: back_buffer_has_depth_ =
true;
What makes this true?

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16997: // override
back_buffer_has_stencil_ too?
What is this comment, is it a TODO? If so, please use the TODO(klausw) syntax,
and maybe file a big (referenced here) about what there is to do.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:17007: // callers must do so
themselves as needed.
We should keep a consistent state. If the default framebuffer (0) is bound we
should make sure the new surface is taken into account. Among others it means we
should MakeCurrent to the new surface.

Beyond this, there are some states whose value depends on whether the default
framebuffer is a FBO or a surface - I'm thinking in particular the read buffer
and the draw buffers (see DoReadBuffer and DoDrawBuffersEXT). These states need
to be reset to the proper value.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right):

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:1467: GLint
surface_handle) {
Raise an error if not implemented?
What is the plan for implementing this in the passthrough decoder?

[klausw, 2016-11-09 21:21:56]

Partial update to address comments.

The following issues are still open:

- extension documentation and security (deferred this since we may change the
approach to an IPC call)

- gpu_surface_lookup layering violation

- tests

- passthrough decoder

Apart from that, does this look better?

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:86: #include
"ui/gl/gl_surface_egl.h"
On 2016/11/09 17:50:06, piman wrote:
> I believe we can't include this on Mac (EGL is not present there). Maybe
> consider including only on Android.

Done, now Android only.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2145: bool
orig_back_buffer_has_depth;
On 2016/11/09 17:50:07, piman wrote:
> saving/restoring the old values seems like additional complexity. I think it
> would be preferable to have SetSurface compute the right values for the
surface
> we use, so that we know it's always accurate and we don't need to deal with
> save/restore state.

I've refactored it by extracting bits of Initialize() into new 
reusable methods, so that I can keep the state consistent without
code duplication. One downside is that I needed to save a copy 
of the context attrib_helper, it's currently doing this unconditionally
but I could potentially change it to do so only if the extension exists. What do
you think?

  // Apply creation attibutes such as depth_bits for the offscreen
  // framebuffer or active surface as applicable.
  GLint ApplySurfaceAttributesAndGetAlphaBits(
      bool offscreen, const ContextCreationAttribHelper& attrib_helper);

  // Apply feature flags and surface-specific workarounds to match the
  // capabilities of the current surface. Used by Initialize() and
  // DoSetSurfaceHandleCHROMIUM.
  void ApplySurfaceFeatures();

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16963: // surface_handle=0 in
between.
On 2016/11/09 17:50:06, piman wrote:
> This is a bit distasteful as an API... It doesn't seem very hard to handle the
> case. But if not, please raise an error instead of failing silently.

Fixed, it raises an error now if the current context isn't offscreen.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16973: surface_handle);
On 2016/11/09 17:50:06, piman wrote:
> This could be NULL? How should we handle this?

Changed to raise an error.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16992: // SurfaceTexture gets
updated.
On 2016/11/09 17:50:06, piman wrote:
> FYI, "async" swap doesn't remove any "control", it just lets the
implementation
> free the main thread (using a callback) instead of blocking.

I removed this, I had misunderstood how it works and async swap actually sounds
useful for our use case.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16994: supports_async_swap_ =
false;
On 2016/11/09 17:50:06, piman wrote:
> what about other properties of the surface, like supports_post_sub_buffer_,
> supports_swap_buffers_with_damage_, supports_commit_overlay_planes_?

See above, the new shared methods should keep them consistent.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16996: back_buffer_has_depth_ =
true;
On 2016/11/09 17:50:06, piman wrote:
> What makes this true?

This was an assumption based on the current use case, I've now changed it to
properly apply the attribute based on the original attrib_helper.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16997: // override
back_buffer_has_stencil_ too?
On 2016/11/09 17:50:07, piman wrote:
> What is this comment, is it a TODO? If so, please use the TODO(klausw) syntax,
> and maybe file a big (referenced here) about what there is to do.

Should be fixed now.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:17007: // callers must do so
themselves as needed.
On 2016/11/09 17:50:06, piman wrote:
> We should keep a consistent state. If the default framebuffer (0) is bound we
> should make sure the new surface is taken into account. Among others it means
we
> should MakeCurrent to the new surface.

I've added MakeCurrent and am applying glViewport and glScissor. Any additional
"among others" it should be doing?

> Beyond this, there are some states whose value depends on whether the default
> framebuffer is a FBO or a surface - I'm thinking in particular the read buffer
> and the draw buffers (see DoReadBuffer and DoDrawBuffersEXT). These states
need
> to be reset to the proper value.

I've attempted to fix this, as far as I can tell I don't need to do
anything for a user-bound FBO, GL_NONE remains valid, but I need to
map GL_BACK <=> GL_COLOR_ATTACHMENT0. Does that sound right?

On second thought, do I actually need to do this? Lots of places in 
the code call CheckBoundDrawFramebufferValid and equivalent, and as
far as I can tell that does this fixup internally as a side effect.

I haven't tested the read buffer, but the draw buffer appeared to work fine
without the fixup.

[piman, 2016-11-09 22:26:10]

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:17007: // callers must do so
themselves as needed.
On 2016/11/09 21:21:55, klausw wrote:
> On 2016/11/09 17:50:06, piman wrote:
> > We should keep a consistent state. If the default framebuffer (0) is bound
we
> > should make sure the new surface is taken into account. Among others it
means
> we
> > should MakeCurrent to the new surface.
> 
> I've added MakeCurrent and am applying glViewport and glScissor. Any
additional
> "among others" it should be doing?
> 
> > Beyond this, there are some states whose value depends on whether the
default
> > framebuffer is a FBO or a surface - I'm thinking in particular the read
buffer
> > and the draw buffers (see DoReadBuffer and DoDrawBuffersEXT). These states
> need
> > to be reset to the proper value.
> 
> I've attempted to fix this, as far as I can tell I don't need to do
> anything for a user-bound FBO, GL_NONE remains valid, but I need to
> map GL_BACK <=> GL_COLOR_ATTACHMENT0. Does that sound right?

Correct.

> 
> On second thought, do I actually need to do this? Lots of places in 
> the code call CheckBoundDrawFramebufferValid and equivalent, and as
> far as I can tell that does this fixup internally as a side effect.

It'll only change the draw buffer if the back buffer needs to be cleared, and it
is *not* GL_BACK - it does that so that the draw buffer correctly targets the
buffer we want to clear, and then resets it. It's an implementation detail of
that function... but either way, it won't fix up the draw buffer in the general
case - nor the read buffer. 

> 
> I haven't tested the read buffer, but the draw buffer appeared to work fine
> without the fixup.

https://codereview.chromium.org/2456213002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2456213002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:17050:
context_->MakeCurrent(surface_.get());
You need to do the MakeCurrent before the ApplySurfaceAttributesAndGetAlphaBits,
otherwise the queries will return properties of the old surface.

Either way, you want to do the MakeCurrent regardless of whether there's a bound
FBO or not, otherwise if the client unbinds the FBO and tries to draw, it'll go
to the wrong surface.

https://codereview.chromium.org/2456213002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:17052: // dimensions. Re-apply
the settings from the current state.
Actually, MakeCurrent only sets viewport/scissor on a new context, not a new
surface, so I don't think you need this.

https://codereview.chromium.org/2456213002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:17089:
ApplySurfaceAttributesAndGetAlphaBits(true, attrib_helper_);
You probably want to MakeCurrent here to the offscreen surface too. It doesn't
exactly matter for correctness, but as long as the direct surface is current, it
won't be freed.

https://codereview.chromium.org/2456213002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:17094:
gfx::Size(state_.viewport_width, state_.viewport_height))) {
You want to use offscreen_size_, not the viewport size, which is orthogonal to
the back buffer size.
Actually, what is the expectation relative to the back buffer size, if it was
resized while a separate surface was attached, and we go back to the offscreen
one? I.e. should we reset offscreen_size_ to direct_surface->GetSize() ?

[klausw, 2016-11-10 02:28:08]

Thanks for the explanations, I'm still rather confused by GL's
surface/buffer/binding logic. The open issues mentioned in my previous update
are still open.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:17007: // callers must do so
themselves as needed.
On 2016/11/09 22:26:09, piman wrote:
> On 2016/11/09 21:21:55, klausw wrote:
> > On 2016/11/09 17:50:06, piman wrote:
> > > We should keep a consistent state. If the default framebuffer (0) is bound
> we
> > > should make sure the new surface is taken into account. Among others it
> means
> > we
> > > should MakeCurrent to the new surface.
> > 
> > I've added MakeCurrent and am applying glViewport and glScissor. Any
> additional
> > "among others" it should be doing?
> > 
> > > Beyond this, there are some states whose value depends on whether the
> default
> > > framebuffer is a FBO or a surface - I'm thinking in particular the read
> buffer
> > > and the draw buffers (see DoReadBuffer and DoDrawBuffersEXT). These states
> > need
> > > to be reset to the proper value.
> > 
> > I've attempted to fix this, as far as I can tell I don't need to do
> > anything for a user-bound FBO, GL_NONE remains valid, but I need to
> > map GL_BACK <=> GL_COLOR_ATTACHMENT0. Does that sound right?
> 
> Correct.
> 
> > 
> > On second thought, do I actually need to do this? Lots of places in 
> > the code call CheckBoundDrawFramebufferValid and equivalent, and as
> > far as I can tell that does this fixup internally as a side effect.
> 
> It'll only change the draw buffer if the back buffer needs to be cleared, and
it
> is *not* GL_BACK - it does that so that the draw buffer correctly targets the
> buffer we want to clear, and then resets it. It's an implementation detail of
> that function... but either way, it won't fix up the draw buffer in the
general
> case - nor the read buffer. 
> 
> > 
> > I haven't tested the read buffer, but the draw buffer appeared to work fine
> > without the fixup.
> 

Acknowledged.

https://codereview.chromium.org/2456213002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2456213002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:17050:
context_->MakeCurrent(surface_.get());
On 2016/11/09 22:26:09, piman wrote:
> You need to do the MakeCurrent before the
ApplySurfaceAttributesAndGetAlphaBits,
> otherwise the queries will return properties of the old surface.
> 
> Either way, you want to do the MakeCurrent regardless of whether there's a
bound
> FBO or not, otherwise if the client unbinds the FBO and tries to draw, it'll
go
> to the wrong surface.

Done, I was misunderstanding how MakeCurrent interacts with FBOs. So MakeCurrent
basically maps the default backbuffer destination to the specified surface, even
if you're not currently using the default backbuffer due to a bound FBO?

https://codereview.chromium.org/2456213002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:17052: // dimensions. Re-apply
the settings from the current state.
On 2016/11/09 22:26:09, piman wrote:
> Actually, MakeCurrent only sets viewport/scissor on a new context, not a new
> surface, so I don't think you need this.

Done.

https://codereview.chromium.org/2456213002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:17089:
ApplySurfaceAttributesAndGetAlphaBits(true, attrib_helper_);
On 2016/11/09 22:26:10, piman wrote:
> You probably want to MakeCurrent here to the offscreen surface too. It doesn't
> exactly matter for correctness, but as long as the direct surface is current,
it
> won't be freed.

Done.

https://codereview.chromium.org/2456213002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:17094:
gfx::Size(state_.viewport_width, state_.viewport_height))) {
On 2016/11/09 22:26:09, piman wrote:
> You want to use offscreen_size_, not the viewport size, which is orthogonal to
> the back buffer size.
> Actually, what is the expectation relative to the back buffer size, if it was
> resized while a separate surface was attached, and we go back to the offscreen
> one? I.e. should we reset offscreen_size_ to direct_surface->GetSize() ?

Changed, I think the most intuitive behavior would be to copy the
current size from offscreen to surface on activation and the reverse
on deactivation.

[klausw, 2016-11-11 00:04:10]

Please see suggestions below with a proposal to address the layering and
security issues.

piman@, if possible please let me know ASAP if this sounds reasonable overall or
if you have further questions or concerns, I'm holding off on starting
implementation until I hear back.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:68: #include
"gpu/ipc/common/gpu_surface_lookup.h"
On 2016/11/09 17:50:07, piman wrote:
> So, including gpu/ipc is a layering violation. Indeed, the file needs to work
in
> a context where we don't use IPCs (e.g. for in-process tests).
> 
> One option is to move gpu_surface_lookup to gpu/command_buffer/common, because
> it doesn't really need anything from IPC. That being said, the command buffer
> code will be used in a context where GpuSurfaceLookup::GetInstance is NULL -
> tests in particular - because the production implementation of
GpuSurfaceLookup
> needs things from outside of gpu/. When that is the case, we should make sure
to
> avoid exposing the extension.

I'm a bit confused about the layering violation and how to fix it. How about
something like this:

- create a DirectDrawSurfaceManager class that lives in gpu/ipc/service. It
provides a "GLSurface GetSurface(UnguessableToken)" method.

- add a GetDirectDrawSurfaceManager method to gles2_cmd_decoder, analogous to
its current GetImageManager(). This can return null if no manager is present.

- gpu_command_buffer_stub calls decoder_->SetDirectDrawSurfaceManager(new
DirectDrawSurfaceManager)

Does that sound plausible?

> 
> One other option is to provide this functionality as an IPC instead of a
> command. As an example to follow, you can take a look at how TakeFrontBuffer
is
> routed. It may be easier to provide the security guarantee (see below). A side
> benefit for that is that we could then feed a generic GLSurface to the
decoder,
> avoiding making this an android-specific feature.

As far as I can tell WebKit doesn't currently use direct GpuCommandBuffer IPCs,
so I'm worried this would then create concerns about layer violations elsewhere
or make it necessary to add more platform wrappers.

https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16975: new
gl::NativeViewGLSurfaceEGL(window);
On 2016/11/09 17:50:07, piman wrote:
> So, this is an extremely powerful capability. If this extension is exposed to
a
> generic renderer, and that renderer gets compromized, it can use this API to
> take over the main window and fake the browser UI, which is a security issue.
We
> need a mechanism to make sure the renderer can only access surfaces it should
> have access to. There's essentially 2 ways of doing that. Either:
> 1- provide a mechanism where the browser gives to the GPU process a reference
to
> the window as associated with a given client. Then this would check that the
> client does have access to that window. This would be similar to how we handle
> binding GLImages to GpuMemoryBuffers (see e.g.
> GpuMemoryBufferFactoryIOSurface::CreateImageForGpuMemoryBuffer)
> 2- make the surface handle unguessable. It would need to be a crypto-secure
> identifier that's large enough - base::UnguessableToken is a good primitive
for
> that.

After discussion with Brandon, it sounds best to go with unguessable tokens. As
far as I can tell there's currently no direct IPC channel from the browser
process (where the Surface is created and registered) to the GPU process other
than the Binder/AIDL "fetch surface by ID" method which is initiated from the
GPU process.

This would require a new parallel interface for gpu_surface_lookup using tokens
instead of IDs, basically implementing a new surface namespace that's usable by
less-trusted clients. It would be impossible to retrieve the main window via
token since this would never be registered with a token, so there's no reference
to it available in the token namespace.

At first glance I'll need to add registration and lookup method by token
alongside the existing by-id methods in these files:

    gpu/ipc/common/gpu_surface_lookup
    content/app/android/child_process_service_impl
    content/public/android/java/src/org/chromium/content/browser/
        ChildProcessLauncher.java
    content/public/android/java/src/org/chromium/content/app/
        ChildProcessServiceImpl.java
    content/public/android/java/src/org/chromium/content/common/
        IChildProcessCallback.aidl

This would be similar to forwardSurfaceForSurfaceRequest which already uses
"long requestTokenHigh, long requestTokenLow", so probably best to follow that
example for parameters.

I'm not entirely sure yet how ChildProcessServiceImpl and ChildProcessLauncher
are connected and if I need to edit both.

[piman, 2016-11-11 20:51:06]

On 2016/11/11 00:04:10, klausw wrote:
> Please see suggestions below with a proposal to address the layering and
> security issues.
> 
> piman@, if possible please let me know ASAP if this sounds reasonable overall
or
> if you have further questions or concerns, I'm holding off on starting
> implementation until I hear back.
> 
>
https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:68: #include
> "gpu/ipc/common/gpu_surface_lookup.h"
> On 2016/11/09 17:50:07, piman wrote:
> > So, including gpu/ipc is a layering violation. Indeed, the file needs to
work
> in
> > a context where we don't use IPCs (e.g. for in-process tests).
> > 
> > One option is to move gpu_surface_lookup to gpu/command_buffer/common,
because
> > it doesn't really need anything from IPC. That being said, the command
buffer
> > code will be used in a context where GpuSurfaceLookup::GetInstance is NULL -
> > tests in particular - because the production implementation of
> GpuSurfaceLookup
> > needs things from outside of gpu/. When that is the case, we should make
sure
> to
> > avoid exposing the extension.
> 
> I'm a bit confused about the layering violation and how to fix it. How about
> something like this:
> 
> - create a DirectDrawSurfaceManager class that lives in gpu/ipc/service. It
> provides a "GLSurface GetSurface(UnguessableToken)" method.
> 
> - add a GetDirectDrawSurfaceManager method to gles2_cmd_decoder, analogous to
> its current GetImageManager(). This can return null if no manager is present.
> 
> - gpu_command_buffer_stub calls decoder_->SetDirectDrawSurfaceManager(new
> DirectDrawSurfaceManager)
> 
> Does that sound plausible?

It's one possibility, with 2 things:
1- you need to create an interface for DirectDrawSurfaceManager, that would live
in gpu/command_buffer/service, that GLES2Decoder can know about (implemented by
a class in gpu/ipc/service)
2- you still need to deal with it being null, and/or provide a stub for that
interface for things that run GLES2Decoder in process.


> 
> > 
> > One other option is to provide this functionality as an IPC instead of a
> > command. As an example to follow, you can take a look at how TakeFrontBuffer
> is
> > routed. It may be easier to provide the security guarantee (see below). A
side
> > benefit for that is that we could then feed a generic GLSurface to the
> decoder,
> > avoiding making this an android-specific feature.
> 
> As far as I can tell WebKit doesn't currently use direct GpuCommandBuffer
IPCs,
> so I'm worried this would then create concerns about layer violations
elsewhere
> or make it necessary to add more platform wrappers.

You can add non-commands to the GLES2Interface API (what you use from blink).
That's implemented by GLES2Implementation and it can send IPCs via the
GpuControl interface. See e.g. how CreateImageCHROMIUM is done.

  
> 
>
https://codereview.chromium.org/2456213002/diff/40001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:16975: new
> gl::NativeViewGLSurfaceEGL(window);
> On 2016/11/09 17:50:07, piman wrote:
> > So, this is an extremely powerful capability. If this extension is exposed
to
> a
> > generic renderer, and that renderer gets compromized, it can use this API to
> > take over the main window and fake the browser UI, which is a security
issue.
> We
> > need a mechanism to make sure the renderer can only access surfaces it
should
> > have access to. There's essentially 2 ways of doing that. Either:
> > 1- provide a mechanism where the browser gives to the GPU process a
reference
> to
> > the window as associated with a given client. Then this would check that the
> > client does have access to that window. This would be similar to how we
handle
> > binding GLImages to GpuMemoryBuffers (see e.g.
> > GpuMemoryBufferFactoryIOSurface::CreateImageForGpuMemoryBuffer)
> > 2- make the surface handle unguessable. It would need to be a crypto-secure
> > identifier that's large enough - base::UnguessableToken is a good primitive
> for
> > that.
> 
> After discussion with Brandon, it sounds best to go with unguessable tokens.
As
> far as I can tell there's currently no direct IPC channel from the browser
> process (where the Surface is created and registered) to the GPU process other
> than the Binder/AIDL "fetch surface by ID" method which is initiated from the
> GPU process.

There is an IPC channel from the browser to the GPU process. This is
GpuProcessHost (on the browser side, IO thread) going to GpuChildThread (on the
GPU process side, main thread) 

> 
> This would require a new parallel interface for gpu_surface_lookup using
tokens
> instead of IDs, basically implementing a new surface namespace that's usable
by
> less-trusted clients. It would be impossible to retrieve the main window via
> token since this would never be registered with a token, so there's no
reference
> to it available in the token namespace.

If you go that route, maybe you could simply change the existing
GpuSurfaceLookup API to use unguessable tokens, rather than adding a parallel
one? 

> 
> At first glance I'll need to add registration and lookup method by token
> alongside the existing by-id methods in these files:
> 
>     gpu/ipc/common/gpu_surface_lookup
>     content/app/android/child_process_service_impl
>     content/public/android/java/src/org/chromium/content/browser/
>         ChildProcessLauncher.java
>     content/public/android/java/src/org/chromium/content/app/
>         ChildProcessServiceImpl.java
>     content/public/android/java/src/org/chromium/content/common/
>         IChildProcessCallback.aidl
> 
> This would be similar to forwardSurfaceForSurfaceRequest which already uses
> "long requestTokenHigh, long requestTokenLow", so probably best to follow that
> example for parameters.
> 
> I'm not entirely sure yet how ChildProcessServiceImpl and ChildProcessLauncher
> are connected and if I need to edit both.

[piman, 2016-11-11 23:58:35]

Just updating on the questions below.

On Wed, Nov 9, 2016 at 6:28 PM, <klausw@chromium.org> wrote:

> Thanks for the explanations, I'm still rather confused by GL's
> surface/buffer/binding logic. The open issues mentioned in my previous
> update
> are still open.
>
>
> https://codereview.chromium.org/2456213002/diff/40001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
>
> https://codereview.chromium.org/2456213002/diff/40001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc#newcode17007
> gpu/command_buffer/service/gles2_cmd_decoder.cc:17007: // callers must
> do so themselves as needed.
> On 2016/11/09 22:26:09, piman wrote:
> > On 2016/11/09 21:21:55, klausw wrote:
> > > On 2016/11/09 17:50:06, piman wrote:
> > > > We should keep a consistent state. If the default framebuffer (0)
> is bound
> > we
> > > > should make sure the new surface is taken into account. Among
> others it
> > means
> > > we
> > > > should MakeCurrent to the new surface.
> > >
> > > I've added MakeCurrent and am applying glViewport and glScissor. Any
> > additional
> > > "among others" it should be doing?
> > >
> > > > Beyond this, there are some states whose value depends on whether
> the
> > default
> > > > framebuffer is a FBO or a surface - I'm thinking in particular the
> read
> > buffer
> > > > and the draw buffers (see DoReadBuffer and DoDrawBuffersEXT).
> These states
> > > need
> > > > to be reset to the proper value.
> > >
> > > I've attempted to fix this, as far as I can tell I don't need to do
> > > anything for a user-bound FBO, GL_NONE remains valid, but I need to
> > > map GL_BACK <=> GL_COLOR_ATTACHMENT0. Does that sound right?
> >
> > Correct.
> >
> > >
> > > On second thought, do I actually need to do this? Lots of places in
> > > the code call CheckBoundDrawFramebufferValid and equivalent, and as
> > > far as I can tell that does this fixup internally as a side effect.
> >
> > It'll only change the draw buffer if the back buffer needs to be
> cleared, and it
> > is *not* GL_BACK - it does that so that the draw buffer correctly
> targets the
> > buffer we want to clear, and then resets it. It's an implementation
> detail of
> > that function... but either way, it won't fix up the draw buffer in
> the general
> > case - nor the read buffer.
> >
> > >
> > > I haven't tested the read buffer, but the draw buffer appeared to
> work fine
> > > without the fixup.
> >
>
> Acknowledged.
>
> https://codereview.chromium.org/2456213002/diff/60001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
>
> https://codereview.chromium.org/2456213002/diff/60001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc#newcode17050
> gpu/command_buffer/service/gles2_cmd_decoder.cc:17050:
> context_->MakeCurrent(surface_.get());
> On 2016/11/09 22:26:09, piman wrote:
> > You need to do the MakeCurrent before the
> ApplySurfaceAttributesAndGetAlphaBits,
> > otherwise the queries will return properties of the old surface.
> >
> > Either way, you want to do the MakeCurrent regardless of whether
> there's a bound
> > FBO or not, otherwise if the client unbinds the FBO and tries to draw,
> it'll go
> > to the wrong surface.
>
> Done, I was misunderstanding how MakeCurrent interacts with FBOs. So
> MakeCurrent basically maps the default backbuffer destination to the
> specified surface, even if you're not currently using the default
> backbuffer due to a bound FBO?
>

That's correct. The default framebuffer is special and a bit anachronistic
(well you have to go back to the origins of GL to understand where it comes
from), but yeah, you make a context current together with a surface, and
that's what defines the bind buffer when no FBO is bound (i.e. you bind 0).

>
>
> https://codereview.chromium.org/2456213002/diff/60001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc#newcode17052
> gpu/command_buffer/service/gles2_cmd_decoder.cc:17052: // dimensions.
> Re-apply the settings from the current state.
> On 2016/11/09 22:26:09, piman wrote:
> > Actually, MakeCurrent only sets viewport/scissor on a new context, not
> a new
> > surface, so I don't think you need this.
>
> Done.
>
> https://codereview.chromium.org/2456213002/diff/60001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc#newcode17089
> gpu/command_buffer/service/gles2_cmd_decoder.cc:17089:
> ApplySurfaceAttributesAndGetAlphaBits(true, attrib_helper_);
> On 2016/11/09 22:26:10, piman wrote:
> > You probably want to MakeCurrent here to the offscreen surface too. It
> doesn't
> > exactly matter for correctness, but as long as the direct surface is
> current, it
> > won't be freed.
>
> Done.
>
> https://codereview.chromium.org/2456213002/diff/60001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc#newcode17094
> gpu/command_buffer/service/gles2_cmd_decoder.cc:17094:
> gfx::Size(state_.viewport_width, state_.viewport_height))) {
> On 2016/11/09 22:26:09, piman wrote:
> > You want to use offscreen_size_, not the viewport size, which is
> orthogonal to
> > the back buffer size.
> > Actually, what is the expectation relative to the back buffer size, if
> it was
> > resized while a separate surface was attached, and we go back to the
> offscreen
> > one? I.e. should we reset offscreen_size_ to direct_surface->GetSize()
> ?
>
> Changed, I think the most intuitive behavior would be to copy the
> current size from offscreen to surface on activation and the reverse
> on deactivation.
>

Sounds very reasonable, thanks!


>
> https://codereview.chromium.org/2456213002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[klausw, 2017-01-05 20:36:11]

Description was changed from

==========
WebVR: implement SetSurfaceHandleCHROMIUM extension for gvr_device.

This extension supports attaching an offscreen GL rendering context
to a native Android Surface for drawing, and restoring it to the
original offscreen context again later.

This is based on bajones@'s "Temp plumbing for the surface swap" patch.

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

to

==========
WebVR: implement SetSurfaceHandleCHROMIUM extension for gvr_device.

This extension supports attaching an offscreen GL rendering context
to a native Android Surface for drawing, and restoring it to the
original offscreen context again later.

This is based on bajones@'s "Temp plumbing for the surface swap" patch.

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