[Mesa-dev] [PATCH] mesa: Check glReadBuffer enums against the ES3 table.

[Eduardo Lima Mitev]

On 03/24/2016 07:12 PM, Kenneth Graunke wrote:
> On Thursday, March 24, 2016 10:29:44 AM PDT Eduardo Lima Mitev wrote:
>> On 03/24/2016 07:54 AM, Kenneth Graunke wrote:
>>>   From the ES 3.2 spec, section 16.1.1 (Selecting Buffers for Reading):
>>>
>>>      "An INVALID_ENUM error is generated if src is not BACK or one of
>>>       the values from table 15.5."
>>>
>>> Table 15.5 contains NONE and COLOR_ATTACHMENTi.
>>>
>>> Mesa properly returned INVALID_ENUM for unknown enums, but it decided
>>> what was known by using read_buffer_enum_to_index, which handles all
>>> enums in every API.  So enums that were valid in GL were making it
>>> past the "valid enum" check.  Such targets would then be classified
>>> as unsupported, and we'd raise INVALID_OPERATION, but that's technically
>>> the wrong error code.
>>>
>>> Fixes dEQP-GLES31's
>>> functional.debug.negative_coverage.get_error.buffer.read_buffer
>>>
>>> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
>>> ---
>>>    src/mesa/main/buffers.c | 10 ++++++++++
>>>    1 file changed, 10 insertions(+)
>>>
>>> diff --git a/src/mesa/main/buffers.c b/src/mesa/main/buffers.c
>>> index 26dafd1..da90e00 100644
>>> --- a/src/mesa/main/buffers.c
>>> +++ b/src/mesa/main/buffers.c
>>> @@ -222,6 +222,12 @@ read_buffer_enum_to_index(GLenum buffer)
>>>       }
>>>    }
>>>
>>> +static bool
>>> +is_legal_es3_readbuffer_enum(GLenum buf)
>>> +{
>>> +   return buf == GL_BACK || buf == GL_NONE ||
>>> +          (buf >= GL_COLOR_ATTACHMENT0 && buf <= GL_COLOR_ATTACHMENT31);
>>> +}
>>>
>>
>> Technically, reading past what is reported by GL_MAX_COLOR_ATTACHMENTS
>> is not legal, so here you should probably use
>> ctx->Const.MaxColorAttachments.
>>
>>   From GLES 3.1, (in various sections):
>>
>>       "i in COLOR_ATTACHMENTi may range from zero to the value of
>>        MAX_COLOR_ATTACHMENTS minus one".
>
> You're right - it does say that.  However, handling that here would
> cause an INVALID_ENUM error to be produced.  The spec explicitly says
> that it should be INVALID_OPERATION:
>
> "An INVALID_OPERATION error is generated if the GL is bound to a draw
>   framebuffer object and src is BACK or COLOR_ATTACHMENTm where m is
>   greater than or equal to the value of MAX_COLOR_ATTACHMENTS."
>
> The dEQP test also starts failing if I make this change.  Without it,
> it falls through to the "supported?" case and raises INVALID_OPERATION
> correctly.
>
> Are you okay with leaving this part as is?
>

Ok, I see. I didn't initially realize that this situation was already 
handled later. Please, leave it as is.

Eduardo

>>>    /**
>>>     * Called by glDrawBuffer() and glNamedFramebufferDrawBuffer().
>>> @@ -716,6 +722,10 @@ read_buffer(struct gl_context *ctx, struct
> gl_framebuffer *fb,
>>>       else {
>>>          /* general case / window-system framebuffer */
>>>          srcBuffer = read_buffer_enum_to_index(buffer);
>>> +
>>> +      if (_mesa_is_gles3(ctx) && !is_legal_es3_readbuffer_enum(buffer))
>>> +         srcBuffer = -1;
>>> +
>>
>> I think you can move this block above the previous one that sets
>> srcBuffer, so that if buffer is not legal, you don't need to call
>> read_buffer_enum_to_index().
>
> Sure.  I've changed it to:
>
>       if (_mesa_is_gles3(ctx) && !is_legal_es3_readbuffer_enum(buffer))
>          srcBuffer = -1;
>       else
>          srcBuffer = read_buffer_enum_to_index(buffer);
>
> Thanks for the feedback!
>
>>>          if (srcBuffer == -1) {
>>>             _mesa_error(ctx, GL_INVALID_ENUM,
>>>                         "%s(invalid buffer %s)", caller,
>>>
>>
>> With those two things, patch is:
>>
>> Reviewed-by: Eduardo Lima Mitev <elima at igalia.com>
>>
>> Eduardo