[Mesa-dev] [PATCH] mesa: Fix error code generation in glReadPixels()

[Anuj Phogat]

On Fri, Feb 14, 2014 at 7:00 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 02/14/2014 04:49 PM, Anuj Phogat wrote:
>> Section 4.3.1, page 220, of OpenGL 3.3 specification explains
>> the error conditions for glreadPixels():
>>
>>    "If the format is DEPTH_STENCIL, then values are taken from
>>     both the depth buffer and the stencil buffer. If there is
>>     no depth buffer or if there is no stencil buffer, then the
>>     error INVALID_OPERATION occurs. If the type parameter is
>>     not UNSIGNED_INT_24_8 or FLOAT_32_UNSIGNED_INT_24_8_REV,
>>     then the error INVALID_ENUM occurs."
>
> Add this quotation to the code with an extra comment "OpenGL ES still
> generates GL_INVALID_OPERATION because glReadPixels cannot be used to
> read depth or stencil in that API."
>
>> Fixes failing Khronos CTS test packed_depth_stencil_error.test
>>
>> Cc: <mesa-stable at lists.freedesktop.org>
>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>> ---
>>  src/mesa/main/glformats.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/main/glformats.c b/src/mesa/main/glformats.c
>> index 77cf263..b797900 100644
>> --- a/src/mesa/main/glformats.c
>> +++ b/src/mesa/main/glformats.c
>> @@ -1257,6 +1257,9 @@ _mesa_error_check_format_and_type(const struct gl_context *ctx,
>>            ctx->Extensions.ARB_texture_rgb10_a2ui) {
>>           break; /* OK */
>>        }
>> +      if (format == GL_DEPTH_STENCIL && _mesa_is_desktop_gl(ctx)) {
>> +         return GL_INVALID_ENUM;
>> +      }
>
> Wouldn't it be easier to just add the following before the
> switch-statement?
>
Yes, that'll avoid code duplication. I'll use your suggestion.

>    /* Section 4.3.1, page 220, of OpenGL 3.3 specification explains
>     * the error conditions for glreadPixels():
>     *
>     *     "If the format is DEPTH_STENCIL, then values are taken from both
>     *     the depth buffer and the stencil buffer. If there is no depth
>     *     buffer or if there is no stencil buffer, then the error
>     *     INVALID_OPERATION occurs. If the type parameter is not
>     *     UNSIGNED_INT_24_8 or FLOAT_32_UNSIGNED_INT_24_8_REV, then the
>     *     error INVALID_ENUM occurs."
>     *
>     * OpenGL ES still generates GL_INVALID_OPERATION because glReadPixels
>     * cannot be used to read depth or stencil in that API.
>     */
>    if (_mesa_is_desktop_gl(ctx) && format == GL_DEPTH_STENCIL
>        && type != GL_UNSIGNED_INT_24_8
>        && type != GL_FLOAT_32_UNSIGNED_INT_24_8_REV)
>       return GL_INVALID_ENUM;
>
>>        return GL_INVALID_OPERATION;
>>
>>     case GL_UNSIGNED_SHORT_4_4_4_4:
>> @@ -1280,6 +1283,9 @@ _mesa_error_check_format_and_type(const struct gl_context *ctx,
>>            ctx->API == API_OPENGLES2) {
>>           break; /* OK by GL_EXT_texture_type_2_10_10_10_REV */
>>        }
>> +      if (format == GL_DEPTH_STENCIL && _mesa_is_desktop_gl(ctx)) {
>> +         return GL_INVALID_ENUM;
>> +      }
>>        return GL_INVALID_OPERATION;
>>
>>     case GL_UNSIGNED_INT_24_8:
>> @@ -1298,7 +1304,8 @@ _mesa_error_check_format_and_type(const struct gl_context *ctx,
>>        return GL_NO_ERROR;
>>
>>     case GL_UNSIGNED_INT_10F_11F_11F_REV:
>> -      if (!ctx->Extensions.EXT_packed_float) {
>> +      if (!ctx->Extensions.EXT_packed_float ||
>> +          (format == GL_DEPTH_STENCIL && _mesa_is_desktop_gl(ctx))) {
>>           return GL_INVALID_ENUM;
>>        }
>>        if (format != GL_RGB) {
>>
>