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

[Ian Romanick]

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.