[Piglit] [PATCH] [v3] Add test case on error check on glGetTexImage with type
Zhao, Jian J
jian.j.zhao at intel.com
Sun Dec 18 18:46:25 PST 2011
> -----Original Message-----
> From: Ian Romanick [mailto:idr at freedesktop.org]
> Sent: Saturday, December 17, 2011 8:24 AM
> To: Zhao, Jian J
> Cc: piglit at lists.freedesktop.org
> Subject: Re: [PATCH] [v3] Add test case on error check on glGetTexImage
> with type
>
> On 12/14/2011 06:10 PM, jian.j.zhao at intel.com wrote:
> > From: Jian Zhao<jian.j.zhao at intel.com>
> >
> > As glGetTexImage in OpenGL 4.2 Reference Pages:
> > "GL_INVALID_OPERATION is returned if type is one of
> GL_UNSIGNED_BYTE_3_3_2,
> > GL_UNSIGNED_BYTE_2_3_3_REV, GL_UNSIGNED_SHORT_5_6_5,
> > GL_UNSIGNED_SHORT_5_6_5_REV, or
> GL_UNSIGNED_INT_10F_11F_11F_REV and format is
> > not GL_RGB".
>
Hi Ian,
Thanks for your comments.
> All of these formats should be tested.
OK. I will add it.
>
> > v2: change the test name to be more specific and change some code style
> to be
> > easier to read (per Brian's suggest).
> >
> > v3: move the case to the spec directory and rename it to
> getteximage_error_check.c (per Brian's suggest)
> >
> > Signed-off-by: Jian Zhao<jian.j.zhao at intel.com>
> > ---
> > tests/spec/ext_packed_float/CMakeLists.gl.txt | 1 +
> > .../ext_packed_float/getteximage-error-check.c | 84
> ++++++++++++++++++++
> > 2 files changed, 85 insertions(+), 0 deletions(-)
> > create mode 100644
> tests/spec/ext_packed_float/getteximage-error-check.c
> >
> > diff --git a/tests/spec/ext_packed_float/CMakeLists.gl.txt
> b/tests/spec/ext_packed_float/CMakeLists.gl.txt
> > index 78bbcd1..b6f8356 100644
> > --- a/tests/spec/ext_packed_float/CMakeLists.gl.txt
> > +++ b/tests/spec/ext_packed_float/CMakeLists.gl.txt
> > @@ -13,5 +13,6 @@ link_libraries (
> > )
> >
> > add_executable (ext_packed_float-pack pack.c)
> > +add_executable (getteximage-error-check getteximage-error-check.c)
> >
> > # vim: ft=cmake:
> > diff --git a/tests/spec/ext_packed_float/getteximage-error-check.c
> b/tests/spec/ext_packed_float/getteximage-error-check.c
> > new file mode 100644
> > index 0000000..50608fa
> > --- /dev/null
> > +++ b/tests/spec/ext_packed_float/getteximage-error-check.c
> > @@ -0,0 +1,84 @@
> > +/*
> > + * Copyright © 2009 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 getteximage-error-check.c
>
> The name should actually be descriptive. Use
> getteximage-invalid-format-for-packed-type. This matches the naming
> pattern of other tests. Try 'find tests -name "*invalid*.c"' from the
> top-level piglit directory.
Yes. I find some similar tests named in this way. I will change it to getteximage-invalid-format-for-packed-type.c.
>
> > + *
> > + * Tests on error check on glGetTexImage with type
> > + * GL_UNSIGNED_INT_10F_11F_11F_REV should be
> GL_INVALID_OPERATION.
>
> Please replace the text above with the text below without modification.
> It's important in API tests like this to include specific
> justification from the OpenGL spec.
>
> * Page 262 (page 282 of the PDF) of the OpenGL 4.2 Compatibility
> * Profile spec says:
> *
> * "The number of components per packed pixel is fixed by the
> * type, and must match the number of components per group
> * indicated by the format parameter, as listed in table 3.8.
> * The error INVALID_OPERATION is generated by any command
> * processing pixel rectangles if a mismatch occurs."
> *
> * Table 3.8 says:
> *
> * "type Parameter Token Name ... Matching Pixel Formats"
> * UNSIGNED_BYTE_3_3_2 RGB, RGB_INTEGER
> * UNSIGNED_BYTE_2_3_3_REV RGB, RGB_INTEGER
> * UNSIGNED_SHORT_5_6_5 RGB, RGB_INTEGER
> * UNSIGNED_SHORT_5_6_5_REV RGB, RGB_INTEGER
> * UNSIGNED_SHORT_4_4_4_4 RGBA, BGRA,
> RGBA_INTEGER,
> * BGRA_INTEGER
> * UNSIGNED_SHORT_4_4_4_4_REV RGBA, BGRA,
> RGBA_INTEGER,
> * BGRA_INTEGER
> * UNSIGNED_SHORT_5_5_5_1 RGBA, BGRA,
> RGBA_INTEGER,
> * BGRA_INTEGER
> * UNSIGNED_SHORT_1_5_5_5_REV RGBA, BGRA,
> RGBA_INTEGER,
> * BGRA_INTEGER
> * UNSIGNED_INT_8_8_8_8 RGBA, BGRA,
> RGBA_INTEGER,
> * BGRA_INTEGER
> * UNSIGNED_INT_8_8_8_8_REV RGBA, BGRA,
> RGBA_INTEGER,
> * BGRA_INTEGER
> * UNSIGNED_INT_10_10_10_2 RGBA, BGRA,
> RGBA_INTEGER,
> * BGRA_INTEGER
> * UNSIGNED_INT_2_10_10_10_REV RGBA, BGRA,
> RGBA_INTEGER,
> * BGRA_INTEGER
> * UNSIGNED_INT_24_8 DEPTH_STENCIL
> * UNSIGNED_INT_10F_11F_11F_REV RGB, RGB_INTEGER
> * UNSIGNED_INT_5_9_9_9_REV RGB, RGB_INTEGER
> * FLOAT_32_UNSIGNED_INT_24_8_REV DEPTH_STENCIL"
>
OK. Added it.
> > + */
> > +
> > +#include "piglit-util.h"
> > +
> > +int piglit_width = 100, piglit_height = 100;
> > +int piglit_window_mode = GLUT_RGBA | GLUT_DOUBLE;
> > +
> > +const GLenum formatTypes[] = {
>
> We usually make these static too.
OK. Add it.
>
> > + GL_RGBA,
> > + GL_RGB,
> > + GL_RED,
> > + GL_GREEN,
> > + GL_BLUE,
> > + GL_ALPHA,
> > + GL_LUMINANCE,
> > + GL_LUMINANCE_ALPHA,
> > +};
> > +
> > +
> > +enum piglit_result
> > +piglit_display(void)
> > +{
> > + long rcvError, expError = GL_NO_ERROR;
> > + GLfloat *pxBuffer;
> > + int i;
> > +
> > + pxBuffer = (GLfloat *)malloc(10 * 10 * 10 * 4 * sizeof(GLfloat));
>
> Why malloc a fixed-size buffer that is never freed? Since there's no
> texture set, this shouldn't need any data at all.
>
> > + if (!pxBuffer)
> > + return PIGLIT_FAIL;
> > +
> > + for(i=0; i< ARRAY_SIZE(formatTypes); i++)
> > + {
>
> Whitespace errors.
Can you please tell me what is the expected whitespace?
>
> > + glGetTexImage(GL_TEXTURE_2D, 0, formatTypes[i],
> > + GL_UNSIGNED_INT_10F_11F_11F_REV_EXT,
>
> Should also test the non-REV format.
Do you mean also test other types like GL_UNSIGNED_BYTE_3_3_2, GL_UNSIGNED_BYTE_2_3_3_REV...? If so I will add them in the next patch update.
>
> > + pxBuffer);
> > + rcvError = glGetError();
> > + if ( formatTypes[i] == GL_RGB )
>
> Whitespace errors.
>
> > + expError = GL_NO_ERROR;
> > + else
> > + expError = GL_INVALID_OPERATION;
> > +
> > + if (rcvError != expError)
> > + return PIGLIT_FAIL;
>
> This whole body of this loop be:
>
> GLubyte buf[4];
> const GLenum expected = (formatTypes[i] == GL_RGB)
> ? GL_NO_ERROR : GL_INVALID_OPERATION;
>
> glGetTexImage(GL_TEXTURE_2D, 0, formatTypes[i],
> GL_UNSIGNED_INT_10F_11F_11F_REV_EXT,
> buf);
> piglit_check_gl_error(expected, PIGLIT_FAIL);
>
> This will need some changes to support testing formats other than
> GL_UNSIGNED_INT_10F_11F_11F_REV_EXT. An inner loop (that loops over
> the
> packed formats) seems like the right way to go.
>
> > + }
>
> This code doesn't display anything, and it's not a rendering test. All
> of the checks should be in piglit_init. There are a lot of other tests
> that work like that. See
> tests/spec/gl-3.0/api/clearbuffer-invalid-buffer.c for a recent example.
OK. I will change it.
>
> > +
> > + return PIGLIT_PASS;
> > +}
> > +
> > +
> > +void
> > +piglit_init(int argc, char **argv)
> > +{
> > + piglit_require_extension("GL_EXT_packed_float");
> > + glewInit();
>
> There's no need to call glewInit. It's already called by the piglit
> framework.
OK. Thanks.
Best regards
ZhaoJian
>
> > +}
More information about the Piglit
mailing list