[Piglit] [PATCH v2 3/4] Add miptree tests for khr_texture_compression_astc
Nanley Chery
nanleychery at gmail.com
Mon Sep 14 17:19:50 PDT 2015
On Wed, Sep 9, 2015 at 5:04 PM, Chad Versace <chad.versace at intel.com> wrote:
> 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.
>
> Done.
> > +
> > +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.
>
> Done.
>
> > +
> > +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.
>
> Done.
> > +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.
>
> Fixed.
> > + * 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.
>
> Fixed.
> > + * the decompressed texture on the right. Each miplevel of both images
> are
> > + * compared for equality after each level is drawn.
> ^^^^^
> drawn
>
I'm assuming you were meaning to say that "each level" is redundant, so I
modified the sentence accordingly.
> > + *
> > + * [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.
>
> Done.
> > +
> > +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,
> };
>
> Fixed.
>
> > +
> > +
> > +/**
> > + * 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.
>
Fixed.
> > +
> > + *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().
>
> Fixed.
> > + 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.
>
> Fixed.
> > +
> > + /* 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.
>
> Fixed.
> > + 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'.
>
> Fixed.
> > +
> > + /* 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.
>
> Done.
> > +
> > + /* 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.
>
> Fixed.
> > +
> > + /* 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.
>
> Fixed. I had to include a function prototype before this block to get this
to work.
> > +
> > + 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[] = {
> ...
> };
>
> Fixed.
> > + {
> > + "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]
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20150914/8a6b3466/attachment-0001.html>
More information about the Piglit
mailing list