On Wed, Sep 9, 2015 at 5:04 PM, Chad Versace <chad.vers...@intel.com> wrote:
> On Fri 28 Aug 2015, Nanley Chery wrote: > > From: Nanley Chery <nanley.g.ch...@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.ch...@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] >
_______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit