[Piglit] [PATCH] Add a test for GLES2 using GL_EXT_unpack_subimage with GL_BGRA_EXT

[Brian Paul]

On 10/02/2012 09:37 AM, Neil Roberts wrote:
> Hi,
>
> Rob Bradford was discussing a bug in Mesa where the fast path code for
> uploading texture data to BGRA textures on Sandybridge does not work
> with a subregion of the buffer using GL_UNPACK_SKIP_*. Here is a test
> for Piglit that exposes this bug on GLES2. It is based on a previous
> test called ext_unpack_subimage.c.
>
> Regards,
> - Neil
>
> -- >8 --
>
> This adds a test which creates a large empty texture and then sets two
> pixels of it using a subregion of a larger buffer. The texture is in
> GL_BGRA format. This tests a specific bug in Mesa introduced in commit
> 413c4914129c.
> ---
>   tests/bugs/CMakeLists.gles2.txt   |  17 +++
>   tests/bugs/unpack-subimage-bgra.c | 211 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 228 insertions(+)
>   create mode 100644 tests/bugs/CMakeLists.gles2.txt
>   create mode 100644 tests/bugs/unpack-subimage-bgra.c
>
> diff --git a/tests/bugs/CMakeLists.gles2.txt b/tests/bugs/CMakeLists.gles2.txt
> new file mode 100644
> index 0000000..ba289d4
> --- /dev/null
> +++ b/tests/bugs/CMakeLists.gles2.txt
> @@ -0,0 +1,17 @@
> +#add_definitions(-DSOURCE_DIR="${piglit_SOURCE_DIR}/")
> +
> +include_directories(
> +	${OPENGL_INCLUDE_PATH}
> +	)
> +
> +link_libraries(
> +	${OPENGL_gles2_LIBRARY}
> +	${OPENGL_egl_LIBRARY}
> +	piglitutil_gles2
> +	)
> +
> +piglit_add_executable(unpack_subimage_bgra
> +	unpack-subimage-bgra.c
> +	)
> +
> +# vim: ft=cmake:
> diff --git a/tests/bugs/unpack-subimage-bgra.c b/tests/bugs/unpack-subimage-bgra.c
> new file mode 100644
> index 0000000..09dac0b
> --- /dev/null
> +++ b/tests/bugs/unpack-subimage-bgra.c
> @@ -0,0 +1,211 @@
> +/*
> + * Copyright © 2012 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Author: Neil Roberts<neil at linux.intel.com>
> + */
> +
> +/** @file unpack-subimage-bgra.c
> + *
> + * Test using the GL_EXT_unpack_subimage extension to upload a
> + * subregion of a buffer to a large texture in BGRA format. This
> + * doesn't work on Sandybridge since commit 413c4914129c of Mesa.
> + */
> +
> +#include<EGL/egl.h>
> +
> +#include "piglit-util-gl-common.h"
> +
> +PIGLIT_GL_TEST_MAIN(
> +    100 /*window_width*/,
> +    100 /*window_height*/,
> +    GLUT_RGB | GLUT_DOUBLE)
> +
> +static GLboolean pass;

Could this var be moved into piglit_display()?


