gpu: Extend CopyTextureCHROMIUM with support for copying part of source texture.

gpu: Extend CopyTextureCHROMIUM with support for copying part of source texture.

This makes it possible to specify the area of the source texture
that is copied to the destination texture.

BUG=490889
TEST=gl_tests
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/b6643fb0eedfc95a7990cba21769ceded5bfccaf
Cr-Commit-Position: refs/heads/master@{#331326}

[reveman, 2015-05-22 12:42:00]

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

[reveman, 2015-05-22 12:43:22]

fyi, this depends on https://codereview.chromium.org/1152723002 which I'll land
once this looks good.

[piman, 2015-05-22 21:11:03]

lgtm

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

https://codereview.chromium.org/1143373004/diff/1/gpu/command_buffer/cmd_buff...
gpu/command_buffer/cmd_buffer_functions.txt:278: GL_APICALL void        
GL_APIENTRY glCopySubTextureCHROMIUM (GLenum target, GLenum source_id, GLenum
dest_id, GLint xoffset, GLint yoffset, GLint x, GLint y, GLsizei width, GLsizei
height);
Can you update the extension spec at
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt

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

https://codereview.chromium.org/1143373004/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12118: // instead of using
kIdentityMatrix crbug.com/226218.
nit: can you keep the TODO and a reference to this bug?

[reveman, 2015-05-22 22:18:13]

reveman@chromium.org changed reviewers:
+ sievers@chromium.org

[reveman, 2015-05-22 22:18:14]

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

https://codereview.chromium.org/1143373004/diff/1/gpu/command_buffer/cmd_buff...
gpu/command_buffer/cmd_buffer_functions.txt:278: GL_APICALL void        
GL_APIENTRY glCopySubTextureCHROMIUM (GLenum target, GLenum source_id, GLenum
dest_id, GLint xoffset, GLint yoffset, GLint x, GLint y, GLsizei width, GLsizei
height);
On 2015/05/22 at 21:11:03, piman (Very slow to review) wrote:
> Can you update the extension spec at
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt

Done. Thanks for reminding me.

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

https://codereview.chromium.org/1143373004/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12118: // instead of using
kIdentityMatrix crbug.com/226218.
On 2015/05/22 at 21:11:03, piman (Very slow to review) wrote:
> nit: can you keep the TODO and a reference to this bug?

We have this TODO and bug reference in GLES2DecoderImpl::DoCopyTextureCHROMIUM.
It was copied here when CopyTexture forked into CopyTexture+CopySubTexture. I
don't think we need to support stream textures in the CopySubTexture case and
applying a generic transformation matrix when performing a sub copy makes
everything more complicated.

+sievers to make sure I understand this correctly.

[no sievers, 2015-05-22 22:37:55]

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

https://codereview.chromium.org/1143373004/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12118: // instead of using
kIdentityMatrix crbug.com/226218.
On 2015/05/22 22:18:14, reveman wrote:
> On 2015/05/22 at 21:11:03, piman (Very slow to review) wrote:
> > nit: can you keep the TODO and a reference to this bug?
> 
> We have this TODO and bug reference in
GLES2DecoderImpl::DoCopyTextureCHROMIUM.
> It was copied here when CopyTexture forked into CopyTexture+CopySubTexture. I
> don't think we need to support stream textures in the CopySubTexture case and
> applying a generic transformation matrix when performing a sub copy makes
> everything more complicated.
> 
> +sievers to make sure I understand this correctly.

This stuff is a bit unfortunate. You can put a TODO, although I don't think
anything would use CopyTexSubImage() with a stream texture (but the full copy
API is broken in the same way).

It might actually be easy to implement: We only need to worry about the matrix
if there is an image attached to the texture. So we'd probably need to have a
GLImage API to get the matrix which the StreamTexture implementation on Android
can then simply return. 

But again, this is much more interesting for CopyTextureCHROMIUM than
CopySubTexture touched in this patch.

[reveman, 2015-05-23 18:56:14]

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

https://codereview.chromium.org/1143373004/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12118: // instead of using
kIdentityMatrix crbug.com/226218.
On 2015/05/22 22:37:55, sievers wrote:
> On 2015/05/22 22:18:14, reveman wrote:
> > On 2015/05/22 at 21:11:03, piman (Very slow to review) wrote:
> > > nit: can you keep the TODO and a reference to this bug?
> > 
> > We have this TODO and bug reference in
> GLES2DecoderImpl::DoCopyTextureCHROMIUM.
> > It was copied here when CopyTexture forked into CopyTexture+CopySubTexture.
I
> > don't think we need to support stream textures in the CopySubTexture case
and
> > applying a generic transformation matrix when performing a sub copy makes
> > everything more complicated.
> > 
> > +sievers to make sure I understand this correctly.
> 
> This stuff is a bit unfortunate. You can put a TODO, although I don't think
> anything would use CopyTexSubImage() with a stream texture (but the full copy
> API is broken in the same way).
> 
> It might actually be easy to implement: We only need to worry about the matrix
> if there is an image attached to the texture. So we'd probably need to have a
> GLImage API to get the matrix which the StreamTexture implementation on
Android
> can then simply return. 

Can we simply implement the copy in GLImage::CopyTexSubImage for stream
textures? I think that would be cleaner than pulling some magic matrix out the
generic GLImage.

> 
> But again, this is much more interesting for CopyTextureCHROMIUM than
> CopySubTexture touched in this patch.

Ok, thanks for explaining. I kept the TODO for now as it shouldn't hurt to have
it here.

[reveman, 2015-05-26 01:43:55]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-05-26 01:43:55]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1143373004/#ps40001
(title: "keep TODO for now")

[commit-bot: I haz the power, 2015-05-26 01:44:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1143373004/40001

[commit-bot: I haz the power, 2015-05-26 06:05:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-26 06:06:32]

Patchset 3 (id:??) landed as
https://crrev.com/b6643fb0eedfc95a7990cba21769ceded5bfccaf
Cr-Commit-Position: refs/heads/master@{#331326}