Summary: | Validate *tex* functions input parameters according to ES2 conformance | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zhenyao Mo <zmo> | ||||||||||
Component: | WebGL | Assignee: | Zhenyao Mo <zmo> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, cmarrin, commit-queue, dglazkov, eric, gman, kbr, oliver, rolandsteiner, tony, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 29230, 40346, 40603 | ||||||||||||
Attachments: |
|
Description
Zhenyao Mo
2010-04-26 15:30:11 PDT
The active texture unit limits should be queries (currently it's hardwired to 32). [copy]tex[sub]image/getTexParameter/texParameter/generateMipmap should generate INVALID_OPERATION if no WebGLTexture is bound. (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. Created attachment 57278 [details]
patch
This patch does NOT deal with the default texture (0) situation.
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.
(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. Created attachment 57697 [details]
revised patch : responding to Ken Russell's review
Created attachment 57698 [details]
revised patch: fix the url in changeLog.
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.
(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. Created attachment 57828 [details]
revised patch: fix the getError() mistake.
*** Bug 39524 has been marked as a duplicate of this bug. *** Comment on attachment 57828 [details]
revised patch: fix the getError() mistake.
Looks good to me. Thanks for your persistence on this fix.
Comment on attachment 57828 [details]
revised patch: fix the getError() mistake.
rs=me.
Comment on attachment 57828 [details] revised patch: fix the getError() mistake. Clearing flags on attachment: 57828 Committed r60874: <http://trac.webkit.org/changeset/60874> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/60874 might have broken Chromium Win Release (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? (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. This broke Chromium Win. (In reply to comment #20) > This broke Chromium Win. Roland has a fix. He's going to try to land it now. 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. 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. (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. Will resolve the "level" upper limit issue in a new bug: https://bugs.webkit.org/show_bug.cgi?id=38145. Close this one. Woops, wrong new bug URL. Here is the correct one: https://bugs.webkit.org/show_bug.cgi?id=40603 |