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 <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?
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 <el...@igalia.com>
Eduardo
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
