[Piglit] [PATCH v2 3/4] Add miptree tests for khr_texture_compression_astc
Chad Versace
chad.versace at intel.com
Wed Sep 9 17:04:06 PDT 2015
On Fri 28 Aug 2015, Nanley Chery wrote:
> From: Nanley Chery <nanley.g.chery at intel.com>
>
> These tests run through every ASTC configuration, comparing the
> render of a compressed texture against a render of the decompressed
> version of that compressed texture. The compressed and decompressed
> texture was generated with a reference codec.
>
> Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> diff --git a/tests/spec/khr_texture_compression_astc/CMakeLists.gl.txt b/tests/spec/khr_texture_compression_astc/CMakeLists.gl.txt
> new file mode 100644
> index 0000000..a47c7d3
> --- /dev/null
> +++ b/tests/spec/khr_texture_compression_astc/CMakeLists.gl.txt
> @@ -0,0 +1,14 @@
> +include_directories(
> + ${GLEXT_INCLUDE_DIR}
> + ${OPENGL_INCLUDE_PATH}
> +)
I think you can drop the call to include_directories. And If you can,
you should. Please try rebuilding without it.
> +
> +link_libraries (
> + piglitutil_${piglit_target_api}
> + ${OPENGL_gl_LIBRARY}
> + ${OPENGL_glu_LIBRARY}
> +)
As above, I think you can drop the explicit linking to
OPENGL_gl_LIBRARY. And you can definitiely drop the explicit linking to
OPENGL_glu_LIBRARY.
> +
> +piglit_add_executable(khr_compressed_astc-miptree_${piglit_target_api} khr_compressed_astc-miptree.c)
> +
> +# vim: ft=cmake:
> diff --git a/tests/spec/khr_texture_compression_astc/CMakeLists.gles2.txt b/tests/spec/khr_texture_compression_astc/CMakeLists.gles2.txt
> new file mode 100644
> index 0000000..047b8ac
> --- /dev/null
> +++ b/tests/spec/khr_texture_compression_astc/CMakeLists.gles2.txt
> @@ -0,0 +1,8 @@
> +include_directories(
> + ${GLEXT_INCLUDE_DIR}
> + ${OPENGL_INCLUDE_PATH}
> +)
As for CMakeLists.gl.txt, I think you should drop this call
include_directories if Piglit can build without it.
> +link_libraries(piglitutil_${piglit_target_api})
> +piglit_add_executable(khr_compressed_astc-miptree_${piglit_target_api} khr_compressed_astc-miptree.c)
> +
> +# vim: ft=cmake:
> diff --git a/tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c b/tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c
> new file mode 100644
> index 0000000..a804b9b
> --- /dev/null
> +++ b/tests/spec/khr_texture_compression_astc/khr_compressed_astc-miptree.c
> +/**
> + * \file
> + * \brief Test texturing from an ASTC miptree of a real image.
> + *
> + * This test is an adaptation of the oes_compressed_etc1_rgb8_textures test.
> + *
> + * This test uses eighty-four data files. The files under compressed/ contain
^^^^^^^^^^^
Remove "eighty-four" from the comment. It puts the comment at high risk
of becoming stale.
> + * full miptrees, in the GL_*_ASTC_* formats, of a 2D texture of waffles and
> + * fruit [1]. The base level size was shrunken to 160x106 pixels. The files
> + * under the decompressed directory contain the same miptree in GL_RGBA
> + * format. Each miplevel was obtained by decompressing the corresponding ASTC
> + * texture with astcenc [2].
> + *
> + * This test draws miplevels of the compressed textures according to the
> + * MIPLAYOUT_BELOW organization scheme. It does the same when drawing
^^^^^^^^^^^^^^^
MIPLAYOUT_BELOW is a term specific to Intel hardware that no one but
Intel miptree ninjas like yourself will recognize. (In several more years,
perhaps *no one* will recognize it, because
MIPLAYOUT_BELOW/MIPLAYOUT_THE_OTHER_ONE was configurable only on legacy
hardware). If you want to describe the test's image layout, draw
a little ASCII diagram instead.
> + * the decompressed texture on the right. Each miplevel of both images are
> + * compared for equality after each level is drawn.
^^^^^
drawn
> + *
> + * [1] The reference image is located at http://people.freedesktop.org/~chadversary/permalink/2012-07-09/1574cff2-d091-4421-a3cf-b56c7943d060.jpg.
> + * [2] astcenc is the reference ASTC compression tool, available at http://malideveloper.arm.com/develop-for-mali/tools/software-tools/astc-evaluation-codec/.
> + */
> +
> +#include "piglit-util-gl.h"
> +#include "piglit_ktx.h"
> +
> +#define num_levels 8
> +#define level0_width 160
> +#define level0_height 106
> +
> +#define num_vertices 4
To comply with Piglit's code style, the macros above should ALL_CAPS.
> +
> +static GLuint prog;
> +
> +static struct piglit_gl_test_config *piglit_config;
> +
> +typedef struct _test_data
The struct tag '_test_data' is nowhere used, and so should be removed.
> +{
> + bool hdr_test;
> + bool srgb_test;
> +} test_data;
More importantly, this struct should be converted to an enum.
Each field of the struct is already pairwise mutually exclusive. And the
struct contains an implicit 3rd field, ldr_test, which occurs when
!(hdr_test || sgrb_test). Therefore this enum is a good replacement:
enum test_type {
TEST_TYPE_LDR,
TEST_TYPE_HDR,
TEST_TYPE_SRGB,
};
> +
> +
> +/**
> + * The \a filename is relative to the current test's source directory.
> + *
> + * A new texture is created and returned in \a tex_name.
> + */
> +static void
> +load_texture(const char *dir1, const char *dir2,
> + const char *filename, GLuint *tex_name)
> +{
> + struct piglit_ktx *ktx;
> + const struct piglit_ktx_info *info;
> + char filepath[4096];
> + bool ok = true;
> +
> + piglit_join_paths(filepath, sizeof(filepath), 7,
> + piglit_source_dir(),
> + "tests",
> + "spec",
> + "khr_texture_compression_astc",
> + dir1,
> + dir2,
> + filename);
> +
> + ktx = piglit_ktx_read_file(filepath);
> + if (ktx == NULL)
> + piglit_report_result(PIGLIT_FAIL);
> +
> + info = piglit_ktx_get_info(ktx);
> + assert(info->num_miplevels == num_levels);
> + assert(info->target == GL_TEXTURE_2D);
> + assert(info->pixel_width == level0_width);
> + assert(info->pixel_height== level0_height);
^^^^^
Missing space.
> +
> + *tex_name = 0;
> + ok = piglit_ktx_load_texture(ktx, tex_name, NULL);
> + if (!ok)
> + piglit_report_result(PIGLIT_FAIL);
> +
> + piglit_ktx_destroy(ktx);
> +}
> +
> +/** Compares the compressed texture against the decompressed texture */
> +bool draw_compare_levels(bool check_error, bool check_srgb,
> + GLint level_pixel_size_loc, GLint pixel_offset_loc,
> + GLuint compressed_tex, GLuint decompressed_tex)
> +{
> + /* Fully-saturated magenta */
> + static const float error_color[4] = {1.0, 0.0, 1.0, 1.0};
> +
> + unsigned y = 0;
> + unsigned x = 0;
> + bool pass = true;
> + int level = 0;
> +
> + for (; level < num_levels; ++level) {
> + int w = level0_width >> level;
> + int h = level0_height >> level;
> + glUniform2f(level_pixel_size_loc, (float) w, (float) h);
> +
> +
> + /* Draw miplevel of compressed texture. */
> + glBindTexture(GL_TEXTURE_2D, compressed_tex);
> + if (!check_srgb)
> + glTexParameteri(GL_TEXTURE_2D,
> + GL_TEXTURE_SRGB_DECODE_EXT,
> + GL_SKIP_DECODE_EXT);
> + glUniform2f(pixel_offset_loc, x, y);
> + glDrawArrays(GL_TRIANGLE_FAN, 0, num_vertices);
> +
> + /* Draw miplevel of decompressed texture. */
> + if (!check_error) {
> + glBindTexture(GL_TEXTURE_2D, decompressed_tex);
> + if (!check_srgb)
> + glTexParameteri(GL_TEXTURE_2D,
> + GL_TEXTURE_SRGB_DECODE_EXT,
> + GL_SKIP_DECODE_EXT);
> + glUniform2f(pixel_offset_loc, level0_width + x, y);
> + glDrawArrays(GL_TRIANGLE_FAN, 0, num_vertices);
> + }
> +
> + /* Check the textures (or error-colors) for equivalence. */
> + if (pass) {
> + if (check_error) {
> + pass = piglit_probe_rect_rgba(x, y, w, h,
> + error_color);
> + } else {
> + pass = piglit_probe_rects_equal(x, y,
> + level0_width + x, y,
> + w, h, GL_RGBA);
> + }
> +
> + if (!pass)
> + piglit_loge("Miplevel %d", level);
> + }
> +
> + /* Update the next miplevel arrangement */
> + if (level == 1)
> + x += w;
> + else
> + y += h;
> + }
> +
> + /* Delete bound textures */
> + glDeleteTextures(1, &compressed_tex);
> + glDeleteTextures(1, &decompressed_tex);
> +
> + piglit_present_results();
> + return pass;
> +}
> +
> +enum piglit_result
> +test_miptrees(void* input_data)
> +{
> + int subtest = 0;
> + test_data * data = (test_data*) input_data;
> +
> + static const char * tests[3] = {"hdr", "ldrl", "ldrs"};
> + static const char * block_dim_str[14] = {
> + "4x4",
> + "5x4",
> + "5x5",
> + "6x5",
> + "6x6",
> + "8x5",
> + "8x6",
> + "8x8",
> + "10x5",
> + "10x6",
> + "10x8",
> + "10x10",
> + "12x10",
> + "12x12"
> + };
> +
> + static const char * hdr_str = "GL_KHR_texture_compression_astc_hdr";
Variable 'hdr_str' should be removed, and the string simply inlined in
the call to piglit_is_extension_supported().
> + bool hdr_sys = piglit_is_extension_supported(hdr_str);
Please rename 'hdr_sys' to a name that looks like a boolean. Something
like 'has_hdr', 'is_hdr_supported', or similar.
> +
> + /* If testing sRGB mode, fast-forward to the srgb test. */
> + if (data->srgb_test)
> + subtest = 2;
Being a multiline 'if' statement, the 'if' clause should have braces.
> + else {
> + /* Skip if on an HDR system not running the HDR test
> + * or if on an LDR system running the HDR test.
> + */
> + if (hdr_sys != data->hdr_test)
> + return PIGLIT_SKIP;
> + piglit_require_extension("GL_EXT_texture_sRGB_decode");
> +
> + }
> +
> + GLint pixel_offset_loc = glGetUniformLocation(prog, "pixel_offset");
> + GLint level_pixel_size_loc = glGetUniformLocation(prog,
> + "level_pixel_size");
> +
> + /* Test each submode */
> + for (; subtest < ARRAY_SIZE(tests); ++subtest) {
> +
> + /* Check for error color if an LDR-only sys reading an HDR
> + * texture. No need to draw a reference mipmap in this case.
> + */
> + int check_error = !hdr_sys && subtest == 0; // 0 == hdr
> + int block_dims = 0;
> + for (; block_dims < ARRAY_SIZE(block_dim_str); ++block_dims) {
> + /* Texture objects. */
> + GLuint tex_cd[2];
^^^^^^
Please explain the cryptin meaning of 'cd'.
> +
> + /* Generate filename for compressed texture */
> + char cur_file[20];
> + snprintf(cur_file, sizeof(cur_file), "waffles-%s.ktx",
> + block_dim_str[block_dims]);
The full filename is partially constructed here, and partially
constructed in load_texture(). The code would be more understandable if
the full filename was fully constructed in a single place.
> +
> + /* Load texture for current submode and block size */
> + glGenTextures(2, tex_cd);
> + load_texture("compressed", tests[subtest], cur_file,
> + &tex_cd[0]);
> + if (!check_error)
> + load_texture("decompressed", tests[subtest],
> + cur_file, &tex_cd[1]);
The above 'if' statement is multiline. Please use braces to prevent
future bugs.
> +
> + /* Draw and compare each level of the two textures */
> + glClear(GL_COLOR_BUFFER_BIT);
> + if (!draw_compare_levels(check_error, data->srgb_test,
> + level_pixel_size_loc,
> + pixel_offset_loc,
> + tex_cd[0], tex_cd[1])) {
> + piglit_loge("Mode %s Block %s.",
> + tests[subtest],
> + block_dim_str[block_dims]);
> + return PIGLIT_FAIL;
> + }
> + }
> + }
> + return PIGLIT_PASS;
> +}
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
The config block should occur as early as possible in the source file.
Preferably, the config block should occur above all functions.
> +
> + piglit_config = &config;
> + config.supports_gl_compat_version = 11;
> + config.supports_gl_es_version = 10;
> +
> + config.window_width = 2 * level0_width;
> + config.window_height = level0_height + (level0_height >> 1);
> + config.window_visual = PIGLIT_GL_VISUAL_RGB | PIGLIT_GL_VISUAL_DOUBLE;
> +
> + /* struct initialization: {hdr_test, srgb_test} */
> + static test_data ldr_test = {false, false};
> + static test_data hdr_test = {true , false};
> + static test_data srgb_test = {false, true };
> + config.subtests = (struct piglit_subtest[]) {
C99's compound expression syntax is illegal in C89. To remove the
compound expression, declare the array of subtests as
static const struct piglit_subtest subtests[] = {
...
};
> + {
> + "LDR Profile",
> + "ldr",
> + test_miptrees,
> + &ldr_test,
> + },
> + {
> + "HDR Profile",
> + "hdr",
> + test_miptrees,
> + &hdr_test,
> + },
> + {
> + "sRGB decode",
> + "srgb",
> + test_miptrees,
> + &srgb_test,
> + },
> + {NULL},
> + };
> +
> +PIGLIT_GL_TEST_CONFIG_END
[snip]
More information about the Piglit
mailing list