[Piglit] [PATCH] arb_copy_image: fix a few error check tests

[Ian Romanick]

On 08/31/2015 04:22 PM, Brian Paul wrote:
> Some of the error checks were incorrect before.  Per the spec:
> 
> 1. GL_TEXTURE_BUFFER and GL_TEXTURE_CUBE_MAP_+/-_XYZ are not legal targets
> and should be flagged as invalid enums.

I think we should not check these targets.  Checking for either error
assumes that the implementation checks for the (multiple) error
conditions in a particular order.

> 2. GL_INVALID_OPERATION should be generated when trying to copy between
> compressed/uncompressed formats whose block/texel size do not match.
> ---
>  tests/spec/arb_copy_image/api_errors.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/spec/arb_copy_image/api_errors.c b/tests/spec/arb_copy_image/api_errors.c
> index 0ef1eda..6f94d06 100644
> --- a/tests/spec/arb_copy_image/api_errors.c
> +++ b/tests/spec/arb_copy_image/api_errors.c
> @@ -140,7 +140,14 @@ test_simple_errors(GLenum src_target, GLenum dst_target)
>  			glCopyImageSubData(src, targets[i], 0, 0, 0, 0,
>  					   dst, dst_target, 0, 0, 0, 0,
>  					   0, 0, 0);
> -			pass &= piglit_check_gl_error(GL_INVALID_ENUM);

I'd support a follow-up patch that changes this test to follow the
piglit idiom of

	pass = test() && pass;

> +                        if (targets[i] == GL_TEXTURE_BUFFER ||
> +			    (targets[i] >= GL_TEXTURE_CUBE_MAP_POSITIVE_X &&
> +			     targets[i] <= GL_TEXTURE_CUBE_MAP_NEGATIVE_Z)) {
> +				pass &= piglit_check_gl_error(GL_INVALID_ENUM);
> +			}
> +			else {
> +				pass &= piglit_check_gl_error(GL_INVALID_VALUE);
> +			}
>  			if (!pass)
>  				return false;
>  		}
> @@ -154,7 +161,14 @@ test_simple_errors(GLenum src_target, GLenum dst_target)
>  			glCopyImageSubData(src, src_target, 0, 0, 0, 0,
>  					   dst, targets[i], 0, 0, 0, 0,
>  					   0, 0, 0);
> -			pass &= piglit_check_gl_error(GL_INVALID_ENUM);
> +                        if (targets[i] == GL_TEXTURE_BUFFER ||
> +			    (targets[i] >= GL_TEXTURE_CUBE_MAP_POSITIVE_X &&
> +			     targets[i] <= GL_TEXTURE_CUBE_MAP_NEGATIVE_Z)) {
> +				pass &= piglit_check_gl_error(GL_INVALID_ENUM);
> +			}
> +			else {
> +				pass &= piglit_check_gl_error(GL_INVALID_VALUE);
> +			}
>  			if (!pass)
>  				return false;
>  		}
> @@ -235,14 +249,15 @@ test_compressed_alignment_errors()

Ugh.  This test is woefully lacking in spec quotations. :(  If you
replace the comment above this line with:

	/* Section 18.3.2 (Copying Between Images) of the OpenGL 4.5 Core
	 * Profile spec says:
	 *
	 *     "An INVALID_OPERATION error is generated if the texel size of
	 *     the uncompressed image is not equal to the block size of the
	 *     compressed image."
	 */

>  	glTexStorage2D(GL_TEXTURE_2D, 1, GL_RGB16UI, 32, 32);
>  	glCopyImageSubData(tex[0], GL_TEXTURE_2D, 0, 0, 0, 0,
>  			   tex[2], GL_TEXTURE_2D, 0, 0, 0, 0, 20, 20, 1);
> -	pass &= piglit_check_gl_error(GL_INVALID_VALUE);
> +	pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
>  
> +        /* Check for invalid copy between different compressed formats */

and replace this comment with

	/* Section 18.3.2 (Copying Between Images) of the OpenGL 4.5 Core
	 * Profile spec says:
	 *
	 *     "An INVALID_OPERATION error is generated if the formats are
         *     not compatible."
         *
         * The definition of compatible refers back to table 8.22 "Compatible
	 * internal formats for TextureView."  This table does not contain
	 * S3TC formats because they are from an older extension.  Given the
	 * different encodings of DXT1 and DXT3 textures, it is reasonable to
	 * assume they would not be compatible for texture views, and this
	 * matches at least NVIDIA's implementation.
	 */

then the last two hunks are

Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

With the amount of effort that justification required, I almost wonder if we
should change the test to use RGTC formats (which are in table 8.22 instead).

It also looks like this test needs to check that one of the S3TC extensions
is supported. :(  I can submit a patch for that... assuming you don't want
to change the test to use RGTC instead.

>  	glBindTexture(GL_TEXTURE_2D, tex[3]);
>  	glTexStorage2D(GL_TEXTURE_2D, 1,
>  		       GL_COMPRESSED_RGBA_S3TC_DXT1_EXT, 32, 32);
>  	glCopyImageSubData(tex[0], GL_TEXTURE_2D, 0, 0, 0, 0,
>  			   tex[3], GL_TEXTURE_2D, 0, 0, 0, 0, 20, 20, 1);
> -	pass &= piglit_check_gl_error(GL_INVALID_VALUE);
> +	pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
>  
>  	glDeleteTextures(4, tex);
>