Addition of GL_CHROMIUM_copy_texture extension.

Introduction of CHROMIUM_copy_texture extension that respects pixel-store semantics, and allows copying of BGRA textures.

OpenGL ES does not natively allow for copying textures with a BGRA format. The EXT_texture_format_BGRA8888 extension does not specify support for glCopyTexImage calls on these textures.

This extension provides a routine to perform texture copies to/from BGRA-backed textures that also respects the following CHROMIUM pixel storage modifiers:
UNPACK_FLIP_Y_CHROMIUM
UNPACK_PREMULTIPLY_ALPHA_CHROMIUM

This extension will be useful for the following purposes:
- Copying accelerated Canvas2D contents to WebGL textures without a software readback. (And potentially the same for video-webgl texture copies.)
- Copying Canvas2D contents to the compositor backing store.

BUG=101051
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132934
Reverted: https://chromiumcodereview.appspot.com/10078006

[Jeff Timanus, 2012-04-04 15:58:32]

Hi Gregg & Al,

Can you guys please take a look at this?  It is a first pass at an extension
that allows copying BGRA textures on ES contexts.

Some points that I feel are already worth discussing:
 - The link between the state that is corrupted by the extension invocation and
the restoration of the state is a little brittle.  I considered adding a
push-like routine to the extension class so that it could receive the current
state, and restore it, but I decided to use the current state helper functions
instead.
 - I would like to test this routine.  Can either of you provide some direction
on how to best test this?
 - I am pushing some WebGL-specific pixel-storei parameters into the service: 
UNPACK_PREMULTIPLY_ALPHA_WEBGL, and UNPACK_COLORSPACE_CONVERSION_WEBGL.  In this
patch, only the premultiply alpha storage setting is respected by the copy.

 - I arbitrarily chose to force the behaviour of the routine to read from level
0 of the source texture, and allow specification of the level of the destination
texture.
 - Cube-map faces are not supported.  Should they be?

Thanks,

Jeff

[apatrick_chromium, 2012-04-04 19:40:14]

I haven't looked at this in detail yet but, for the purpose of copying a BGRA
texture into a WebGL texture, why can't the renderer process attach the WebGL
texture (which will not be BGRA) to an FBO and then draw the BGRA texture into
it, optionally flipping by switching the v texture coord and optionally
premultiplying in the fragment shader or with alpha blending? I guess I don't
see what the GPU process can do here that the renderer process can't. Probably
missing something obvious though :)

Al

[greggman, 2012-04-04 20:12:24]

unit tests?

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:7: //#include
"gpu/command_buffer/common/gles2_cmd_format.h"
remove?

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:169: for (int
shader = 0; shader < kNumShaders; ++shader)
Once you've linked the programs there's no need to keep the shaders is there? If
not you could delete the shaders in Initialized and then there's no reason to
store their ids in CopyTextureCHROMIUMResourceManager

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:216:
glUniform1i(sampler_locations_[program], 0);
do you need to call glActiveTexture?

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6071: viewport_x_ = x;
The viewport will be clipped to the max size and I believe negative values are
not allowed which means you probably need to validate that stuff here otherwise
you'll record one set of values that gl will reject and the 2 will not be in
sync.

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8381:
SetGLError(GL_INVALID_VALUE, "");
Do you mind filling out the "" here and below?

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8403: if (dest_width !=
source_width && dest_height != source_height) {
Should it be ok to copy to a new size? CopyTexImage does.
You could just call glTexImage2D here with NULL and check for error if the dest
size is different? You'll need to update the level info for that level.

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8427: ApplyDirtyState();
I get the impression more things need to be restored here. If nothing else
vertex attribs and texture bindings are not being restored AFAICT.

[Jeff Timanus, 2012-04-04 20:21:10]

On 2012/04/04 19:40:14, apatrick_chromium wrote:
> I haven't looked at this in detail yet but, for the purpose of copying a BGRA
> texture into a WebGL texture, why can't the renderer process attach the WebGL
> texture (which will not be BGRA) to an FBO and then draw the BGRA texture into
> it, optionally flipping by switching the v texture coord and optionally
> premultiplying in the fragment shader or with alpha blending? I guess I don't
> see what the GPU process can do here that the renderer process can't. Probably
> missing something obvious though :)

