Validate internalformat/format/type for uploading DOM elements to texture

Validate internalformat/format/type for uploading DOM elements to texture

When the data source is a DOM elements, supported
internalformat/format/type combination by TexImage is specified in WebGL
2.0 spec:
https://www.khronos.org/registry/webgl/specs/latest/2.0/#TEXTURE_TYPES_FORMATS_FROM_DOM_ELEMENTS_TABLE.

BUG=650213
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/cf9bdd36fdab41372d9df8f5765f0e388f9d2429
Cr-Commit-Position: refs/heads/master@{#422324}

[qiankun, 2016-09-26 10:15:55]

Description was changed from

==========
Validate internalformat/format/type for uploading DOM elements to texture

When the data source is a DOM elements, supported
internalformat/format/type combination by TexImage is specified in WebGL
2.0 spec:
https://www.khronos.org/registry/webgl/specs/latest/2.0/#TEXTURE_TYPES_FORMAT....

BUG=650213
==========

to

==========
Validate internalformat/format/type for uploading DOM elements to texture

When the data source is a DOM elements, supported
internalformat/format/type combination by TexImage is specified in WebGL
2.0 spec:
https://www.khronos.org/registry/webgl/specs/latest/2.0/#TEXTURE_TYPES_FORMAT....

BUG=650213
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[qiankun, 2016-09-26 14:34:10]

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

[qiankun, 2016-09-26 14:34:11]

PTAL.

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

https://codereview.chromium.org/2370703003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5582: if
(sourceType == SourceHTMLImageElement || sourceType == SourceHTMLCanvasElement
|| sourceType == SourceHTMLVideoElement) {
I am not sure should SourceImageData and SourceImageBitmap apply the same rule.
What's your opinion?

[qiankun, 2016-09-26 15:49:22]

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

[commit-bot: I haz the power, 2016-09-26 15:49:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 18:23:25]

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

[commit-bot: I haz the power, 2016-09-26 18:23:26]

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

[Ken Russell (switch to Gerrit), 2016-09-26 22:19:43]

Sorry, this change doesn't look correct.

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

https://codereview.chromium.org/2370703003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5582: if
(sourceType == SourceHTMLImageElement || sourceType == SourceHTMLCanvasElement
|| sourceType == SourceHTMLVideoElement) {
On 2016/09/26 14:34:11, qiankun wrote:
> I am not sure should SourceImageData and SourceImageBitmap apply the same
rule.
> What's your opinion?

Yes, the same rules apply to all of the types in the TexImageSource union type.
I just clarified this in https://github.com/KhronosGroup/WebGL/pull/2063 .

Even if different rules applied, the code change below would not be correct.
This would make it so that the first call to validateTexFuncFormatAndType would
add one set of enums or the other to the various validators, making them wrong
for the other kinds of uploads for the lifetime of the context. Either more
validators would be needed, or some other solution would need to be found.

[Ken Russell (switch to Gerrit), 2016-09-26 22:20:33]

Also -- is there a conformance test for this change? If not, then please add one
-- and please make sure that it would catch the bug mentioned in the code
review. Thanks.

[qiankun, 2016-09-27 10:22:25]

Thanks for review. I updated the code accordingly. Please review again.

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

https://codereview.chromium.org/2370703003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5582: if
(sourceType == SourceHTMLImageElement || sourceType == SourceHTMLCanvasElement
|| sourceType == SourceHTMLVideoElement) {
On 2016/09/26 22:19:43, Ken Russell wrote:
> On 2016/09/26 14:34:11, qiankun wrote:
> > I am not sure should SourceImageData and SourceImageBitmap apply the same
> rule.
> > What's your opinion?
> 
> Yes, the same rules apply to all of the types in the TexImageSource union
type.
> I just clarified this in https://github.com/KhronosGroup/WebGL/pull/2063 .
> 
> Even if different rules applied, the code change below would not be correct.
> This would make it so that the first call to validateTexFuncFormatAndType
would
> add one set of enums or the other to the various validators, making them wrong
> for the other kinds of uploads for the lifetime of the context. Either more
> validators would be needed, or some other solution would need to be found.

I added another function
WebGLRenderingContextBase::validateTexImageSourceFormatAndType to do format and
type validation for TexImage taking TexImageSource data source. It's clearer
than adding related validations in
WebGLRenderingContextBase::validateTexFuncFormatAndType. 

A conformance test is added to validate TexImage with different data source:
https://github.com/KhronosGroup/WebGL/pull/2064.

I am also preparing other conformance tests to verify invalid
internalformat/format/type for canvas/video/image/image_data/.

[qiankun, 2016-10-01 01:45:40]

Two conformance tests https://github.com/KhronosGroup/WebGL/pull/2064 and
https://github.com/KhronosGroup/WebGL/pull/2068 landed.
They passed with this CL. 

Please help to review when you free. Thanks.

[Zhenyao Mo, 2016-10-01 02:01:44]

On 2016/10/01 01:45:40, qiankun wrote:
> Two conformance tests https://github.com/KhronosGroup/WebGL/pull/2064 and
> https://github.com/KhronosGroup/WebGL/pull/2068 landed.
> They passed with this CL. 
> 
> Please help to review when you free. Thanks.

lgtm

[Ken Russell (switch to Gerrit), 2016-10-01 03:52:05]

Thanks Qiankun for thinking through and revising this.

The new code LGTM. Assuming it passes tests, please feel free to land it. A
couple of questions.

Note also that a massive reformatting of Blink's code just landed to make it
match Chromiums style, so you'll probably have to significantly reformat your
CL.

https://codereview.chromium.org/2370703003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2370703003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:897:
GL_INT,
From the table for texImage2D taking TexImageSource in
https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.6 it looks to me
like GL_UNSIGNED_SHORT and GL_SHORT, GL_UNSIGNED_INT and GL_INT are deliberately
excluded from this list. Can you double-check?

https://codereview.chromium.org/2370703003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1039:
ADD_VALUES_TO_SET(m_supportedTexImageSourceInternalFormats,
kSupportedInternalFormatsES2);
Minor nit: kSupportedInternalFormatsES2 is redundant because ES2 required that
format==internalformat. It could be removed, and replaced everywhere with
kSupportedFormatsES2.

https://codereview.chromium.org/2370703003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5651: }
Only some of these internalformat/format/type combinations are valid; have you
added enough negative tests to ensure that the code in
Source/platform/graphics/gpu/WebGLImageConversion.cpp won't crash if an invalid
combination is specified? I'm thinking something like:

gl.texImage2D(gl.TEXTURE_2D, 0, gl.RG8, width, height, 0, gl.RG, gl.FLOAT,
image_element);

I think this will work OK -- the image conversion code seems to only look at
format and type, and the command buffer will catch the invalid internalformat
usage. I can't see any problems with the combinations listed in the
format/type/internalformat table, but wanted to ask.

[qiankun, 2016-10-01 14:27:30]

https://codereview.chromium.org/2370703003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2370703003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:897:
GL_INT,
On 2016/10/01 03:52:05, Ken Russell wrote:
> From the table for texImage2D taking TexImageSource in
> https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.6 it looks to me
> like GL_UNSIGNED_SHORT and GL_SHORT, GL_UNSIGNED_INT and GL_INT are
deliberately
> excluded from this list. Can you double-check?

Yes. These types are excluded. Thanks.

https://codereview.chromium.org/2370703003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1039:
ADD_VALUES_TO_SET(m_supportedTexImageSourceInternalFormats,
kSupportedInternalFormatsES2);
On 2016/10/01 03:52:05, Ken Russell wrote:
> Minor nit: kSupportedInternalFormatsES2 is redundant because ES2 required that
> format==internalformat. It could be removed, and replaced everywhere with
> kSupportedFormatsES2.

Right, they are same. Removed kSupportedInternalFormatsES2.

https://codereview.chromium.org/2370703003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5651: }
On 2016/10/01 03:52:05, Ken Russell wrote:
> Only some of these internalformat/format/type combinations are valid; have you
> added enough negative tests to ensure that the code in
> Source/platform/graphics/gpu/WebGLImageConversion.cpp won't crash if an
invalid
> combination is specified? I'm thinking something like:
> 
> gl.texImage2D(gl.TEXTURE_2D, 0, gl.RG8, width, height, 0, gl.RG, gl.FLOAT,
> image_element);
> 
> I think this will work OK -- the image conversion code seems to only look at
> format and type, and the command buffer will catch the invalid internalformat
> usage. I can't see any problems with the combinations listed in the
> format/type/internalformat table, but wanted to ask.

Thanks for catching this potential issue.
Code in WebGLImageConversion.cpp won't crash for (RG8, RG, FLOAT) combination.
Command buffer will catch the INVALID_OPERATION error.

I can add such negative case to conformance test: internalformat, format and
type are valid, but their combination is invalid.

[qiankun, 2016-10-01 15:17:22]

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

[qiankun, 2016-10-01 15:17:22]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/2370703003/#ps80001
(title: "fix some nits")

[commit-bot: I haz the power, 2016-10-01 15:17:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-01 18:52:26]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-01 18:53:51]

Description was changed from

==========
Validate internalformat/format/type for uploading DOM elements to texture

When the data source is a DOM elements, supported
internalformat/format/type combination by TexImage is specified in WebGL
2.0 spec:
https://www.khronos.org/registry/webgl/specs/latest/2.0/#TEXTURE_TYPES_FORMAT....

BUG=650213
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Validate internalformat/format/type for uploading DOM elements to texture

When the data source is a DOM elements, supported
internalformat/format/type combination by TexImage is specified in WebGL
2.0 spec:
https://www.khronos.org/registry/webgl/specs/latest/2.0/#TEXTURE_TYPES_FORMAT....

BUG=650213
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/cf9bdd36fdab41372d9df8f5765f0e388f9d2429
Cr-Commit-Position: refs/heads/master@{#422324}
==========

[commit-bot: I haz the power, 2016-10-01 18:53:52]

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