[Mesa-dev] [PATCH v2 10/17] mesa: Add missing check of format and type in glTexSubImageXD on GLES 3.0

[Brian Paul]

On 08/04/2015 05:07 AM, Eduardo Lima Mitev wrote:
> Argument validation for glTexSubImageXD is missing a check of format and type
> against texture object's internal format when profile is OpenGL-ES 3.0+.
>
> This patch also groups together all format and type checks into a single
> block of code for clarity.
>
> Fixes 2 dEQP tests:
> * dEQP-GLES3.functional.negative_api.texture.texsubimage2d
> * dEQP-GLES3.functional.negative_api.texture.texsubimage3d
> ---
>   src/mesa/main/teximage.c | 100 +++++++++++++++++++++++++----------------------
>   1 file changed, 53 insertions(+), 47 deletions(-)
>
> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
> index d19ad64..b53f027 100644
> --- a/src/mesa/main/teximage.c
> +++ b/src/mesa/main/teximage.c
> @@ -2088,6 +2088,37 @@ texture_formats_agree(GLenum internalFormat,
>      return true;
>   }
>

Can you put a comment on this function, at least explaining the return 
value?

> +static bool
> +texture_format_error_check_gles(struct gl_context *ctx, GLenum format,
> +                                GLenum type, GLenum internalFormat,
> +                                GLuint dimensions, const char *callerName) {

Opening brace on next line.


> +   GLenum err;
> +
> +   if (!_mesa_is_gles3(ctx)) {
> +      err = _mesa_es_error_check_format_and_type(format, type, dimensions);
> +      if (err != GL_NO_ERROR) {
> +         _mesa_error(ctx, err, "%s(format = %s, type = %s)",
> +                     callerName, _mesa_enum_to_string(format),
> +                     _mesa_enum_to_string(type));
> +         return true;
> +      }
> +   }
> +   else {
> +      err = _mesa_es3_error_check_format_and_type(ctx, format, type,
> +                                                  internalFormat);
> +      if (err != GL_NO_ERROR) {
> +         _mesa_error(ctx, err,
> +                     "%s(format = %s, type = %s, internalformat = %s)",
> +                     callerName, _mesa_enum_to_string(format),
> +                     _mesa_enum_to_string(type),
> +                     _mesa_enum_to_string(internalFormat));
> +         return true;
> +      }
> +   }

How about using 'if (_mesa_is_gles3(ctx)) {' and swapping the if/else 
clauses?  I think that's a little simpler to read.


> +
> +   return false;
> +}
> +
>   /**
>    * Test the glTexImage[123]D() parameters for errors.
>    *
> @@ -2159,32 +2190,10 @@ texture_error_check( struct gl_context *ctx,
>       * Formats and types that require additional extensions (e.g., GL_FLOAT
>       * requires GL_OES_texture_float) are filtered elsewhere.
>       */
> -
> -   if (_mesa_is_gles(ctx)) {
> -      if (_mesa_is_gles3(ctx)) {
> -         err = _mesa_es3_error_check_format_and_type(ctx, format, type,
> -                                                     internalFormat);
> -      } else {
> -         if (format != internalFormat) {
> -            _mesa_error(ctx, GL_INVALID_OPERATION,
> -                        "glTexImage%dD(format = %s, internalFormat = %s)",
> -                        dimensions,
> -                        _mesa_enum_to_string(format),
> -                        _mesa_enum_to_string(internalFormat));
> -            return GL_TRUE;
> -         }
> -
> -         err = _mesa_es_error_check_format_and_type(format, type, dimensions);
> -      }
> -      if (err != GL_NO_ERROR) {
> -         _mesa_error(ctx, err,
> -                     "glTexImage%dD(format = %s, type = %s, internalFormat = %s)",
> -                     dimensions,
> -                     _mesa_enum_to_string(format),
> -                     _mesa_enum_to_string(type),
> -                     _mesa_enum_to_string(internalFormat));
> -         return GL_TRUE;
> -      }
> +   if (_mesa_is_gles(ctx) &&
> +       texture_format_error_check_gles(ctx, format, type, internalFormat,
> +                                       dimensions, "glTexImage%dD")) {
> +     return GL_TRUE;
>      }
>
>      /* Check internalFormat */
> @@ -2493,19 +2502,12 @@ texsubimage_error_check(struct gl_context *ctx, GLuint dimensions,
>         return GL_TRUE;
>      }
>
> -   /* OpenGL ES 1.x and OpenGL ES 2.0 impose additional restrictions on the
> -    * combinations of format and type that can be used.  Formats and types
> -    * that require additional extensions (e.g., GL_FLOAT requires
> -    * GL_OES_texture_float) are filtered elsewhere.
> -    */
> -   if (_mesa_is_gles(ctx) && !_mesa_is_gles3(ctx)) {
> -      err = _mesa_es_error_check_format_and_type(format, type, dimensions);
> -      if (err != GL_NO_ERROR) {
> -         _mesa_error(ctx, err, "%s(format = %s, type = %s)",
> -                     callerName, _mesa_enum_to_string(format),
> -                     _mesa_enum_to_string(type));
> -         return GL_TRUE;
> -      }
> +   texImage = _mesa_select_tex_image(texObj, target, level);
> +   if (!texImage) {
> +      /* non-existant texture level */
> +      _mesa_error(ctx, GL_INVALID_OPERATION, "%s(invalid texture image)",
> +                  callerName);
> +      return GL_TRUE;
>      }
>
>      err = _mesa_error_check_format_and_type(ctx, format, type);
> @@ -2517,6 +2519,18 @@ texsubimage_error_check(struct gl_context *ctx, GLuint dimensions,
>         return GL_TRUE;
>      }
>
> +   /* OpenGL ES 1.x and OpenGL ES 2.0 impose additional restrictions on the
> +    * combinations of format, internalFormat, and type that can be used.
> +    * Formats and types that require additional extensions (e.g., GL_FLOAT
> +    * requires GL_OES_texture_float) are filtered elsewhere.
> +    */
> +   if (_mesa_is_gles(ctx) &&
> +       texture_format_error_check_gles(ctx, format, type,
> +                                       texImage->InternalFormat,
> +                                       dimensions, callerName)) {
> +      return GL_TRUE;
> +   }
> +
>      /* validate the bound PBO, if any */
>      if (!_mesa_validate_pbo_source(ctx, dimensions, &ctx->Unpack,
>                                     width, height, depth, format, type,
> @@ -2524,14 +2538,6 @@ texsubimage_error_check(struct gl_context *ctx, GLuint dimensions,
>         return GL_TRUE;
>      }
>
> -   texImage = _mesa_select_tex_image(texObj, target, level);
> -   if (!texImage) {
> -      /* non-existant texture level */
> -      _mesa_error(ctx, GL_INVALID_OPERATION, "%s(invalid texture image)",
> -                  callerName);
> -      return GL_TRUE;
> -   }
> -
>      if (error_check_subtexture_dimensions(ctx, dimensions,
>                                            texImage, xoffset, yoffset, zoffset,
>                                            width, height, depth, callerName)) {
>

The rest looks good.

-Brian