gpu: introduce glCopySubTextureCHROMIUM

gpu: introduce glCopySubTextureCHROMIUM

Introduce glCopySubTextureCHROMIUM to support immutable texture as well as to
optimize WebGL texSubImage2D(video | canvas).

This CL changes gl_renderer to use glCopySubTextureCHROMIUM because the
destination texture is immutable.

TEST=GLCopyTextureCHROMIUMTest
BUG=443151
Committed: https://crrev.com/fac6a2d44323ad51429ee728e241fe22242f5ebd
Cr-Commit-Position: refs/heads/master@{#318855}

[dshwang, 2015-01-20 22:34:26]

dongseong.hwang@intel.com changed reviewers:
+ kbr@chromium.org, piman@chromium.org, sievers@chromium.org, zmo@chromium.org

[dshwang, 2015-01-20 22:34:27]

@sievers, @piman could you review?
This CL introduces glCopySubTextureCHROMIUM as discussed in Issue 443151

It makes GLCopyTextureCHROMIUMTest unittests cover both glCopyTextureCHROMIUM
and glCopySubTextureCHROMIUM.
In addition, I add tests for immutable texture and dimension check for
glCopySubTextureCHROMIUM.

btw, I applied this CL to blink to optimize texSubImage2D(canvas & video) and
then test WebGL conformance tests. It works very well.

[piman, 2015-01-21 00:01:32]

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

https://codereview.chromium.org/864513004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10553: }
Why do we disallow >0 levels?

https://codereview.chromium.org/864513004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10555: if (source_texture_ref ==
dest_texture_ref) {
Actually what you want to check is if source_texture_ref->texture() ==
dest_texture_ref->texture(). 2 different texture refs can reference the same
texture.

https://codereview.chromium.org/864513004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10748: "level must be 0");
Why?

https://codereview.chromium.org/864513004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10752: if (source_texture_ref ==
dest_texture_ref) {
Same as above.

https://codereview.chromium.org/864513004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10769: int source_width,
source_height;
nit: please initialize.

https://codereview.chromium.org/864513004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10839: source_internal_format ==
GL_BGRA_EXT;
nit: can this check be shared with the DoCopyTextureCHROMIUM version?

[dshwang, 2015-01-21 13:36:38]

Thank you for reviewing. I address nits and answer the question about level.

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

https://codereview.chromium.org/864513004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10553: }
On 2015/01/21 00:01:32, piman (Very slow to review) wrote:
> Why do we disallow >0 levels?

CHROMIUM_copy_texture.txt defines like that.
"    Texture level 0 is copied from the source image to level <level> of the
    destination texture. The level parameter must be 0 at present."

I guess there are two reasons
1. any clients require level > 0
2. implementation and unittests code would be bigger.

https://codereview.chromium.org/864513004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10748: "level must be 0");
On 2015/01/21 00:01:31, piman (Very slow to review) wrote:
> Why?

same reason to above.

https://codereview.chromium.org/864513004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10752: if (source_texture_ref ==
dest_texture_ref) {
On 2015/01/21 00:01:32, piman (Very slow to review) wrote:
> Same as above.

Done.

https://codereview.chromium.org/864513004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10769: int source_width,
source_height;
On 2015/01/21 00:01:31, piman (Very slow to review) wrote:
> nit: please initialize.

Done.

https://codereview.chromium.org/864513004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10839: source_internal_format ==
GL_BGRA_EXT;
On 2015/01/21 00:01:31, piman (Very slow to review) wrote:
> nit: can this check be shared with the DoCopyTextureCHROMIUM version?

Done.

[piman, 2015-01-21 18:05:55]

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

https://codereview.chromium.org/864513004/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10553: }
On 2015/01/21 13:36:38, dshwang wrote:
> On 2015/01/21 00:01:32, piman (Very slow to review) wrote:
> > Why do we disallow >0 levels?
> 
> CHROMIUM_copy_texture.txt defines like that.
> "    Texture level 0 is copied from the source image to level <level> of the
>     destination texture. The level parameter must be 0 at present."
> 
> I guess there are two reasons
> 1. any clients require level > 0
> 2. implementation and unittests code would be bigger.

If we don't support level>0 we should remove level.
But previous code looked like it worked correctly for level>0, so I don' know
why we should add this limitation now.

[dshwang, 2015-01-21 18:35:51]

On 2015/01/21 18:05:55, piman (Very slow to review) wrote:
>
https://codereview.chromium.org/864513004/diff/1/gpu/command_buffer/service/g...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/864513004/diff/1/gpu/command_buffer/service/g...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:10553: }
> On 2015/01/21 13:36:38, dshwang wrote:
> > On 2015/01/21 00:01:32, piman (Very slow to review) wrote:
> > > Why do we disallow >0 levels?
> > 
> > CHROMIUM_copy_texture.txt defines like that.
> > "    Texture level 0 is copied from the source image to level <level> of the
> >     destination texture. The level parameter must be 0 at present."
> > 
> > I guess there are two reasons
> > 1. any clients require level > 0
> > 2. implementation and unittests code would be bigger.
> 
> If we don't support level>0 we should remove level.
> But previous code looked like it worked correctly for level>0, so I don' know
> why we should add this limitation now.

After considering your comment, IMO supporting level is better than removing
level because WebGL texImage(Video || canvas) can take advantage of it.
As you mentioned, CopyTextureCHROMIUMResourceManager already support level. I'll
remove level restriction and add unittests in another CL after this CL.

[Ken Russell (switch to Gerrit), 2015-01-24 01:33:24]

Looks generally good. A few comments. I'm not an OWNER so you'll need others'
reviews.

https://codereview.chromium.org/864513004/diff/20001/gpu/GLES2/extensions/CHR...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):

https://codereview.chromium.org/864513004/diff/20001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:102: <dest_id>.
Please expand upon this sentence and say exactly what it does to the texture
|dest_id|. glCopyTextureCHROMIUM is ambiguous about this; it's worth expanding
its documentation to say that it reallocates the destination texture to be the
same size as the source texture.

This function's documentation should explicitly say that it copies the contents
of the texture |source_id| to a location in the texture |dest_id| without
reallocating the destination texture.

https://codereview.chromium.org/864513004/diff/20001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:105: arguments.
Are you going to generalize the CopySubTexture variant to handle level > 0? The
code handles it. If so then please don't refer to CopyTextureCHROMIUM for the
level argument, but define its semantics here.

https://codereview.chromium.org/864513004/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://codereview.chromium.org/864513004/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:35: GLfloat
kIdentityMatrix[16] = {1.0f, 0.0f, 0.0f, 0.0f,
This should be const.

https://codereview.chromium.org/864513004/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/864513004/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:89: GLfloat kIdentityMatrix[16]
= {1.0f, 0.0f, 0.0f, 0.0f,
Should be const.

https://codereview.chromium.org/864513004/diff/20001/gpu/command_buffer/tests...
File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right):

https://codereview.chromium.org/864513004/diff/20001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:829:
TEST_F(GLCopyTextureCHROMIUMTest, CopySubTextureOffset) {
Need another new test for level > 0 if that is supported by the CopySubTexture
API -- which I think it should be.

https://codereview.chromium.org/864513004/diff/20001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:835: unsigned int
pixel[4] = {0, 0, 0, 0};
It would be more clear to define this as uint8 pixel[16], with the appropriate
initializer.

[dshwang, 2015-02-10 18:11:56]

Sorry for delaying response.
@kbr, @piman thx for reviewing.

@piman @sievers, could you review again?

https://codereview.chromium.org/864513004/diff/20001/gpu/GLES2/extensions/CHR...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):

https://codereview.chromium.org/864513004/diff/20001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:102: <dest_id>.
On 2015/01/24 01:33:24, Ken Russell wrote:
> Please expand upon this sentence and say exactly what it does to the texture
> |dest_id|. glCopyTextureCHROMIUM is ambiguous about this; it's worth expanding
> its documentation to say that it reallocates the destination texture to be the
> same size as the source texture.

Yes, I explicitly explain this for glCopyTextureCHROMIUM

> This function's documentation should explicitly say that it copies the
contents
> of the texture |source_id| to a location in the texture |dest_id| without
> reallocating the destination texture.

Yes, I explicitly explain this also.

https://codereview.chromium.org/864513004/diff/20001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:105: arguments.
On 2015/01/24 01:33:24, Ken Russell wrote:
> Are you going to generalize the CopySubTexture variant to handle level > 0?
The
> code handles it. If so then please don't refer to CopyTextureCHROMIUM for the
> level argument, but define its semantics here.

level > 0 will never be supported because glFramebufferTexture2D doesn't support
level > 0. CopyTextureCHROMIUM implementation requires <dest_id> texture bound
to FBO.
https://www.khronos.org/opengles/sdk/docs/man/xhtml/glFramebufferTexture2D.xml

https://codereview.chromium.org/864513004/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://codereview.chromium.org/864513004/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:35: GLfloat
kIdentityMatrix[16] = {1.0f, 0.0f, 0.0f, 0.0f,
On 2015/01/24 01:33:24, Ken Russell wrote:
> This should be const.

Done.

https://codereview.chromium.org/864513004/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/864513004/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:89: GLfloat kIdentityMatrix[16]
= {1.0f, 0.0f, 0.0f, 0.0f,
On 2015/01/24 01:33:24, Ken Russell wrote:
> Should be const.

Done.

https://codereview.chromium.org/864513004/diff/20001/gpu/command_buffer/tests...
File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right):

https://codereview.chromium.org/864513004/diff/20001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:829:
TEST_F(GLCopyTextureCHROMIUMTest, CopySubTextureOffset) {
On 2015/01/24 01:33:24, Ken Russell wrote:
> Need another new test for level > 0 if that is supported by the CopySubTexture
> API -- which I think it should be.

I add test for level. the test checks if non-zero level fails.

https://codereview.chromium.org/864513004/diff/20001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:835: unsigned int
pixel[4] = {0, 0, 0, 0};
On 2015/01/24 01:33:24, Ken Russell wrote:
> It would be more clear to define this as uint8 pixel[16], with the appropriate
> initializer.

Done.

https://codereview.chromium.org/864513004/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/864513004/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10755: // because
glFramebufferTexture2D() doesn't support level > 0.
add comment why level > 0 is not supported.

https://codereview.chromium.org/864513004/diff/40001/gpu/command_buffer/tests...
File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right):

https://codereview.chromium.org/864513004/diff/40001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:103:
TEST_P(GLCopyTextureCHROMIUMTest, Level) {
add new test for level > 0

[dshwang, 2015-02-25 14:16:20]

hi piman and kbr, could you review again?

[piman, 2015-02-25 21:32:17]

https://codereview.chromium.org/864513004/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/864513004/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10755: // because
glFramebufferTexture2D() doesn't support level > 0.
On 2015/02/10 18:11:55, dshwang wrote:
> add comment why level > 0 is not supported.

If we won't ever support it, we should remove the parameter altogether.

[Ken Russell (switch to Gerrit), 2015-02-25 22:19:10]

Still looks good to me overall. Again, you'll need OWNERS reviews.

https://codereview.chromium.org/864513004/diff/40001/gpu/GLES2/extensions/CHR...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):

https://codereview.chromium.org/864513004/diff/40001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:121: INVALID_VALUE may
be generated if <level> >log2(max), where max is
I think this should say "is" instead of "may be".

[dshwang, 2015-02-26 13:43:49]

Thanks for reviewing. Could you review again?

On 2015/02/25 21:32:17, piman (Very slow to review) wrote:
>
https://codereview.chromium.org/864513004/diff/40001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/864513004/diff/40001/gpu/command_buffer/servi...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:10755: // because
> glFramebufferTexture2D() doesn't support level > 0.
> On 2015/02/10 18:11:55, dshwang wrote:
> > add comment why level > 0 is not supported.
> 
> If we won't ever support it, we should remove the parameter altogether.

yes, I remove |level| altogether.

On 2015/02/25 22:19:10, Ken Russell wrote:
> Still looks good to me overall. Again, you'll need OWNERS reviews.
> 
>
https://codereview.chromium.org/864513004/diff/40001/gpu/GLES2/extensions/CHR...
> File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):
> 
>
https://codereview.chromium.org/864513004/diff/40001/gpu/GLES2/extensions/CHR...
> gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:121: INVALID_VALUE may
> be generated if <level> >log2(max), where max is
> I think this should say "is" instead of "may be".

As |level| is removed, this line is also removed.

[dshwang, 2015-02-26 13:45:00]

Patchset #4 (id:60001) has been deleted

[piman, 2015-02-26 19:52:31]

https://codereview.chromium.org/864513004/diff/80001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://codereview.chromium.org/864513004/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:492:
DCHECK(yoffset >= 0 && yoffset + source_height <= dest_height);
Where is this validated in the calling code? I didn't see any code that did.

[dshwang, 2015-02-27 11:56:42]

Thx for reviewing.

https://codereview.chromium.org/864513004/diff/80001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/864513004/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11213: if
(!dest_texture->ValidForTexture(dest_texture->target(), 0, xoffset,
On 2015/02/26 19:52:31, piman (Very slow to review) wrote:
> Where is this validated in the calling code? I didn't see any code that did.

Here.

https://codereview.chromium.org/864513004/diff/80001/gpu/command_buffer/tests...
File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right):

https://codereview.chromium.org/864513004/diff/80001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:795:
TEST_F(GLCopyTextureCHROMIUMTest, CopySubTextureDimension) {
In addition, this test covers the code.

[piman, 2015-02-27 21:58:52]

OK, thanks, LGTM.

[dshwang, 2015-02-28 05:57:08]

dongseong.hwang@intel.com changed reviewers:
+ scherkus@chromium.org

[dshwang, 2015-02-28 05:57:09]

On 2015/02/27 21:58:52, piman (Very slow to review) wrote:
> OK, thanks, LGTM.

thx!

scherkus@, could you review media/ ?

[scherkus (not reviewing), 2015-03-02 18:46:48]

lgtm

[dshwang, 2015-03-03 07:37:57]

On 2015/03/02 18:46:48, scherkus wrote:
> lgtm

thx for reviewing!

[dshwang, 2015-03-03 07:38:03]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2015-03-03 07:39:24]

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

[commit-bot: I haz the power, 2015-03-03 08:43:08]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-03-03 08:44:11]

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

[ulan, 2015-03-03 10:09:18]

On 2015/03/03 08:44:11, I haz the power (commit-bot) wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/fac6a2d44323ad51429ee728e241fe22242f5ebd
> Cr-Commit-Position: refs/heads/master@{#318855}

This have broken the build:
https://code.google.com/p/chromium/issues/detail?id=463439

I am going to revert.

[ulan, 2015-03-03 10:12:00]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.chromium.org/972833004/ by ulan@chromium.org.

The reason for reverting is: This broke telemetry_unittests and
content_browsertests on Mac (see issue 463439).

[dshwang, 2015-03-03 14:20:57]

thx for handling it. I'll figure out and reland