Pass GL context bind_generates_resources flag to GPU Service.

Pass GL context bind_generates_resources flag to GPU Service.

GPU Client has been respecting the bind_generates_resources attrib,
however the GPU Service has been forcing it to always 'true'.  This
means glBind semantic validation has been incorrect when
bind_generates_resources is 'false' on the Client.

This patch passes the bind_generates_resources attrib for
WebGraphicsContext3DCommandBufferImpl, and checks that all contexts
in the same ContextGroup use the same setting.

BUG=333063, 244968
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261563

[vmiura, 2014-04-01 21:10:01]

PTAL.

[danakj, 2014-04-01 21:18:34]

Can we unit-test this?

[Ken Russell (switch to Gerrit), 2014-04-01 21:29:38]

I agree with Dana's comment about unit testing this change. It's a subtle change
and should be tested thoroughly.

https://codereview.chromium.org/213743003/diff/20001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/213743003/diff/20001/content/common/gpu/gpu_c...
content/common/gpu/gpu_command_buffer_stub.cc:149:
attrib_parser.bind_generates_resource_);
How much testing has this change undergone? Have you verified with a debug build
that ordinary composited content, accelerated 2D canvas content, WebGL content,
Pepper 3D content, etc. all works?

https://codereview.chromium.org/213743003/diff/20001/content/common/gpu/gpu_c...
content/common/gpu/gpu_command_buffer_stub.cc:156:
attrib_parser.bind_generates_resource_);  // need to pass this in...
I don't think the comment adds any value.

Upon further thought, I'd be concerned that some of the client-side contexts in
the same share group might set BIND_GENERATES_RESOURCES and some might not.
Previously this would silently work, since the value coming in from the client
was ignored. Do you know definitively whether or not this is the case?

[piman, 2014-04-01 22:18:11]

LGTM modulo tests.

I did make a cursory pass over the places we create contexts in production. The
only place we set bind_generate_textures is for Pepper contexts, which don't
share with chrome contexts (compositor/skia/webgl/video/gl_helper/...?). The
other ones set it to false.

Maybe one way to convince ourselves would be to remove the
bind_generate_textures option from WebGraphicsContext3DCommandBufferImpl (and
InProcess), forcing us to check all call sites.

[vmiura, 2014-04-01 22:21:49]

On 2014/04/01 21:29:38, Ken Russell wrote:
> I agree with Dana's comment about unit testing this change. It's a subtle
change
> and should be tested thoroughly.
> 
>
https://codereview.chromium.org/213743003/diff/20001/content/common/gpu/gpu_c...
> File content/common/gpu/gpu_command_buffer_stub.cc (right):
> 
>
https://codereview.chromium.org/213743003/diff/20001/content/common/gpu/gpu_c...
> content/common/gpu/gpu_command_buffer_stub.cc:149:
> attrib_parser.bind_generates_resource_);
> How much testing has this change undergone? Have you verified with a debug
build
> that ordinary composited content, accelerated 2D canvas content, WebGL
content,
> Pepper 3D content, etc. all works?
> 
>
https://codereview.chromium.org/213743003/diff/20001/content/common/gpu/gpu_c...
> content/common/gpu/gpu_command_buffer_stub.cc:156:
> attrib_parser.bind_generates_resource_);  // need to pass this in...
> I don't think the comment adds any value.
> 
> Upon further thought, I'd be concerned that some of the client-side contexts
in
> the same share group might set BIND_GENERATES_RESOURCES and some might not.
> Previously this would silently work, since the value coming in from the client
> was ignored. Do you know definitively whether or not this is the case?

I agree this is subtle but could have unintended side effects.

The current production mode has been 'bind_generates_resource == true' on
Service side, except
it seems in the InProcessCommandBuffer.

However all our WebGraphicsContext3DCommandBufferImpl actually expect it to
behave as 'bind_generates_resource == false'.

We have already some decoder unit tests for 'bind_generates_resource == false'.
https://codereview.chromium.org/154263011/diff/160001/gpu/command_buffer/serv...

