[Mesa-dev] [PATCH 1/3] mesa: new _mesa_error_check_format_and_type() function
Ian Romanick
idr at freedesktop.org
Thu Feb 2 09:02:10 PST 2012
On 02/02/2012 07:14 AM, Brian Paul wrote:
> This replaces the _mesa_is_legal_format_and_type() function.
>
> According to the spec, some invalid format/type combinations to
> glDrawPixels, ReadPixels and glTexImage should generate
> GL_INVALID_ENUM but others should generate GL_INVALID_OPERATION.
>
> With the old function we didn't make that distinction and generated
> GL_INVALID_ENUM errors instead of GL_INVALID_OPERATION. The new
> function returns one of those errors or GL_NO_ERROR.
>
> This will also let us remove some redundant format/type checks in
> follow-on commit.
> ---
> src/mesa/main/image.c | 212 ++++++++++++++++++++++++++++++-------------
> src/mesa/main/image.h | 6 +-
> src/mesa/main/readpix.c | 9 +-
> src/mesa/main/texgetimage.c | 16 ++--
> src/mesa/main/teximage.c | 25 ++---
> 5 files changed, 174 insertions(+), 94 deletions(-)
>
> diff --git a/src/mesa/main/image.c b/src/mesa/main/image.c
> index 3491704..cc44468 100644
> --- a/src/mesa/main/image.c
> +++ b/src/mesa/main/image.c
> @@ -356,18 +356,79 @@ _mesa_bytes_per_pixel( GLenum format, GLenum type )
>
>
> /**
> - * Test for a legal pixel format and type.
> + * Do error checking of format/type combinations for glReadPixels,
> + * glDrawPixels and glTex[Sub]Image. Note that depending on the format
> + * and type values, we may either generate GL_INVALID_OPERATION or
> + * GL_INVALID_ENUM.
> *
> * \param format pixel format.
> * \param type pixel type.
> *
> - * \return GL_TRUE if the given pixel format and type are legal, or GL_FALSE
> - * otherwise.
> + * \return GL_INVALID_ENUM, GL_INVALID_OPERATION or GL_NO_ERROR
> */
> -GLboolean
> -_mesa_is_legal_format_and_type(const struct gl_context *ctx,
> - GLenum format, GLenum type)
> +GLenum
> +_mesa_error_check_format_and_type(const struct gl_context *ctx,
> + GLenum format, GLenum type)
> {
> + /* special type-based checks (see glReadPixels, glDrawPixels error lists) */
> + switch (type) {
> + case GL_BITMAP:
> + if (format != GL_COLOR_INDEX&&
> + format != GL_STENCIL_INDEX) {
> + return GL_INVALID_ENUM;
> + }
> + break;
> +
> + case GL_UNSIGNED_BYTE_3_3_2:
> + case GL_UNSIGNED_BYTE_2_3_3_REV:
> + case GL_UNSIGNED_SHORT_5_6_5:
> + case GL_UNSIGNED_SHORT_5_6_5_REV:
> + if (format != GL_RGB&&
> + format != GL_RGB_INTEGER_EXT) {
> + return GL_INVALID_OPERATION;
I don't think the packed types can be used with *_INTEGER. At the very
least, the code in pack.c can't handle it. Table 3.8 in the 3.0 spec
only lists RGB, RGBA, BGRA, and DEPTH_STENCIL as valid formats for
packed types. The EXT_abgr spec adds ABGR_EXT, of course.
> + }
> + break;
> +
> + case GL_UNSIGNED_SHORT_4_4_4_4:
> + case GL_UNSIGNED_SHORT_4_4_4_4_REV:
> + case GL_UNSIGNED_SHORT_5_5_5_1:
> + case GL_UNSIGNED_SHORT_1_5_5_5_REV:
> + case GL_UNSIGNED_INT_8_8_8_8:
> + case GL_UNSIGNED_INT_8_8_8_8_REV:
> + case GL_UNSIGNED_INT_10_10_10_2:
> + case GL_UNSIGNED_INT_2_10_10_10_REV:
> + if (format != GL_RGBA&&
> + format != GL_BGRA&&
> + format != GL_ABGR_EXT&&
> + format != GL_RGBA_INTEGER_EXT&&
> + format != GL_BGRA_INTEGER_EXT) {
> + return GL_INVALID_OPERATION;
> + }
> + break;
> +
> + case GL_UNSIGNED_INT_24_8:
> + if (!ctx->Extensions.EXT_packed_depth_stencil) {
> + return GL_INVALID_ENUM;
> + }
> + if (format != GL_DEPTH_STENCIL) {
> + return GL_INVALID_OPERATION;
> + }
> + return GL_NO_ERROR;
> +
> + case GL_FLOAT_32_UNSIGNED_INT_24_8_REV:
> + if (!ctx->Extensions.ARB_depth_buffer_float) {
> + return GL_INVALID_ENUM;
> + }
> + if (format != GL_DEPTH_STENCIL) {
> + return GL_INVALID_OPERATION;
> + }
> + return GL_NO_ERROR;
> +
> + default:
> + ; /* fall-through */
> + }
> +
> + /* now, for each format, check the type for compatibility */
> switch (format) {
> case GL_COLOR_INDEX:
> case GL_STENCIL_INDEX:
> @@ -380,12 +441,14 @@ _mesa_is_legal_format_and_type(const struct gl_context *ctx,
> case GL_INT:
> case GL_UNSIGNED_INT:
> case GL_FLOAT:
> - return GL_TRUE;
> - case GL_HALF_FLOAT_ARB:
> - return ctx->Extensions.ARB_half_float_pixel;
> + return GL_NO_ERROR;
> + case GL_HALF_FLOAT:
> + return ctx->Extensions.ARB_half_float_pixel
> + ? GL_NO_ERROR : GL_INVALID_ENUM;
> default:
> - return GL_FALSE;
> + return GL_INVALID_ENUM;
> }
> +
> case GL_RED:
> case GL_GREEN:
> case GL_BLUE:
> @@ -404,16 +467,17 @@ _mesa_is_legal_format_and_type(const struct gl_context *ctx,
> case GL_INT:
> case GL_UNSIGNED_INT:
> case GL_FLOAT:
> - return GL_TRUE;
> - case GL_HALF_FLOAT_ARB:
> - return ctx->Extensions.ARB_half_float_pixel;
> + return GL_NO_ERROR;
> + case GL_HALF_FLOAT:
> + return ctx->Extensions.ARB_half_float_pixel
> + ? GL_NO_ERROR : GL_INVALID_ENUM;
> default:
> - return GL_FALSE;
> + return GL_INVALID_ENUM;
> }
> +
> case GL_RG:
> if (!ctx->Extensions.ARB_texture_rg)
> - return GL_FALSE;
> -
> + return GL_INVALID_ENUM;
> switch (type) {
> case GL_BYTE:
> case GL_UNSIGNED_BYTE:
> @@ -422,12 +486,14 @@ _mesa_is_legal_format_and_type(const struct gl_context *ctx,
> case GL_INT:
> case GL_UNSIGNED_INT:
> case GL_FLOAT:
> - return GL_TRUE;
> - case GL_HALF_FLOAT_ARB:
> - return ctx->Extensions.ARB_half_float_pixel;
> + return GL_NO_ERROR;
> + case GL_HALF_FLOAT:
> + return ctx->Extensions.ARB_half_float_pixel
> + ? GL_NO_ERROR : GL_INVALID_ENUM;
> default:
> - return GL_FALSE;
> + return GL_INVALID_ENUM;
> }
> +
> case GL_RGB:
> switch (type) {
> case GL_BYTE:
> @@ -441,16 +507,20 @@ _mesa_is_legal_format_and_type(const struct gl_context *ctx,
> case GL_UNSIGNED_BYTE_2_3_3_REV:
> case GL_UNSIGNED_SHORT_5_6_5:
> case GL_UNSIGNED_SHORT_5_6_5_REV:
> - return GL_TRUE;
> - case GL_HALF_FLOAT_ARB:
> - return ctx->Extensions.ARB_half_float_pixel;
> + return GL_NO_ERROR;
> + case GL_HALF_FLOAT:
> + return ctx->Extensions.ARB_half_float_pixel
> + ? GL_NO_ERROR : GL_INVALID_ENUM;
> case GL_UNSIGNED_INT_5_9_9_9_REV:
> - return ctx->Extensions.EXT_texture_shared_exponent;
> + return ctx->Extensions.EXT_texture_shared_exponent
> + ? GL_NO_ERROR : GL_INVALID_ENUM;
> case GL_UNSIGNED_INT_10F_11F_11F_REV:
> - return ctx->Extensions.EXT_packed_float;
> + return ctx->Extensions.EXT_packed_float
> + ? GL_NO_ERROR : GL_INVALID_ENUM;
> default:
> - return GL_FALSE;
> + return GL_INVALID_ENUM;
> }
> +
> case GL_BGR:
> switch (type) {
> /* NOTE: no packed types are supported with BGR. That's
> @@ -463,12 +533,14 @@ _mesa_is_legal_format_and_type(const struct gl_context *ctx,
> case GL_INT:
> case GL_UNSIGNED_INT:
> case GL_FLOAT:
> - return GL_TRUE;
> - case GL_HALF_FLOAT_ARB:
> - return ctx->Extensions.ARB_half_float_pixel;
> + return GL_NO_ERROR;
> + case GL_HALF_FLOAT:
> + return ctx->Extensions.ARB_half_float_pixel
> + ? GL_NO_ERROR : GL_INVALID_ENUM;
> default:
> - return GL_FALSE;
> + return GL_INVALID_ENUM;
> }
> +
> case GL_RGBA:
> case GL_BGRA:
> case GL_ABGR_EXT:
> @@ -488,28 +560,37 @@ _mesa_is_legal_format_and_type(const struct gl_context *ctx,
> case GL_UNSIGNED_INT_8_8_8_8_REV:
> case GL_UNSIGNED_INT_10_10_10_2:
> case GL_UNSIGNED_INT_2_10_10_10_REV:
> - return GL_TRUE;
> - case GL_HALF_FLOAT_ARB:
> - return ctx->Extensions.ARB_half_float_pixel;
> + return GL_NO_ERROR;
> + case GL_HALF_FLOAT:
> + return ctx->Extensions.ARB_half_float_pixel
> + ? GL_NO_ERROR : GL_INVALID_ENUM;
> default:
> - return GL_FALSE;
> + return GL_INVALID_ENUM;
> }
> +
> case GL_YCBCR_MESA:
> + if (!ctx->Extensions.MESA_ycbcr_texture)
> + return GL_INVALID_ENUM;
> if (type == GL_UNSIGNED_SHORT_8_8_MESA ||
> type == GL_UNSIGNED_SHORT_8_8_REV_MESA)
> - return GL_TRUE;
> + return GL_NO_ERROR;
> else
> - return GL_FALSE;
> + return GL_INVALID_OPERATION;
> +
> case GL_DEPTH_STENCIL_EXT:
> - if ((ctx->Extensions.EXT_packed_depth_stencil&&
> - type == GL_UNSIGNED_INT_24_8_EXT) ||
> - (ctx->Extensions.ARB_depth_buffer_float&&
> - type == GL_FLOAT_32_UNSIGNED_INT_24_8_REV))
> - return GL_TRUE;
> + if (ctx->Extensions.EXT_packed_depth_stencil&&
> + type == GL_UNSIGNED_INT_24_8)
> + return GL_NO_ERROR;
> + else if (ctx->Extensions.ARB_depth_buffer_float&&
> + type == GL_FLOAT_32_UNSIGNED_INT_24_8_REV)
> + return GL_NO_ERROR;
> else
> - return GL_FALSE;
> + return GL_INVALID_ENUM;
> +
> case GL_DUDV_ATI:
> case GL_DU8DV8_ATI:
> + if (!ctx->Extensions.ATI_envmap_bumpmap)
> + return GL_INVALID_ENUM;
> switch (type) {
> case GL_BYTE:
> case GL_UNSIGNED_BYTE:
> @@ -518,9 +599,9 @@ _mesa_is_legal_format_and_type(const struct gl_context *ctx,
> case GL_INT:
> case GL_UNSIGNED_INT:
> case GL_FLOAT:
> - return GL_TRUE;
> + return GL_NO_ERROR;
> default:
> - return GL_FALSE;
> + return GL_INVALID_ENUM;
> }
>
> /* integer-valued formats */
> @@ -536,10 +617,11 @@ _mesa_is_legal_format_and_type(const struct gl_context *ctx,
> case GL_UNSIGNED_SHORT:
> case GL_INT:
> case GL_UNSIGNED_INT:
> - return ctx->VersionMajor>= 3 ||
> - ctx->Extensions.EXT_texture_integer;
> + return (ctx->VersionMajor>= 3 ||
> + ctx->Extensions.EXT_texture_integer)
> + ? GL_NO_ERROR : GL_INVALID_ENUM;
> default:
> - return GL_FALSE;
> + return GL_INVALID_ENUM;
> }
>
> case GL_RGB_INTEGER_EXT:
> @@ -550,15 +632,17 @@ _mesa_is_legal_format_and_type(const struct gl_context *ctx,
> case GL_UNSIGNED_SHORT:
> case GL_INT:
> case GL_UNSIGNED_INT:
> - return ctx->VersionMajor>= 3 ||
> - ctx->Extensions.EXT_texture_integer;
> + return (ctx->VersionMajor>= 3 ||
> + ctx->Extensions.EXT_texture_integer)
> + ? GL_NO_ERROR : GL_INVALID_ENUM;
> case GL_UNSIGNED_BYTE_3_3_2:
> case GL_UNSIGNED_BYTE_2_3_3_REV:
> case GL_UNSIGNED_SHORT_5_6_5:
> case GL_UNSIGNED_SHORT_5_6_5_REV:
> - return ctx->Extensions.ARB_texture_rgb10_a2ui;
> + return ctx->Extensions.ARB_texture_rgb10_a2ui
> + ? GL_NO_ERROR : GL_INVALID_ENUM;
> default:
> - return GL_FALSE;
> + return GL_INVALID_ENUM;
> }
>
> case GL_BGR_INTEGER_EXT:
> @@ -570,10 +654,11 @@ _mesa_is_legal_format_and_type(const struct gl_context *ctx,
> case GL_INT:
> case GL_UNSIGNED_INT:
> /* NOTE: no packed formats w/ BGR format */
> - return ctx->VersionMajor>= 3 ||
> - ctx->Extensions.EXT_texture_integer;
> + return (ctx->VersionMajor>= 3 ||
> + ctx->Extensions.EXT_texture_integer)
> + ? GL_NO_ERROR : GL_INVALID_ENUM;
> default:
> - return GL_FALSE;
> + return GL_INVALID_ENUM;
> }
>
> case GL_RGBA_INTEGER_EXT:
> @@ -585,8 +670,9 @@ _mesa_is_legal_format_and_type(const struct gl_context *ctx,
> case GL_UNSIGNED_SHORT:
> case GL_INT:
> case GL_UNSIGNED_INT:
> - return ctx->VersionMajor>= 3 ||
> - ctx->Extensions.EXT_texture_integer;
> + return (ctx->VersionMajor>= 3 ||
> + ctx->Extensions.EXT_texture_integer)
> + ? GL_NO_ERROR : GL_INVALID_ENUM;
> case GL_UNSIGNED_SHORT_4_4_4_4:
> case GL_UNSIGNED_SHORT_4_4_4_4_REV:
> case GL_UNSIGNED_SHORT_5_5_5_1:
> @@ -595,9 +681,10 @@ _mesa_is_legal_format_and_type(const struct gl_context *ctx,
> case GL_UNSIGNED_INT_8_8_8_8_REV:
> case GL_UNSIGNED_INT_10_10_10_2:
> case GL_UNSIGNED_INT_2_10_10_10_REV:
> - return ctx->Extensions.ARB_texture_rgb10_a2ui;
> + return ctx->Extensions.ARB_texture_rgb10_a2ui
> + ? GL_NO_ERROR : GL_INVALID_ENUM;
> default:
> - return GL_FALSE;
> + return GL_INVALID_ENUM;
> }
>
> case GL_LUMINANCE_INTEGER_EXT:
> @@ -609,15 +696,16 @@ _mesa_is_legal_format_and_type(const struct gl_context *ctx,
> case GL_UNSIGNED_SHORT:
> case GL_INT:
> case GL_UNSIGNED_INT:
> - return ctx->Extensions.EXT_texture_integer;
> + return ctx->Extensions.EXT_texture_integer
> + ? GL_NO_ERROR : GL_INVALID_ENUM;
> default:
> - return GL_FALSE;
> + return GL_INVALID_ENUM;
> }
>
> default:
> - ; /* fall-through */
> + return GL_INVALID_ENUM;
> }
> - return GL_FALSE;
> + return GL_NO_ERROR;
> }
>
>
> diff --git a/src/mesa/main/image.h b/src/mesa/main/image.h
> index e4961ed..f1ed883 100644
> --- a/src/mesa/main/image.h
> +++ b/src/mesa/main/image.h
> @@ -53,9 +53,9 @@ _mesa_components_in_format( GLenum format );
> extern GLint
> _mesa_bytes_per_pixel( GLenum format, GLenum type );
>
> -extern GLboolean
> -_mesa_is_legal_format_and_type(const struct gl_context *ctx,
> - GLenum format, GLenum type);
> +extern GLenum
> +_mesa_error_check_format_and_type(const struct gl_context *ctx,
> + GLenum format, GLenum type);
>
> extern GLboolean
> _mesa_is_color_format(GLenum format);
> diff --git a/src/mesa/main/readpix.c b/src/mesa/main/readpix.c
> index 908a55e..6c64cbe 100644
> --- a/src/mesa/main/readpix.c
> +++ b/src/mesa/main/readpix.c
> @@ -588,6 +588,7 @@ _mesa_error_check_format_type(struct gl_context *ctx, GLenum format,
> {
> const char *readDraw = drawing ? "Draw" : "Read";
> const GLboolean reading = !drawing;
> + GLenum err;
>
> /* state validation should have already been done */
> ASSERT(ctx->NewState == 0x0);
> @@ -609,9 +610,9 @@ _mesa_error_check_format_type(struct gl_context *ctx, GLenum format,
> }
>
> /* basic combinations test */
> - if (!_mesa_is_legal_format_and_type(ctx, format, type)) {
> - _mesa_error(ctx, GL_INVALID_ENUM,
> - "gl%sPixels(format or type)", readDraw);
> + err = _mesa_error_check_format_and_type(ctx, format, type);
> + if (err != GL_NO_ERROR) {
> + _mesa_error(ctx, err, "gl%sPixels(format or type)", readDraw);
> return GL_TRUE;
> }
>
> @@ -715,7 +716,7 @@ _mesa_error_check_format_type(struct gl_context *ctx, GLenum format,
> }
> break;
> default:
> - /* this should have been caught in _mesa_is_legal_format_type() */
> + /* this should have been caught in _mesa_error_check_format_type() */
> _mesa_problem(ctx, "unexpected format in _mesa_%sPixels", readDraw);
> return GL_TRUE;
> }
> diff --git a/src/mesa/main/texgetimage.c b/src/mesa/main/texgetimage.c
> index 42495c8..58fed11 100644
> --- a/src/mesa/main/texgetimage.c
> +++ b/src/mesa/main/texgetimage.c
> @@ -734,7 +734,7 @@ getteximage_error_check(struct gl_context *ctx, GLenum target, GLint level,
> struct gl_texture_image *texImage;
> const GLint maxLevels = _mesa_max_texture_levels(ctx, target);
> const GLuint dimensions = (target == GL_TEXTURE_3D) ? 3 : 2;
> - GLenum baseFormat;
> + GLenum baseFormat, err;
>
> if (maxLevels == 0) {
> _mesa_error(ctx, GL_INVALID_ENUM, "glGetTexImage(target=0x%x)", target);
> @@ -777,6 +777,12 @@ getteximage_error_check(struct gl_context *ctx, GLenum target, GLint level,
> if (!ctx->Extensions.ATI_envmap_bumpmap
> && _mesa_is_dudv_format(format)) {
> _mesa_error(ctx, GL_INVALID_ENUM, "glGetTexImage(format)");
> + return;
> + }
> +
> + err = _mesa_error_check_format_and_type(ctx, format, type);
> + if (err != GL_NO_ERROR) {
> + _mesa_error(ctx, err, "glGetTexImage(format/type)");
> return GL_TRUE;
> }
>
> @@ -787,14 +793,6 @@ getteximage_error_check(struct gl_context *ctx, GLenum target, GLint level,
> return GL_TRUE;
> }
>
> - if (!_mesa_is_legal_format_and_type(ctx, format, type)) {
> - /* GL_INVALID_OPERATION is generated by a format/type
> - * mismatch (see the 1.2 spec page 94, sec 3.6.4.)
> - */
> - _mesa_error(ctx, GL_INVALID_OPERATION, "glGetTexImage(target)");
> - return GL_TRUE;
> - }
> -
> texImage = _mesa_select_tex_image(ctx, texObj, target, level);
> if (!texImage) {
> /* non-existant texture image */
> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
> index d11425d..2ac3e40 100644
> --- a/src/mesa/main/teximage.c
> +++ b/src/mesa/main/teximage.c
> @@ -1511,6 +1511,7 @@ texture_error_check( struct gl_context *ctx,
> const GLboolean isProxy = target == proxyTarget;
> GLboolean sizeOK = GL_TRUE;
> GLboolean colorFormat;
> + GLenum err;
>
> /* Even though there are no color-index textures, we still have to support
> * uploading color-index data and remapping it to RGB via the
> @@ -1579,16 +1580,10 @@ texture_error_check( struct gl_context *ctx,
> }
>
> /* Check incoming image format and type */
> - if (!_mesa_is_legal_format_and_type(ctx, format, type)) {
> - /* Normally, GL_INVALID_OPERATION is generated by a format/type
> - * mismatch (see the 1.2 spec page 94, sec 3.6.4.). But with the
> - * GL_EXT_texture_integer extension, some combinations should generate
> - * GL_INVALID_ENUM instead (grr!).
> - */
> + err = _mesa_error_check_format_and_type(ctx, format, type);
> + if (err != GL_NO_ERROR) {
> if (!isProxy) {
> - GLenum error = _mesa_is_integer_format(format)
> - ? GL_INVALID_ENUM : GL_INVALID_OPERATION;
> - _mesa_error(ctx, error,
> + _mesa_error(ctx, err,
> "glTexImage%dD(incompatible format 0x%x, type 0x%x)",
> dimensions, format, type);
> }
> @@ -1737,6 +1732,8 @@ subtexture_error_check( struct gl_context *ctx, GLuint dimensions,
> GLint width, GLint height, GLint depth,
> GLenum format, GLenum type )
> {
> + GLenum err;
> +
> /* Basic level check */
> if (level< 0 || level>= MAX_TEXTURE_LEVELS) {
> _mesa_error(ctx, GL_INVALID_ENUM, "glTexSubImage2D(level=%d)", level);
> @@ -1760,13 +1757,9 @@ subtexture_error_check( struct gl_context *ctx, GLuint dimensions,
> return GL_TRUE;
> }
>
> - if (!_mesa_is_legal_format_and_type(ctx, format, type)) {
> - /* As with the glTexImage2D check above, the error code here
> - * depends on texture integer.
> - */
> - GLenum error = _mesa_is_integer_format(format)
> - ? GL_INVALID_OPERATION : GL_INVALID_ENUM;
> - _mesa_error(ctx, error,
> + err = _mesa_error_check_format_and_type(ctx, format, type);
> + if (err != GL_NO_ERROR) {
> + _mesa_error(ctx, err,
> "glTexSubImage%dD(incompatible format 0x%x, type 0x%x)",
> dimensions, format, type);
> return GL_TRUE;
More information about the mesa-dev
mailing list