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

[Eduardo Lima Mitev]

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".

>   /**
>    * 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().

>         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