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 <[email protected]> > > --- > > 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 <[email protected]>
>
> Eduardo
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
