Improve CopyTex{Sub}Image2D format validation.

Improve CopyTex{Sub}Image2D format validation.

1) Unify the validation code into a helper function.
2) In ES3, for sized formats, the read_format and internal_format's color bits
   have to match exactly.

BUG=429053
TEST=gpu_unittests,webgl{2}_conformance
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/25966f2863c58fb04faa48986150c363d30be6fa
Cr-Commit-Position: refs/heads/master@{#397913}

[Zhenyao Mo, 2016-06-01 21:57:53]

Description was changed from

==========
Improve CopyTex{Sub}Image2D format validation.

1) Unify the validation code into a helper function.
2) In ES3, for sized formats, the read_format and internal_format's color bits
   have to match exactly.

BUG=429053
TEST=gpu_unittests,webgl{2}_conformance
R=piman@chromium.org
==========

to

==========
Improve CopyTex{Sub}Image2D format validation.

1) Unify the validation code into a helper function.
2) In ES3, for sized formats, the read_format and internal_format's color bits
   have to match exactly.

BUG=429053
TEST=gpu_unittests,webgl{2}_conformance
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Zhenyao Mo, 2016-06-01 21:58:10]

The CQ bit was checked by zmo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 21:58:43]

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

[commit-bot: I haz the power, 2016-06-02 00:49:58]

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

[commit-bot: I haz the power, 2016-06-02 00:49:59]

Dry run: This issue passed the CQ dry run.

[Zhenyao Mo, 2016-06-02 18:15:44]

Note that patch set 1 is refactoring, patch set 2 is adding the fix.

piman: PTAL
kbr, cwallez: FYI (feel free to review)

[Zhenyao Mo, 2016-06-02 18:15:50]

The CQ bit was checked by zmo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-02 18:16:45]

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

[commit-bot: I haz the power, 2016-06-02 20:00:44]

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

[commit-bot: I haz the power, 2016-06-02 20:00:45]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ken Russell (switch to Gerrit), 2016-06-02 20:08:08]

kbr@chromium.org changed reviewers:
+ kbr@chromium.org

[Ken Russell (switch to Gerrit), 2016-06-02 20:08:09]

Nice fix. LGTM for what it's worth.

[Corentin Wallez, 2016-06-02 21:25:41]

On 2016/06/02 at 18:15:44, zmo wrote:
> Note that patch set 1 is refactoring, patch set 2 is adding the fix.
> 
> piman: PTAL
> kbr, cwallez: FYI (feel free to review)

lgtm, some small comments.

[Corentin Wallez, 2016-06-02 21:29:39]

cwallez@chromium.org changed reviewers:
+ cwallez@chromium.org

[Corentin Wallez, 2016-06-02 21:29:40]

https://codereview.chromium.org/2029803002/diff/20001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (left):

https://codereview.chromium.org/2029803002/diff/20001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:113: ['win'],
bug=483282)
likewise from Windows only to general.

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

https://codereview.chromium.org/2029803002/diff/20001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils.cc:1264: default:
All the default cases could be UNREACHABLE()

[Zhenyao Mo, 2016-06-03 18:29:48]

https://codereview.chromium.org/2029803002/diff/20001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (left):

https://codereview.chromium.org/2029803002/diff/20001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:113: ['win'],
bug=483282)
On 2016/06/02 21:29:40, Corentin Wallez wrote:
> likewise from Windows only to general.

I actually did that :)

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

https://codereview.chromium.org/2029803002/diff/20001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils.cc:1264: default:
On 2016/06/02 21:29:40, Corentin Wallez wrote:
> All the default cases could be UNREACHABLE()

Good idea. Done.

[Zhenyao Mo, 2016-06-03 18:29:57]

The CQ bit was checked by zmo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-03 18:30:34]

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

[commit-bot: I haz the power, 2016-06-03 18:56:23]

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

[commit-bot: I haz the power, 2016-06-03 18:56:24]

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

[Zhenyao Mo, 2016-06-03 19:55:29]

The CQ bit was checked by zmo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-03 19:56:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029803002/60001

[piman, 2016-06-03 21:26:04]

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