Al, thanks for taking a brief look.  You're not necessarily missing anything
obvious.

The above could be implemented in the renderer process.  

Since you mention alpha-blending, I think that is a state that I need to save
and restore as well.  I chose to use a shader for these tasks because I thought
the uniformity was simpler.

Some months ago, before this work was back-burnered, it was suggested on a
thread that it would be useful to be able to copy arbitrary textures.  

See
https://groups.google.com/a/google.com/forum/?fromgroups#!searchin/chrome-gpu...

The benefit of having this path in the gpu process is that by having it here,
all of the state management is consolidated in a single place.  If this path
were implemented in the renderer, and used at different call sites
(Canvas2D->WebGL, Canvas2D->compositor backing store), then tracking of the
affected state would be more spread out.

Jeff

> 
> Al

[apatrick_chromium, 2012-04-04 20:33:29]

Scissor rectangle as well off the top of my head.

Have you seen this extension? I think it might be possible to implement it
instead. It doesn't bypass the fragment pipeline though.

http://www.khronos.org/registry/gles/extensions/OES/OES_draw_texture.txt

Also, gman and I are considering implementing these as an alternative to putting
everything in the same share group client side, which might throw a wrench in
things:


http://www.khronos.org/registry/egl/extensions/KHR/EGL_KHR_image.txt
http://www.khronos.org/registry/egl/extensions/KHR/EGL_KHR_gl_image.txt
http://www.khronos.org/registry/gles/extensions/OES/OES_EGL_image_external.txt
http://www.khronos.org/registry/gles/extensions/OES/OES_EGL_image.txt

[Jeff Timanus, 2012-04-04 21:43:57]

On 2012/04/04 20:33:29, apatrick_chromium wrote:
> Scissor rectangle as well off the top of my head.

I disable the scissor test, and it is re-enabled in
GLES2DecoderImpl::ApplyDirtyState.  I shouldn't need to touch the actual
rectangle, no?

> 
> Have you seen this extension? I think it might be possible to implement it
> instead. It doesn't bypass the fragment pipeline though.

The extension looks vaguely familiar, but I hadn't reviewed it in the context of
this work.  It looks like the extension only draws to the screen, so clients
would still have to setup the FBO state to perform a texture copy.  If that
extension was used, the client would also have to build shaders to respect the
pixel storage modifiers.

Performing a texture copy seems simpler, but only marginally so.

> 
> http://www.khronos.org/registry/gles/extensions/OES/OES_draw_texture.txt
> 
> Also, gman and I are considering implementing these as an alternative to
putting
> everything in the same share group client side, which might throw a wrench in
> things:
> 
> 
> http://www.khronos.org/registry/egl/extensions/KHR/EGL_KHR_image.txt
> http://www.khronos.org/registry/egl/extensions/KHR/EGL_KHR_gl_image.txt
> http://www.khronos.org/registry/gles/extensions/OES/OES_EGL_image_external.txt
> http://www.khronos.org/registry/gles/extensions/OES/OES_EGL_image.txt

The original extension that Gregg indicated to me was NV_copy_image, which
allows for copying textures across contexts.  Would there be a way to extend the
extension I'm working on to pass in a context identifier, so that if sharing
were disabled, the copy would still work?

I reviewed these extensions briefly:  Will they resolve all of our bgra issues,
or will that still be a problem?

[Jeff Timanus, 2012-04-04 22:34:14]

I've addressed most of the comments.

I'm adding a new unit test that will ensure that the state is not corrupted, and
that the pixel storage modifiers are respected.

The current failures on the set of trys for patch 0 are due to the extra GL
calls during decoder construction.

Jeff

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:7: //#include
"gpu/command_buffer/common/gles2_cmd_format.h"
On 2012/04/04 20:12:24, greggman wrote:
> remove?

