[Piglit] [PATCH 2/2] teximage-errors: Test the combinations of depth and depth-stencil formats

[Brian Paul]

On 03/12/2014 12:56 PM, Anuj Phogat wrote:
>
>
>
> On Wed, Mar 12, 2014 at 6:24 AM, Brian Paul <brianp at vmware.com
> <mailto:brianp at vmware.com>> wrote:
>
>     On 03/11/2014 06:51 PM, Anuj Phogat wrote:
>
>         This patch is to verify a bug fix in mesa commit 079bff5. This
>         commit
>         allowed GL_DEPTH_COMPONENT and GL_DEPTH_STENCIL combinations in
>         glTexImage{123}D functions.
>
>         Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com
>         <mailto:anuj.phogat at gmail.com>>
>         ---
>            tests/texturing/teximage-__errors.c | 73
>         ++++++++++++++++++++++++++++++__++++++++-
>            1 file changed, 72 insertions(+), 1 deletion(-)
>
>         diff --git a/tests/texturing/teximage-__errors.c
>         b/tests/texturing/teximage-__errors.c
>         index 39b7e9b..c01c5e0 100644
>         --- a/tests/texturing/teximage-__errors.c
>         +++ b/tests/texturing/teximage-__errors.c
>         @@ -36,7 +36,37 @@ PIGLIT_GL_TEST_CONFIG_BEGIN
>
>            PIGLIT_GL_TEST_CONFIG_END
>
>         +struct format_desc {
>         +   GLenum internalformat;
>         +   GLenum format;
>         +   GLenum type;
>         +};
>
>         +static const struct format_desc formats_allowed[] = {
>         +   {GL_DEPTH_COMPONENT16, GL_DEPTH_STENCIL, GL_UNSIGNED_INT_24_8},
>         +   {GL_DEPTH_COMPONENT24, GL_DEPTH_STENCIL, GL_UNSIGNED_INT_24_8},
>         +   {GL_DEPTH_COMPONENT32F, GL_DEPTH_STENCIL, GL_UNSIGNED_INT_24_8},
>         +
>         +   {GL_DEPTH_COMPONENT16, GL_DEPTH_COMPONENT, GL_FLOAT},
>         +   {GL_DEPTH_COMPONENT24, GL_DEPTH_COMPONENT, GL_FLOAT},
>         +   {GL_DEPTH_COMPONENT32F, GL_DEPTH_COMPONENT, GL_FLOAT},
>         +
>         +   {GL_DEPTH24_STENCIL8, GL_DEPTH_COMPONENT, GL_FLOAT},
>         +   {GL_DEPTH32F_STENCIL8, GL_DEPTH_COMPONENT, GL_FLOAT},
>
>
>     So for that combination, do the texture's stencil values get set to
>     zero?
>
>   From OpenGL 3.3 spec, page 140:
>      "If the base internal format is DEPTH_STENCIL and format is not DEPTH_-
>       STENCIL, then the values of the stencil index texture components
> are undefined."
>
>
>
>         +   {GL_DEPTH24_STENCIL8, GL_DEPTH_STENCIL, GL_UNSIGNED_INT_24_8},
>         +   {GL_DEPTH32F_STENCIL8, GL_DEPTH_STENCIL, GL_UNSIGNED_INT_24_8}};
>         +
>         +static const struct format_desc formats_not_allowed[] = {
>         +   {GL_DEPTH_COMPONENT16, GL_STENCIL_INDEX, GL_INT},
>         +   {GL_DEPTH_COMPONENT24, GL_STENCIL_INDEX, GL_INT},
>         +   {GL_DEPTH_COMPONENT32F, GL_STENCIL_INDEX, GL_INT},
>         +
>         +   {GL_DEPTH24_STENCIL8, GL_STENCIL_INDEX, GL_INT},
>         +   {GL_DEPTH32F_STENCIL8, GL_STENCIL_INDEX, GL_INT},
>         +
>         +   {GL_RGBA8, GL_DEPTH_COMPONENT, GL_FLOAT},
>         +   {GL_RGBA8, GL_DEPTH_STENCIL, GL_UNSIGNED_INT_24_8}};
>
>            /** Test target params to glTexImage functions */
>            static GLboolean
>         @@ -167,7 +197,34 @@ test_pos_and_sizes(void)
>               return GL_TRUE;
>            }
>
>         -
>         +/* Test the combinations of depth formats in glTexImage{123}D() */
>         +static GLboolean
>
>
>     bool
>
>
>
>         +test_depth_formats(const struct format_desc *test, GLenum
>         expected_error,
>         +                   GLint n_tests)
>         +{
>         +   int i;
>         +   bool result = true;
>         +
>         +   for(i = 0; i < n_tests; i++) {
>
>
>     We usually put a space after "for" and "if"
>
>
>
>         +      if((test[i].internalformat == GL_DEPTH_COMPONENT32F ||
>         +          test[i].internalformat == GL_DEPTH32F_STENCIL8) &&
>         +
>           !piglit_is_extension___supported("GL_ARB_depth___buffer_float"))
>         +         continue;
>         +
>         +      glTexImage1D(GL_TEXTURE_1D, 0, test[i].internalformat, 16, 0,
>         +                   test[i].format, test[i].type, NULL);
>         +      result = piglit_check_gl_error(__expected_error) && result;
>         +
>         +      glTexImage2D(GL_TEXTURE_2D, 0, test[i].internalformat,
>         16, 16,
>         +                   0, test[i].format, test[i].type, NULL);
>         +      result = piglit_check_gl_error(__expected_error) && result;
>         +
>         +      glTexImage3D(GL_TEXTURE_2D___ARRAY, 0,
>         test[i].internalformat,
>         +                   16, 16, 16, 0, test[i].format, test[i].type,
>         NULL);
>
>
>     Do we need to check if the texture array extension is available?
>
> Yes. I'll add a check before  glTexImage3D() call.
>
>
>
>         +      result = piglit_check_gl_error(__expected_error) && result;
>         +   }
>         +   return result;
>         +}
>            enum piglit_result
>            piglit_display(void)
>            {
>         @@ -175,6 +232,20 @@ piglit_display(void)
>               pass = test_targets() && pass;
>               pass = test_pos_and_sizes() && pass;
>
>         +  /*  From OpenGL 3.3 spec, page 141:
>         +   *    "Textures with a base internal format of DEPTH_COMPONENT or
>         +   *     DEPTH_STENCIL require either depth component data or
>         depth/stencil
>         +   *     component data. Textures with other base internal
>         formats require
>         +   *     RGBA component data. The error INVALID_OPERATION is
>         generated if
>         +   *     one of the base internal format and format is
>         DEPTH_COMPONENT or
>         +   *     DEPTH_STENCIL, and the other is neither of these values."
>         +   */
>         +   pass = test_depth_formats(formats___allowed, GL_NO_ERROR,
>         +                             ARRAY_SIZE(formats_allowed))
>         +          && pass;
>         +   pass = test_depth_formats(formats___not_allowed,
>         GL_INVALID_OPERATION,
>         +                             ARRAY_SIZE(formats_not___allowed))
>         +          && pass;
>               return pass ? PIGLIT_PASS: PIGLIT_FAIL;
>            }
>
>
>
>     Looks OK otherwise.
>
>   After fixing your comments, should I consider this r-b you?

Yes.  Reviewed-by: Brian Paul <brianp at vmware.com>