Bug 38145 - Validate *tex* functions input parameters according to ES2 conformance
Summary: Validate *tex* functions input parameters according to ES2 conformance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
: 39524 (view as bug list)
Depends on:
Blocks: 29230 40346 40603
  Show dependency treegraph
 
Reported: 2010-04-26 15:30 PDT by Zhenyao Mo
Modified: 2010-06-23 13:01 PDT (History)
11 users (show)

See Also:


Attachments
patch (30.95 KB, patch)
2010-05-27 15:30 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch : responding to Ken Russell's review (33.75 KB, patch)
2010-06-02 13:51 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch: fix the url in changeLog. (33.76 KB, patch)
2010-06-02 13:54 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch: fix the getError() mistake. (36.24 KB, patch)
2010-06-03 16:13 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhenyao Mo 2010-04-26 15:30:11 PDT
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.
Comment 1 Zhenyao Mo 2010-05-05 18:25:35 PDT
The active texture unit limits should be queries (currently it's hardwired to 32).
Comment 2 Zhenyao Mo 2010-05-05 18:41:46 PDT
[copy]tex[sub]image/getTexParameter/texParameter/generateMipmap should generate INVALID_OPERATION if no WebGLTexture is bound.
Comment 3 Kenneth Russell 2010-05-06 18:15:38 PDT
(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.
Comment 4 Zhenyao Mo 2010-05-27 15:30:50 PDT
Created attachment 57278 [details]
patch

This patch does NOT deal with the default texture (0) situation.
Comment 5 Kenneth Russell 2010-06-01 14:43:38 PDT
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.
Comment 6 Zhenyao Mo 2010-06-02 13:51:09 PDT
(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.
Comment 7 Zhenyao Mo 2010-06-02 13:51:53 PDT
Created attachment 57697 [details]
revised patch : responding to Ken Russell's review
Comment 8 Zhenyao Mo 2010-06-02 13:54:30 PDT
Created attachment 57698 [details]
revised patch: fix the url in changeLog.
Comment 9 Kenneth Russell 2010-06-03 11:51:35 PDT
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.
Comment 10 Zhenyao Mo 2010-06-03 11:59:07 PDT
(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.
Comment 11 Zhenyao Mo 2010-06-03 16:13:58 PDT
Created attachment 57828 [details]
revised patch: fix the getError() mistake.
Comment 12 Zhenyao Mo 2010-06-03 17:11:23 PDT
*** Bug 39524 has been marked as a duplicate of this bug. ***
Comment 13 Kenneth Russell 2010-06-08 13:47:28 PDT
Comment on attachment 57828 [details]
revised patch: fix the getError() mistake.

Looks good to me. Thanks for your persistence on this fix.
Comment 14 Dimitri Glazkov (Google) 2010-06-08 14:14:37 PDT
Comment on attachment 57828 [details]
revised patch: fix the getError() mistake.

rs=me.
Comment 15 WebKit Commit Bot 2010-06-08 17:45:35 PDT
Comment on attachment 57828 [details]
revised patch: fix the getError() mistake.

Clearing flags on attachment: 57828

Committed r60874: <http://trac.webkit.org/changeset/60874>
Comment 16 WebKit Commit Bot 2010-06-08 17:45:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 WebKit Review Bot 2010-06-08 17:59:05 PDT
http://trac.webkit.org/changeset/60874 might have broken Chromium Win Release
Comment 18 Tony Chang 2010-06-08 21:08:47 PDT
(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?
Comment 19 Dimitri Glazkov (Google) 2010-06-08 21:14:09 PDT
(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.
Comment 20 Eric Seidel (no email) 2010-06-08 22:14:33 PDT
This broke Chromium Win.
Comment 21 Tony Chang 2010-06-08 22:16:48 PDT
(In reply to comment #20)
> This broke Chromium Win.

Roland has a fix.  He's going to try to land it now.
Comment 22 Roland Steiner 2010-06-08 22:37:55 PDT
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.
Comment 23 Zhenyao Mo 2010-06-11 15:12:41 PDT
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.
Comment 24 Kenneth Russell 2010-06-11 15:18:22 PDT
(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.
Comment 25 Zhenyao Mo 2010-06-14 18:16:18 PDT
Will resolve the "level" upper limit issue in a new bug: https://bugs.webkit.org/show_bug.cgi?id=38145.  Close this one.
Comment 26 Zhenyao Mo 2010-06-14 18:17:16 PDT
Woops, wrong new bug URL.  Here is the correct one: https://bugs.webkit.org/show_bug.cgi?id=40603