WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
111012
Refactor validation checks for texture uploads
https://bugs.webkit.org/show_bug.cgi?id=111012
Summary
Refactor validation checks for texture uploads
Kenneth Russell
Reported
2013-02-27 14:36:00 PST
Currently the validation checks performed during texture uploads in WebGL are scattered in many different places in the call stack. The majority of the checks occur in texImage2DBase and texSubImage2DBase, but some of the entry points which call these methods need to pack incoming data into a specified format. Therefore the format and type combination have to be verified as valid beforehand, leading to redundant checks in certain situations. All of the necessary validation code should be refactored and called from the user-facing entry points before calling down further, and in particular, before calling any of the texture data conversion code. Then the checks performed in texImage2DBase and texSubImage2DBase will be unnecessary and can be turned into assertions.
Attachments
Patch
(26.67 KB, patch)
2013-03-13 10:12 PDT
,
Jun Jiang
no flags
Details
Formatted Diff
Diff
Patch
(26.70 KB, patch)
2013-03-13 23:17 PDT
,
Jun Jiang
no flags
Details
Formatted Diff
Diff
Patch
(27.75 KB, patch)
2013-03-26 22:53 PDT
,
Jun Jiang
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Jun Jiang
Comment 1
2013-03-13 10:12:34 PDT
Created
attachment 192942
[details]
Patch
Jun Jiang
Comment 2
2013-03-13 10:20:16 PDT
As Kenneth suggested, move all the validation checks to the entry level and turn the check at bottom(tex{Sub}Image2DBase) into assertions.
Early Warning System Bot
Comment 3
2013-03-13 10:22:27 PDT
Comment on
attachment 192942
[details]
Patch
Attachment 192942
[details]
did not pass qt-ews (qt): Output:
http://webkit-commit-queue.appspot.com/results/17136221
Early Warning System Bot
Comment 4
2013-03-13 10:24:15 PDT
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
WebKit Review Bot
Comment 5
2013-03-13 10:32:41 PDT
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
EFL EWS Bot
Comment 6
2013-03-13 10:35:36 PDT
Comment on
attachment 192942
[details]
Patch
Attachment 192942
[details]
did not pass efl-ews (efl): Output:
http://webkit-commit-queue.appspot.com/results/17036757
Build Bot
Comment 7
2013-03-13 10:40:52 PDT
Comment on
attachment 192942
[details]
Patch
Attachment 192942
[details]
did not pass mac-ews (mac): Output:
http://webkit-commit-queue.appspot.com/results/17207116
Peter Beverloo (cr-android ews)
Comment 8
2013-03-13 10:45:43 PDT
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
Build Bot
Comment 9
2013-03-13 11:13:25 PDT
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
Kenneth Russell
Comment 10
2013-03-13 11:16:39 PDT
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.
Jun Jiang
Comment 11
2013-03-13 23:17:46 PDT
Created
attachment 193069
[details]
Patch
Jun Jiang
Comment 12
2013-03-13 23:22:07 PDT
Fixed the issue that some variables were only used in Debug build and resulted in "unused variable" error for Release build.
Kenneth Russell
Comment 13
2013-03-22 17:16:51 PDT
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?
Jun Jiang
Comment 14
2013-03-25 06:04:00 PDT
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.
Brandon Jones
Comment 15
2013-03-25 10:06:12 PDT
(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!
Zhenyao Mo
Comment 16
2013-03-25 10:28:28 PDT
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.
Zhenyao Mo
Comment 17
2013-03-25 10:28:53 PDT
Looks good to me otherwise. Thanks for the refactoring.
Jun Jiang
Comment 18
2013-03-26 01:01:44 PDT
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.
Zhenyao Mo
Comment 19
2013-03-26 09:36:47 PDT
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.
Jun Jiang
Comment 20
2013-03-26 22:53:25 PDT
Created
attachment 195225
[details]
Patch
Jun Jiang
Comment 21
2013-03-26 23:55:58 PDT
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.
Zhenyao Mo
Comment 22
2013-03-27 09:42:03 PDT
(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.)
Kenneth Russell
Comment 23
2013-03-27 12:27:06 PDT
Comment on
attachment 195225
[details]
Patch Nice work. Let's wait for this to clear the mac-ews bot before cq+'ing.
Kenneth Russell
Comment 24
2013-03-27 12:27:37 PDT
zmo, bajones: thanks for the informal reviews.
Kenneth Russell
Comment 25
2013-03-27 18:00:02 PDT
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+.
WebKit Review Bot
Comment 26
2013-03-27 18:33:18 PDT
Comment on
attachment 195225
[details]
Patch Clearing flags on attachment: 195225 Committed
r147042
: <
http://trac.webkit.org/changeset/147042
>
WebKit Review Bot
Comment 27
2013-03-27 18:33:23 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug