Summary: | Non-power-of-two texture handle optimization | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Przemyslaw Szymanski <p.szymanski3> | ||||||||||||
Component: | WebGL | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bfulgham, cdumez, commit-queue, dino, esprehn+autocc, gyuyoung.kim, kondapallykalyan, roger_fong, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Przemyslaw Szymanski
2013-07-05 02:05:08 PDT
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? 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. |