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

[Brian Paul]

On 09/01/2015 02:33 PM, Ian Romanick wrote:
> 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);
>>
>

I'm posting a v2 which addresses most of your points.  I'm checking for 
GL_EXT_texture_compression_s3tc now.  I didn't change the pass &= test() 
statements.

I'll assume your R-b still applies if I don't see another review from 
you in a day.

-Brian