https://codereview.chromium.org/2029803002/diff/60001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils.cc:1337: default:
I think anything valid in FormatTypeValidator::kSupportedFormatTypes in
texture_manager.cc can end up here, some combinations are exposed by extensions.
E.g. GL_ALPHA/GL_UNSIGNED_BYTE, */GL_HALF_FLOAT_OES, */GL_FLOAT, GL_RED/*,
GL_RG/*

[Zhenyao Mo, 2016-06-03 21:31:27]

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

https://codereview.chromium.org/2029803002/diff/60001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils.cc:1337: default:
On 2016/06/03 21:26:04, piman OOO back 2016-6-2 wrote:
> I think anything valid in FormatTypeValidator::kSupportedFormatTypes in
> texture_manager.cc can end up here, some combinations are exposed by
extensions.
> E.g. GL_ALPHA/GL_UNSIGNED_BYTE, */GL_HALF_FLOAT_OES, */GL_FLOAT, GL_RED/*,
> GL_RG/*

This is ES3 only, so all these extensions have been folded into core as sized
formats.  Except for the GL_ALPHA/GL_UNSIGNED_BYTE, which I left out and am
fixing now.

[piman, 2016-06-03 21:40:57]

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

https://codereview.chromium.org/2029803002/diff/60001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils.cc:1337: default:
On 2016/06/03 21:31:27, Zhenyao Mo wrote:
> On 2016/06/03 21:26:04, piman OOO back 2016-6-2 wrote:
> > I think anything valid in FormatTypeValidator::kSupportedFormatTypes in
> > texture_manager.cc can end up here, some combinations are exposed by
> extensions.
> > E.g. GL_ALPHA/GL_UNSIGNED_BYTE, */GL_HALF_FLOAT_OES, */GL_FLOAT, GL_RED/*,
> > GL_RG/*
> 
> This is ES3 only, so all these extensions have been folded into core as sized
> formats.  Except for the GL_ALPHA/GL_UNSIGNED_BYTE, which I left out and am
> fixing now.

AFAICT, even if we enable ES3, we still enable e.g. GL_OES_texture_float, which
means it's still valid for the client to call TexImage2D(GL_TEXTURE_2D, 0,
GL_RGBA, ..., GL_RGBA, GL_FLOAT, NULL). I don't think we rename that GL_RGBA
internal format to GL_RGBA32F anywhere (maybe we should when on ES3?)

[Zhenyao Mo, 2016-06-03 22:18:31]

On 2016/06/03 21:40:57, piman OOO back 2016-6-2 wrote:
>
https://codereview.chromium.org/2029803002/diff/60001/gpu/command_buffer/comm...
> File gpu/command_buffer/common/gles2_cmd_utils.cc (right):
> 
>
https://codereview.chromium.org/2029803002/diff/60001/gpu/command_buffer/comm...
> gpu/command_buffer/common/gles2_cmd_utils.cc:1337: default:
> On 2016/06/03 21:31:27, Zhenyao Mo wrote:
> > On 2016/06/03 21:26:04, piman OOO back 2016-6-2 wrote:
> > > I think anything valid in FormatTypeValidator::kSupportedFormatTypes in
> > > texture_manager.cc can end up here, some combinations are exposed by
> > extensions.
> > > E.g. GL_ALPHA/GL_UNSIGNED_BYTE, */GL_HALF_FLOAT_OES, */GL_FLOAT, GL_RED/*,
> > > GL_RG/*
> > 
> > This is ES3 only, so all these extensions have been folded into core as
sized
> > formats.  Except for the GL_ALPHA/GL_UNSIGNED_BYTE, which I left out and am
> > fixing now.
> 
> AFAICT, even if we enable ES3, we still enable e.g. GL_OES_texture_float,
which
> means it's still valid for the client to call TexImage2D(GL_TEXTURE_2D, 0,
> GL_RGBA, ..., GL_RGBA, GL_FLOAT, NULL). I don't think we rename that GL_RGBA
> internal format to GL_RGBA32F anywhere (maybe we should when on ES3?)

OK, this is a helper function, so I guess it's always OK to handle all cases. 
Validators should worry about which is allowed and which isn't.

[Zhenyao Mo, 2016-06-03 22:41:47]

Patchset #5 (id:80001) has been deleted

[Zhenyao Mo, 2016-06-03 22:42:46]

The CQ bit was checked by zmo@chromium.org to run a CQ dry run

[Zhenyao Mo, 2016-06-03 22:43:02]

Revised. Please take another look.

[commit-bot: I haz the power, 2016-06-03 22:43:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029803002/100001

[piman, 2016-06-03 22:46:39]

LGTM, thanks.

[commit-bot: I haz the power, 2016-06-04 01:19:44]

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

[commit-bot: I haz the power, 2016-06-04 01:19:45]

Dry run: This issue passed the CQ dry run.

[Zhenyao Mo, 2016-06-04 11:43:57]

The CQ bit was checked by zmo@chromium.org

[Zhenyao Mo, 2016-06-04 11:43:57]

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

[commit-bot: I haz the power, 2016-06-04 11:44:07]

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

[commit-bot: I haz the power, 2016-06-04 11:48:10]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-06-04 11:49:28]

Description was changed from

==========
Improve CopyTex{Sub}Image2D format validation.

1) Unify the validation code into a helper function.
2) In ES3, for sized formats, the read_format and internal_format's color bits
   have to match exactly.

BUG=429053
TEST=gpu_unittests,webgl{2}_conformance
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Improve CopyTex{Sub}Image2D format validation.

1) Unify the validation code into a helper function.
2) In ES3, for sized formats, the read_format and internal_format's color bits
   have to match exactly.

BUG=429053
TEST=gpu_unittests,webgl{2}_conformance
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/25966f2863c58fb04faa48986150c363d30be6fa
Cr-Commit-Position: refs/heads/master@{#397913}
==========

[commit-bot: I haz the power, 2016-06-04 11:49:29]

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