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 <kenn...@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 <el...@igalia.com>
>
> Eduardo
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
