Bug (NPOTT) – Non-power-of-two texture handle optimization

• Component: WebGL
• Status: RESOLVED FIXED

[Przemyslaw Szymanski]

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.

[Przemyslaw Szymanski]

Created attachment 206132 [details]
Non power of two texture handle optimization

[WebKit Commit Bot]

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.

[Przemyslaw Szymanski]

Created attachment 206133 [details]
Non power of two texture handle optimization

update according to failed webkit style

[Noam Rosenthal]

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?

[Przemyslaw Szymanski]

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

[Przemyslaw Szymanski]

Noam Rosenthal, could you review this patch again? Thank you

[Brent Fulgham]

(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?

[Radar WebKit Bug Importer]

<rdar://problem/25307980>

[Brent Fulgham]

Created attachment 274725 [details]
Patch

[Alex Christensen]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 274755 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 274755 [details]
Patch

Clearing flags on attachment: 274755

Committed r198588: <http://trac.webkit.org/changeset/198588>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.