Closed Bug 1239541 Opened 9 years ago Closed 9 years ago

Pass WebGL2 conformance test tex-input-validation

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: ethlin, Assigned: ethlin)

References

()

Details

Attachments

(1 file, 2 obsolete files)

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
Attached patch Fix texture checking (obsolete) (deleted) — Splinter Review
I did some fixes for passing the test.
Attachment #8707735 - Flags: review?(jgilbert)
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 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-
Attached patch Fix texture checking (obsolete) (deleted) — Splinter Review
Apply jgilbert's comments.
Attachment #8707735 - Attachment is obsolete: true
Attachment #8709831 - Flags: review?(jgilbert)
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+
Address jgilbert's comment.
Attachment #8709831 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: ch
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: