[Piglit] [PATCH] [v5] Add test case of error check on glGetTexImage.

[Zhao, Jian J]

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu at linux.intel.com]
> Sent: Thursday, December 29, 2011 2:43 PM
> To: Zhao, Jian J
> Cc: piglit at lists.freedesktop.org; brianp at vmware.com
> Subject: Re: [PATCH] [v5] Add test case of error check on glGetTexImage.
> 
> On Tue, Dec 27, 2011 at 01:17:38PM +0800, jian.j.zhao at intel.com wrote:
> > From: Jian Zhao <jian.j.zhao at intel.com>
> >
> > Add test case on error check on glGetTexImage with types(like
> > GL_UNSIGNED_INT_10F_11F_11F_REV) that only work with format GL_RGB,
> > but format  isn't GL_RGB the result should be GL_INVALID_OPERATION.
> >
> > 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".
> >
> > 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)
> >
> > v4: add all the formats that only work with GL_RGB format, rename it
> > more specific and some minor changes. (per Ian's suggest)
> >
> > v5: some changes in code styles, like "One space on either side of a
> > binary operator or =". (per Ian's suggest)
> >
> > Signed-off-by: Jian Zhao <jian.j.zhao at intel.com>
> > Reviewed-by: Brian Paul <brianp at vmware.com>
> > ---
> >  tests/spec/ext_packed_float/CMakeLists.gl.txt      |    1 +
> >  .../getteximage-invalid-format-for-packed-type.c   |  126
> ++++++++++++++++++++
> >  2 files changed, 127 insertions(+), 0 deletions(-)
> 
> You need modify all.tests to include your test, too.

Yes. Included now. 

> 
> >  create mode 100644
> > tests/spec/ext_packed_float/getteximage-invalid-format-for-packed-type
> > .c
> >
> > diff --git a/tests/spec/ext_packed_float/CMakeLists.gl.txt
> > b/tests/spec/ext_packed_float/CMakeLists.gl.txt
> > index 78bbcd1..c6ff374 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-invalid-format-for-packed-type
> > +getteximage-invalid-format-for-packed-type.c)
> >
> >  # vim: ft=cmake:
> > diff --git
> > a/tests/spec/ext_packed_float/getteximage-invalid-format-for-packed-ty
> > pe.c
> > b/tests/spec/ext_packed_float/getteximage-invalid-format-for-packed-ty
> > pe.c
> > new file mode 100644
> > index 0000000..891b6de
> > --- /dev/null
> > +++ b/tests/spec/ext_packed_float/getteximage-invalid-format-for-packe
> > +++ d-type.c
> > @@ -0,0 +1,126 @@
> > +/*
> > + * Copyright (c) 2009 Intel Corporation
> 
> 2011?
> 
> > + *
> > + * 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.
> > + *
> > + *
> > + */
> > +
> > +/**
> 
> Trailing whitespace. (There are several whitespaces in your patch)

Modified now. 

> 
> > + * file
> > + tests/spec/ext_packed_float/getteximage-invalid-format-for-packed-ty
> > + pe.c
> > + * test with some invalid type when the format is not GL_RGB.
> > + *
> > + * Page 262 (page 282 of the PDF) of the OpenGL 4.2 Compatibility
> > + * Profile spec says:
> > + *
> > + */
> > +
> > +#include "piglit-util.h"
> > +
> > +int piglit_width = 100, piglit_height = 100; int piglit_window_mode =
> > +GLUT_RGBA | GLUT_DOUBLE;
> > +
> > +static const GLenum formatTypes[] = {
> > +    GL_RGBA,
> > +    GL_RGB,
> > +    GL_RED,
> > +    GL_GREEN,
> > +    GL_BLUE,
> > +    GL_ALPHA,
> > +    GL_LUMINANCE,
> > +    GL_LUMINANCE_ALPHA,
> > +};
> > +
> > +static const GLenum testedTypes[] = {
> > +    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,
> > +    GL_UNSIGNED_INT_10F_11F_11F_REV,
> > +};
> 
> Wrong indent format here.

Modified. 
> 
> > +
> > +
> > +enum piglit_result
> > +piglit_display(void)
> > +{
> > +	return PIGLIT_FAIL;
> > +}
> > +
> > +
> > +void
> > +piglit_init(int argc, char **argv)
> > +{
> > +	long rcvError, expError = GL_NO_ERROR;
> > +	GLfloat pxBuffer[4];
> > +	int i, j;
> > +
> > +	piglit_require_extension("GL_EXT_packed_float");
> > +
> > +	for (j = 0; j < ARRAY_SIZE(testedTypes); j++) {
> > +		for (i = 0; i< ARRAY_SIZE(formatTypes); i++) {
> > +			glGetTexImage(GL_TEXTURE_2D, 0, formatTypes[i],
> > +				    testedTypes[j], pxBuffer);
> Also not a good indention here.

Modified. 

> 
> 
> 
> If you fixed all the minor issues above, you got my Review-by ;)

Thanks! 

Best regards
Zhaojian

> 
> Thanks,
> Yuanhan Liu