The OpenGL ES 2.0 does not allow mipmapping of NPOT textures. Most desktop OpenGL implementations do. We need to implement the rules specified in the OpenGL ES 2.0 spec, section 3.8.2.
<rdar://problem/7774338>
In addition to the mipmapping restrictions, OpenGL ES 2.0 (and therefore WebGL) requires that the repeat mode CLAMP_TO_EDGE be used for NPOT textures. Per the OpenGL ES 2.0 specification section 3.8.2, if for a NPOT texture the minification filter is not GL_NEAREST or GL_LINEAR, or the clamp mode is not CLAMP_TO_EDGE, then sampling the texture in a shader results in (R, G, B, A) = (0, 0, 0, 1). glGenerateMipmaps for a NPOT texture also needs to produce INVALID_OPERATION.
Created attachment 54229 [details] patch
Attachment 54229 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/html/canvas/WebGLRenderingContext.cpp:472: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/WebGLRenderingContext.cpp:1414: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] WebCore/html/canvas/WebGLRenderingContext.cpp:1909: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/WebGLRenderingContext.cpp:1981: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/WebGLRenderingContext.cpp:2060: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/WebGLRenderingContext.cpp:2140: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/WebGLTexture.cpp:83: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/WebGLTexture.cpp:161: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/WebGLTexture.cpp:203: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/WebGLTexture.cpp:205: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 10 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #3) > Created an attachment (id=54229) [details] > patch Zhenyao you can test for style correctness yourself by running the check-webkit-style script, it may save some time waiting for the style bot to catch up :D
Created attachment 54230 [details] revised patch: fix style issues
Created attachment 54333 [details] revised patch: fix a tiny issue fallback color should be [0, 0, 0, 1] instead of [0, 0, 0, 0] in the original conformance test we copied.
(In reply to comment #5) > (In reply to comment #3) > > Created an attachment (id=54229) [details] [details] > > patch > > Zhenyao you can test for style correctness yourself by running the > check-webkit-style script, it may save some time waiting for the style bot to > catch up :D Thanks for the info. Sometimes I just get over-confident about my coding style so I unwisely skip the style check. :)
Created attachment 55162 [details] revised patch
Created attachment 55191 [details] revised patch: responding to Gregg Tavares comments The following comments are from Gregg Tavares: """ at line 773 you only check m_activeTextureUnit. You actually have to loop over all the texture units as in for (GLint ii = 0; ii < max_active_texture_units; ++ii) { RefPtr<WebGLTexture> tex2D = m_textureUnits[ii].m_texture2DBinding; Then, if the texture needs binding you have to call glActiveTexture to make that the active texture unit before you call glBindTexture Finally, after you call glDraw and you have to loop again, restore all the textures with a similar loop and finally call glActiveTexture to set it back to what it was before you entered this function. """
Comment on attachment 55191 [details] revised patch: responding to Gregg Tavares comments This is good work, but I think it needs some more work before committing. WebCore/html/canvas/WebGLRenderingContext.cpp:109 + addObject(m_blackTextureCubeMap.get()); I think the logic for setting up the 1x1 black texture should be in a private method of the WebGLRenderingContext, not in the WebGLTexture class. That way you can use WebGLRenderingContext::createTexture() to allocate the object and you don't need the manual calls to addObject, which are error prone. WebCore/html/canvas/WebGLRenderingContext.cpp:458 + tex->setSize(target, width, height); This line should not be executed if the texture is a cube map and the width and height are unequal. That is supposed to yield INVALID_VALUE and the texture's state should remain untouched. Consult Chromium's command buffer service code (TextureManager::ValidForTarget, GLES2DecoderImpl::DoTexImage2D). WebCore/html/canvas/WebGLRenderingContext.cpp:1833 + tex->setSize(target, width, height); Ditto. WebCore/html/canvas/WebGLRenderingContext.cpp:3089 + } These two routines contain nearly identical code. Please refactor them into one method taking a boolean argument. Perhaps "handleNPOTTextures(bool preparingForDraw)". Also please add FIXME around use of constant 32. WebCore/html/canvas/WebGLTexture.cpp:87 + void WebGLTexture::setDimensionality(unsigned long dimensionality) "Dimensionality" is a confusing choice of word. Since everywhere else in the OpenGL spec this concept is "target", please use that term instead. WebCore/html/canvas/WebGLTexture.cpp:165 + if (m_dimensionality == GraphicsContext3D::TEXTURE_CUBE_MAP) Please factor this "if" clause out of the switch statement. WebCore/html/canvas/WebGLTexture.cpp:199 + return false; I'm not sure this short circuit is correct. At least it is inconsistent with the command buffer implementation. I think this should read if (width && (width & (width - 1))) return true; Similarly for height. WebCore/html/canvas/WebGLTexture.cpp:221 + // We only set NPOT for complete cube map textures. Why is this the case? The command buffer implementation also tracks cube map completeness and manually switches in a black texture if the texture is not cube map complete. I don't think the desktop GL spec has the same guarantees that the OpenGL ES 2.0 spec does in this area (See the OpenGL 2.1 spec, section 3.8.10 "Texture Completeness", subsection "Effects of Completeness on Texture Application") so I think you need all of the logic. LayoutTests/fast/canvas/webgl/texture-npot.html:85 + canvas2d); // internalFormat Incorrect comment. LayoutTests/fast/canvas/webgl/texture-npot.html:92 + canvas2d); // internalFormat Incorrect comment. LayoutTests/fast/canvas/webgl/texture-npot.html:138 + // buf[offset + 3]); Please delete commented out code.
(In reply to comment #11) > (From update of attachment 55191 [details]) > This is good work, but I think it needs some more work before committing. > > > WebCore/html/canvas/WebGLRenderingContext.cpp:109 > + addObject(m_blackTextureCubeMap.get()); > I think the logic for setting up the 1x1 black texture should be in a private method of the WebGLRenderingContext, not in the WebGLTexture class. That way you can use WebGLRenderingContext::createTexture() to allocate the object and you don't need the manual calls to addObject, which are error prone. Done. > > > WebCore/html/canvas/WebGLRenderingContext.cpp:458 > + tex->setSize(target, width, height); > This line should not be executed if the texture is a cube map and the width and height are unequal. That is supposed to yield INVALID_VALUE and the texture's state should remain untouched. Consult Chromium's command buffer service code (TextureManager::ValidForTarget, GLES2DecoderImpl::DoTexImage2D). Done inside setSize(). > > WebCore/html/canvas/WebGLRenderingContext.cpp:1833 > + tex->setSize(target, width, height); > Ditto. > Ditto. > WebCore/html/canvas/WebGLRenderingContext.cpp:3089 > + } > These two routines contain nearly identical code. Please refactor them into one method taking a boolean argument. Perhaps "handleNPOTTextures(bool preparingForDraw)". Also please add FIXME around use of constant 32. > Done. > WebCore/html/canvas/WebGLTexture.cpp:87 > + void WebGLTexture::setDimensionality(unsigned long dimensionality) > "Dimensionality" is a confusing choice of word. Since everywhere else in the OpenGL spec this concept is "target", please use that term instead. > Done. > > WebCore/html/canvas/WebGLTexture.cpp:165 > + if (m_dimensionality == GraphicsContext3D::TEXTURE_CUBE_MAP) > Please factor this "if" clause out of the switch statement. Done. > > WebCore/html/canvas/WebGLTexture.cpp:199 > + return false; > I'm not sure this short circuit is correct. At least it is inconsistent with the command buffer implementation. I think this should read > > if (width && (width & (width - 1))) > return true; > > Similarly for height. Per discussion, we won't raise NPOT for incomplete textures. Desktop GL will handle those case. > > WebCore/html/canvas/WebGLTexture.cpp:221 > + // We only set NPOT for complete cube map textures. > Why is this the case? The command buffer implementation also tracks cube map completeness and manually switches in a black texture if the texture is not cube map complete. I don't think the desktop GL spec has the same guarantees that the OpenGL ES 2.0 spec does in this area (See the OpenGL 2.1 spec, section 3.8.10 "Texture Completeness", subsection "Effects of Completeness on Texture Application") so I think you need all of the logic. Per discussion, we won't raise NPOT for incomplete textures. Desktop GL will handle those case. > > > LayoutTests/fast/canvas/webgl/texture-npot.html:85 > + canvas2d); // internalFormat > Incorrect comment. Done. > > > LayoutTests/fast/canvas/webgl/texture-npot.html:92 > + canvas2d); // internalFormat > Incorrect comment. Done (need to fix this in webgl conformance text also). > > > LayoutTests/fast/canvas/webgl/texture-npot.html:138 > + // buf[offset + 3]); > Please delete commented out code. Done.
Created attachment 55789 [details] revised patch : responding to Ken Russell's review
Created attachment 55790 [details] revised patch: tiny modification After setting up black 1x1 textures, need to unbind them.
Comment on attachment 55790 [details] revised patch: tiny modification Looks good. Thanks for doing a thorough job on this.
Comment on attachment 55790 [details] revised patch: tiny modification rs=me.
Comment on attachment 55790 [details] revised patch: tiny modification Clearing flags on attachment: 55790 Committed r59470: <http://trac.webkit.org/changeset/59470>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/59470 might have broken Leopard Intel Debug (Tests)
The sherriff is right. This broke leopard. Zmo: please advise.
Test results linked from here: http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r59472%20(14407)/results.html
The NPOT test is failing on a bot. Reopen this to investigate the cause.
Created attachment 56090 [details] patch Removed the test cases that failed on leopard bot (temporarily). Created another bug to track this: https://bugs.webkit.org/show_bug.cgi?id=39128
Comment on attachment 56090 [details] patch OK. CQ won't work for this since the tree is red. But I"ll land manually.
Comment on attachment 56090 [details] patch Clearing flags on attachment: 56090 Committed r59479: <http://trac.webkit.org/changeset/59479>