Fix TexStorage3D with compressed ETC2/EAC formats

Fix TexStorage3D with compressed ETC2/EAC formats

These formats don't work for TexStorage3D if target is TEXTURE_3D.
TEXTURE_2D_ARRAY should work.

BUG=295792
TEST=conformance2/textures/misc/tex-storage-compressed-formats.html

Committed: https://crrev.com/5b5a54a2d7c287e09f644d04e29718b310949221
Cr-Commit-Position: refs/heads/master@{#360760}

Committed: https://crrev.com/8fe7430e2bea06a91f1e2611c29b2c0633aadca0
Cr-Commit-Position: refs/heads/master@{#361180}

[qiankun, 2015-11-19 11:02:36]

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

[qiankun, 2015-11-19 11:02:36]

This is a follow-up CL for https://github.com/KhronosGroup/WebGL/pull/1310 .
PTAL.

[bajones, 2015-11-19 19:16:21]

LGTM, but I've got a couple of readability suggestions.

https://codereview.chromium.org/1463503002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/1463503002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:605: }
A couple of really minor nits here: Could you change the logic to read "&&
target != GL_TEXTURE_2D_ARRAY" instead of "&& target == GL_TEXTURE_3D"? I know
it's functionally equivalent, but it states the purpose of the validation more
explicitly. I had to re-read the statement a couple of times to make sure I knew
what it was testing.

Also, the error string reads strangely to me. I'd suggest simply "target for
ETC2/EAC internal formats must be TEXTURE_2D_ARRAY", and the browser should
provide context for the function which produced the error.

[qiankun, 2015-11-19 22:54:08]

Thank you for review, brandon.

https://codereview.chromium.org/1463503002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/1463503002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:605: }
On 2015/11/19 19:16:21, bajones wrote:
> A couple of really minor nits here: Could you change the logic to read "&&
> target != GL_TEXTURE_2D_ARRAY" instead of "&& target == GL_TEXTURE_3D"? I know
> it's functionally equivalent, but it states the purpose of the validation more
> explicitly. I had to re-read the statement a couple of times to make sure I
knew
> what it was testing.
> 
> Also, the error string reads strangely to me. I'd suggest simply "target for
> ETC2/EAC internal formats must be TEXTURE_2D_ARRAY", and the browser should
> provide context for the function which produced the error.

Done.

[bajones, 2015-11-19 23:04:23]

Thanks, LGTM again!

[qiankun, 2015-11-19 23:10:48]

The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 23:12:20]

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

[Ken Russell (switch to Gerrit), 2015-11-19 23:25:55]

https://codereview.chromium.org/1463503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/1463503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:109:
const GLenum kCompressedTextureFormatsETC2EAC[] = {
Less code duplication could have been achieved by removing these formats from
kSupportedInternalFormatsStorage and writing below:

m_supportedInternalFormatsStorage.insert(kSupportedInternalFormatsStorage,
kSupportedInternalFormatsStorage + arraysize(kSupportedInternalFormatsStorage));
m_supportedInternalFormatsStorage.insert(kCompressedTextureFormatsETC2EAC,
kCompressedTextureFormatsETC2EAC + arraysize(kCompressedTextureFormatsETC2EAC));
m_compressedTextureFormatsETC2EAC.insert(kCompressedTextureFormatsETC2EAC,
kCompressedTextureFormatsETC2EAC + arraysize(kCompressedTextureFormatsETC2EAC));

[qiankun, 2015-11-19 23:50:04]

https://codereview.chromium.org/1463503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/1463503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:109:
const GLenum kCompressedTextureFormatsETC2EAC[] = {
On 2015/11/19 23:25:55, Ken Russell wrote:
> Less code duplication could have been achieved by removing these formats from
> kSupportedInternalFormatsStorage and writing below:
> 
> m_supportedInternalFormatsStorage.insert(kSupportedInternalFormatsStorage,
> kSupportedInternalFormatsStorage +
arraysize(kSupportedInternalFormatsStorage));
> m_supportedInternalFormatsStorage.insert(kCompressedTextureFormatsETC2EAC,
> kCompressedTextureFormatsETC2EAC +
arraysize(kCompressedTextureFormatsETC2EAC));
> m_compressedTextureFormatsETC2EAC.insert(kCompressedTextureFormatsETC2EAC,
> kCompressedTextureFormatsETC2EAC +
arraysize(kCompressedTextureFormatsETC2EAC));

Good suggestion! Done.

[Ken Russell (switch to Gerrit), 2015-11-19 23:52:07]

Thanks for the cleanup. LGTM

[qiankun, 2015-11-20 03:03:17]

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

[qiankun, 2015-11-20 03:03:17]

The patchset sent to the CQ was uploaded after l-g-t-m from bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/1463503002/#ps40001
(title: "remove duplicate code")

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

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

[commit-bot: I haz the power, 2015-11-20 05:09:23]

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

[commit-bot: I haz the power, 2015-11-20 05:09:23]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[qiankun, 2015-11-20 06:03:40]

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

[commit-bot: I haz the power, 2015-11-20 06:03:53]

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

[commit-bot: I haz the power, 2015-11-20 06:08:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-20 06:09:45]

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

[Jamie Madill, 2015-11-20 16:50:45]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1462143003/ by jmadill@chromium.org.

The reason for reverting is: Failing Windows WebGL 2 CTS:

https://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20Release%20%28NV...

WebglConformance.conformance2_textures_misc_tex_storage_compressed_formats:
Failure: getError expected: NO_ERROR. Was INVALID_OPERATION : texStorage3D
should succeed for COMPRESSED_R11_EAC
FAIL getError expected: NO_ERROR. Was INVALID_OPERATION : texStorage3D should
succeed for COMPRESSED_R11_EAC
getError expected: NO_ERROR. Was INVALID_OPERATION : texStorage3D should succeed
for COMPRESSED_SIGNED_R11_EAC
FAIL getError expected: NO_ERROR. Was INVALID_OPERATION : texStorage3D should
succeed for COMPRESSED_SIGNED_R11_EAC
getError expected: NO_ERROR. Was INVALID_OPERATION : texStorage3D should succeed
for COMPRESSED_RG11_EAC
etc.

[zmo, 2015-11-20 18:31:45]

The issue is the conformance test in Chromium is out of date and incorrect.

I'll roll conformance tests  today and reland this CL.

[zmo, 2015-11-23 18:56:38]

Description was changed from

==========
Fix TexStorage3D with compressed ETC2/EAC formats

These formats don't work for TexStorage3D if target is TEXTURE_3D.
TEXTURE_2D_ARRAY should work.

BUG=295792
TEST=conformance2/textures/misc/tex-storage-compressed-formats.html

Committed: https://crrev.com/5b5a54a2d7c287e09f644d04e29718b310949221
Cr-Commit-Position: refs/heads/master@{#360760}
==========

to

==========
Fix TexStorage3D with compressed ETC2/EAC formats

These formats don't work for TexStorage3D if target is TEXTURE_3D.
TEXTURE_2D_ARRAY should work.

BUG=295792
TEST=conformance2/textures/misc/tex-storage-compressed-formats.html

Committed: https://crrev.com/5b5a54a2d7c287e09f644d04e29718b310949221
Cr-Commit-Position: refs/heads/master@{#360760}
==========

[zmo, 2015-11-23 18:57:43]

On 2015/11/20 18:31:45, zmo wrote:
> The issue is the conformance test in Chromium is out of date and incorrect.
> 
> I'll roll conformance tests  today and reland this CL.

Conformance test roll landed, so relanding this.

[zmo, 2015-11-23 18:57:46]

The CQ bit was checked by zmo@google.com

[commit-bot: I haz the power, 2015-11-23 18:58:54]

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

[commit-bot: I haz the power, 2015-11-23 20:43:01]

Committed patchset #3 (id:40001)

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

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