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;

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to