<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Wed, Sep 9, 2015 at 5:04 PM, Chad Versace <<a href="mailto:chad.versace@intel.com" target="_blank">chad.versace@intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri 28 Aug 2015, Nanley Chery wrote: > From: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>> > > 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 <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>> > 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. </blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + > +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. </blockquote>Done.<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + > +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. </blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > +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. </blockquote><div>Fixed. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + * 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. </blockquote><div>Fixed. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + * the decompressed texture on the right. Each miplevel of both images are > + * compared for equality after each level is drawn. ^^^^^ drawn </blockquote><div>I'm assuming you were meaning to say that "each level" is redundant, so I modified the sentence accordingly. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + * > + * [1] The reference image is located at <a href="http://people.freedesktop.org/~chadversary/permalink/2012-07-09/1574cff2-d091-4421-a3cf-b56c7943d060.jpg" rel="noreferrer" target="_blank">http://people.freedesktop.org/~chadversary/permalink/2012-07-09/1574cff2-d091-4421-a3cf-b56c7943d060.jpg</a>. > + * [2] astcenc is the reference ASTC compression tool, available at <a href="http://malideveloper.arm.com/develop-for-mali/tools/software-tools/astc-evaluation-codec/" rel="noreferrer" target="_blank">http://malideveloper.arm.com/develop-for-mali/tools/software-tools/astc-evaluation-codec/</a>. > + */ > + > +#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. </blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + > +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, <div><div class="h5"> }; </div></div></blockquote>Fixed.<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > + > + > +/** > + * 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); </div></div> ^^^^^ Missing space. </blockquote><div>Fixed. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5">> + > + *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"; </div></div>Variable 'hdr_str' should be removed, and the string simply inlined in the call to piglit_is_extension_supported(). </blockquote><div>Fixed. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + 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. </blockquote><div>Fixed. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + > + /* 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. <div><div class="h5"> </div></div></blockquote><div>Fixed. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > + 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]; </div></div> ^^^^^^ Please explain the cryptin meaning of 'cd'. </blockquote><div>Fixed. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + > + /* 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. </blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + > + /* 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. </blockquote><div>Fixed. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + > + /* 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. </blockquote><div>Fixed. I had to include a function prototype before this block to get this to work. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + > + 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[] = { ... }; </blockquote><div>Fixed. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + { > + "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] </blockquote></div> </div></div>