Bug 118409 (NPOTT) - Non-power-of-two texture handle optimization
Summary: Non-power-of-two texture handle optimization
Status: RESOLVED FIXED
Alias: NPOTT
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-07-05 02:05 PDT by Przemyslaw Szymanski
Modified: 2016-03-23 12:21 PDT (History)
9 users (show)

See Also:


Attachments
Non power of two texture handle optimization (12.31 KB, patch)
2013-07-05 04:19 PDT, Przemyslaw Szymanski
no flags Details | Formatted Diff | Diff
Non power of two texture handle optimization (12.30 KB, patch)
2013-07-05 04:27 PDT, Przemyslaw Szymanski
noam: review-
Details | Formatted Diff | Diff
Non power of two texture handle optimization (12.43 KB, patch)
2013-07-09 04:27 PDT, Przemyslaw Szymanski
no flags Details | Formatted Diff | Diff
Patch (9.62 KB, patch)
2016-03-22 21:11 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (21.75 KB, patch)
2016-03-23 10:15 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Przemyslaw Szymanski 2013-07-05 02:05:08 PDT
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.
Comment 1 Przemyslaw Szymanski 2013-07-05 04:19:48 PDT
Created attachment 206132 [details]
Non power of two texture handle optimization
Comment 2 WebKit Commit Bot 2013-07-05 04:22:09 PDT
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.
Comment 3 Przemyslaw Szymanski 2013-07-05 04:27:37 PDT
Created attachment 206133 [details]
Non power of two texture handle optimization

update according to failed webkit style
Comment 4 Noam Rosenthal 2013-07-09 02:19:05 PDT
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?
Comment 5 Przemyslaw Szymanski 2013-07-09 04:27:25 PDT
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
Comment 6 Przemyslaw Szymanski 2013-07-11 23:35:25 PDT
Noam Rosenthal, could you review this patch again? Thank you
Comment 7 Brent Fulgham 2016-03-21 17:18:21 PDT
(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?
Comment 8 Radar WebKit Bug Importer 2016-03-22 21:03:33 PDT
<rdar://problem/25307980>
Comment 9 Brent Fulgham 2016-03-22 21:11:19 PDT
Created attachment 274725 [details]
Patch
Comment 10 Alex Christensen 2016-03-22 22:47:18 PDT
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 11 Brent Fulgham 2016-03-23 10:14:20 PDT
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.
Comment 12 Brent Fulgham 2016-03-23 10:15:00 PDT
Created attachment 274755 [details]
Patch
Comment 13 WebKit Commit Bot 2016-03-23 12:21:13 PDT
Comment on attachment 274755 [details]
Patch

Clearing flags on attachment: 274755

Committed r198588: <http://trac.webkit.org/changeset/198588>
Comment 14 WebKit Commit Bot 2016-03-23 12:21:19 PDT
All reviewed patches have been landed.  Closing bug.