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

[Kenneth Graunke]

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?

> >   /**
> >    * 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160324/60d2c771/attachment-0001.sig>