WebVR: implement SetSurfaceHandleCHROMIUM extension for gvr_device.

gpu/command_buffer/service/gles2_cmd_decoder.cc

Index: gpu/command_buffer/service/gles2_cmd_decoder.cc
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder.cc b/gpu/command_buffer/service/gles2_cmd_decoder.cc
index c480b71567dca3814c19aeca0b4f7185bc338e42..eeee706c51c834c111fed9093c0c1f7f88314883 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -65,6 +65,7 @@
 #include "gpu/command_buffer/service/transform_feedback_manager.h"
 #include "gpu/command_buffer/service/vertex_array_manager.h"
 #include "gpu/command_buffer/service/vertex_attrib_manager.h"
+#include "gpu/ipc/common/gpu_surface_lookup.h"

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

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.

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

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

 #include "third_party/angle/src/image_util/loadimage.h"
 #include "third_party/smhasher/src/City.h"
 #include "ui/gfx/buffer_types.h"
@@ -82,6 +83,7 @@
 #include "ui/gl/gl_image.h"
 #include "ui/gl/gl_implementation.h"
 #include "ui/gl/gl_surface.h"
+#include "ui/gl/gl_surface_egl.h"

[piman, 2016/11/09 17:50:06]

I believe we can't include this on Mac (EGL is not present there). Maybe
consider including only on Android.

[klausw, 2016/11/09 21:21:55]

Done, now Android only.

 #include "ui/gl/gl_version_info.h"
 #include "ui/gl/gpu_timing.h"
 
