gpu: glCopyTextureCHROMIUM() checks dest internal format incorrectly.

gpu: glCopyTextureCHROMIUM() checks dest internal format incorrectly.

|internal_format| which a client passes as argument and dest internal format
which is the format of current destination texture can be different. If
different, glCopyTextureCHROMIUM redefines the destination texture. So we
should check dest internal format using |internal_format| argument.

In addition, some platforms reports error when |internal_format| is GL_ALPHA,
GL_LUMINANCE or GL_LUMINANCE_ALPHA, because those formats can not attached as
color attachment of FBO on some platforms. So we restrict |internal_format| to
GL_RGB and GL_RGBA.

TEST=GLCopyTextureCHROMIUMTest.InternalFormat,
     GLCopyTextureCHROMIUMTest.RedefineDestinationTexture

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

[dshwang, 2014-08-22 11:07:12]

Could you review? It's follow-up CL of crrev/374193002
crrev/374193002 made potential bugs, and it fixes them.

[no sievers, 2014-08-22 18:07:19]

https://codereview.chromium.org/481913005/diff/1/gpu/GLES2/extensions/CHROMIU...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):

https://codereview.chromium.org/481913005/diff/1/gpu/GLES2/extensions/CHROMIU...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:71: 
INVALID_xxx is generated if <internal_format> is not one of the valid formats
described above.

INVALID_xxx is generated if the internal format of <source_id> is not one of
formats from the table above.

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

https://codereview.chromium.org/481913005/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10061: bool valid_source_format
= (source_internal_format >= GL_ALPHA &&
nit: mind making this comparison more explicit? i.e. fmt == GL_ALPHA || fmt ==
GL_RGB ||...

https://codereview.chromium.org/481913005/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10064: if (!valid_source_format
|| !valid_dest_format) {
Should we make it GL_INVALID_OPERATION if !valid_source_format?

https://codereview.chromium.org/481913005/diff/1/gpu/command_buffer/tests/gl_...
File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right):

https://codereview.chromium.org/481913005/diff/1/gpu/command_buffer/tests/gl_...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:76:
EXPECT_EQ(glGetError(), GL_NO_ERROR);
nit: EXPECT_EQ(GL_NO_ERROR, glGetError()) here and below

https://codereview.chromium.org/481913005/diff/1/gpu/command_buffer/tests/gl_...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:109: 
nit: can you cut this off into a separate test here? like
"InternalFormatNotSupported" or something

https://codereview.chromium.org/481913005/diff/1/gpu/command_buffer/tests/gl_...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:159:
EXPECT_EQ(glGetError(), GL_INVALID_VALUE);
hmm not sure these tests for TexSubImage2D() behavior belong in here

https://codereview.chromium.org/481913005/diff/1/gpu/command_buffer/tests/gl_...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:184:
GLTestHelper::CheckPixels(1, 1, 1, 1, 0, pixels);
Althought you define all pixels to the same value above, are you checking the
right pixel here though? Aren't you comparing pixels[0] (top-left) with bottom
right (1,1)?

[dshwang, 2014-08-22 19:07:55]

Thank you for quick and awesome review. I fixes your suggestions. Could you
review again?

https://codereview.chromium.org/481913005/diff/1/gpu/GLES2/extensions/CHROMIU...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):

https://codereview.chromium.org/481913005/diff/1/gpu/GLES2/extensions/CHROMIU...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:71: 
On 2014/08/22 18:07:19, sievers wrote:
> INVALID_xxx is generated if <internal_format> is not one of the valid formats
> described above.
> 
> INVALID_xxx is generated if the internal format of <source_id> is not one of
> formats from the table above.

Thank you! GL_INVALID_OPERATION is proper because glCopyTexImage2D also reports
it at the same situation.
https://www.khronos.org/opengles/sdk/docs/man/xhtml/glCopyTexImage2D.xml

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

https://codereview.chromium.org/481913005/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10061: bool valid_source_format
= (source_internal_format >= GL_ALPHA &&
On 2014/08/22 18:07:19, sievers wrote:
> nit: mind making this comparison more explicit? i.e. fmt == GL_ALPHA || fmt ==
> GL_RGB ||... 

Done.

https://codereview.chromium.org/481913005/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10064: if (!valid_source_format
|| !valid_dest_format) {
On 2014/08/22 18:07:19, sievers wrote:
> Should we make it GL_INVALID_OPERATION if !valid_source_format?

Yes! GL_INVALID_OPERATION is right.

https://codereview.chromium.org/481913005/diff/1/gpu/command_buffer/tests/gl_...
File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right):

https://codereview.chromium.org/481913005/diff/1/gpu/command_buffer/tests/gl_...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:76:
EXPECT_EQ(glGetError(), GL_NO_ERROR);
On 2014/08/22 18:07:19, sievers wrote:
> nit: EXPECT_EQ(GL_NO_ERROR, glGetError()) here and below

Done.

https://codereview.chromium.org/481913005/diff/1/gpu/command_buffer/tests/gl_...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:109: 
On 2014/08/22 18:07:19, sievers wrote:
> nit: can you cut this off into a separate test here? like
> "InternalFormatNotSupported" or something

Yes. I separated it.

https://codereview.chromium.org/481913005/diff/1/gpu/command_buffer/tests/gl_...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:159:
EXPECT_EQ(glGetError(), GL_INVALID_VALUE);
On 2014/08/22 18:07:19, sievers wrote:
> hmm not sure these tests for TexSubImage2D() behavior belong in here

Yes, I piggy back TexSubImage2D()'s error check, because there is no way to
query texture internal format until we support OpenGL 4.3 and/or
ARB_internalformat_query2.
glGetInternalformativ(texture_target, GL_RGBA8, GL_TEXTURE_IMAGE_FORMAT, 1,
&format)

I also feel uneasy but I cannot find better way. Do you have good idea?

https://codereview.chromium.org/481913005/diff/1/gpu/command_buffer/tests/gl_...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:184:
GLTestHelper::CheckPixels(1, 1, 1, 1, 0, pixels);
On 2014/08/22 18:07:19, sievers wrote:
> Althought you define all pixels to the same value above, are you checking the
> right pixel here though? Aren't you comparing pixels[0] (top-left) with bottom
> right (1,1)?

That's right. Now I compare it with &pixel[12]

[dshwang, 2014-08-22 19:11:43]

https://codereview.chromium.org/481913005/diff/1/gpu/command_buffer/tests/gl_...
File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right):

https://codereview.chromium.org/481913005/diff/1/gpu/command_buffer/tests/gl_...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:76:
EXPECT_EQ(glGetError(), GL_NO_ERROR);
On 2014/08/22 19:07:55, dshwang wrote:
> On 2014/08/22 18:07:19, sievers wrote:
> > nit: EXPECT_EQ(GL_NO_ERROR, glGetError()) here and below
> 
> Done.

ah... clang preprocessor converts macro to int while glGetError() returns
unsigned int..
I'll rollback this change to build on clang.

../../gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:76:3: note:
in instantiation of function template specialization
'testing::internal::EqHelper<true>::Compare<int, unsigned int>' requested here
  EXPECT_EQ(GL_NO_ERROR, glGetError());

[no sievers, 2014-08-22 21:02:12]

lgtm

[dshwang, 2014-08-22 21:24:25]

On 2014/08/22 21:02:12, sievers wrote:
> lgtm

Thank you for reviewing.

[dshwang, 2014-08-22 21:24:28]

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

[commit-bot: I haz the power, 2014-08-22 21:26:24]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/481913005/40001

[commit-bot: I haz the power, 2014-08-22 22:20:49]

Committed patchset #3 (40001) as 291528