WebKit sets a black texture (with color: 0,0,0,255) instead of a regular texture if some texture parameters are invalid. It has to be done because drivers sets the black texture with color 0,0,0,0 and OpenGL ES specification requires 0,0,0,1 (in float) as invalid texture. But method to handle this case is named as handleNPOTTTextures which in my opinion is wrong. This method handles not only invalid non-power-of-two textures but also power-of-two with invalid texture parameters. So I propose to change its name to handleInvalidTextures. Additionally in WebGLRenderingContext there is a variable m_isNPOTStrict that is a negation of OpenGL NPOT extension existing. Usage of this variable in my opinion is not easy to understand. I propose to change its name to m_isNPOTTSupport which means that OpenGL supports NPOT extension and usage of this variable should be more clear then. And the last one is a little optimization. On modern hardware with OpenGL 4 there can be up to 160 texture units supported. handleNPOTTextures is called two times for each draw call (first call sets a black texture and second call sets a regular texture) for each texture unit (so up to 160x2 iterations!) and checks some conditions. For a number of drawcalls it can cause some performance loss. In a perfect case there should be no invalid textures in WebGL application so method handleNPOTTextures should set no black textures. It is not need to do any additionally iterations if there was not any black texture set.
Created attachment 206132 [details] Non power of two texture handle optimization
Attachment 206132 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/html/canvas/WebGLRenderingContext.cpp', u'Source/WebCore/html/canvas/WebGLRenderingContext.h']" exit_code: 1 Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4826: An else should appear on the same line as the preceding } [whitespace/newline] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 206133 [details] Non power of two texture handle optimization update according to failed webkit style
Comment on attachment 206133 [details] Non power of two texture handle optimization View in context: https://bugs.webkit.org/attachment.cgi?id=206133&action=review Needs a lot of cleanup, mainly names etc. > Source/WebCore/ChangeLog:8 > + This patch optimizes usage of handleNPOTTextures. It is not need to is -> does > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:547 > + if (isNPOTTSupport()) hasNPOTSupport > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:1987 > + bool invalidTex = false; Tex -> Texture > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2072 > + bool invalidTex = false; Ditto > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4803 > +bool WebGLRenderingContext::handleInvalidTextures(const char* functionName, bool prepareToDraw) What are invalid textures? Maybe this should be prepareNonRenderableTextures? > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4806 > + bool blackTexSet = false; something like hasBlackTexture?
Created attachment 206305 [details] Non power of two texture handle optimization update according to the review. I used NPOTT not NPOT name as shortcut of NonPowerOfTwoTextures
Noam Rosenthal, could you review this patch again? Thank you
(In reply to comment #6) > Noam Rosenthal, could you review this patch again? Thank you Is this patch still relevant? If so, could you please update the patch to be compatible with the current WebKit tree and we'll work to get it reviewed?
<rdar://problem/25307980>
Created attachment 274725 [details] Patch
Comment on attachment 274725 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274725&action=review > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:4038 > + return needsToUseBlack2DTexture || needsToUseBlack3DTexture; This will only return true if the final texture is bad. If the second-to-last texture is bad but the final texture is good, this will return false. We need a bool hadInvalidTexture=false; then |= in the loop, then return hadInvalidTexture. Also, you should add a test that has an invalid texture followed by a valid texture to exercise this.
Comment on attachment 274725 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274725&action=review >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:4038 >> + return needsToUseBlack2DTexture || needsToUseBlack3DTexture; > > This will only return true if the final texture is bad. If the second-to-last texture is bad but the final texture is good, this will return false. We need a bool hadInvalidTexture=false; then |= in the loop, then return hadInvalidTexture. Also, you should add a test that has an invalid texture followed by a valid texture to exercise this. Oh, good catch! In fact, this isn't really right either, because it doesn't account for the case where a texture is no longer in the "bad" state, and should not be flagged. I've got a better patch for this. I've also created a test case.
Created attachment 274755 [details] Patch
Comment on attachment 274755 [details] Patch Clearing flags on attachment: 274755 Committed r198588: <http://trac.webkit.org/changeset/198588>
All reviewed patches have been landed. Closing bug.