Closed
Bug 1239541
Opened 9 years ago
Closed 9 years ago
Pass WebGL2 conformance test tex-input-validation
Categories
(Core :: Graphics: CanvasWebGL, defect)
Core
Graphics: CanvasWebGL
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: ethlin, Assigned: ethlin)
References
()
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
failed: getError expected: INVALID_ENUM. Was NO_ERROR : paramName: 0x8e42
failed: getError expected one of: INVALID_ENUM or INVALID_VALUE. Was INVALID_OPERATION : internalFormat: RED target: TEXTURE_2D format: RED type: UNSIGNED_BYTE border: 0
failed: getError expected: INVALID_VALUE. Was INVALID_OPERATION : internalFormat: RG target: TEXTURE_3D format: RGBA type: UNSIGNED_BYTE border: 0
failed: getError expected: INVALID_ENUM. Was INVALID_OPERATION : internalFormat: RGBA target: TEXTURE_3D format: RG8 type: UNSIGNED_BYTE border: 0
failed: getError expected: INVALID_ENUM. Was INVALID_OPERATION : format: 0x80e0 type: UNSIGNED_BYTE
Assignee | ||
Comment 1•9 years ago
|
||
I did some fixes for passing the test.
Attachment #8707735 -
Flags: review?(jgilbert)
Assignee | ||
Comment 2•9 years ago
|
||
Comment on attachment 8707735 [details] [diff] [review]
Fix texture checking
Review of attachment 8707735 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/canvas/WebGL2ContextTextures.cpp
@@ +222,1 @@
> return true;
These pnames are from WebGL2 spec[1].
[1] https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.6
::: dom/canvas/WebGLTextureUpload.cpp
@@ +1042,5 @@
> + mContext->ErrorInvalidValue("%s: Invalid internalformat: 0x%04x",
> + funcName, internalFormat);
> + return;
> + }
> +
The spec[1] says:
GL_INVALID_ENUM is generated if type is not a type constant.
GL_INVALID_VALUE is generated if internalFormat is not one of the accepted resolution and format symbolic constants.
GL_INVALID_OPERATION is generated if the combination of internalFormat, format and type is not one of those in the tables above.
So I think we should check type and format first, if both are valid, then return GL_INVALID_OPERATION.
[1] https://www.khronos.org/opengles/sdk/docs/man3/html/glTexImage2D.xhtml
Comment 3•9 years ago
|
||
Comment on attachment 8707735 [details] [diff] [review]
Fix texture checking
Review of attachment 8707735 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/canvas/WebGL2ContextTextures.cpp
@@ +222,1 @@
> return true;
The ones you added are all covered by WebGLContext::IsTexParamValid, which is called below.
Remove the texture swizzles only.
::: dom/canvas/WebGLTextureUpload.cpp
@@ +1042,5 @@
> + mContext->ErrorInvalidValue("%s: Invalid internalformat: 0x%04x",
> + funcName, internalFormat);
> + return;
> + }
> +
These comments should go in the code, not in the review.
This is correct, though I was trying to let us get away with doing less work just to figure out which error to give.
Our code is such that we can more easily say whether a combo is valid, but it's harder to say why it's not valid. Generally, this would be an advantage, but WebGL unfortunately requires us to use a different error for different failure cases. As long as we're doing this *after* we do out quick check of 'is this ok?' and get back 'no', it doesn't hurt to do this. (except if we can reduce the amount of code we have. Less code is always better!)
The changes in this file are r+.
Attachment #8707735 -
Flags: review?(jgilbert) → review-
Assignee | ||
Comment 4•9 years ago
|
||
Apply jgilbert's comments.
Attachment #8707735 -
Attachment is obsolete: true
Attachment #8709831 -
Flags: review?(jgilbert)
Comment 5•9 years ago
|
||
Comment on attachment 8709831 [details] [diff] [review]
Fix texture checking
Review of attachment 8709831 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/canvas/WebGLTextureUpload.cpp
@@ +1036,5 @@
> + * internalformat that is not listed as a valid combination
> + * in tables 3.2 or 3.3 generates the error INVALID_OPERATION."
> + */
> + GL_INVALID_OPERATION is generated if the combination of internalFormat,
> + format and type is not one of those in the tables above.
You left some text outside the comment block.
Also since this comment block is talking about INVALID_OPERATION only, it should be moved down to right before we call ErrorInvalidOperation.
Attachment #8709831 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 6•9 years ago
|
||
Address jgilbert's comment.
Attachment #8709831 -
Attachment is obsolete: true
Assignee | ||
Comment 7•9 years ago
|
||
Whiteboard: ch
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Whiteboard: ch
Keywords: checkin-needed
Comment 9•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment 10•9 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•