[Mesa-dev] [PATCH 1/3] mesa: new _mesa_error_check_format_and_type() function

[Brian Paul]

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:	case 
PIPE_FORMAT_B10G10R10A2_UINT:

Not sure if the format is actually supported though.  I haven't traced 
through the is_format_supported() code.

-Brian