[Piglit] [PATCH 10/10] MSAA/formats: fix testing A, L, LA, and I integer formats

[Brian Paul]

On 03/15/2014 10:53 AM, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak at amd.com>
>
> ---
>   tests/spec/ext_framebuffer_multisample/formats.cpp | 14 +++-
>   tests/util/piglit-util-gl-common.h                 |  1 +
>   tests/util/piglit-util-gl.c                        | 87 ++++++++++++++++++++++
>   3 files changed, 100 insertions(+), 2 deletions(-)
>
> diff --git a/tests/spec/ext_framebuffer_multisample/formats.cpp b/tests/spec/ext_framebuffer_multisample/formats.cpp
> index 3694302..7a54851 100644
> --- a/tests/spec/ext_framebuffer_multisample/formats.cpp
> +++ b/tests/spec/ext_framebuffer_multisample/formats.cpp
> @@ -168,18 +168,28 @@ PatternRenderer::try_setup(GLenum internalformat)
>   		GL_FRAMEBUFFER_ATTACHMENT_COMPONENT_TYPE,
>   		(GLint *) &component_type);
>
> +	piglit_get_luminance_intensity_bits(internalformat, color_bits);

Do you want to check the return value here to make sure that 
internalformat is handled?


> +
> +	int num_bits = 0;
> +	for (int i = 0; i < 4 && !num_bits; i++)
> +		num_bits = color_bits[i];
> +	if (!num_bits) {
> +		printf("Red, green, blue, and alpha sizes are all zero.\n");
> +		return false;
> +	}
> +
>   	color_clamping_mode = GL_FIXED_ONLY;
>   	switch (component_type) {
>   	case GL_INT:
>   		assert(test_pattern_ivec4);
>   		test_pattern = test_pattern_ivec4;
> -		color_offset = 1.0 - pow(2.0, color_bits[0] - 1);
> +		color_offset = 1.0 - pow(2.0, num_bits - 1);
>   		color_scale = -2.0 * color_offset;
>   		break;
>   	case GL_UNSIGNED_INT:
>   		assert(test_pattern_uvec4);
>   		test_pattern = test_pattern_uvec4;
> -		color_scale = pow(2.0, color_bits[0]) - 1.0;
> +		color_scale = pow(2.0, num_bits) - 1.0;
>   		color_offset = 0.0;
>   		break;
>   	case GL_UNSIGNED_NORMALIZED:
> diff --git a/tests/util/piglit-util-gl-common.h b/tests/util/piglit-util-gl-common.h
> index 73f2f83..ffa3781 100644
> --- a/tests/util/piglit-util-gl-common.h
> +++ b/tests/util/piglit-util-gl-common.h
> @@ -121,6 +121,7 @@ void piglit_require_gl_version(int required_version_times_10);
>   void piglit_require_extension(const char *name);
>   void piglit_require_not_extension(const char *name);
>   unsigned piglit_num_components(GLenum base_format);
> +bool piglit_get_luminance_intensity_bits(GLenum internalformat, int *bits);
>   int piglit_probe_pixel_rgb_silent(int x, int y, const float* expected, float *out_probe);
>   int piglit_probe_pixel_rgba_silent(int x, int y, const float* expected, float *out_probe);
>   int piglit_probe_pixel_rgb(int x, int y, const float* expected);
> diff --git a/tests/util/piglit-util-gl.c b/tests/util/piglit-util-gl.c
> index 3bb4adf..4f38336 100644
> --- a/tests/util/piglit-util-gl.c
> +++ b/tests/util/piglit-util-gl.c
> @@ -64,6 +64,93 @@ piglit_num_components(GLenum base_format)
>   	}
>   }
>
> +/* OpenGL lacks luminace and intensity bits queries for framebuffers
> + * and renderbuffers. */

You might beef up the comment to say something like the returned number 
of bits is an approximation but should be no less than the actual number 
of bits for the format chosen by OpenGL.