Done.

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:169: for (int
shader = 0; shader < kNumShaders; ++shader)
On 2012/04/04 20:12:24, greggman wrote:
> Once you've linked the programs there's no need to keep the shaders is there?
If
> not you could delete the shaders in Initialized and then there's no reason to
> store their ids in CopyTextureCHROMIUMResourceManager

Thanks for the suggestion.  Done.

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:216:
glUniform1i(sampler_locations_[program], 0);
On 2012/04/04 20:12:24, greggman wrote:
> do you need to call glActiveTexture?

Yes!  I do need to call glActiveTexture!

Done.

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6071: viewport_x_ = x;
On 2012/04/04 20:12:24, greggman wrote:
> The viewport will be clipped to the max size and I believe negative values are
> not allowed which means you probably need to validate that stuff here
otherwise
> you'll record one set of values that gl will reject and the 2 will not be in
> sync.

The arguments are validated against 0 in the auto-generated viewport handler. 
However, they are not clipped.

Done.

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8381:
SetGLError(GL_INVALID_VALUE, "");
On 2012/04/04 20:12:24, greggman wrote:
> Do you mind filling out the "" here and below?

Done.

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8403: if (dest_width !=
source_width && dest_height != source_height) {
On 2012/04/04 20:12:24, greggman wrote:
> Should it be ok to copy to a new size? CopyTexImage does.
> You could just call glTexImage2D here with NULL and check for error if the
dest
> size is different? You'll need to update the level info for that level.

It should be ok to copy to a new size.  I'm looking through the texture manager
code now to see the easiest way to update the size information for a level.

I'm thinking that the semantics will be that if the level doesn't exist at all,
that will be an error, but if the level exists, but is not the correct size,
then the level will be resized.  Reasonable?

https://chromiumcodereview.appspot.com/9968113/diff/7001/gpu/command_buffer/s...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8427: ApplyDirtyState();
On 2012/04/04 20:12:24, greggman wrote:
> I get the impression more things need to be restored here. If nothing else
> vertex attribs and texture bindings are not being restored AFAICT.

Ongoing.  I'll add the extra state tracking to double check that everything is
being restored properly.

[apatrick_chromium, 2012-04-05 21:58:39]

https://chromiumcodereview.appspot.com/9968113/diff/15001/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/15001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:99:
FreeResources();
What if the wrong GL context is current when these objects are destroyed?

https://chromiumcodereview.appspot.com/9968113/diff/15001/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/15001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8418: source_height, 0, GL_RGBA,
type, NULL);
GL_RGBA -> internal_format. I don't know of any extensions that make it valid
for them to be different.

https://chromiumcodereview.appspot.com/9968113/diff/15001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8433:
glUseProgram(current_program_->service_id());
If there was no program in use before, should probably glUnuseProgram the one
that DoCopyTexture used.

https://chromiumcodereview.appspot.com/9968113/diff/15001/third_party/khronos...
File third_party/khronos/GLES2/gl2ext.h (right):

https://chromiumcodereview.appspot.com/9968113/diff/15001/third_party/khronos...
third_party/khronos/GLES2/gl2ext.h:1963: #ifndef
UNPACK_COLORSPACE_CONVERSION_CHROMIUM
These are missing GL_ prefixes.

[Jeff Timanus, 2012-04-13 00:13:29]

Thanks for your reviews.

I saw the GL-unittest change went through.  Would it be possible to land this
change, and then iterate with a unit-test?

This change should introduce no regressions in any of the current tests.

https://chromiumcodereview.appspot.com/9968113/diff/15001/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/15001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:99:
FreeResources();
On 2012/04/05 21:58:39, apatrick_chromium wrote:
> What if the wrong GL context is current when these objects are destroyed?

That's a good point.  This class is presently destroyed in ~GLES2DecoderImpl. 
Can we make assumptions about which context is current during the destruction of
this class?

https://chromiumcodereview.appspot.com/9968113/diff/15001/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/15001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8418: source_height, 0, GL_RGBA,
type, NULL);
On 2012/04/05 21:58:39, apatrick_chromium wrote:
> GL_RGBA -> internal_format. I don't know of any extensions that make it valid
> for them to be different.

