Re: [Mesa-dev] [PATCH 1/3] mesa: new _mesa_error_check_format_and_type() function
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
Re: [Mesa-dev] [PATCH 1/3] mesa: new _mesa_error_check_format_and_type() function
[don't know why the indention is messed up below, but anyway...] On 02/02/2012 10:02 AM, Ian Romanick wrote: 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. It looks like packed integer formats are added in GL 3.3. But they're also listed in GL_ARB_texture_rgb10_a2ui which is supposedly based on GL 3.2. It's not clear to me if GL_ARB_texture_rgb10_a2ui is supposed to enable all the packed integer formats or just the 10/10/10/2 packed format. Hmmm. Anyway, if you look further down in that function you'll see finer-grained format/type checking which checks for ctx-Extensions.ARB_texture_rgb10_a2ui for those cases. But I suppose we need more checks at the earlier point to generate GL_INVALID_OPERATION anyway. I can do that. I haven't looked if any drivers support GL_ARB_texture_rgb10_a2ui. -Brian ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] mesa: new _mesa_error_check_format_and_type() function
On 02/02/2012 10:26 AM, Brian Paul wrote: [don't know why the indention is messed up below, but anyway...] On 02/02/2012 10:02 AM, Ian Romanick wrote: 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. It looks like packed integer formats are added in GL 3.3. But they're also listed in GL_ARB_texture_rgb10_a2ui which is supposedly based on GL 3.2. It's not clear to me if GL_ARB_texture_rgb10_a2ui is supposed to enable all the packed integer formats or just the 10/10/10/2 packed format. Hmmm. That's right! That's issue #4 in the GL_ARB_texture_rgb10_a2ui spec: 4. It is possible to load packed 10_10_10_2 unsigned integer data into GL via TexImage without this extension? RESOLVED: No. The EXT_texture_integer extension, as later incorporated into OpenGL 3.0, added new integer pixel format enums (e.g., RGBA_INTEGER) and texture formats (e.g., RGBA16UI). All texture formats added by that extension had a matching non-packed format and type combination, so there wasn't a strong need to explicitly support packed pixel types for integer pixel formats. The texture format added by this extension logically maps to a packed format/type combination, so we add this support by adding RGB_INTEGER, RGBA_INTEGER, and BGRA_INTEGER (as appropriate) to the list of formats supported by packed pixel data types. Even though we are only adding one packed texture format, we chose to allow all packed types with corresponding integer formats for orthogonality. Anyway, if you look further down in that function you'll see finer-grained format/type checking which checks for ctx-Extensions.ARB_texture_rgb10_a2ui for those cases. But I suppose we need more checks at the earlier point to generate GL_INVALID_OPERATION anyway. I can do that. I haven't looked if any drivers support GL_ARB_texture_rgb10_a2ui. 'grep -r ARB_texture_rgb10_a2ui src/' indicates not. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] mesa: new _mesa_error_check_format_and_type() function
On 02/02/2012 10:44 AM, Ian Romanick wrote: On 02/02/2012 10:26 AM, Brian Paul wrote: [don't know why the indention is messed up below, but anyway...] On 02/02/2012 10:02 AM, Ian Romanick wrote: 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. It looks like packed integer formats are added in GL 3.3. But they're also listed in GL_ARB_texture_rgb10_a2ui which is supposedly based on GL 3.2. It's not clear to me if GL_ARB_texture_rgb10_a2ui is supposed to enable all the packed integer formats or just the 10/10/10/2 packed format. Hmmm. That's right! That's issue #4 in the GL_ARB_texture_rgb10_a2ui spec: 4. It is possible to load packed 10_10_10_2 unsigned integer data into GL via TexImage without this extension? RESOLVED: No. The EXT_texture_integer extension, as later incorporated into OpenGL 3.0, added new integer pixel format enums (e.g., RGBA_INTEGER) and texture formats (e.g., RGBA16UI). All texture formats added by that extension had a matching non-packed format and type combination, so there wasn't a strong need to explicitly support packed pixel types for integer pixel formats. The texture format added by this extension logically maps to a packed format/type combination, so we add this support by adding RGB_INTEGER, RGBA_INTEGER, and BGRA_INTEGER (as appropriate) to the list of formats supported by packed pixel data types. Even though we are only adding one packed texture format, we chose to allow all packed types with corresponding integer formats for orthogonality. OK, so are you saying we should add the checks per GL_ARB_texture_rgb10_a2ui or just forget about packed integer formats entirely for now? Either way is OK by me. Anyway, if you look further down in that function you'll see finer-grained format/type checking which checks for ctx-Extensions.ARB_texture_rgb10_a2ui for those cases. But I suppose we need more checks at the earlier point to generate GL_INVALID_OPERATION anyway. I can do that. I haven't looked if any drivers support GL_ARB_texture_rgb10_a2ui. 'grep -r ARB_texture_rgb10_a2ui src/' indicates not. But the gallium state tracker queries PIPE_FORMAT_B10G10R10A2_UINT to turn on GL_ARB_texture_rgb10_a2ui and there's some mention of that format in the r600g driver: $ grep PIPE_FORMAT_B10G10R10A2_UINT -r src/gallium/drivers src/gallium/drivers/r600/r600_state.c: case PIPE_FORMAT_B10G10R10A2_UINT: src/gallium/drivers/r600/r600_state.c: case PIPE_FORMAT_B10G10R10A2_UINT: src/gallium/drivers/r600/evergreen_state.c: case PIPE_FORMAT_B10G10R10A2_UINT: src/gallium/drivers/r600/evergreen_state.c: