Fix framebuffer attachment enum validation

In GLES 3.0, GL_COLOR_ATTACHMENTi enums run all the way up to i=31, and don't stop at i=15 which the validation was previously checking against. It's acceptable to use this new enum range also for EXT_draw_buffers, since an error will still be generated if an enum is outside the range of maximum supported attachments. Also, generate INVALID_ENUM when dEQP tests expect it to be generated for color attachment number that's outside the supported range. This is not in line with the published 3.0 spec, but that's just an oversight in the spec document. Also fix incorrect INVALID_VALUE error in the validation of renderbufferStorageMultisample to INVALID_OPERATION. BUG=angleproject:1101 TEST=dEQP-GLES3.functional.negative_api.buffer.* (all pass) Change-Id: Ib8cf92651d29ef8fe8da0ce4bfa456cbc4d48850 Reviewed-on: https://chromium-review.googlesource.com/332140Reviewed-by: Corentin Wallez <cwallez@chromium.org> Commit-Queue: Olli Etuaho <oetuaho@nvidia.com>

Fix framebuffer attachment enum validation
84c9f593 · Olli Etuaho · Commit Bot · c979aabf · 84c9f593 · 84c9f593
Commit 84c9f593 authored Mar 09, 2016 by Olli Etuaho Committed by Commit Bot Mar 14, 2016
Showing with 18 additions and 10 deletions

validationES.cpp src/libANGLE/validationES.cpp +14 -6

validationES3.cpp src/libANGLE/validationES3.cpp +4 -2

deqp_gles3_test_expectations.txt src/tests/deqp_support/deqp_gles3_test_expectations.txt +0 -2

No files found.
--- a/src/libANGLE/validationES.cpp
+++ b/src/libANGLE/validationES.cpp
@@ -2265,7 +2265,7 @@ bool ValidateDiscardFramebufferBase(Context *context, GLenum target, GLsizei num
    for (GLsizei i = 0; i < numAttachments; ++i)
    {
-        if (attachments[i] >= GL_COLOR_ATTACHMENT0 && attachments[i] <= GL_COLOR_ATTACHMENT15)
+        if (attachments[i] >= GL_COLOR_ATTACHMENT0 && attachments[i] <= GL_COLOR_ATTACHMENT31)
        {
            if (defaultFramebuffer)
            {
@@ -2569,13 +2569,21 @@ bool ValidateDrawBuffersBase(ValidationContext *context, GLsizei n, const GLenum
        const GLenum attachment = GL_COLOR_ATTACHMENT0_EXT + colorAttachment;
        if (bufs[colorAttachment] != GL_NONE && bufs[colorAttachment] != GL_BACK &&
-            (bufs[colorAttachment] < GL_COLOR_ATTACHMENT0_EXT ||
+            (bufs[colorAttachment] < GL_COLOR_ATTACHMENT0 ||
-             bufs[colorAttachment] >= maxColorAttachment))
+             bufs[colorAttachment] > GL_COLOR_ATTACHMENT31))
        {
            // Value in bufs is not NONE, BACK, or GL_COLOR_ATTACHMENTi
-            // In the 3.0 specs, the error should return GL_INVALID_OPERATION.
+            // The 3.0.4 spec says to generate GL_INVALID_OPERATION here, but this
-            // When we move to 3.1 specs, we should change the error to be GL_INVALID_ENUM
+            // was changed to GL_INVALID_ENUM in 3.1, which dEQP also expects.
-            context->recordError(Error(GL_INVALID_OPERATION, "Invalid buffer value"));
+            // 3.1 is still a bit ambiguous about the error, but future specs are
+            // expected to clarify that GL_INVALID_ENUM is the correct error.
+            context->recordError(Error(GL_INVALID_ENUM, "Invalid buffer value"));
+            return false;
+        }
+        else if (bufs[colorAttachment] >= maxColorAttachment)
+        {
+            context->recordError(
+                Error(GL_INVALID_OPERATION, "Buffer value is greater than MAX_DRAW_BUFFERS"));
            return false;
        }
        else if (bufs[colorAttachment] != GL_NONE && bufs[colorAttachment] != attachment &&

--- a/src/libANGLE/validationES3.cpp
+++ b/src/libANGLE/validationES3.cpp
@@ -1355,7 +1355,9 @@ bool ValidateES3RenderbufferStorageParameters(gl::Context *context, GLenum targe
    const TextureCaps &formatCaps = context->getTextureCaps().get(internalformat);
    if (static_cast<GLuint>(samples) > formatCaps.getMaxSamples())
    {
-        context->recordError(Error(GL_INVALID_VALUE));
+        context->recordError(
+            Error(GL_INVALID_OPERATION,
+                  "Samples must not be greater than maximum supported value for the format."));
        return false;
    }
@@ -1440,7 +1442,7 @@ bool ValidateReadBuffer(Context *context, GLenum src)
        return true;
    }
-    if (src != GL_BACK && (src < GL_COLOR_ATTACHMENT0 || src > GL_COLOR_ATTACHMENT15))
+    if (src != GL_BACK && (src < GL_COLOR_ATTACHMENT0 || src > GL_COLOR_ATTACHMENT31))
    {
        context->recordError(gl::Error(GL_INVALID_ENUM, "Unknown enum for 'src' in ReadBuffer"));
        return false;

--- a/src/tests/deqp_support/deqp_gles3_test_expectations.txt
+++ b/src/tests/deqp_support/deqp_gles3_test_expectations.txt
@@ -50,8 +50,6 @@
 // Windows and Linux failures
 1101 WIN LINUX : dEQP-GLES3.functional.lifetime.delete_active.transform_feedback = FAIL
-1101 WIN LINUX : dEQP-GLES3.functional.negative_api.buffer.draw_buffers = FAIL
-1101 WIN LINUX : dEQP-GLES3.functional.negative_api.buffer.renderbuffer_storage_multisample = FAIL
 1101 WIN LINUX : dEQP-GLES3.functional.negative_api.texture.compressedtexsubimage2d = FAIL
 1101 WIN LINUX : dEQP-GLES3.functional.negative_api.texture.compressedtexsubimage2d_invalid_size = FAIL
 1101 WIN LINUX : dEQP-GLES3.functional.negative_api.texture.compressedtexsubimage3d = FAIL