Done.

https://chromiumcodereview.appspot.com/9968113/diff/15001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8433:
glUseProgram(current_program_->service_id());
On 2012/04/05 21:58:39, apatrick_chromium wrote:
> If there was no program in use before, should probably glUnuseProgram the one
> that DoCopyTexture used.

Done.

https://chromiumcodereview.appspot.com/9968113/diff/15001/third_party/khronos...
File third_party/khronos/GLES2/gl2ext.h (right):

https://chromiumcodereview.appspot.com/9968113/diff/15001/third_party/khronos...
third_party/khronos/GLES2/gl2ext.h:1963: #ifndef
UNPACK_COLORSPACE_CONVERSION_CHROMIUM
On 2012/04/05 21:58:39, apatrick_chromium wrote:
> These are missing GL_ prefixes.

Done.

[greggman, 2012-04-13 00:19:15]

I'm fine with waiting on the gl_tests assuming you need them.

Up to this point I've kind of taken the position that as long as we are calling
GL "correctly" we should get the correct results.

Mo is going to help me get the running on the bots. It's probably best not to
add anything until they are running there.

https://chromiumcodereview.appspot.com/9968113/diff/15001/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/15001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:99:
FreeResources();
On 2012/04/13 00:13:29, Jeff Timanus wrote:
> On 2012/04/05 21:58:39, apatrick_chromium wrote:
> > What if the wrong GL context is current when these objects are destroyed?
> 
> That's a good point.  This class is presently destroyed in ~GLES2DecoderImpl. 
> Can we make assumptions about which context is current during the destruction
of
> this class?

Not if "have_context" is false in GLES2DecoderImpl::Destroy

[Jeff Timanus, 2012-04-13 04:13:45]

Please have a final look.  

I updated the new test, SubImageUnpack, which was also failing due to the new
flip-y command.

https://chromiumcodereview.appspot.com/9968113/diff/15001/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/15001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:99:
FreeResources();
On 2012/04/13 00:19:15, greggman wrote:
> On 2012/04/13 00:13:29, Jeff Timanus wrote:
> > On 2012/04/05 21:58:39, apatrick_chromium wrote:
> > > What if the wrong GL context is current when these objects are destroyed?
> > 
> > That's a good point.  This class is presently destroyed in
~GLES2DecoderImpl. 
> > Can we make assumptions about which context is current during the
destruction
> of
> > this class?
> 
> Not if "have_context" is false in GLES2DecoderImpl::Destroy

I believe that I fixed this issue by conditionally freeing the extension
resources in GLES2DecoderImpl::Destroy.

[greggman, 2012-04-15 22:22:41]

