Update CopyTexImage2D to do internal format validation in ES3

Update CopyTexImage2D to do internal format validation in ES3

BUG=429053, 532708
Committed: https://crrev.com/8e9c15565d4e538ebb9b31cbaa00bdc2ad890eba
Cr-Commit-Position: refs/heads/master@{#354205}

[qiankun, 2015-10-09 14:56:32]

qiankun.miao@intel.com changed reviewers:
+ bajones@chromium.org, kbr@chromium.org, piman@chromium.org, zmo@chromium.org

[qiankun, 2015-10-09 14:56:32]

This CL follows section 3.8.5 Alternate Texture Image Specification Commands in
es 3.0 spec.
 "The error INVALID_OPERATION is generated
if floating-point RGBA data is required; if signed integer RGBA data is required
and the format of the current color buffer is not signed integer; if unsigned
integer
RGBA data is required and the format of the current color buffer is not unsigned
integer; or if fixed-point RGBA data is required and the format of the current
color
buffer is not fixed-point. The error INVALID_OPERATION is also generated if the
value of FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING for the framebuffer attachment
corresponding to the read buffer is LINEAR (see section 6.1.13) and
internalformat
is one of the sRGB formats described in section 3.8.16, or if the value
of FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING is SRGB and internalformat
is not one of the sRGB formats."

[Ken Russell (switch to Gerrit), 2015-10-12 23:55:20]

zmo@ is the best person to review this.

[Zhenyao Mo, 2015-10-13 01:19:32]

This CL itself LGTM with some nits

However, there are still some constraints that need to be checked.  Look at ES
3.0.4 spec, page 140, starting from "if internalformat is size..."

Can you implement that check in a followup CL.

https://codereview.chromium.org/1403483002/diff/20001/gpu/command_buffer/comm...
File gpu/command_buffer/common/gles2_cmd_utils.cc (right):

https://codereview.chromium.org/1403483002/diff/20001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils.cc:1226: case GL_DEPTH32F_STENCIL8:
I think you only need to include color formats.

https://codereview.chromium.org/1403483002/diff/20001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils.cc:1240: case
GL_COMPRESSED_SRGB8_PUNCHTHROUGH_ALPHA1_ETC2:
I don't think you need to include compressed formats.

https://codereview.chromium.org/1403483002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc
(right):

https://codereview.chromium.org/1403483002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc:1915:
EXPECT_CALL(*gl_, GetFramebufferAttachmentParameterivEXT(_, _, _, _))
We can probably be more specific and put
GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING instead _ here, and also a few places
below.

[Zhenyao Mo, 2015-10-13 01:22:49]

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

https://codereview.chromium.org/1403483002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11023:
GLES2Util::IsSRGBFormat(internal_format)) ||
One optimization is, instead of asking IsSRGBFormat, you can have a function
called GetColorEncodingFromInternalFormat().  Then you don't even have to call
the gl function, you can just infer from read_format.  Also, you will not need
to call IsSRGBFormat() twice here, you can just do if (color_endoding !=
GetReadFramebufferAttachmentColorEncoding())

Just an idea.

[qiankun, 2015-10-13 14:28:01]

Thanks you @zmo. I updated the CL. Would you mind to take a look again?

> However, there are still some constraints that need to be checked.  Look at ES
> 3.0.4 spec, page 140, starting from "if internalformat is size..."
> 
> Can you implement that check in a followup CL.

Yes, I will implement this validation in future. It seems we should introduce
"effective internal format" for texture image which is new feature in ES3.

https://codereview.chromium.org/1403483002/diff/20001/gpu/command_buffer/comm...
File gpu/command_buffer/common/gles2_cmd_utils.cc (right):

https://codereview.chromium.org/1403483002/diff/20001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils.cc:1226: case GL_DEPTH32F_STENCIL8:
On 2015/10/13 01:19:31, Zhenyao Mo wrote:
> I think you only need to include color formats.

Done.

https://codereview.chromium.org/1403483002/diff/20001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils.cc:1240: case
GL_COMPRESSED_SRGB8_PUNCHTHROUGH_ALPHA1_ETC2:
On 2015/10/13 01:19:32, Zhenyao Mo wrote:
> I don't think you need to include compressed formats.

Done.

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

https://codereview.chromium.org/1403483002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11023:
GLES2Util::IsSRGBFormat(internal_format)) ||
On 2015/10/13 01:22:49, Zhenyao Mo wrote:
> One optimization is, instead of asking IsSRGBFormat, you can have a function
> called GetColorEncodingFromInternalFormat().  Then you don't even have to call
> the gl function, you can just infer from read_format.  Also, you will not need
> to call IsSRGBFormat() twice here, you can just do if (color_endoding !=
> GetReadFramebufferAttachmentColorEncoding())
> 
> Just an idea.

Thanks for this suggestion! I implemented this optimization.

https://codereview.chromium.org/1403483002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc
(right):

https://codereview.chromium.org/1403483002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc:1915:
EXPECT_CALL(*gl_, GetFramebufferAttachmentParameterivEXT(_, _, _, _))
On 2015/10/13 01:19:32, Zhenyao Mo wrote:
> We can probably be more specific and put
> GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING instead _ here, and also a few places
> below.

Done.

[Zhenyao Mo, 2015-10-13 17:00:24]

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

https://codereview.chromium.org/1403483002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1571: GLint
GetColorEncodingFromInternalFormat(GLenum internalformat);
Make it static.

https://codereview.chromium.org/1403483002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11037: GLint color_encoding =
GetReadFramebufferAttachmentColorEncoding();
Here you don't need the GetReadFramebufferAttachmentColorEncoding() function. 
You can just do GetColorEncodingFromInternalFormat(read_format).

[qiankun, 2015-10-13 23:48:54]

Patchset #4 (id:60001) has been deleted

[qiankun, 2015-10-13 23:54:08]

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

https://codereview.chromium.org/1403483002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1571: GLint
GetColorEncodingFromInternalFormat(GLenum internalformat);
On 2015/10/13 17:00:23, Zhenyao Mo wrote:
> Make it static.

Done.

https://codereview.chromium.org/1403483002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11037: GLint color_encoding =
GetReadFramebufferAttachmentColorEncoding();
On 2015/10/13 17:00:24, Zhenyao Mo wrote:
> Here you don't need the GetReadFramebufferAttachmentColorEncoding() function. 
> You can just do GetColorEncodingFromInternalFormat(read_format).

Done.

[Zhenyao Mo, 2015-10-14 18:39:20]

LGTM with unused function removed

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

https://codereview.chromium.org/1403483002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11037: GLint color_encoding =
GetReadFramebufferAttachmentColorEncoding();
On 2015/10/13 23:54:07, qiankun wrote:
> On 2015/10/13 17:00:24, Zhenyao Mo wrote:
> > Here you don't need the GetReadFramebufferAttachmentColorEncoding()
function. 
> > You can just do GetColorEncodingFromInternalFormat(read_format).
> 
> Done.

Then please get rid of the GetReadFramebufferAttachmentColorEncoding() function.
 It's no longer used.

[qiankun, 2015-10-15 01:28:45]

The CQ bit was checked by qiankun.miao@intel.com

[qiankun, 2015-10-15 01:28:45]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1403483002/#ps100001
(title: "remove unused function")

[commit-bot: I haz the power, 2015-10-15 01:29:47]

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

[commit-bot: I haz the power, 2015-10-15 02:30:56]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-10-15 02:32:29]

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