@@ -1034,6 +1036,8 @@ class GLES2DecoderImpl : public GLES2Decoder, public ErrorStateClient {
   void DoFlushMappedBufferRange(
       GLenum target, GLintptr offset, GLsizeiptr size);
 
+  void DoSetSurfaceHandleCHROMIUM(GLint surface_handle);
+
   // Creates a Program for the given program.
   Program* CreateProgram(GLuint client_id, GLuint service_id) {
     return program_manager()->CreateProgram(client_id, service_id);
@@ -2131,6 +2135,17 @@ class GLES2DecoderImpl : public GLES2Decoder, public ErrorStateClient {
   scoped_refptr<gl::GLSurface> surface_;
   scoped_refptr<gl::GLContext> context_;
 
+  // Direct render mode data for switching and restoring the
+  // underlying destination surface, cf. DoSetSurfaceHandleCHROMIUM.
+  struct DirectRenderState{
+    scoped_refptr<gl::GLSurface> direct_surface;
+    scoped_refptr<gl::GLSurface> orig_surface;
+    std::unique_ptr<BackFramebuffer> orig_offscreen_target_frame_buffer;
+    bool orig_supports_async_swap;
+    bool orig_back_buffer_has_depth;

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

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.

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

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();

+  };
+  std::unique_ptr<DirectRenderState> direct_render_state_;
+
   // The ContextGroup for this decoder uses to track resources.
   scoped_refptr<ContextGroup> group_;
 
@@ -16937,6 +16952,85 @@ void GLES2DecoderImpl::DoApplyScreenSpaceAntialiasingCHROMIUM() {
   }
 }
 
+void GLES2DecoderImpl::DoSetSurfaceHandleCHROMIUM(GLint surface_handle) {
+#if defined(OS_ANDROID)
+  if (surface_handle) {
+    // Use the specified surface as a render target, and save decoder
+    // state so that this can be reverted later.
+
+    // Ignore redundant calls. For now we don't support switching between
+    // different surfaces, if you need this you have to make a call with
+    // surface_handle=0 in between.

[piman, 2016/11/09 17:50:06]

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.

[klausw, 2016/11/09 21:21:55]

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

+    if (direct_render_state_) {
+      return;
+    }
+
+    direct_render_state_.reset(new DirectRenderState());
+
+    // One-time initialization when first activating a direct draw surface.
+    ANativeWindow* window =
+        gpu::GpuSurfaceLookup::GetInstance()->AcquireNativeWidget(
+            surface_handle);

[piman, 2016/11/09 17:50:06]

This could be NULL? How should we handle this?

[klausw, 2016/11/09 21:21:55]

Changed to raise an error.

+    direct_render_state_->direct_surface =
+        new gl::NativeViewGLSurfaceEGL(window);

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

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.

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

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.

+
+    // It would be useful if we could get a custom surface format such
+    // as one with a depth buffer here, but the new surface must use
+    // the same format as the one originally used for creating the GL
+    // context. Anything else just results in BAD_MATCH errors.
+    bool initialize_success = direct_render_state_->direct_surface->Initialize(
+        surface_->GetFormat());
+    if (!initialize_success) {
+      LOG(ERROR) << "Direct render surface init failed for handle " <<
+          surface_handle;
+    }
+    ANativeWindow_release(window);
+
+    direct_render_state_->orig_surface = surface_;
+
+    // We don't want async swap since we want full control over when the
+    // SurfaceTexture gets updated.

[piman, 2016/11/09 17:50:06]

FYI, "async" swap doesn't remove any "control", it just lets the implementation
free the main thread (using a callback) instead of blocking.

[klausw, 2016/11/09 21:21:55]

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

+    direct_render_state_->orig_supports_async_swap = supports_async_swap_;
+    supports_async_swap_ = false;

[piman, 2016/11/09 17:50:06]

what about other properties of the surface, like supports_post_sub_buffer_,
supports_swap_buffers_with_damage_, supports_commit_overlay_planes_?

[klausw, 2016/11/09 21:21:55]

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

+    direct_render_state_->orig_back_buffer_has_depth = back_buffer_has_depth_;
+    back_buffer_has_depth_ = true;

[piman, 2016/11/09 17:50:06]

What makes this true?

[klausw, 2016/11/09 21:21:55]

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.

+    // override back_buffer_has_stencil_ too?

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

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.

[klausw, 2016/11/09 21:21:55]

Should be fixed now.

+
+    // Offscreen surfaces don't support SwapBuffers, make sure it's not
+    // considered offscreen. Various code in this file checks for is_offscreen
+    // = !!offscreen_target_frame_buffer_.get().
+    direct_render_state_->orig_offscreen_target_frame_buffer =
+        std::move(offscreen_target_frame_buffer_);
+    offscreen_target_frame_buffer_.reset(nullptr);
+
+    // Activate the surface. This does *not* rebind the active framebuffer,
+    // callers must do so themselves as needed.

[piman, 2016/11/09 17:50:06]

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.

[klausw, 2016/11/09 21:21:55]

I've added MakeCurrent and am applying glViewport and glScissor. Any additional
"among others" it should be doing?
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:09]

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

[klausw, 2016/11/10 02:28:07]

Acknowledged.

+    SetSurface(direct_render_state_->direct_surface);
+  } else {
+    // Restore original state for drawing offscreen.
+
+    // Ignore redundant calls.
+    if (!direct_render_state_) return;
+
+    supports_async_swap_ = direct_render_state_->orig_supports_async_swap;
+    back_buffer_has_depth_ = direct_render_state_->orig_back_buffer_has_depth;
+    offscreen_target_frame_buffer_ =
+        std::move(direct_render_state_->orig_offscreen_target_frame_buffer);
+
+    SetSurface(direct_render_state_->orig_surface);
+
+    // Trust the state struct's destructors to handle refcounting for
+    // surfaces and trigger apprpriate cleanup.
+    direct_render_state_.reset();
+  }
+#else  // !OS_ANDROID
+  LOCAL_SET_GL_ERROR(
+      GL_INVALID_OPERATION,
+      "glSetSurfaceHandleCHROMIUM",
+      "Direct render surface is unsupported on this platform");
+#endif
+}
+
 void GLES2DecoderImpl::DoInsertEventMarkerEXT(
     GLsizei length, const GLchar* marker) {
   if (!marker) {