Summary: | Refactor validation checks for texture uploads | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kenneth Russell <kbr> | ||||||||
Component: | WebGL | Assignee: | Jun Jiang <jun.a.jiang> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bajones, buildbot, cmarrin, dglazkov, dino, esprehn+autocc, gman, jfeldstein, jun.a.jiang, nayankk, ojan.autocc, peter+ews, rniwa, tonikitoo, webkit-ews, webkit.review.bot, zmo | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | 110931 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Kenneth Russell
2013-02-27 14:36:00 PST
Created attachment 192942 [details]
Patch
As Kenneth suggested, move all the validation checks to the entry level and turn the check at bottom(tex{Sub}Image2DBase) into assertions. Comment on attachment 192942 [details] Patch Attachment 192942 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17136221 Comment on attachment 192942 [details] Patch Attachment 192942 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17175088 Comment on attachment 192942 [details] Patch Attachment 192942 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17203040 Comment on attachment 192942 [details] Patch Attachment 192942 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17036757 Comment on attachment 192942 [details] Patch Attachment 192942 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17207116 Comment on attachment 192942 [details] Patch Attachment 192942 [details] did not pass cr-android-ews (chromium-android): Output: http://webkit-commit-queue.appspot.com/results/17169478 Comment on attachment 192942 [details] Patch Attachment 192942 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17126354 Comment on attachment 192942 [details]
Patch
Before this is reviewed in detail could you please rebase it on to top of tree and upload another version? Thanks.
Created attachment 193069 [details]
Patch
Fixed the issue that some variables were only used in Debug build and resulted in "unused variable" error for Release build. Comment on attachment 193069 [details]
Patch
Thanks for doing this refactoring. It looks great. bajones, zmo, could you both please take a look at this and see if it looks correct to you? i.e., all the same checks are being done, and none were lost?
What tests were run? Have you done a complete WebGL conformance test run? Mozilla's performance tests? Others?
Hi, Kenneth. Thanks for your comments. I had tested it on WebKit Layout tests, WebGL conformance test and Mozilla's performance tests. If any other tests are needed but missing, I am glad to have a try. (In reply to comment #13) > (From update of attachment 193069 [details]) > Thanks for doing this refactoring. It looks great. bajones, zmo, could you both please take a look at this and see if it looks correct to you? i.e., all the same checks are being done, and none were lost? It looks to me like all the checks are still in place. Thanks for the refactor, the new code certainly feels cleaner! Comment on attachment 193069 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193069&action=review > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3699 > + ASSERT(isGLES2NPOTStrict() || !level || !WebGLTexture::isNPOT(width, height)); You don't neeed isGLES2NPOTStrict() here as it is ASSERT. > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3786 > + if (xoffset + width > texture->getWidth(target, level) || yoffset + height > texture->getHeight(target, level)) { Probably another CL since the code is just copied over, but it seems to me that we need to check for overflow with xoffset + width and yoffset + height. Here and a few other places. Looks good to me otherwise. Thanks for the refactoring. Comment on attachment 193069 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193069&action=review >> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3699 >> + ASSERT(isGLES2NPOTStrict() || !level || !WebGLTexture::isNPOT(width, height)); > > You don't neeed isGLES2NPOTStrict() here as it is ASSERT. Hi, Zhenyao. I was not quite sure if I had caught your idea right. The ASSERT() is much like ASSERT(! (!isGLES2NPOTStrict() && level && WebGLTexture::isNPOT(width, height)) ) here. And isGLES2NPOTStrict() only returns false when extensions "GL_OES_texture_npot" or "GL_ARB_texture_non_power_of_two" is enabled. It doesn't return false constantly. In fact, when I saw the condition statement (if(!isGLES2NPOTStrict())) to determine if to do validation on level and POT status of width and height, I felt the condition statement should be removed. It seems level, width & height won't be validated when isGLES2NPOTStrict() returns true. >> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3786 >> + if (xoffset + width > texture->getWidth(target, level) || yoffset + height > texture->getHeight(target, level)) { > > Probably another CL since the code is just copied over, but it seems to me that we need to check for overflow with xoffset + width and yoffset + height. > > Here and a few other places. Thanks for your comments. It was a nice hit on the potential overflow issue. Adding the overflow checking can catch the error in an early stage and I will add it soon. Comment on attachment 193069 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193069&action=review >>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3699 >>> + ASSERT(isGLES2NPOTStrict() || !level || !WebGLTexture::isNPOT(width, height)); >> >> You don't neeed isGLES2NPOTStrict() here as it is ASSERT. > > Hi, Zhenyao. I was not quite sure if I had caught your idea right. The ASSERT() is much like ASSERT(! (!isGLES2NPOTStrict() && level && WebGLTexture::isNPOT(width, height)) ) here. And isGLES2NPOTStrict() only returns false when extensions "GL_OES_texture_npot" or "GL_ARB_texture_non_power_of_two" is enabled. It doesn't return false constantly. > In fact, when I saw the condition statement (if(!isGLES2NPOTStrict())) to determine if to do validation on level and POT status of width and height, I felt the condition statement should be removed. It seems level, width & height won't be validated when isGLES2NPOTStrict() returns true. We check isGLES2NPOTStrict() only to avoid double effort, it basically checked whether the underlying implementation also does the same NPOT check. It's no big deal, you can leave it here, but removing it in ASSERT makes the code cleaner. Created attachment 195225 [details]
Patch
Thanks for Zhenyao's comments. Uploaded a new patch that removed isGLES2NPOTStrict() in the ASSERT and added overflow check with xoffset + width and yoffset + height. (In reply to comment #21) > Thanks for Zhenyao's comments. Uploaded a new patch that removed isGLES2NPOTStrict() in the ASSERT and added overflow check with xoffset + width and yoffset + height. Thanks. The patch looks good. (Usually we use CheckedInt class from mozilla for overflow check. But your code is OK too.) Comment on attachment 195225 [details]
Patch
Nice work. Let's wait for this to clear the mac-ews bot before cq+'ing.
zmo, bajones: thanks for the informal reviews. Comment on attachment 195225 [details]
Patch
It doesn't look like the mac-ews bot is making progress, and the test failures it reported previously don't look related to this patch. This refactoring is important, so marking cq+.
Comment on attachment 195225 [details] Patch Clearing flags on attachment: 195225 Committed r147042: <http://trac.webkit.org/changeset/147042> All reviewed patches have been landed. Closing bug. |