[Piglit] [PATCH 11/24] arb_direct_state_access: Added tests for glGetCompressedTextureImage.

Ian Romanick idr at freedesktop.org
Tue Jan 20 17:27:16 PST 2015


I know that these patches have landed.  After seeing Vinson's patches to
these test cases, I went back and found the original patches to provide
feedback.

On 12/15/2014 05:24 PM, Laura Ekstrand wrote:
> ---
>  tests/all.py                                       |   3 +-
>  .../spec/arb_direct_state_access/CMakeLists.gl.txt |   2 +
>  .../compressedtextureimage.c                       | 288 ++++++++++++++
>  .../getcompressedtextureimage.c                    | 418 +++++++++++++++++++++

It seems like there are two separate tests added in one commit.

>  tests/util/piglit-util-gl.c                        |   2 +-
>  tests/util/piglit-util-gl.h                        |   1 +
>  6 files changed, 712 insertions(+), 2 deletions(-)
>  create mode 100644 tests/spec/arb_direct_state_access/compressedtextureimage.c
>  create mode 100644 tests/spec/arb_direct_state_access/getcompressedtextureimage.c
> 
> diff --git a/tests/all.py b/tests/all.py
> index 8dfb5ac..b79ae8b 100644
> --- a/tests/all.py
> +++ b/tests/all.py
> @@ -4321,7 +4321,8 @@ spec['ARB_direct_state_access']['gettextureimage-formats'] = PiglitGLTest('arb_d
>  spec['ARB_direct_state_access']['gettextureimage-luminance'] = PiglitGLTest('arb_direct_state_access-gettextureimage-luminance', run_concurrent=True)
>  spec['ARB_direct_state_access']['gettextureimage-simple'] = PiglitGLTest('arb_direct_state_access-gettextureimage-simple', run_concurrent=True)
>  spec['ARB_direct_state_access']['gettextureimage-targets'] = PiglitGLTest('arb_direct_state_access-gettextureimage-targets', run_concurrent=True)
> -
> +spec['ARB_direct_state_access']['compressedtextureimage'] = PiglitGLTest('arb_direct_state_access-compressedtextureimage GL_COMPRESSED_RGBA_FXT1_3DFX', run_concurrent=True)
> +spec['ARB_direct_state_access']['getcompressedtextureimage'] = PiglitGLTest('arb_direct_state_access-getcompressedtextureimage', run_concurrent=True)
>  
>  profile.tests['hiz'] = hiz
>  profile.tests['fast_color_clear'] = fast_color_clear
> diff --git a/tests/spec/arb_direct_state_access/CMakeLists.gl.txt b/tests/spec/arb_direct_state_access/CMakeLists.gl.txt
> index d5a496e..4e80a99 100644
> --- a/tests/spec/arb_direct_state_access/CMakeLists.gl.txt
> +++ b/tests/spec/arb_direct_state_access/CMakeLists.gl.txt
> @@ -23,4 +23,6 @@ piglit_add_executable (arb_direct_state_access-gettextureimage-formats gettextur
>  piglit_add_executable (arb_direct_state_access-gettextureimage-luminance gettextureimage-luminance.c)
>  piglit_add_executable (arb_direct_state_access-gettextureimage-simple gettextureimage-simple.c)
>  piglit_add_executable (arb_direct_state_access-gettextureimage-targets gettextureimage-targets.c)
> +piglit_add_executable (arb_direct_state_access-compressedtextureimage compressedtextureimage.c)
> +piglit_add_executable (arb_direct_state_access-getcompressedtextureimage getcompressedtextureimage.c)
>  # vim: ft=cmake:
> diff --git a/tests/spec/arb_direct_state_access/compressedtextureimage.c b/tests/spec/arb_direct_state_access/compressedtextureimage.c
> new file mode 100644
> index 0000000..d8875b6
> --- /dev/null
> +++ b/tests/spec/arb_direct_state_access/compressedtextureimage.c
> @@ -0,0 +1,288 @@
> +/*
> + * Copyright © 2011 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.
> + */
> +
> +/** @file compressedtextureimage.c
> + *
> + * Tests that fetching and uploading compressed texture data works
> + * correctly.
> + *
> + * The other compressed texture tests are about decoding of data that
> + * was uploaded from uncompressed, while this tries a round-trip after
> + * the initial upload, testing glGetCompressedTexImage() and
> + * glCompressedTexImage2D().
> + *
> + * Adapted for testing glGetCompressedTextureImage in ARB_direct_state_access
> + * by Laura Ekstrand <laura at jlekstrand.net>, November 2014.
> + */
> +
> +#include "piglit-util-gl.h"
> +
> +#define SIZE 128
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> +	config.supports_gl_compat_version = 10;

Uh... I thought we're only exposing GL_ARB_direct_state_access in core
profile.

> +
> +	config.window_width = (SIZE*2)+60;
> +	config.window_height = SIZE+20;
> +	config.window_visual = PIGLIT_GL_VISUAL_RGB | PIGLIT_GL_VISUAL_DOUBLE;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +struct format {
> +	const char *name;
> +	GLenum token;
> +	const char **extension;
> +};
> +
> +static struct format *format;
> +
> +const char *FXT1[] = {

All of these should be static too.

> +	"GL_3DFX_texture_compression_FXT1",
> +	NULL
> +};
> +
> +const char *S3TC[] = {
> +	"GL_EXT_texture_compression_s3tc",
> +	NULL
> +};
> +
> +const char *S3TC_srgb[] = {
> +	"GL_EXT_texture_compression_s3tc",
> +	"GL_EXT_texture_sRGB",
> +	NULL
> +};
> +
> +const char *RGTC[] = {
> +	"GL_ARB_texture_compression_rgtc",
> +	NULL
> +};
> +
> +const char *RGTC_signed[] = {
> +	"GL_ARB_texture_compression_rgtc",
> +	"GL_EXT_texture_snorm",
> +	NULL
> +};
> +
> +const char *BPTC[] = {
> +	"GL_ARB_texture_compression_bptc",
> +	NULL
> +};
> +
> +#define FORMAT(t, ext) { #t, t, ext }
> +static struct format formats[] = {
> +	FORMAT(GL_COMPRESSED_RGB_FXT1_3DFX, FXT1),
> +	FORMAT(GL_COMPRESSED_RGBA_FXT1_3DFX, FXT1),
> +
> +	FORMAT(GL_COMPRESSED_RGB_S3TC_DXT1_EXT, S3TC),
> +	FORMAT(GL_COMPRESSED_RGBA_S3TC_DXT1_EXT, S3TC),
> +	FORMAT(GL_COMPRESSED_RGBA_S3TC_DXT3_EXT, S3TC),
> +	FORMAT(GL_COMPRESSED_RGBA_S3TC_DXT5_EXT, S3TC),
> +
> +	FORMAT(GL_COMPRESSED_SRGB_S3TC_DXT1_EXT, S3TC_srgb),
> +	FORMAT(GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT1_EXT, S3TC_srgb),
> +	FORMAT(GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT3_EXT, S3TC_srgb),
> +	FORMAT(GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT5_EXT, S3TC_srgb),
> +
> +	FORMAT(GL_COMPRESSED_RGBA_BPTC_UNORM, BPTC),
> +	FORMAT(GL_COMPRESSED_SRGB_ALPHA_BPTC_UNORM, BPTC),
> +	FORMAT(GL_COMPRESSED_RGB_BPTC_UNSIGNED_FLOAT, BPTC),
> +	FORMAT(GL_COMPRESSED_RGB_BPTC_SIGNED_FLOAT, BPTC),
> +
> +	FORMAT(GL_COMPRESSED_RED_RGTC1_EXT, RGTC),
> +	FORMAT(GL_COMPRESSED_SIGNED_RED_RGTC1_EXT, RGTC_signed),
> +	FORMAT(GL_COMPRESSED_RED_GREEN_RGTC2_EXT, RGTC),
> +	FORMAT(GL_COMPRESSED_SIGNED_RED_GREEN_RGTC2_EXT, RGTC_signed),
> +};
> +
> +static void
> +display_mipmaps(int x, int y)
> +{
> +	int i;
> +
> +	glEnable(GL_TEXTURE_2D);

This doesn't work in core profile.  See comment (far) above.

> +
> +	/* Disply all the mipmap levels */
> +	for (i = SIZE; i > 0; i /= 2) {
> +		piglit_draw_rect_tex(x, y, i, i,
> +				     0, 0, 1, 1);
> +
> +		x += i + 5;
> +	}
> +}
> +
> +static GLboolean
> +check_resulting_mipmaps(int x, int y)
> +{
> +	GLboolean pass = GL_TRUE;
> +	int size;
> +	float red[4] =   {1.0, 0.0, 0.0, 1.0};
> +	float green[4] = {0.0, 1.0, 0.0, 1.0};
> +	float blue[4] =  {0.0, 0.0, 1.0, 1.0};
> +	float white[4] = {1.0, 1.0, 1.0, 1.0};
> +
> +	/* for r, rg textures, overwrite what the expected colors are. */
> +	if (format->token == GL_COMPRESSED_RED_RGTC1_EXT ||
> +	    format->token == GL_COMPRESSED_SIGNED_RED_RGTC1_EXT) {
> +		green[1] = 0;
> +		blue[2] = 0;
> +		white[1] = 0;
> +		white[2] = 0;
> +	}
> +	if (format->token == GL_COMPRESSED_RED_GREEN_RGTC2_EXT ||
> +	    format->token == GL_COMPRESSED_SIGNED_RED_GREEN_RGTC2_EXT) {
> +		blue[2] = 0;
> +		white[2] = 0;
> +	}
> +
> +	for (size = SIZE; size > 0; size /= 2) {
> +		if (size == 4)
> +			pass = pass && piglit_probe_pixel_rgb(x + 2, y + 2,
> +							      red);

  pass = do_something() && pass;

> +		else if (size == 2)
> +			pass = pass && piglit_probe_pixel_rgb(x + 1, y + 1,
> +							      green);
> +		else if (size == 1)
> +			pass = pass && piglit_probe_pixel_rgb(x, y,
> +							      blue);
> +		else {
> +			pass = pass && piglit_probe_pixel_rgb(x + size / 4,
> +							      y + size / 4,
> +							      red);
> +			pass = pass && piglit_probe_pixel_rgb(x + size * 3 / 4,
> +							      y + size / 4,
> +							      green);
> +			pass = pass && piglit_probe_pixel_rgb(x + size / 4,
> +							      y + size * 3 / 4,
> +							      blue);
> +			pass = pass && piglit_probe_pixel_rgb(x + size * 3 / 4,
> +							      y + size * 3 / 4,
> +							      white);
> +		}
> +		x += size + 5;
> +	}
> +
> +	return pass;
> +}
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> +	GLuint tex, tex_src;
> +	bool pass;
> +	int level;
> +	unsigned bw, bh, bs;
> +
> +	piglit_get_compressed_block_size(format->token, &bw, &bh, &bs);
> +	glClearColor(0.5, 0.5, 0.5, 0.5);
> +	glClear(GL_COLOR_BUFFER_BIT);
> +
> +	tex_src = piglit_rgbw_texture(format->token, SIZE, SIZE,
> +				      GL_TRUE, GL_FALSE,
> +				      GL_UNSIGNED_NORMALIZED);
> +	glCreateTextures(GL_TEXTURE_2D, 1, &tex);
> +
> +	for (level = 0; (SIZE >> level) > 0; level++) {
> +		int w, h;
> +		int expected_size, size;
> +		void *compressed;
> +
> +		w = SIZE >> level;
> +		h = SIZE >> level;
> +		expected_size = piglit_compressed_image_size(format->token, w, h);
> +
> +		glBindTexture(GL_TEXTURE_2D, tex_src);
> +		glGetTexLevelParameteriv(GL_TEXTURE_2D, level,
> +					 GL_TEXTURE_COMPRESSED_IMAGE_SIZE,
> +					 &size);
> +
> +		if (size != expected_size) {
> +			fprintf(stderr, "Format %s level %d (%dx%d) size %d "
> +				"doesn't match expected size %d\n",
> +				format->name, level, w, h, size, expected_size);
> +			piglit_report_result(PIGLIT_FAIL);

Shouldn't this just set pass = false?

> +		}
> +
> +		compressed = malloc(size);
> +
> +		glGetCompressedTextureImage(tex_src, level, size, compressed);
> +
> +		glBindTexture(GL_TEXTURE_2D, tex);
> +		glCompressedTexImage2D(GL_TEXTURE_2D, level, format->token,
> +				       w, h, 0, size, compressed);
> +		if (!piglit_check_gl_error(GL_NO_ERROR))
> +			piglit_report_result(PIGLIT_FAIL);

Ditto.

> +
> +		free(compressed);
> +	}
> +
> +	glDeleteTextures(1, &tex_src);
> +	glBindTextureUnit(tex, 0);
> +
> +	display_mipmaps(10, 10);
> +	pass = check_resulting_mipmaps(10, 10);
> +
> +	piglit_present_results();
> +
> +	return pass ? PIGLIT_PASS : PIGLIT_FAIL;
> +}
> +
> +static void
> +usage(int argc, char **argv)
> +{
> +	int i;
> +
> +	fprintf(stderr, "Usage: %s <format>\n", argv[0]);
> +	fprintf(stderr, "format is one of:\n");
> +	for (i = 0; i < ARRAY_SIZE(formats); i++) {
> +		fprintf(stderr, "  %s\n", formats[i].name);
> +	}
> +	exit(1);
> +}
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
> +	int i;
> +	piglit_require_extension("GL_ARB_direct_state_access");
> +
> +	piglit_ortho_projection(piglit_width, piglit_height, GL_FALSE);
> +
> +	if (argc != 2)
> +		usage(argc, argv);
> +
> +	format = NULL;
> +
> +	for (i = 0; i < ARRAY_SIZE(formats); i++) {
> +		if (strcmp(formats[i].name, argv[1]) == 0) {
> +			format = &formats[i];
> +			break;
> +		}
> +	}
> +
> +	if (!format)
> +		usage(argc, argv);
> +
> +	for (i = 0; format->extension[i]; i++)
> +		piglit_require_extension(format->extension[i]);
> +}
> diff --git a/tests/spec/arb_direct_state_access/getcompressedtextureimage.c b/tests/spec/arb_direct_state_access/getcompressedtextureimage.c
> new file mode 100644
> index 0000000..abbffcf
> --- /dev/null
> +++ b/tests/spec/arb_direct_state_access/getcompressedtextureimage.c
> @@ -0,0 +1,418 @@
> +/*
> + * Copyright © 2012 Marek Olšák <maraeo at gmail.com>
> + * Copyright © 2014 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.
> + */
> +
> +/**
> + * @file getcompressedtextureimage-targets.c
> + *
> + * Adapted for testing glGetCompressedTextureImage in ARB_direct_state_access
> + * by Laura Ekstrand <laura at jlekstrand.net>, November 2014.

We haven't put by-lines or dates in file headers for a long, long time.
 GIT will remember this. :)

> + */
> +
> +#include "piglit-util-gl.h"
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> +	config.window_width = 216;
> +	config.supports_gl_compat_version = 10;
> +
> +	config.window_visual = PIGLIT_GL_VISUAL_RGBA |
> +			       PIGLIT_GL_VISUAL_DOUBLE;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +#define IMAGE_WIDTH 32
> +#define IMAGE_HEIGHT 32
> +#define IMAGE_SIZE (IMAGE_WIDTH * IMAGE_HEIGHT * 4)
> +#define DISPLAY_GAP 4
> +
> +static void
> +show_image(GLubyte *data, int num_layers, const char *title)
> +{
> +	GLuint name;
> +	int i;
> +	char junk[50];
> +
> +	if (!piglit_automatic) {
> +		/* Create the texture handle. */
> +		glCreateTextures(GL_TEXTURE_2D, 1, &name);
> +		glTextureStorage2D(name, 1, GL_RGBA8, IMAGE_WIDTH,
> +			IMAGE_HEIGHT);
> +		glTextureParameteri(name, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
> +		glTextureParameteri(name, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
> +		glEnable(GL_TEXTURE_2D);
> +		glBindTextureUnit(0, name);
> +
> +		/* Draw the layers, separated by some space */
> +		glClear(GL_COLOR_BUFFER_BIT);
> +		for (i = 0; i < num_layers; ++i) {
> +			int x = (IMAGE_WIDTH + DISPLAY_GAP) * (i % 6);
> +			int y = (IMAGE_HEIGHT + DISPLAY_GAP) * (i / 6);
> +			glTextureSubImage2D(name, 0, 0, 0,
> +					    IMAGE_WIDTH, IMAGE_HEIGHT,
> +					    GL_RGBA, GL_UNSIGNED_BYTE,
> +					    data + i * IMAGE_SIZE);
> +			piglit_draw_rect_tex(x, y, IMAGE_WIDTH, IMAGE_HEIGHT,
> +					     0, 0, 1, 1);
> +		}
> +
> +		/* Make the title. */
> +		printf("****** %s ******\n", title);
> +
> +		piglit_present_results();
> +
> +		/* Pause. */
> +		printf("Enter any char to continue.\n>>>>>>");
> +		scanf("%s", junk);
> +		printf("\n");

This is just not acceptable.  There are other tests in piglit that
display multiple results, and none of them resort to using scanf to get
user input.  Display multiple things (at different locations) in the
same window, allow a single subtest to be run independently, etc.

> +
> +		glDeleteTextures(1, &name);
> +	}
> +}
> +
> +static GLubyte *
> +make_layer_data(int num_layers)
> +{
> +	int z;
> +	GLubyte *layer_data =
> +		malloc(num_layers * IMAGE_SIZE * sizeof(GLubyte));
> +
> +	for (z = 0; z < num_layers; z++) {
> +		GLubyte *data = piglit_rgbw_image_ubyte(IMAGE_WIDTH,
> +							IMAGE_HEIGHT, true);
> +		memcpy(layer_data + IMAGE_SIZE * z, data, IMAGE_SIZE);
> +	}
> +
> +	/* Show the first layer of the completed layer data. */
> +	show_image(layer_data, num_layers, "Test Data");
> +
> +	return layer_data;
> +}
> +
> +static bool
> +compare_layer(int layer, int num_elements, int tolerance,
> +			  GLubyte *data, GLubyte *expected)
> +{
> +	int i;
> +
> +	for (i = 0; i < num_elements; ++i) {
> +		if (fabs((int)data[i] - (int)expected[i]) > tolerance) {
> +			printf("GetCompressedTextureImage() returns incorrect"
> +			       " data in byte %i for layer %i\n",
> +			       i, layer);
> +			printf("    corresponding to (%i,%i), channel %i\n",
> +			       (i / 4) / IMAGE_WIDTH, (i / 4) % IMAGE_HEIGHT,
> +				i % 4);
> +			printf("    expected: %i\n", expected[i]);
> +			printf("    got: %i\n", data[i]);
> +			return false;
> +		}
> +	}
> +	return true;
> +}
> +
> +static enum piglit_result
> +getTexImage(bool doPBO, GLenum target, GLubyte *data,
> +	    GLenum internalformat, int tolerance)
> +{
> +	int i;
> +	int num_layers=1, num_faces=1, layer_size;
> +	GLubyte *data2 = NULL;
> +	GLubyte *dataGet;
> +	GLuint packPBO;
> +	bool pass = true;
> +	GLuint name;
> +	GLint compressed;
> +	GLint comp_size;
> +
> +	/* Upload the data. */
> +	switch (target) {
> +
> +	/* These are all targets that can be compressed according to
> +	 * _mesa_target_can_be_compressed */

The format in Mesa and piglit for multiline comments is for the closing
*/ to be on a line by itself.

> +
> +	case GL_TEXTURE_2D:
> +		glCreateTextures(target, 1, &name);
> +		glTextureStorage2D(name, 1, internalformat, IMAGE_WIDTH,
> +				   IMAGE_HEIGHT);
> +		glTextureSubImage2D(name, 0, 0, 0, IMAGE_WIDTH, IMAGE_HEIGHT,
> +				    GL_RGBA, GL_UNSIGNED_BYTE, data);
> +		layer_size = IMAGE_SIZE;
> +		break;
> +
> +	case GL_TEXTURE_CUBE_MAP:
> +		num_faces = 6;
> +		glCreateTextures(target, 1, &name);
> +		/* This is invalid. You must use 2D storage call for cube. */
> +		glTextureStorage3D(name, 1, internalformat,
> +				   IMAGE_WIDTH, IMAGE_HEIGHT, num_faces);
> +		pass &= piglit_check_gl_error(GL_INVALID_ENUM);
> +		glTextureStorage2D(name, 1, internalformat,
> +				   IMAGE_WIDTH, IMAGE_HEIGHT);
> +		/* This is legal. */
> +		glTextureSubImage3D(name, 0, 0, 0, 0, IMAGE_WIDTH,
> +				    IMAGE_HEIGHT, num_faces, GL_RGBA,
> +				    GL_UNSIGNED_BYTE, data);
> +		layer_size = IMAGE_SIZE;
> +		break;
> +
> +	case GL_TEXTURE_2D_ARRAY:
> +		num_layers = 7; /* Fall through. */

Either use /* FALLTHROUGH */ or /* FALLTHRU */ on a line by itself.
Many static analysis tools, dating all the way back to lint in the 70's,
grok the meaning of this.  Other spellings are just comments.

Also... and more importantly, this sets num_layers, but the first line
of the next case re-sets num_layers.  I doubt this is what you wanted.

> +	case GL_TEXTURE_CUBE_MAP_ARRAY:
> +		num_layers = 6 * 3;
> +		glCreateTextures(target, 1, &name);
> +		glTextureStorage3D(name, 1, internalformat, IMAGE_WIDTH,
> +				   IMAGE_HEIGHT, num_layers);
> +		glTextureSubImage3D(name, 0, 0, 0, 0,
> +				    IMAGE_WIDTH, IMAGE_HEIGHT, num_layers,
> +				    GL_RGBA, GL_UNSIGNED_BYTE, data);
> +		layer_size = IMAGE_SIZE;
> +		break;
> +
> +	default:
> +		puts("Invalid texture target.");
> +		return PIGLIT_FAIL;
> +
> +	}
> +
> +	/* Make sure the driver has compressed the image. */
> +	glGetTextureLevelParameteriv(name, 0, GL_TEXTURE_COMPRESSED,
> +		&compressed);
> +	printf("\tIs the texture compressed? %s.\n",
> +		compressed ? "yes" : "no");
> +
> +	glGetTextureLevelParameteriv(name, 0,
> +				     GL_TEXTURE_COMPRESSED_IMAGE_SIZE,
> +				     &comp_size);
> +	/*  The OpenGL 4.5 core spec
> +	 *  (30.10.2014) Section 8.11 Texture Queries says:
> +	 *       "For GetTextureLevelParameter* only, texture may also be a
> +	 *       cube map texture object.  In this case the query is always
> +	 *       performed for face zero (the TEXTURE_CUBE_MAP_POSITIVE_X
> +	 *       face), since there is no way to specify another face."
> +	 */
> +	if (target == GL_TEXTURE_CUBE_MAP)
> +		comp_size *= num_faces;
> +	printf("\tThe size of the texture in bytes is %d.\n", comp_size);
> +
> +	/* Show the uncompressed data. */
> +	show_image(data, num_layers * num_faces, "Data Before Compression");
> +
> +
> +	/* Setup the PBO or data array to read into from
> +	 * glGetCompressedTextureImage */
> +	if (doPBO) {
> +		glGenBuffers(1, &packPBO);
> +		glBindBuffer(GL_PIXEL_PACK_BUFFER, packPBO);
> +		/* Make the buffer big enough to hold uncompressed data. */
> +		glBufferData(GL_PIXEL_PACK_BUFFER, layer_size * num_faces *
> +			     num_layers * sizeof(GLubyte),
> +			     NULL, GL_STREAM_READ);
> +	} else {
> +		glBindBuffer(GL_PIXEL_PACK_BUFFER, 0);
> +		data2 = malloc(layer_size * num_faces * num_layers *
> +			       sizeof(GLubyte));
> +		memset(data2, 123, layer_size * num_faces * num_layers *
> +			       sizeof(GLubyte));

Instead of having the long expression for the size typed three times, it
is better to use a temporary.

            image_bytes = layer_size * num_faces * num_layers *
                sizeof(GLubyte);

I /think/ we could even use C99 mixed-declarations-with-code to make
this a constant.  I'd CC Jose on such a patch, though.

> +	}
> +	pass = piglit_check_gl_error(GL_NO_ERROR) && pass;
> +	assert(num_layers * num_faces * layer_size <= 18 * IMAGE_SIZE);
> +
> +
> +	/* Download the compressed texture image. */
> +	if (doPBO)
> +		glGetCompressedTextureImage(name, 0, comp_size, NULL);
> +	else
> +		glGetCompressedTextureImage(name, 0, comp_size, data2);

This logic (especially with the separated 'if (doPBO)' blocks) gets hard
to follow.  It seems like you could rename data2 to, for example,
allocated_pixels, and rewrite these blocks as

    /* In the PBO case, allocated_pixels is NULL, so the Get will write
     * compressed data to offset 0 of the PBO.  Note: allocated_pixels
     * is NULL iff doPBO.
     */
    assert(doPBO == (allocated_pixels == NULL));
    glGetCompressedTextureImage(name, 0, comp_size, allocated_pixels);

    if (doPBO) {
	dataGet = (GLubyte *) glMapBufferRange(
				       GL_PIXEL_PACK_BUFFER, 0,
				       comp_size,
				       GL_MAP_READ_BIT);
    } else {
        dataGet = allocated_pixels;
    }

This kind of usage is a big part of the reason that VBOs and PBOs didn't
add new entry points to the API.  Re-using the same functions makes it
easier to write transitional code like this.

> +	pass = piglit_check_gl_error(GL_NO_ERROR) && pass;
> +
> +	if (doPBO)
> +		dataGet = (GLubyte *) glMapBufferRange(
> +					       GL_PIXEL_PACK_BUFFER, 0,
> +					       comp_size,
> +					       GL_MAP_READ_BIT);
> +	else
> +		dataGet = data2;
> +
> +	/* Re-upload the texture in compressed form. */
> +	switch (target) {
> +	case GL_TEXTURE_2D:
> +		glCompressedTextureSubImage2D(name, 0, 0, 0,
> +					      IMAGE_WIDTH, IMAGE_HEIGHT,
> +					      internalformat, comp_size,
> +					      dataGet);

Uh... I'm not sure this is legal in the PBO case.  I don't think you're
allowed to pass a pointer back to OpenGL that came from glMapBuffer.  I
believe this is one of the "undefined, possibly including program
termination" cases.  If the data is in a PBO, why not re-upload it from
the PBO?

> +		break;
> +
> +	case GL_TEXTURE_CUBE_MAP:
> +		glCompressedTextureSubImage3D(name, 0, 0, 0, 0,
> +					      IMAGE_WIDTH, IMAGE_HEIGHT,
> +					      num_faces,
> +					      internalformat, comp_size,
> +					      dataGet);
> +		break;
> +
> +	case GL_TEXTURE_2D_ARRAY:
> +	case GL_TEXTURE_CUBE_MAP_ARRAY:
> +		glCompressedTextureSubImage3D(name, 0, 0, 0, 0,
> +					      IMAGE_WIDTH, IMAGE_HEIGHT,
> +					      num_layers,
> +					      internalformat, comp_size,
> +					      dataGet);
> +		break;
> +	}
> +
> +
> +	/* Get the uncompressed version for comparison. */
> +	if (doPBO) {
> +		glUnmapBuffer(GL_PIXEL_PACK_BUFFER);
> +		glGetTextureImage(name, 0, GL_RGBA, GL_UNSIGNED_BYTE,
> +			layer_size * num_layers * num_faces * sizeof(GLubyte),
> +			NULL);
> +	}
> +	else {
> +		glGetTextureImage(name, 0, GL_RGBA, GL_UNSIGNED_BYTE,
> +			layer_size * num_layers * num_faces * sizeof(GLubyte),
> +			data2);
> +	}
> +	pass = piglit_check_gl_error(GL_NO_ERROR) && pass;

Similar comment here as above:

	if (doPBO)
		glUnmapBuffer(GL_PIXEL_PACK_BUFFER);

	/* In the PBO case, allocated_pixels is NULL, so the Get will
	 * write compressed data to offset 0 of the PBO.  Note:
	 * allocated_pixels is NULL iff doPBO.
	 */
	assert(doPBO == (allocated_pixels == NULL));
	glGetTextureImage(name, 0, GL_RGBA, GL_UNSIGNED_BYTE,
			image_bytes, allocated_pixels);

> +	if (doPBO)
> +		dataGet = (GLubyte *) glMapBufferRange(
> +					       GL_PIXEL_PACK_BUFFER, 0,
> +					       layer_size * num_layers *
> +					       num_faces * sizeof(GLubyte),
> +					       GL_MAP_READ_BIT);
> +	else
> +		dataGet = data2;
> +
> +	/* Examine the image after pulling it off the graphics card. */
> +	show_image(dataGet, num_layers * num_faces, "Data After Compression");
> +
> +	/* Do the comparison */
> +	for (i = 0; i < num_faces * num_layers; i++) {
> +		pass = compare_layer(i, layer_size, tolerance, dataGet,
> +				     data + (i * layer_size)) && pass;
> +		dataGet += layer_size;
> +	}
> +
> +	if (doPBO) {
> +		glUnmapBuffer(GL_PIXEL_PACK_BUFFER);
> +		glDeleteBuffers(1, &packPBO);
> +	}
> +
> +	glDeleteTextures(1, &name);
> +	free(data2);
> +
> +	return pass ? PIGLIT_PASS : PIGLIT_FAIL;
> +}
> +
> +struct target_and_mask {
> +	GLenum target;
> +	bool mask;
> +};
> +
> +static struct target_and_mask targets[] = {
> +	{GL_TEXTURE_2D, 1},
> +	{GL_TEXTURE_CUBE_MAP, 1},
> +	{GL_TEXTURE_2D_ARRAY, 1},
> +	{GL_TEXTURE_CUBE_MAP_ARRAY, 1},
> +};
> +
> +static void
> +clear_target_mask(GLenum target)
> +{
> +	int i;
> +	for (i = 0; i < ARRAY_SIZE(targets); ++i) {
> +		if (targets[i].target == target) {
> +			targets[i].mask = 0;

Since this is a bool, I think you want false.

> +		}
> +	}
> +}
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
> +	piglit_require_extension("GL_ARB_direct_state_access");
> +
> +	if (!piglit_is_extension_supported("GL_ARB_texture_cube_map"))
> +		clear_target_mask(GL_TEXTURE_CUBE_MAP);
> +	if (!piglit_is_extension_supported("GL_EXT_texture_array")) {
> +		clear_target_mask(GL_TEXTURE_2D_ARRAY);
> +	}
> +	if (!piglit_is_extension_supported("GL_ARB_texture_cube_map_array"))
> +		clear_target_mask(GL_TEXTURE_CUBE_MAP_ARRAY);

Why is GL_EXT_texture_array the only case with { }?

> +
> +	glClearColor(0.5, 0.5, 0.5, 1);
> +	piglit_ortho_projection(piglit_width, piglit_height, GL_FALSE);
> +}
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> +	int i;
> +	GLenum internalformat = GL_COMPRESSED_RGBA_FXT1_3DFX;
> +	int tolerance = 8;
> +	GLubyte *data;
> +	enum piglit_result subtest;
> +	enum piglit_result result = PIGLIT_PASS;
> +
> +	piglit_require_extension("GL_3DFX_texture_compression_FXT1");

Why is this here?  This also seems like a really bad choice.  The i965
and i915 are the only drivers (closed- or open-source) that support this
antiquated extension.

I don't think this test should require an on-line compressor at all.
There are very few compressed texture formats that matter.  We can
pretty easily generate an S3TC and an ETC version of the RGBW image for
this test.  Anything that supports texture compression will support at
least one of those formats.

> +
> +	data = make_layer_data(18);
> +
> +	for (i = 0; i < ARRAY_SIZE(targets); ++i) {
> +		if (!targets[i].mask)
> +			continue;
> +
> +		printf("Testing %s into PBO\n",
> +			piglit_get_gl_enum_name(targets[i].target));
> +		subtest = getTexImage(true, targets[i].target, data,
> +				      internalformat, tolerance);
> +		piglit_report_subtest_result(subtest, "getTexImage %s PBO",
> +					     piglit_get_gl_enum_name(
> +						targets[i].target));
> +		if (subtest == PIGLIT_FAIL)
> +			result = PIGLIT_FAIL;
> +
> +		printf("\n"); /* Separate tests with some white space. */
> +
> +		printf("Testing %s into client array\n",
> +			piglit_get_gl_enum_name(targets[i].target));
> +		subtest = getTexImage(false, targets[i].target, data,
> +				      internalformat, tolerance);
> +		piglit_report_subtest_result(subtest, "getTexImage %s",
> +					     piglit_get_gl_enum_name(
> +						targets[i].target));
> +		if (subtest == PIGLIT_FAIL)
> +			result = PIGLIT_FAIL;
> +
> +		printf("\n\n"); /* Separate targets with some white space. */
> +
> +		if (!piglit_check_gl_error(GL_NO_ERROR))
> +			result = PIGLIT_FAIL;
> +	}
> +
> +	free(data);
> +
> +	return result;
> +}
> +
> diff --git a/tests/util/piglit-util-gl.c b/tests/util/piglit-util-gl.c
> index e6b7738..aae6df0 100644
> --- a/tests/util/piglit-util-gl.c
> +++ b/tests/util/piglit-util-gl.c
> @@ -2191,7 +2191,7 @@ piglit_rgbw_image(GLenum internalFormat, int w, int h,
>  	return data;
>  }
>  
> -static GLubyte *
> +GLubyte *
>  piglit_rgbw_image_ubyte(int w, int h, GLboolean alpha)
>  {
>  	GLubyte red[4]   = {255, 0, 0, 0};
> diff --git a/tests/util/piglit-util-gl.h b/tests/util/piglit-util-gl.h
> index adf98d8..9d6393c 100644
> --- a/tests/util/piglit-util-gl.h
> +++ b/tests/util/piglit-util-gl.h
> @@ -201,6 +201,7 @@ GLuint piglit_checkerboard_texture(GLuint tex, unsigned level,
>  GLuint piglit_miptree_texture(void);
>  GLfloat *piglit_rgbw_image(GLenum internalFormat, int w, int h,
>                             GLboolean alpha, GLenum basetype);
> +GLubyte *piglit_rgbw_image_ubyte(int w, int h, GLboolean alpha);
>  GLuint piglit_rgbw_texture(GLenum internalFormat, int w, int h, GLboolean mip,
>  		    GLboolean alpha, GLenum basetype);
>  GLuint piglit_depth_texture(GLenum target, GLenum format, int w, int h, int d, GLboolean mip);
> 



More information about the Piglit mailing list