Add glCompressedCopySubTextureCHROMIUM

Add glCompressedCopySubTextureCHROMIUM

This CL adds a GPU command for copying sub regions of compressed textures.
The implementation is based on glCopySubTextureCHROMIUM.

BUG=434699
Committed: https://crrev.com/00b21807f5c050f8766685c20e1cfc2ed9db636e
Cr-Commit-Position: refs/heads/master@{#344220}

[christiank, 2015-08-05 14:36:25]

christiank@opera.com changed reviewers:
+ piman@chromium.org, reveman@chromium.org

[christiank, 2015-08-05 14:36:26]

Hi,

This CL adds a glCompressedCopySubTextureCHROMIUM as a follow up to the
glCompressedCopyTextureCHROMIUM command I added a while back:
https://codereview.chromium.org/1119723003

While working at this I got hit by the following assert:
DCHECK(xoffset >= 0 && xoffset + source_width <= dest_width);
in CopyTextureCHROMIUMResourceManager::DoCopyTextureInternal().

From what I can tell it should really assert against the xoffset + width <=
dest_width. Otherwise the destination texture needs to be larger than the source
texture when offsetting in the destination texture.

GLCopyTextureCHROMIUMTest actually requires the behavior that I suggest, but it
never reaches CopyTextureCHROMIUMResourceManager::DoCopyTextureInternal() but
uses CopyTextureCHROMIUMResourceManager::DoCopyTexSubImage2D() instead.

Maybe this is by design and I am missing something?

Sorry for the lengthy mail, but please also have a look at the changes I have
made in this regard (updated asserts and documentation in
CHROMIUM_copy_texture.txt).

Thanks!

Regards,
Christian

[reveman, 2015-08-05 16:07:40]

On 2015/08/05 at 14:36:26, christiank wrote:
> Hi,
> 
> This CL adds a glCompressedCopySubTextureCHROMIUM as a follow up to the
glCompressedCopyTextureCHROMIUM command I added a while back:
> https://codereview.chromium.org/1119723003
> 
> While working at this I got hit by the following assert:
> DCHECK(xoffset >= 0 && xoffset + source_width <= dest_width);
> in CopyTextureCHROMIUMResourceManager::DoCopyTextureInternal().
> 
> From what I can tell it should really assert against the xoffset + width <=
dest_width. Otherwise the destination texture needs to be larger than the source
texture when offsetting in the destination texture.
> 
> GLCopyTextureCHROMIUMTest actually requires the behavior that I suggest, but
it never reaches CopyTextureCHROMIUMResourceManager::DoCopyTextureInternal() but
uses CopyTextureCHROMIUMResourceManager::DoCopyTexSubImage2D() instead.
> 
> Maybe this is by design and I am missing something?
> 
> Sorry for the lengthy mail, but please also have a look at the changes I have
made in this regard (updated asserts and documentation in
CHROMIUM_copy_texture.txt).

Looks like that was a mistake. Thanks for fixing it.

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

https://codereview.chromium.org/1272153004/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:475:
DCHECK(yoffset >= 0 && yoffset + height <= dest_height);
While here, can you split this into 4 DCHECKs instead?

DCHECK_GE(xoffset, 0);
DCHECK_LE(xoffset + width, dest_width);
....

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

https://codereview.chromium.org/1272153004/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12676: GLsizei height) {
How are we handling cases where for example width is not a multiple of 4?

https://codereview.chromium.org/1272153004/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12820: GL_UNSIGNED_BYTE, NULL);
What about the old contents? Updating a sub rect should not discard whatever is
outside that rect... Should we instead copy the old texture into this RGBA
texture?

[christiank, 2015-08-06 13:48:21]

Another question... the CopySubTexture commands requires the destination texture
to be allocated beforehand. For non-compressed textures this is done by calling
glTexImage2D with a null pointer. However, to my knowledge
glCompressedTexImage2D cannot be used in this way.

This is why the test cases allocate compressed texture memory by using a pointer
to some memory filled with zeroes.

Do you have any ideas on how to tackle this? I think it might be a problem
further on, for example when ResourceProvider::LazyAllocate needs to preallocate
compressed memory.

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

https://codereview.chromium.org/1272153004/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:475:
DCHECK(yoffset >= 0 && yoffset + height <= dest_height);
On 2015/08/05 16:07:40, reveman wrote:
> While here, can you split this into 4 DCHECKs instead?
> 
> DCHECK_GE(xoffset, 0);
> DCHECK_LE(xoffset + width, dest_width);
> ....

Done.

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

https://codereview.chromium.org/1272153004/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12676: GLsizei height) {
On 2015/08/05 16:07:40, reveman wrote:
> How are we handling cases where for example width is not a multiple of 4?

We're falling back to copying into an uncompressed texture. Basically,
GLImage::CopyTexSubImage will fail (I assume it will) and we'll end up copying
into an uncompressed texture. I am not sure if this breaks contract as the
command requires the destination texture to be defined?

https://codereview.chromium.org/1272153004/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12820: GL_UNSIGNED_BYTE, NULL);
On 2015/08/05 16:07:40, reveman wrote:
> What about the old contents? Updating a sub rect should not discard whatever
is
> outside that rect... Should we instead copy the old texture into this RGBA
> texture?

You're right, I completely missed this detail. I made a solution that copies the
destination texture (compressed) to a temporary texture and then copies it back
to the redefined (non-compressed) destination texture. Not the nicest solution
perhaps, but I couldn't think of another way to covert a texture to RGBA while
keeping the same service id.

[reveman, 2015-08-06 18:38:29]

On 2015/08/06 at 13:48:21, christiank wrote:
> Another question... the CopySubTexture commands requires the destination
texture to be allocated beforehand. For non-compressed textures this is done by
calling glTexImage2D with a null pointer. However, to my knowledge
glCompressedTexImage2D cannot be used in this way.
> 
> This is why the test cases allocate compressed texture memory by using a
pointer to some memory filled with zeroes.
> 
> Do you have any ideas on how to tackle this? I think it might be a problem
further on, for example when ResourceProvider::LazyAllocate needs to preallocate
compressed memory.

Ah, didn't know that the data pointer passed to CompressedTexImage2D can't be
null. Can we use zeroed memory for now but fix that later by handling this
special case in the one-copy tile task worker by using CopyTexture instead of
CopySubTexture the first time we update a texture?

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

https://codereview.chromium.org/1272153004/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12676: GLsizei height) {
On 2015/08/06 at 13:48:21, christiank wrote:
> On 2015/08/05 16:07:40, reveman wrote:
> > How are we handling cases where for example width is not a multiple of 4?
> 
> We're falling back to copying into an uncompressed texture. Basically,
GLImage::CopyTexSubImage will fail (I assume it will) and we'll end up copying
into an uncompressed texture. I am not sure if this breaks contract as the
command requires the destination texture to be defined?

I would prefer if we generated an INVALID_OPERATION error in these cases. That
would be more consistent with CompressedTexSubImage2D and avoid the risk that we
accidentally trigger this fallback to RGBA.

[christiank, 2015-08-10 10:55:46]

Sounds good, let's use zero memory here and the special case for the one-copy
case as you suggested.

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

https://codereview.chromium.org/1272153004/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12676: GLsizei height) {
On 2015/08/06 18:38:29, reveman wrote:
> On 2015/08/06 at 13:48:21, christiank wrote:
> > On 2015/08/05 16:07:40, reveman wrote:
> > > How are we handling cases where for example width is not a multiple of 4?
> > 
> > We're falling back to copying into an uncompressed texture. Basically,
> GLImage::CopyTexSubImage will fail (I assume it will) and we'll end up copying
> into an uncompressed texture. I am not sure if this breaks contract as the
> command requires the destination texture to be defined?
> 
> I would prefer if we generated an INVALID_OPERATION error in these cases. That
> would be more consistent with CompressedTexSubImage2D and avoid the risk that
we
> accidentally trigger this fallback to RGBA.

Done.

[reveman, 2015-08-10 16:00:46]

lgtm

https://codereview.chromium.org/1272153004/diff/60001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt (right):

https://codereview.chromium.org/1272153004/diff/60001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt:91: <x> and
<y> specify specify a texel offset in the x and y direction
nit: s/specify specify/specify/

[piman, 2015-08-17 23:04:25]

lgtm

[christiank, 2015-08-18 07:40:54]

The CQ bit was checked by christiank@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-18 07:41:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272153004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272153004/80001

[commit-bot: I haz the power, 2015-08-18 07:42:22]

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

[commit-bot: I haz the power, 2015-08-18 07:42:22]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[christiank, 2015-08-18 14:34:44]

The CQ bit was checked by christiank@opera.com

[christiank, 2015-08-18 14:34:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1272153004/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-18 14:34:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272153004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272153004/100001

[commit-bot: I haz the power, 2015-08-18 16:52:46]

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

[commit-bot: I haz the power, 2015-08-18 16:52:47]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[christiank, 2015-08-19 09:34:26]

The CQ bit was checked by christiank@opera.com

[christiank, 2015-08-19 09:34:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1272153004/#ps120001
(title: "glCompressedTexSubImage2D does not support ATC and ETC formats")

[commit-bot: I haz the power, 2015-08-19 09:34:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272153004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272153004/120001

[commit-bot: I haz the power, 2015-08-19 09:36:09]

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

[commit-bot: I haz the power, 2015-08-19 09:36:10]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[christiank, 2015-08-19 11:26:50]

The CQ bit was checked by christiank@opera.com

[christiank, 2015-08-19 11:26:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1272153004/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-19 11:27:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272153004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272153004/140001

[commit-bot: I haz the power, 2015-08-19 14:30:45]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-08-19 14:31:27]

Patchset 8 (id:??) landed as
https://crrev.com/00b21807f5c050f8766685c20e1cfc2ed9db636e
Cr-Commit-Position: refs/heads/master@{#344220}