https://chromiumcodereview.appspot.com/9968113/diff/36001/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/36001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:161: void
CopyTextureCHROMIUMResourceManager::Destroy() {
nit: if I do this

c = new CopyTextureChromiumResourceManager();
c->Destroy();

Bad things will happen. Probably no one will ever separate new from Initialize
far enough that this will ever come up but I just thought I'd point it out. Up
to you.

https://chromiumcodereview.appspot.com/9968113/diff/36001/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/36001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8349: if (!bound_array_buffer_)
This isn't enough to restore the state changed in DoCopyTexture.

You need to call glEnable/DsiableVertexAttribArray for the 2 locations you
change. You also need to call glVertexAttribPointer for those to locations. Then
you can call the code here to restore the current binding.

It's probably best if you call glBindAttribLocation in
CopyTextureCHROMIUM::Initialize so you don't have to pass the locations back out
here nor would you have to remember the locations at all for that matter.

There's code for restoring in GLES2DecoderImpl::RestoreStateForSimulatedAttrib0.
You could refactor into RestoreStateForAttrib(GLuint attrib)?

[apatrick_chromium, 2012-04-16 19:17:21]

A few nits.

https://chromiumcodereview.appspot.com/9968113/diff/36001/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/36001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:100: GLfloat
texture_vertices[] = { -1.0f, -1.0f, 0.0f, 1.0f,
These two could be const static. Not a big deal but it saves assembling the
arrays on the stack.

https://chromiumcodereview.appspot.com/9968113/diff/36001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:112:
glBufferData(GL_ARRAY_BUFFER, 16 * sizeof(GLfloat), texture_vertices,
sizeof(texture_vertices)

https://chromiumcodereview.appspot.com/9968113/diff/36001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:116:
glBufferData(GL_ARRAY_BUFFER, 8 * sizeof(GLfloat), texture_coords,
sizeof(texture_coords)

[Jeff Timanus, 2012-04-17 01:27:34]

Thanks again for the reviews.

I addressed the comments, and corrected the state that was being clobbered.

https://chromiumcodereview.appspot.com/9968113/diff/36001/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/36001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:100: GLfloat
texture_vertices[] = { -1.0f, -1.0f, 0.0f, 1.0f,
On 2012/04/16 19:17:21, apatrick_chromium wrote:
> These two could be const static. Not a big deal but it saves assembling the
> arrays on the stack.

Done.

https://chromiumcodereview.appspot.com/9968113/diff/36001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:112:
glBufferData(GL_ARRAY_BUFFER, 16 * sizeof(GLfloat), texture_vertices,
On 2012/04/16 19:17:21, apatrick_chromium wrote:
> sizeof(texture_vertices)

Done.

https://chromiumcodereview.appspot.com/9968113/diff/36001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:116:
glBufferData(GL_ARRAY_BUFFER, 8 * sizeof(GLfloat), texture_coords,
On 2012/04/16 19:17:21, apatrick_chromium wrote:
> sizeof(texture_coords)

Done.

https://chromiumcodereview.appspot.com/9968113/diff/36001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:161: void
CopyTextureCHROMIUMResourceManager::Destroy() {
On 2012/04/15 22:22:41, greggman wrote:
> nit: if I do this
> 
> c = new CopyTextureChromiumResourceManager();
> c->Destroy();
> 
> Bad things will happen. Probably no one will ever separate new from Initialize
> far enough that this will ever come up but I just thought I'd point it out. Up
> to you.

A good observation.  I added an initialized_ flag to the class that early-outs
these routines if initialized is not called.

Done.

https://chromiumcodereview.appspot.com/9968113/diff/36001/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/36001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8349: if (!bound_array_buffer_)
On 2012/04/15 22:22:41, greggman wrote:
> This isn't enough to restore the state changed in DoCopyTexture.
> 
> You need to call glEnable/DsiableVertexAttribArray for the 2 locations you
> change. You also need to call glVertexAttribPointer for those to locations.
Then
> you can call the code here to restore the current binding.
> 
> It's probably best if you call glBindAttribLocation in
> CopyTextureCHROMIUM::Initialize so you don't have to pass the locations back
out
> here nor would you have to remember the locations at all for that matter.

I follow the need to restore the settings.  I had been confused as to if the
active vertex attribute settings were tracked per program, or globally.  Since
they are tracked globally, they do need to be restored.

I'm not sure I follow how performing the bind during initialize will prevent the
need to restore their state.  If they were bound, and the attrib 

> 
> There's code for restoring in
GLES2DecoderImpl::RestoreStateForSimulatedAttrib0.
> You could refactor into RestoreStateForAttrib(GLuint attrib)?
> 

Thanks for the suggestion.  
Done.

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
File gpu/command_buffer/client/gles2_implementation_unittest.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
gpu/command_buffer/client/gles2_implementation_unittest.cc:299: static const
int32 kNumCommandEntries = 500;
I had to boost the size because GLES2ImplementationTest. SubImageUnpack was
causing an overflow of the command buffer.  An unexpected flush in the command
stream causes the test to fail expectations match.

This is because assigning the pixel-store flip parameter is now sending a
command.

[Jeff Timanus, 2012-04-17 01:28:47]

Oh, and I also re-added the extension summary file, which had been lost while
shifting the change to different machines.

See CHROMIUM_copy_texture.txt

[greggman, 2012-04-17 04:58:06]

Sorry Jeff, I missed something before.

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
File gpu/command_buffer/client/gles2_implementation_unittest.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
gpu/command_buffer/client/gles2_implementation_unittest.cc:299: static const
int32 kNumCommandEntries = 500;
On 2012/04/17 01:27:34, Jeff Timanus wrote:
> I had to boost the size because GLES2ImplementationTest. SubImageUnpack was
> causing an overflow of the command buffer.  An unexpected flush in the command
> stream causes the test to fail expectations match.
> 
> This is because assigning the pixel-store flip parameter is now sending a
> command.

As long as the rest of the tests pass this is fine.

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:149:
glBindAttribLocation(programs_[program], kVertexPositionAttrib,
These BindAttribLocation statements need to come before glLinkProgram

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right):

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:25: static const
GLuint kVertexTextureAttrib = 1;
Do you mind adding

COMPILE_ASSERT(
    kVertexPositionAttrib == 0u ||
    kVertexTextureAttrib == 0u,
    CopyTexture_One_of_these_attribs_must_be_0);

Just so someone doesn't change them to 1 and 2 which will fail on Desktop GL.

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5064: 0, info->size(),
info->type(), info->normalized(), info->gl_stride(),
this first 0, needs to be attrib
Same at line 5067

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8335:
dest_info->SetLevelSize(GL_TEXTURE_2D, level, source_width, source_height);
See comments in other places about the problems with this.

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
File gpu/command_buffer/service/texture_manager.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
gpu/command_buffer/service/texture_manager.cc:329: bool
TextureManager::TextureInfo::SetLevelSize(
Honestly, I feel like it might be better if you delete this and call
TextureManager::SetLevelInfo. There are other flags set when calling this stuff.
We can work through all of them if you want or you can just call SetLevelInfo

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
File gpu/command_buffer/service/texture_manager.h (right):

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
gpu/command_buffer/service/texture_manager.h:146: bool SetLevelSize(GLint face,
GLint level, GLsizei width, GLsizei height);
I'm sorry Jeff,I should have been paying more attention.

You'll need to make this private and then make a corresponding
TextureManager::SetLevelSize(TextureInfo* info, GLint face, GLint level, GLsizei
width, GLsizei height);

The reason is there is bookkeeping in TextureManager to see if any textures are
breaking any rules. See TextureManager::SetLevelInfo

[greggman, 2012-04-18 01:04:30]

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8331:
dest_info->GetLevelType(GL_TEXTURE_2D, level, &type, &internal_format);
Hi Jeff, continued from the email
This code needs to be as follows.

// Check that this type of texture is allowed.
if (!texture_manager()->ValidForTarget(
    GL_TEXTURE_2D, level, source_width, source_height, 1)) {
   SetGLError(GL_INVALID_VALUE, 
              "glCopyTextureCHROMIUM: Bad dimensions");
   return;
}

// make sure the glTexImage2D succeeds.
CopyRealGLErrorsToWrapper();
WrappedTexImage2D(
   GL_TEXTURE_2D, level, internal_format, source_width,
   source_height, 0, internal_format, type, NULL);
GLenum error = PeekGLError();
if (error != GL_NO_ERROR) 
   return;
}

// Update the texture info
texture_manager()->SetLevelInfo(
    dest_info,
    GL_TEXUTRE_2D, level, internal_format,
    source_width, source_height, 1, 0, internal_format,
    type, true);

[Jeff Timanus, 2012-04-18 14:59:08]

Thanks again for the review, Gregg.  I made the changes that you suggested.

I'm working on a parallel webkit change that will make use of this extension to
copy Canvas2D contents to WebGL textures.  Hopefully this change can land soon,
so that the other can follow.

I hope that jury duty is all the fun that it's made up to be!

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:149:
glBindAttribLocation(programs_[program], kVertexPositionAttrib,
On 2012/04/17 04:58:06, greggman wrote:
> These BindAttribLocation statements need to come before glLinkProgram

Done.

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right):

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:25: static const
GLuint kVertexTextureAttrib = 1;
On 2012/04/17 04:58:06, greggman wrote:
> Do you mind adding
> 
> COMPILE_ASSERT(
>     kVertexPositionAttrib == 0u ||
>     kVertexTextureAttrib == 0u,
>     CopyTexture_One_of_these_attribs_must_be_0);
> 
> Just so someone doesn't change them to 1 and 2 which will fail on Desktop GL.

Done.

Why would they fail on DesktopGL?  Are 1 and 2 the values for the default
attribs in desktop GL?  The use of value 1 is permitted as it is?

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5064: 0, info->size(),
info->type(), info->normalized(), info->gl_stride(),
On 2012/04/17 04:58:06, greggman wrote:
> this first 0, needs to be attrib
> Same at line 5067

Done.

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8331:
dest_info->GetLevelType(GL_TEXTURE_2D, level, &type, &internal_format);
On 2012/04/18 01:04:30, greggman wrote:
> Hi Jeff, continued from the email
> This code needs to be as follows.
> 
> // Check that this type of texture is allowed.
> if (!texture_manager()->ValidForTarget(
>     GL_TEXTURE_2D, level, source_width, source_height, 1)) {
>    SetGLError(GL_INVALID_VALUE, 
>               "glCopyTextureCHROMIUM: Bad dimensions");
>    return;
> }
> 
> // make sure the glTexImage2D succeeds.
> CopyRealGLErrorsToWrapper();
> WrappedTexImage2D(
>    GL_TEXTURE_2D, level, internal_format, source_width,
>    source_height, 0, internal_format, type, NULL);
> GLenum error = PeekGLError();
> if (error != GL_NO_ERROR) 
>    return;
> }
> 
> // Update the texture info
> texture_manager()->SetLevelInfo(
>     dest_info,
>     GL_TEXUTRE_2D, level, internal_format,
>     source_width, source_height, 1, 0, internal_format,
>     type, true);
> 

Done.

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8335:
dest_info->SetLevelSize(GL_TEXTURE_2D, level, source_width, source_height);
On 2012/04/17 04:58:06, greggman wrote:
> See comments in other places about the problems with this.

Done.

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
File gpu/command_buffer/service/texture_manager.h (right):

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
gpu/command_buffer/service/texture_manager.h:146: bool SetLevelSize(GLint face,
GLint level, GLsizei width, GLsizei height);
On 2012/04/17 04:58:06, greggman wrote:
> I'm sorry Jeff,I should have been paying more attention.
> 
> You'll need to make this private and then make a corresponding
> TextureManager::SetLevelSize(TextureInfo* info, GLint face, GLint level,
GLsizei
> width, GLsizei height);
> 
> The reason is there is bookkeeping in TextureManager to see if any textures
are
> breaking any rules. See TextureManager::SetLevelInfo

I took your suggestion, and reverted these changes.  SetLevelInfo is now called
directly from the DoCopyTextureCHROMIUM handler.

[greggman, 2012-04-19 00:39:38]

lgtm

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right):

https://chromiumcodereview.appspot.com/9968113/diff/44002/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:25: static const
GLuint kVertexTextureAttrib = 1;
On 2012/04/18 14:59:08, Jeff Timanus wrote:
> On 2012/04/17 04:58:06, greggman wrote:
> > Do you mind adding
> > 
> > COMPILE_ASSERT(
> >     kVertexPositionAttrib == 0u ||
> >     kVertexTextureAttrib == 0u,
> >     CopyTexture_One_of_these_attribs_must_be_0);
> > 
> > Just so someone doesn't change them to 1 and 2 which will fail on Desktop
GL.
> 
> Done.
> 
> Why would they fail on DesktopGL?  Are 1 and 2 the values for the default
> attribs in desktop GL?  The use of value 1 is permitted as it is?

It would fail on Desktop GL because, though undocumented, desktopGL doesn't work
unless you use Attrib 0. It's a bug in all desktopGLs. We work around it in the
command buffer code but this code is running below the command buffer and so
would run into the bug.

[apatrick_chromium, 2012-04-19 00:43:47]

lgtm

[commit-bot: I haz the power, 2012-04-19 00:53:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/twiz@chromium.org/9968113/53005

[commit-bot: I haz the power, 2012-04-19 03:20:32]

Change committed as 132934