Bug 38145 – Validate *tex* functions input parameters according to ES2 conformance

• Component: WebGL
• Status: RESOLVED FIXED

[Zhenyao Mo]

If target/format/internalformat/type does not belong to ES2 subset, generate INVALID_ENUM.
If border != 0, generate INVALID_VALUE.
If format != internalformat, generate INVALID_OPERATION.
Simulate glGet* for GL_MAX_CUBE_MAP_TEXTURE_SIZE (map it to GL_MAX_TEXTURE_SIZE).
For *copyTexImage*, if the currently bound framebuffer's format does not contain a superset of the internalformat, generate INVAID_OPERATION.

[Zhenyao Mo]

The active texture unit limits should be queries (currently it's hardwired to 32).

[Zhenyao Mo]

[copy]tex[sub]image/getTexParameter/texParameter/generateMipmap should generate INVALID_OPERATION if no WebGLTexture is bound.

[Kenneth Russell]

(In reply to comment #2)
> [copy]tex[sub]image/getTexParameter/texParameter/generateMipmap should generate
> INVALID_OPERATION if no WebGLTexture is bound.

Gregg Tavares pointed out that it is debatable whether WebGL should enforce its current restrictions around not having a texture bound. Instead it is probably better to match OpenGL ES's behavior and support the notion of an initial texture. There is an email discussion started on the public_webgl list about this.

[Zhenyao Mo]

Created attachment 57278 [details]
patch

This patch does NOT deal with the default texture (0) situation.

[Kenneth Russell]

Comment on attachment 57278 [details]
patch

WebCore/html/canvas/WebGLRenderingContext.h:356
 +          Vector<TextureUnitState> m_textureUnits;
This is good.

WebCore/html/canvas/WebGLRenderingContext.h:357
 +          unsigned m_maxCombinedTextureImageUnits;
This member should be removed and m_textureUnits.size() be used everywhere it is. Having duplicate copies of this data will lead to errors.


WebCore/html/canvas/WebGLRenderingContext.cpp:1893
 +          switch (format) {
The format and type checking is duplicated between this function and texSubImage2D. Can you refactor it into a helper function returning a boolean? It could take take all of format, internalformat and type, as well as a boolean indicating whether to verify format against internalformat.


WebCore/html/canvas/WebGLRenderingContext.cpp:2065
 +          switch (target) {
This verification code is duplicated between texParameterf and texParameteri. Can you refactor it?


WebCore/html/canvas/WebGLTexture.h:56
 +          unsigned long getInternalformat() const { return m_internalformat; }
While I understand that this capitalization matches that in the OpenGL header, it is jarring to read in the WebKit code base. Would you please rename "internalformat" to "internalFormat" and "Internalformat" to "InternalFormat" everywhere in this patch?


LayoutTests/fast/canvas/webgl/tex-input-validation.html:177
 +  gl.copyTexSubImage2D(target, level, xoffset, yoffset, 0, 0, width, height, border);
In this test case, I understand your desire to use named parameters, but the fact that assignments to variables like "border" much higher up in the test case affect later calls make it difficult to understand the logic. There is also a fair amount of duplicated code in the test. Can you think about ways to improve this? One idea would be to have a helper function for each individual test, though that doesn't really solve the problem at the call sites. Maybe consider passing objects which contain "format", "internalformat", etc. attributes like in array-unit-tests.html.

[Zhenyao Mo]

(In reply to comment #5)
> (From update of attachment 57278 [details])
> WebCore/html/canvas/WebGLRenderingContext.h:356
>  +          Vector<TextureUnitState> m_textureUnits;
> This is good.
> 
> WebCore/html/canvas/WebGLRenderingContext.h:357
>  +          unsigned m_maxCombinedTextureImageUnits;
> This member should be removed and m_textureUnits.size() be used everywhere it is. Having duplicate copies of this data will lead to errors.

Done.

> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:1893
>  +          switch (format) {
> The format and type checking is duplicated between this function and texSubImage2D. Can you refactor it into a helper function returning a boolean? It could take take all of format, internalformat and type, as well as a boolean indicating whether to verify format against internalformat.

Done. See function validateTexFuncParameters().

> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:2065
>  +          switch (target) {
> This verification code is duplicated between texParameterf and texParameteri. Can you refactor it?

Done.  See function texParameter().

> 
> 
> WebCore/html/canvas/WebGLTexture.h:56
>  +          unsigned long getInternalformat() const { return m_internalformat; }
> While I understand that this capitalization matches that in the OpenGL header, it is jarring to read in the WebKit code base. Would you please rename "internalformat" to "internalFormat" and "Internalformat" to "InternalFormat" everywhere in this patch?

There are already many internalformat in WebGL (for example, a member in WebGLRenderbuffer, and many function parameters in WebGLRenderingConrext).  I think we should use internalformat here and later create another bug to change all internalformat to internalFormat.  This way, the naming will always be consistent.

> 
> 
> LayoutTests/fast/canvas/webgl/tex-input-validation.html:177
>  +  gl.copyTexSubImage2D(target, level, xoffset, yoffset, 0, 0, width, height, border);
> In this test case, I understand your desire to use named parameters, but the fact that assignments to variables like "border" much higher up in the test case affect later calls make it difficult to understand the logic. There is also a fair amount of duplicated code in the test. Can you think about ways to improve this? One idea would be to have a helper function for each individual test, though that doesn't really solve the problem at the call sites. Maybe consider passing objects which contain "format", "internalformat", etc. attributes like in array-unit-tests.html.

Done.

[Zhenyao Mo]

Created attachment 57697 [details]
revised patch : responding to Ken Russell's review

[Zhenyao Mo]

Created attachment 57698 [details]
revised patch: fix the url in changeLog.

[Kenneth Russell]

Comment on attachment 57698 [details]
revised patch: fix the url in changeLog.

Sorry, I didn't notice this the last time:

WebCore/html/canvas/WebGLRenderingContext.cpp:455
 +      unsigned long error = m_context->getError();
This and the other getError checks in texImage2D, etc. aren't correct. glGetError may return multiple errors in sequential calls. To be correct, getError would need to be called in a loop before the call to copyTexImage2D / texImage2D / etc. and all of the errors detected synthesized again afterward. However I think this is the wrong approach. If you look at the command buffer code in Chrome (GLES2DecoderImpl::DoTexImage2D) it doesn't call glGetError when deciding when to update its notion of the texture's size, format, etc. It does all of the required checks up front. I think the !isGLES2Compliant() case in this code should be structured more like that. It will be much more efficient than calling getError() potentially multiple times upon each call to texImage2D, etc.

The refactoring of the test case looks nice.

Renaming the internalformat arguments and functions in a separate bug sounds fine; please file a cleanup bug and CC: me on it.

[Zhenyao Mo]

(In reply to comment #9)
> (From update of attachment 57698 [details])
> Sorry, I didn't notice this the last time:
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:455
>  +      unsigned long error = m_context->getError();
> This and the other getError checks in texImage2D, etc. aren't correct. glGetError may return multiple errors in sequential calls. To be correct, getError would need to be called in a loop before the call to copyTexImage2D / texImage2D / etc. and all of the errors detected synthesized again afterward. However I think this is the wrong approach. If you look at the command buffer code in Chrome (GLES2DecoderImpl::DoTexImage2D) it doesn't call glGetError when deciding when to update its notion of the texture's size, format, etc. It does all of the required checks up front. I think the !isGLES2Compliant() case in this code should be structured more like that. It will be much more efficient than calling getError() potentially multiple times upon each call to texImage2D, etc.

Current !isGLES2Compliant() does not do a full check - it only checks the difference betwwen GLES and GL.  In order to avoid getError like command buffer implementation, we have to do a full check here too.  I'll revise.

> 
> The refactoring of the test case looks nice.
> 
> Renaming the internalformat arguments and functions in a separate bug sounds fine; please file a cleanup bug and CC: me on it.

[Zhenyao Mo]

Created attachment 57828 [details]
revised patch: fix the getError() mistake.

[Zhenyao Mo]

*** Bug 39524 has been marked as a duplicate of this bug. ***

[Kenneth Russell]

Comment on attachment 57828 [details]
revised patch: fix the getError() mistake.

Looks good to me. Thanks for your persistence on this fix.

[Dimitri Glazkov (Google)]

Comment on attachment 57828 [details]
revised patch: fix the getError() mistake.

rs=me.

[WebKit Commit Bot]

Comment on attachment 57828 [details]
revised patch: fix the getError() mistake.

Clearing flags on attachment: 57828

Committed r60874: <http://trac.webkit.org/changeset/60874>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/60874 might have broken Chromium Win Release

[Tony Chang]

(In reply to comment #17)
> http://trac.webkit.org/changeset/60874 might have broken Chromium Win Release

Specifically, the error message is:
4>..\html\canvas\WebGLRenderingContext.cpp(3227) : error C3861: 'log2': identifier not found
4>..\html\canvas\WebGLRenderingContext.cpp(3238) : error C3861: 'log2': identifier not found

Maybe there's some macro conflict?

[Dimitri Glazkov (Google)]

(In reply to comment #18)
> (In reply to comment #17)
> > http://trac.webkit.org/changeset/60874 might have broken Chromium Win Release
> 
> Specifically, the error message is:
> 4>..\html\canvas\WebGLRenderingContext.cpp(3227) : error C3861: 'log2': identifier not found
> 4>..\html\canvas\WebGLRenderingContext.cpp(3238) : error C3861: 'log2': identifier not found
> 
> Maybe there's some macro conflict?

Ken tried fixing it in http://trac.webkit.org/changeset/60876, but it didn't take. Let's just roll it out and fix locally.

[Eric Seidel (no email)]

This broke Chromium Win.

[Tony Chang]

(In reply to comment #20)
> This broke Chromium Win.

Roland has a fix.  He's going to try to land it now.

[Roland Steiner]

Added a 2nd quick fix in r60881 - adding explicit casts to double and double constants - which seems to have done the trick, at least as far as compilation goes (see http://trac.webkit.org/changeset/60881)

This still should be reviewed whether it's a final solution.

[Zhenyao Mo]

Re-read the GLES 2.0.24 Spec.  It seemed like there is no hard limit as level < log2(MAX_TEXTURE_SIZE).  It only says the maximum allowable width/height may be zero for image arrays of any level-of-detail greater than log2(MAX_TEXTURE_SIZE).

In this case, we should remove the code that checks the level upper limit.

[Kenneth Russell]

(In reply to comment #23)
> Re-read the GLES 2.0.24 Spec.  It seemed like there is no hard limit as level < log2(MAX_TEXTURE_SIZE).  It only says the maximum allowable width/height may be zero for image arrays of any level-of-detail greater than log2(MAX_TEXTURE_SIZE).
> 
> In this case, we should remove the code that checks the level upper limit.

OK, but please file a new bug and make it depend on this one.

[Zhenyao Mo]

Will resolve the "level" upper limit issue in a new bug: https://bugs.webkit.org/show_bug.cgi?id=38145.  Close this one.

[Zhenyao Mo]

Woops, wrong new bug URL.  Here is the correct one: https://bugs.webkit.org/show_bug.cgi?id=40603