> +bool
> +piglit_get_luminance_intensity_bits(GLenum internalformat, int *bits)
> +{
> +	switch (internalformat) {
> +	case GL_LUMINANCE4:
> +		bits[0] = 4; bits[1] = 4; bits[2] = 4; bits[3] = 0;
> +		return true;
> +
> +	case GL_LUMINANCE:
> +	case GL_LUMINANCE_SNORM:
> +	case GL_LUMINANCE8:
> +	case GL_LUMINANCE8_SNORM:
> +	case GL_LUMINANCE8I_EXT:
> +	case GL_LUMINANCE8UI_EXT:
> +		bits[0] = 8; bits[1] = 8; bits[2] = 8; bits[3] = 0;
> +		return true;
> +
> +	case GL_LUMINANCE12:
> +		bits[0] = 12; bits[1] = 12; bits[2] = 12; bits[3] = 0;
> +		return true;
> +
> +	case GL_LUMINANCE16:
> +	case GL_LUMINANCE16_SNORM:
> +	case GL_LUMINANCE16I_EXT:
> +	case GL_LUMINANCE16UI_EXT:
> +	case GL_LUMINANCE16F_ARB:
> +		bits[0] = 16; bits[1] = 16; bits[2] = 16; bits[3] = 0;
> +		return true;
> +
> +	case GL_LUMINANCE32I_EXT:
> +	case GL_LUMINANCE32UI_EXT:
> +	case GL_LUMINANCE32F_ARB:
> +		bits[0] = 32; bits[1] = 32; bits[2] = 32; bits[3] = 0;
> +		return true;
> +
> +	case GL_LUMINANCE4_ALPHA4:
> +	case GL_INTENSITY4:
> +		bits[0] = 4; bits[1] = 4; bits[2] = 4; bits[3] = 4;

Minor nit, I'd write lines like that as:

> +		return true;
> +
> +	case GL_LUMINANCE_ALPHA:
> +	case GL_LUMINANCE_ALPHA_SNORM:
> +	case GL_LUMINANCE8_ALPHA8:
> +	case GL_LUMINANCE8_ALPHA8_SNORM:
> +	case GL_LUMINANCE_ALPHA8I_EXT:
> +	case GL_LUMINANCE_ALPHA8UI_EXT:
> +	case GL_INTENSITY:
> +	case GL_INTENSITY_SNORM:
> +	case GL_INTENSITY8:
> +	case GL_INTENSITY8_SNORM:
> +	case GL_INTENSITY8I_EXT:
> +	case GL_INTENSITY8UI_EXT:
> +		bits[0] = 8; bits[1] = 8; bits[2] = 8; bits[3] = 8;
> +		return true;
> +
> +	case GL_LUMINANCE12_ALPHA12:
> +	case GL_INTENSITY12:
> +		bits[0] = 12; bits[1] = 12; bits[2] = 12; bits[3] = 12;
> +		return true;
> +
> +	case GL_LUMINANCE16_ALPHA16:
> +	case GL_LUMINANCE16_ALPHA16_SNORM:
> +	case GL_LUMINANCE_ALPHA16I_EXT:
> +	case GL_LUMINANCE_ALPHA16UI_EXT:
> +	case GL_LUMINANCE_ALPHA16F_ARB:
> +	case GL_INTENSITY16:
> +	case GL_INTENSITY16_SNORM:
> +	case GL_INTENSITY16I_EXT:
> +	case GL_INTENSITY16UI_EXT:
> +	case GL_INTENSITY16F_ARB:
> +		bits[0] = 16; bits[1] = 16; bits[2] = 16; bits[3] = 16;
> +		return true;
> +
> +	case GL_LUMINANCE_ALPHA32I_EXT:
> +	case GL_LUMINANCE_ALPHA32UI_EXT:
> +	case GL_LUMINANCE_ALPHA32F_ARB:
> +	case GL_INTENSITY32I_EXT:
> +	case GL_INTENSITY32UI_EXT:
> +	case GL_INTENSITY32F_ARB:
> +		bits[0] = 32; bits[1] = 32; bits[2] = 32; bits[3] = 32;
> +		return true;
> +	}

Do you want to set bits[0..3] = 0 here just to be safe?

> +	return false;
> +}
> +
>   /**
>    * Read a pixel from the given location and compare its RGBA value to the
>    * given expected values.
>

Reviewed-by: Brian Paul <brianp at vmware.com>