[Piglit] [PATCH 2/2] arb_direct_state_access: New test for GetCompressedTextureImage.
Ian Romanick
idr at freedesktop.org
Tue Feb 17 14:50:09 PST 2015
On 02/17/2015 02:36 PM, Laura Ekstrand wrote:
>
>
> On Tue, Feb 17, 2015 at 1:34 PM, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
>
> On 02/13/2015 11:36 AM, Laura Ekstrand wrote:
> > ---
> > .../getcompressedtextureimage.c | 198
> +++++++++++++++++++++
> > 1 file changed, 198 insertions(+)
> > create mode 100644
> tests/spec/arb_direct_state_access/getcompressedtextureimage.c
> >
> > 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..e4f15d7
> > --- /dev/null
> > +++ b/tests/spec/arb_direct_state_access/getcompressedtextureimage.c
> > @@ -0,0 +1,198 @@
> > +/*
> > + * Copyright 2015 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.c
> > + *
> > + * Tests to make sure that glGetCompressedTextureImage works
> correctly.
> > + * The basic idea is to create some fake compressed data, upload
> it to the
> > + * driver with the traditional commands, download it with DSA,
> and compare.
> > + * Of course, this assumes that the traditional commands are
> correct, which
> > + * may not actually be the case.
> > + */
> > +
> > +#include "piglit-util-gl.h"
> > +
> > +PIGLIT_GL_TEST_CONFIG_BEGIN
> > +
> > + config.supports_gl_compat_version = 10;
>
> Related to Ilia's comments about checking for additional extensions...
> do we actually want to test compatibility profile? Jason pointed this
> out to me on IRC late January:
>
> "Dependencies
>
> OpenGL 2.0 is required."
>
> This means Mesa will need a flag for the extension because there are
> pre-2.0 drivers. Every DSA function will need to check the flag and
> possibly generate GL_INVALID_OPERATION.
>
> If we only support core profile (and always enable it in core profile),
> the functions won't even be put in the dispatch table for non-core. No
> flag, and no checks in the functions.
>
> So, the $64,000.00 question is: Do we care to support DSA on the few
> cards that can do 2.0 but not 3.1?
>
> Either way, it should be safe to change this to
>
> config.supports_gl_compat_version = 20;
>
> at the very least.
>
> > +
> > + config.window_visual = PIGLIT_GL_VISUAL_RGBA |
> > + PIGLIT_GL_VISUAL_DOUBLE;
> > +
> > +PIGLIT_GL_TEST_CONFIG_END
> > +
> > +/* Copied from Mesa src/mesa/main/formats.csv:
> > + * MESA_FORMAT_RGBA_DXT5, s3tc, 4, 4, x128
> > + */
> > +#define FORMAT GL_COMPRESSED_RGBA_S3TC_DXT5_EXT
> > +#define WIDTH 32
> > +#define HEIGHT 32
> > +#define WIDTH_IN_BLOCKS 8
> > +#define HEIGHT_IN_BLOCKS 8
> > +#define BLOCK_BYTES 16
> > +#define IMAGE_SIZE (WIDTH_IN_BLOCKS * HEIGHT_IN_BLOCKS * BLOCK_BYTES)
> > +#define CHAR_LIMIT 100
> > +
> > +static GLubyte *expected;
> > +static GLboolean pass = GL_TRUE;
>
> static bool pass = true;
>
> > +
> > +static void
> > +subtest(GLboolean local_result, const char *test_name)
> > +{
> > + pass = local_result && pass;
> > + piglit_report_subtest_result(local_result ? PIGLIT_PASS :
> PIGLIT_FAIL,
> > + test_name);
> > +}
> > +
> > +static void
> > +init_random_data(void)
> > +{
> > + int i;
> > +
> > + expected = malloc(18 * IMAGE_SIZE);
> > + for (i = 0; i < 18 * IMAGE_SIZE; ++i) {
> > + expected[i] = (GLubyte) rand();
> > + }
> > +}
> > +
> > +void
> > +piglit_init(int argc, char **argv)
> > +{
> > + piglit_require_extension("GL_ARB_direct_state_access");
> > + srand(0);
> > + init_random_data();
> > +}
> > +
> > +static bool
> > +compare_to_expected(const GLubyte *data, int num_layers)
> > +{
> > + return memcmp(expected, data, num_layers * IMAGE_SIZE) == 0;
> > +}
>
> Why not fold this function into subtest (above)? It looks like either
> function is only ever called as
>
> subtest(compare_to_expected(...), ...);
>
> Martin has a macro SUBTEST_CONDITION that I'm hoping to use here
> instead. I've asked him to send out a commit that adds the macro to
> piglit-util-gl.h. Right now, I'm using this function as a stand-in.
Ah... that makes sense. I sent Martin a R-b on that patch to help move
things along.
> > +
> > +static void
> > +upload_subtest(GLenum target, bool use_pbo)
> > +{
> > + GLuint tex, pbo;
> > + int num_layers;
> > + char test_name [CHAR_LIMIT];
> > + int i;
> > +
> > + /* Prepare to read data from expected */
> > + GLubyte *data = expected;
> > + if (use_pbo) {
> > + glGenBuffers(1, &pbo);
> > + glBindBuffer(GL_PIXEL_UNPACK_BUFFER, pbo);
> > + glBufferData(GL_PIXEL_UNPACK_BUFFER, 18 * IMAGE_SIZE,
> > + expected, GL_STATIC_DRAW);
> > + data = NULL;
> > + }
> > +
> > + /* Upload it using traditional path */
> > + glGenTextures(1, &tex);
> > + glBindTexture(target, tex);
> > +
> > + switch (target) {
> > +
> > + case GL_TEXTURE_2D:
> > + num_layers = 1;
> > + glTexStorage2D(target, 1, FORMAT, WIDTH, HEIGHT);
> > + glCompressedTexSubImage2D(target, 0, 0, 0, WIDTH,
> HEIGHT,
> > + FORMAT, num_layers *
> IMAGE_SIZE,
> > + data);
> > + break;
> > + case GL_TEXTURE_2D_ARRAY:
> > + num_layers = 3;
> > + glTexStorage3D(target, 1, FORMAT, WIDTH, HEIGHT,
> num_layers);
> > + glCompressedTexSubImage3D(target, 0, 0, 0, 0, WIDTH,
> HEIGHT,
> > + num_layers, FORMAT,
> > + num_layers * IMAGE_SIZE,
> data);
> > + break;
> > + case GL_TEXTURE_CUBE_MAP:
> > + num_layers = 6;
> > + glTexStorage2D(target, 1, FORMAT, WIDTH, HEIGHT);
> > + for (i = 0; i < num_layers; ++i) {
> > + glCompressedTexSubImage2D(
> > + GL_TEXTURE_CUBE_MAP_POSITIVE_X + i,
> 0, 0, 0,
> > + WIDTH, HEIGHT, FORMAT, IMAGE_SIZE,
> > + data + i * IMAGE_SIZE);
> > + }
> > + break;
> > + case GL_TEXTURE_CUBE_MAP_ARRAY:
> > + num_layers = 18;
> > + glTexStorage3D(target, 1, FORMAT, WIDTH, HEIGHT,
> num_layers);
> > + glCompressedTexSubImage3D(target, 0, 0, 0, 0, WIDTH,
> HEIGHT,
> > + num_layers, FORMAT,
> > + num_layers * IMAGE_SIZE,
> data);
> > + break;
> > + default:
> > + printf("Bad texture target.\n");
> > + piglit_report_result(PIGLIT_FAIL);
> > +
> > + }
> > +
> > + /* Prepare to download into data. */
> > + if (use_pbo) {
> > + glBindBuffer(GL_PIXEL_PACK_BUFFER, pbo);
> > + glBufferData(GL_PIXEL_PACK_BUFFER, 18 * IMAGE_SIZE,
> > + NULL, GL_STATIC_READ);
> > + /* data = NULL from above */
> > + }
> > + else
> > + data = malloc(18 * IMAGE_SIZE);
> > +
> > + /* Download it using DSA */
> > + glBindTexture(target, 0);
> > + glGetCompressedTextureImage(tex, 0, num_layers * IMAGE_SIZE,
> data);
> > +
> > + if (use_pbo)
> > + data = glMapBuffer(GL_PIXEL_PACK_BUFFER, GL_READ_ONLY);
>
> Using 'data' for multiple things in this way seems really fragile. I
> think it would be both more clear and less fragile as:
>
> I guess I misunderstood your comment on the old test here. I will fix
> this in the v2.
Well... the other test was quite a bit different. :)
> /* Download it using DSA */
> glBindTexture(target, 0);
> if (use_pbo) {
> glBindBuffer(GL_PIXEL_PACK_BUFFER, pbo);
> glBufferData(GL_PIXEL_PACK_BUFFER, 18 * IMAGE_SIZE,
> NULL, GL_STATIC_READ);
>
> glGetCompressedTextureImage(tex, 0, num_layers *
> IMAGE_SIZE, (void *) 0);
>
> data = glMapBuffer(GL_PIXEL_PACK_BUFFER, GL_READ_ONLY);
> } else {
> data = malloc(18 * IMAGE_SIZE);
> glGetCompressedTextureImage(tex, 0, num_layers *
> IMAGE_SIZE, data);
> }
>
> I might also be tempted to use a separate buffer object for the pack
> buffer. glBufferData doesn't guarantee the contents of buffer, and it
> doesn't guarantee that it will be a fresh allocation. If
> glGetCompressedTextureImage does nothing (perhaps because an error was
> generated), you could still have the same data in the buffer resulting
> in a false positive.
>
> Ok, I will correct this in the v2.
If you make these two changes, you can go ahead and put my R-b on it...
unless there is there is other review feedback that necessitates a re-send.
> > +
> > + /* Perform comparison and report the results. */
> > + snprintf(test_name, CHAR_LIMIT, "upload %s%s",
> > + piglit_get_gl_enum_name(target), use_pbo ? " PBO" :
> "");
> > + subtest(compare_to_expected(data, num_layers), test_name);
> > +
> > + /* Clean up */
> > + if (use_pbo)
> > + glDeleteBuffers(1, &pbo);
> > + else
> > + free(data);
> > +}
> > +
> > +enum piglit_result
> > +piglit_display(void)
> > +{
> > + /* Non-PBO tests */
> > + upload_subtest(GL_TEXTURE_2D, false);
> > + upload_subtest(GL_TEXTURE_2D_ARRAY, false);
> > + upload_subtest(GL_TEXTURE_CUBE_MAP, false);
> > + upload_subtest(GL_TEXTURE_CUBE_MAP_ARRAY, false);
> > +
> > + /* PBO tests */
> > + upload_subtest(GL_TEXTURE_2D, true);
> > + upload_subtest(GL_TEXTURE_2D_ARRAY, true);
> > + upload_subtest(GL_TEXTURE_CUBE_MAP, true);
> > + upload_subtest(GL_TEXTURE_CUBE_MAP_ARRAY, true);
> > +
> > + return pass ? PIGLIT_PASS : PIGLIT_FAIL;
> > +}
> >
More information about the Piglit
mailing list