Dana mentioned we have gpu_ipc_browsertests, where we could end-to-end test this
"attrib" setting, but they all fail?

[danakj, 2014-04-01 22:30:39]

On Tue, Apr 1, 2014 at 6:21 PM, <vmiura@chromium.org> wrote:

> On 2014/04/01 21:29:38, Ken Russell wrote:
>
>> I agree with Dana's comment about unit testing this change. It's a subtle
>>
> change
>
>> and should be tested thoroughly.
>>
>
>
> https://codereview.chromium.org/213743003/diff/20001/
> content/common/gpu/gpu_command_buffer_stub.cc
>
>> File content/common/gpu/gpu_command_buffer_stub.cc (right):
>>
>
>
> https://codereview.chromium.org/213743003/diff/20001/
> content/common/gpu/gpu_command_buffer_stub.cc#newcode149
>
>> content/common/gpu/gpu_command_buffer_stub.cc:149:
>> attrib_parser.bind_generates_resource_);
>> How much testing has this change undergone? Have you verified with a debug
>>
> build
>
>> that ordinary composited content, accelerated 2D canvas content, WebGL
>>
> content,
>
>> Pepper 3D content, etc. all works?
>>
>
>
> https://codereview.chromium.org/213743003/diff/20001/
> content/common/gpu/gpu_command_buffer_stub.cc#newcode156
>
>> content/common/gpu/gpu_command_buffer_stub.cc:156:
>> attrib_parser.bind_generates_resource_);  // need to pass this in...
>> I don't think the comment adds any value.
>>
>
>  Upon further thought, I'd be concerned that some of the client-side
>> contexts
>>
> in
>
>> the same share group might set BIND_GENERATES_RESOURCES and some might
>> not.
>> Previously this would silently work, since the value coming in from the
>> client
>> was ignored. Do you know definitively whether or not this is the case?
>>
>
> I agree this is subtle but could have unintended side effects.
>
> The current production mode has been 'bind_generates_resource == true' on
> Service side, except
> it seems in the InProcessCommandBuffer.
>
> However all our WebGraphicsContext3DCommandBufferImpl actually expect it
> to
> behave as 'bind_generates_resource == false'.
>
> We have already some decoder unit tests for 'bind_generates_resource ==
> false'.
> https://codereview.chromium.org/154263011/diff/160001/gpu/
> command_buffer/service/gles2_cmd_decoder_unittest.cc
>
> Dana mentioned we have gpu_ipc_browsertests, where we could end-to-end
> test this
> "attrib" setting, but they all fail?
>

Yeh, they were written to test the process of creating a context and that
launching a GPU process correctly and such. If we can avoid writing a
browser test that'd be really nice.

I guess GpuChannelManager is the IPC entry point. Naively, could a test
make a GpuChannel, and then call GpuChannel::CreateViewCommandBuffer() and
then LookupCommandBuffer() and ensure it has the right state?
<https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...>
>
>
>
> https://codereview.chromium.org/213743003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vmiura, 2014-04-01 22:32:06]

https://codereview.chromium.org/213743003/diff/20001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/213743003/diff/20001/content/common/gpu/gpu_c...
content/common/gpu/gpu_command_buffer_stub.cc:149:
attrib_parser.bind_generates_resource_);
On 2014/04/01 21:29:38, Ken Russell wrote:
> How much testing has this change undergone? Have you verified with a debug
build
> that ordinary composited content, accelerated 2D canvas content, WebGL
content,
> Pepper 3D content, etc. all works?

I have manually tested these cases, and observed bind_generates_resource == true
only in the Pepper 3D case.

[vmiura, 2014-04-02 22:12:31]

PTAL.

I've added a GL_BIND_GENERATES_RESOURCE_CHROMIUM state that the client can
query.  GLES2Implementation::Initialize fails if client & service side
bind_generates_resource is different.  This should catch consistency problems
regardless of where the context is created (WGC3DCBImpl, InProc, mojo, etc.).

All GLES2ImplementationTest and full GPU tests will test this path.

Added FailInitOnBGRMismatch1/FailInitOnBGRMismatch2 tests to make sure the check
itself is tested.

https://codereview.chromium.org/213743003/diff/20001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/213743003/diff/20001/content/common/gpu/gpu_c...
content/common/gpu/gpu_command_buffer_stub.cc:156:
attrib_parser.bind_generates_resource_);  // need to pass this in...
On 2014/04/01 21:29:38, Ken Russell wrote:
> I don't think the comment adds any value.
> 
> Upon further thought, I'd be concerned that some of the client-side contexts
in
> the same share group might set BIND_GENERATES_RESOURCES and some might not.
> Previously this would silently work, since the value coming in from the client
> was ignored. Do you know definitively whether or not this is the case?

Removed comment.

[piman, 2014-04-02 22:23:00]

https://codereview.chromium.org/213743003/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/213743003/diff/100001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:358:
attribs.push_back(bind_generates_resources_ ? 1 : 0);
nit: how about removing bind_generates_resources_ and the parameter from the
constructor, since all clients must be setting this to false already.

[vmiura, 2014-04-02 22:44:24]

https://codereview.chromium.org/213743003/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/213743003/diff/100001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:358:
attribs.push_back(bind_generates_resources_ ? 1 : 0);
On 2014/04/02 22:23:00, piman wrote:
> nit: how about removing bind_generates_resources_ and the parameter from the
> constructor, since all clients must be setting this to false already.

Done.

[piman, 2014-04-02 23:25:49]

Awesome, LGTM!

[vmiura, 2014-04-03 14:38:30]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2014-04-03 14:39:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmiura@chromium.org/213743003/130001

[commit-bot: I haz the power, 2014-04-03 18:25:48]

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

[commit-bot: I haz the power, 2014-04-03 18:25:48]

Failed to apply patch for content/common/gpu/gpu_command_buffer_stub.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/common/gpu/gpu_command_buffer_stub.cc
  Hunk #1 FAILED at 139.
  1 out of 1 hunk FAILED -- saving rejects to file
content/common/gpu/gpu_command_buffer_stub.cc.rej

Patch:       content/common/gpu/gpu_command_buffer_stub.cc
Index: content/common/gpu/gpu_command_buffer_stub.cc
diff --git a/content/common/gpu/gpu_command_buffer_stub.cc
b/content/common/gpu/gpu_command_buffer_stub.cc
index
b26757559392e78ff52656e50b5aafcf1d7b53f1..2becb4b9323a452afd2dea329144336e592752a6
100644
--- a/content/common/gpu/gpu_command_buffer_stub.cc
+++ b/content/common/gpu/gpu_command_buffer_stub.cc
@@ -139,15 +139,21 @@ GpuCommandBufferStub::GpuCommandBufferStub(
       total_gpu_memory_(0) {
   active_url_hash_ = base::Hash(active_url.possibly_invalid_spec());
   FastSetActiveURL(active_url_, active_url_hash_);
+
+  gpu::gles2::ContextCreationAttribHelper attrib_parser;
+  attrib_parser.Parse(requested_attribs_);
+
   if (share_group) {
     context_group_ = share_group->context_group_;
+    DCHECK(context_group_->bind_generates_resource() ==
+           attrib_parser.bind_generates_resource_);
   } else {
-    context_group_ = new gpu::gles2::ContextGroup(
-        mailbox_manager,
-        image_manager,
-        new GpuCommandBufferMemoryTracker(channel),
-        NULL,
-        true);
+    context_group_ =
+        new gpu::gles2::ContextGroup(mailbox_manager,
+                                     image_manager,
+                                     new
GpuCommandBufferMemoryTracker(channel),
+                                     NULL,
+                                     attrib_parser.bind_generates_resource_);
   }
 
   use_virtualized_gl_context_ |=

[vmiura, 2014-04-03 20:17:23]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2014-04-03 20:20:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmiura@chromium.org/213743003/150001

[commit-bot: I haz the power, 2014-04-03 23:00:53]

Change committed as 261563