> +
> +#define TEX_WIDTH 128
> +#define TEX_HEIGHT 128
> +
> +static const GLubyte
> +tex_data[] = {
> +	/* Row 0. This is skipped if the extension is available,
> +	   otherwise it makes the top and bottom texel */
> +	0x00, 0x00, 0xff, 0xff, /* red */
> +	0x00, 0xff, 0x00, 0xff, /* green */
> +	/* Row 1. skipped */
> +	0x00, 0x00, 0x00, 0xff,
> +	0x00, 0x00, 0x00, 0xff,
> +	/* Row 2. skipped */
> +	0x00, 0x00, 0x00, 0xff,
> +	0x00, 0x00, 0x00, 0xff,
> +	/* Row 3. skipped */
> +	0x00, 0x00, 0x00, 0xff,
> +	0x00, 0x00, 0x00, 0xff,
> +	/* Row 4. This is the first row used if the extension is
> +	   available */
> +	/* The first pixel is skipped */
> +	0x00, 0x00, 0x00, 0xff,
> +	/* This texel becomes the top texel */
> +	0xff, 0x00, 0x00, 0xff, /* blue */
> +	/* Row 5. first texel not used */
> +	0x00, 0x00, 0x00, 0x00,
> +	/* second texel becomes the bottom texel */
> +	0xff, 0xff, 0x00, 0xff /* cyan */
> +};
> +
> +static const char
> +vertex_shader[] =
> +	"attribute vec4 pos_attrib;\n"
> +	"attribute vec2 tex_attrib;\n"
> +	"varying vec2 tex_coord;\n"
> +	"void main () {\n"
> +	"gl_Position = pos_attrib;\n"
> +	"tex_coord = tex_attrib;\n"
> +	"}\n";
> +
> +static const char
> +fragment_shader[] =
> +	"uniform sampler2D tex;\n"
> +	"varying vec2 tex_coord;\n"
> +	"void main () {\n"
> +	"gl_FragColor = texture2D(tex, tex_coord);\n"
> +	"}\n";
> +
> +static GLuint
> +make_shader(GLenum type,
> +	    const char *source)
> +{
> +	GLuint shader;
> +	GLint length = strlen (source);
> +	GLint status;
> +
> +	shader = glCreateShader(type);
> +	glShaderSource(shader, 1,&source,&length);
> +	glCompileShader(shader);
> +	glGetShaderiv(shader, GL_COMPILE_STATUS,&status);
> +
> +	if (!status)
> +		fprintf(stderr, "Shader compilation failed\n");
> +
> +	return shader;
> +}
> +
> +static void
> +make_program(const char *vertex_source,
> +	     const char *fragment_source)
> +{
> +	GLuint program, shader;
> +	GLuint uniform;
> +	GLint status;
> +
> +	program = glCreateProgram();
> +	shader = make_shader(GL_VERTEX_SHADER, vertex_source);
> +	glAttachShader(program, shader);
> +	shader = make_shader(GL_FRAGMENT_SHADER, fragment_source);
> +	glAttachShader(program, shader);

I think you could use the piglit_compile_shader_text() and 
piglit_link_simple_program() helpers here instead.


> +
> +	glBindAttribLocation(program, PIGLIT_ATTRIB_POS, "pos_attrib");
> +	glBindAttribLocation(program, PIGLIT_ATTRIB_TEX, "tex_attrib");
> +
> +	glLinkProgram(program);
> +
> +	glGetShaderiv(program, GL_LINK_STATUS,&status);
> +	if (!status)
> +		fprintf(stderr, "Program linking failed\n");

And piglit_report_result(PIGLIT_FAIL)?


> +
> +	uniform = glGetUniformLocation(program, "tex");
> +	glUseProgram(program);
> +	glUniform1i(uniform, 0);

I believe the default value of all uniforms is zero so this line isn't 
needed.  No harm though.


> +}
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> +	GLuint tex;
> +	static const float blue[] = { 0, 0, 1, 1 };
> +	static const float cyan[] = { 0, 1, 1, 1 };
> +
> +	pass = GL_TRUE;
> +
> +	if (!piglit_is_extension_supported("GL_EXT_unpack_subimage")) {
> +		if (!piglit_automatic)
> +			printf("GL_EXT_unpack_subimage is not supported\n");
> +		return PIGLIT_SKIP;
> +	}
> +
> +	if (!piglit_is_extension_supported("GL_EXT_texture_format_BGRA8888")) {
> +		if (!piglit_automatic)
> +			printf("GL_EXT_texture_format_BGRA8888 "
> +			       "is not supported\n");
> +		return PIGLIT_SKIP;
> +	}

I think you could remove those tests and put 
piglit_require_extension() calls in the piglit_init() function instead.


> +
> +	glClear(GL_COLOR_BUFFER_BIT);
> +
> +	/* Create a large empty texture */
> +	glGenTextures(1,&tex);
> +	glBindTexture(GL_TEXTURE_2D, tex);

You should reset the GL_UNPACK_* values to their defaults here in case 
the piglit_display() function is called more than once.  Otherwise 
you'll get the values from the previous call, set below.


> +	glTexImage2D(GL_TEXTURE_2D,
> +		     0, /* level */
> +		     GL_BGRA_EXT, /* internalFormat */
> +		     TEX_WIDTH, /* width */
> +		     TEX_HEIGHT, /* height */
> +		     0, /* border */
> +		     GL_BGRA_EXT, /* format */
> +		     GL_UNSIGNED_BYTE, /* type */
> +		     NULL);
> +	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
> +	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
> +
> +	/* Update a 1x2 region in the bottom left of the texture using
> +	 * a subregion of a buffer */
> +	glPixelStorei(GL_UNPACK_ROW_LENGTH, 2);
> +	glPixelStorei(GL_UNPACK_SKIP_PIXELS, 1);
> +	glPixelStorei(GL_UNPACK_SKIP_ROWS, 4);
> +	glTexSubImage2D(GL_TEXTURE_2D,
> +			0, /* level */
> +			0, /* xoffset */
> +			0, /* yoffset */
> +			1, /* width */
> +			2, /* height */
> +			GL_BGRA_EXT,
> +			GL_UNSIGNED_BYTE,
> +			tex_data);
> +
> +	make_program(vertex_shader, fragment_shader);

I think you could make your shaders/program once in the piglit_init() 
function.

In general, keep in mind that the piglit_display() function might be 
called more than once.  This can happen if the window gets resized, or 
possibly exposed.  I seem to recall that freeglut sometimes called its 
'draw' function twice instead of once upon the initial XMapWindow.

Maybe move the texture creation/initialization code into piglit_init() 
also.  Otherwise you'll create a new texture each time 
piglit_display() is called (and it's not deleted).


> +
> +	piglit_draw_rect_tex(-1, -1, 2, 2,
> +			     0, 0, 1.0f / TEX_WIDTH, 2.0f / TEX_HEIGHT);
> +
> +	pass&= piglit_probe_pixel_rgba(piglit_width / 2,
> +					piglit_height / 4,
> +					blue);
> +	pass&= piglit_probe_pixel_rgba(piglit_width / 2,
> +					piglit_height * 3 / 4,
> +					cyan);
> +

You should probably have a call to piglit_present_results() here so 
you can actually see what's drawn.


> +	return pass ? PIGLIT_PASS : PIGLIT_FAIL;
> +}
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
> +}