1239541 - Pass WebGL2 conformance test tex-input-validation

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Ethan Lin[:ethlin]]

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

Comment 1

[Ethan Lin[:ethlin]]

Attachment: Fix texture checking

I did some fixes for passing the test.

Comment 2

[Ethan Lin[:ethlin]]

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

[Kelsey Gilbert [: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;

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+.

Comment 4

[Ethan Lin[:ethlin]]

Attachment: Fix texture checking

Apply jgilbert's comments.

Comment 5

[Kelsey Gilbert [: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.

Comment 6

[Ethan Lin[:ethlin]]

Attachment: Fix texture checking. (carry r+: jgilbert)

Address jgilbert's comment.

Comment 7

[Ethan Lin[:ethlin]]

try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f0fc65e4980

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55bd36961865

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/55bd36961865

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/55bd36961865