Explicitly disable all attribs before drawing in CopyTextureCHROMIUM

Explicitly disable all attribs and reset all attrib divisors before drawing in CopyTextureCHROMIUM

BUG=342439, 304927
R=kbr@chromium.org, zmo@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250827

[bajones, 2014-02-11 18:54:49]

I'd actually prefer to use a VAO to manage the buffer/attrib state isolation
here, but since it's not supported in core ES2 we would want to use the decoder
version instead, which supports emulation if the feature isn't supported in
hardware. Unfortunately there's no good way to call the required functions on
the DecoderImpl from this location, and I'm not sure if it's worth hacking in an
awkward back door to expose it.

In the meantime this solution isn't terribly elegant but it does fix the
problem.

[Zhenyao Mo, 2014-02-11 19:01:20]

https://codereview.chromium.org/157033009/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://codereview.chromium.org/157033009/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:391: GLint
maxAttribs = 0;
name should be max_attribs

https://codereview.chromium.org/157033009/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:402: for(int i =
1; i < maxAttribs; ++i)
space after for

https://codereview.chromium.org/157033009/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:403:
glDisableVertexAttribArray(i);
Here you should not make the assumption kVertexPositionAttrib.

Instead, it should be 

for (int i = 0; ...)
   if (i != kVertexPositionAttrib)
       glDisable...

https://codereview.chromium.org/157033009/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:429: for(int i =
0; i < maxAttribs; ++i)
space after for

[bajones, 2014-02-11 19:08:26]

Thanks! All feedback addressed.

[Zhenyao Mo, 2014-02-11 19:17:26]

LGTM

[Ken Russell (switch to Gerrit), 2014-02-11 23:15:23]

I was going to object to iterating over all of the vertex attribs before
discovering that a relatively recent MacBook Pro only supports 16 vertex
attributes.

If this number were much larger then I would argue for adding new methods to
ContextState and GLES2Decoder which could disable all the active attrib arrays
and re-enable just those.

LGTM

https://codereview.chromium.org/157033009/diff/100001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://codereview.chromium.org/157033009/diff/100001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:405: }
The vertex attribute divisor also needs to be set to 0 for
kVertexPositionAttrib. That's almost certainly the cause of the other,
non-crasher, bug. It will be restored in RestoreAttribute.

[bajones, 2014-02-12 19:01:45]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-02-12 19:04:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/157033009/180001

[bajones, 2014-02-12 23:16:42]

Committed patchset #3 manually as r250827 (presubmit successful).