Fix error report when active color buffer has no fs output

Also modify or remove some tests to sync up with the expected behavior
stated in spec.

Related to KhronosGroup/WebGL#2780
 If any draw buffer with an attachment does not have a defined fragment shader output,
 draws generate INVALID_OPERATION.

Also remove Framebuffer masking for inactive outputs.

This workaround is no longer necessary as the WebGL spec has changed.
It also was never fully working and had bugs with certain orders of
calls.

Bug: angleproject:2872
Bug: chromium:927908
Bug: chromium:943538
Change-Id: I73715a6ab851ae3db7096f49ea0a9fdd6f576703
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/1530018
Commit-Queue: Shrek Shao <shrekshao@google.com>
Reviewed-by: Jonah Ryan-Davis <jonahr@google.com>

src/libANGLE/ErrorStrings.h

@@ -84,6 +84,7 @@ MSG kDestinationLevelNotDefined = "The destination level of the destination text
 MSG kDestinationTextureTooSmall = "Destination texture too small.";
 MSG kDimensionsMustBePow2 = "Texture dimensions must be power-of-two.";
 MSG kDispatchIndirectBufferNotBound = "Dispatch indirect buffer must be bound.";
+MSG kDrawBufferMaskMismatch = "Active draw buffers with missing fragment shader outputs.";
 MSG kDrawBufferTypeMismatch = "Fragment shader output type does not match the bound framebuffer attachment type.";
 MSG kDrawFramebufferIncomplete = "Draw framebuffer is incomplete";
 MSG kDrawIndirectBufferNotBound = "Draw indirect buffer must be bound.";

src/libANGLE/renderer/gl/ContextGL.cpp

@@ -200,13 +200,6 @@ ANGLE_INLINE angle::Result ContextGL::setDrawArraysState(const gl::Context *cont
                                             count, instanceCount));
     }
 
-    if (context->getExtensions().webglCompatibility)
-    {
-        const gl::State &glState     = context->getState();
-        FramebufferGL *framebufferGL = GetImplAs<FramebufferGL>(glState.getDrawFramebuffer());
-        framebufferGL->maskOutInactiveOutputDrawBuffers(context);
-    }
-
     return angle::Result::Continue;
 }
 
@@ -236,24 +229,6 @@ ANGLE_INLINE angle::Result ContextGL::setDrawElementsState(const gl::Context *co
         *outIndices = indices;
     }
 
-    if (context->getExtensions().webglCompatibility)
-    {
-        FramebufferGL *framebufferGL = GetImplAs<FramebufferGL>(glState.getDrawFramebuffer());
-        framebufferGL->maskOutInactiveOutputDrawBuffers(context);
-    }
-
-    return angle::Result::Continue;
-}
-
-ANGLE_INLINE angle::Result ContextGL::setDrawIndirectState(const gl::Context *context)
-{
-    if (context->getExtensions().webglCompatibility)
-    {
-        const gl::State &glState     = context->getState();
-        FramebufferGL *framebufferGL = GetImplAs<FramebufferGL>(glState.getDrawFramebuffer());
-        framebufferGL->maskOutInactiveOutputDrawBuffers(context);
-    }
-
     return angle::Result::Continue;
 }
 
@@ -375,7 +350,6 @@ angle::Result ContextGL::drawArraysIndirect(const gl::Context *context,
                                             gl::PrimitiveMode mode,
                                             const void *indirect)
 {
-    ANGLE_TRY(setDrawIndirectState(context));
     getFunctions()->drawArraysIndirect(ToGLenum(mode), indirect);
     return angle::Result::Continue;
 }
@@ -385,7 +359,6 @@ angle::Result ContextGL::drawElementsIndirect(const gl::Context *context,
                                               gl::DrawElementsType type,
                                               const void *indirect)
 {
-    ANGLE_TRY(setDrawIndirectState(context));
     getFunctions()->drawElementsIndirect(ToGLenum(mode), ToGLenum(type), indirect);
     return angle::Result::Continue;
 }

src/libANGLE/renderer/gl/ContextGL.h

@@ -231,8 +231,6 @@ class ContextGL : public ContextImpl
                                        GLsizei instanceCount,
                                        const void **outIndices);
 
-    angle::Result setDrawIndirectState(const gl::Context *context);
-
   protected:
     std::shared_ptr<RendererGL> mRenderer;
 };

src/libANGLE/renderer/gl/FramebufferGL.cpp

@@ -726,30 +726,6 @@ bool FramebufferGL::isDefault() const
     return mIsDefault;
 }
 
-void FramebufferGL::maskOutInactiveOutputDrawBuffersImpl(const gl::Context *context,
-                                                         DrawBufferMask targetAppliedDrawBuffers)
-{
-
-    ASSERT(mAppliedEnabledDrawBuffers != targetAppliedDrawBuffers);
-    mAppliedEnabledDrawBuffers = targetAppliedDrawBuffers;
-
-    const auto &stateDrawBuffers = mState.getDrawBufferStates();
-    GLsizei drawBufferCount      = static_cast<GLsizei>(stateDrawBuffers.size());
-    ASSERT(drawBufferCount <= IMPLEMENTATION_MAX_DRAW_BUFFERS);
-
-    GLenum drawBuffers[IMPLEMENTATION_MAX_DRAW_BUFFERS];
-    for (GLenum i = 0; static_cast<int>(i) < drawBufferCount; ++i)
-    {
-        drawBuffers[i] = targetAppliedDrawBuffers[i] ? stateDrawBuffers[i] : GL_NONE;
-    }
-
-    const FunctionsGL *functions = GetFunctionsGL(context);
-    StateManagerGL *stateManager = GetStateManagerGL(context);
-
-    ASSERT(stateManager->getFramebufferID(angle::FramebufferBindingDraw) == mFramebufferID);
-    functions->drawBuffers(drawBufferCount, drawBuffers);
-}
-
 void FramebufferGL::syncClearState(const gl::Context *context, GLbitfield mask)
 {
     const FunctionsGL *functions = GetFunctionsGL(context);

src/libANGLE/renderer/gl/FramebufferGL.h

@@ -86,20 +86,6 @@ class FramebufferGL : public FramebufferImpl
     GLuint getFramebufferID() const;
     bool isDefault() const;
 
-    ANGLE_INLINE void maskOutInactiveOutputDrawBuffers(const gl::Context *context)
-    {
-        ASSERT(context->getExtensions().webglCompatibility);
-
-        const gl::DrawBufferMask &maxSet =
-            context->getState().getProgram()->getActiveOutputVariables();
-
-        gl::DrawBufferMask targetAppliedDrawBuffers = mState.getEnabledDrawBuffers() & maxSet;
-        if (mAppliedEnabledDrawBuffers != targetAppliedDrawBuffers)
-        {
-            maskOutInactiveOutputDrawBuffersImpl(context, targetAppliedDrawBuffers);
-        }
-    }
-
   private:
     void syncClearState(const gl::Context *context, GLbitfield mask);
     void syncClearBufferState(const gl::Context *context, GLenum buffer, GLint drawBuffer);

src/libANGLE/validationES.cpp

@@ -362,6 +362,17 @@ bool ValidateTextureMaxAnisotropyValue(Context *context, GLfloat paramValue)
     return true;
 }
 
+bool ValidateFragmentShaderColorBufferMaskMatch(Context *context)
+{
+    const Program *program         = context->getState().getLinkedProgram(context);
+    const Framebuffer *framebuffer = context->getState().getDrawFramebuffer();
+
+    auto drawBufferMask     = framebuffer->getDrawBufferMask().to_ulong();
+    auto fragmentOutputMask = program->getActiveOutputVariables().to_ulong();
+
+    return drawBufferMask == (drawBufferMask & fragmentOutputMask);
+}
+
 bool ValidateFragmentShaderColorBufferTypeMatch(Context *context)
 {
     const Program *program         = context->getState().getLinkedProgram(context);
@@ -2767,6 +2778,12 @@ const char *ValidateDrawStates(Context *context)
                 return kVertexShaderTypeMismatch;
             }
 
+            // Detect that if there's active color buffer without fragment shader output
+            if (!ValidateFragmentShaderColorBufferMaskMatch(context))
+            {
+                return kDrawBufferMaskMismatch;
+            }
+
             // Detect that the color buffer types match the fragment shader output types
             if (!ValidateFragmentShaderColorBufferTypeMatch(context))
             {

src/tests/gl_tests/DrawBuffersTest.cpp

@@ -357,53 +357,6 @@ TEST_P(DrawBuffersTest, DefaultFramebufferDrawBufferQuery)
     EXPECT_EQ(GL_NONE, drawbuffer);
 }
 
-// Tests masking out some of the draw buffers by not writing to them in the program.
-TEST_P(DrawBuffersWebGL2Test, SomeProgramOutputsDisabled)
-{
-    ANGLE_SKIP_TEST_IF(!setupTest());
-
-    // TODO(ynovikov): Investigate the failure (https://anglebug.com/1533)
-    ANGLE_SKIP_TEST_IF(IsWindows() && IsAMD() && IsDesktopOpenGL());
-
-    bool flags[8]  = {false};
-    GLenum bufs[4] = {GL_NONE};
-
-    constexpr GLuint kMaxBuffers     = 4;
-    constexpr GLuint kHalfMaxBuffers = 2;
-
-    // Enable all draw buffers.
-    for (GLuint texIndex = 0; texIndex < kMaxBuffers; texIndex++)
-    {
-        glBindTexture(GL_TEXTURE_2D, mTextures[texIndex]);
-        glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0 + texIndex, GL_TEXTURE_2D,
-                               mTextures[texIndex], 0);
-        bufs[texIndex] = GL_COLOR_ATTACHMENT0 + texIndex;
-
-        // Mask out the first two buffers.
-        flags[texIndex] = texIndex >= kHalfMaxBuffers;
-    }
-
-    GLuint program;
-    setupMRTProgram(flags, &program);
-
-    setDrawBuffers(kMaxBuffers, bufs);
-    drawQuad(program, positionAttrib(), 0.5, 1.0f, true);
-
-    for (GLuint texIndex = 0; texIndex < kHalfMaxBuffers; texIndex++)
-    {
-        verifyAttachment2DUnwritten(texIndex, mTextures[texIndex], GL_TEXTURE_2D, 0);
-    }
-
-    for (GLuint texIndex = kHalfMaxBuffers; texIndex < kMaxBuffers; texIndex++)
-    {
-        verifyAttachment2D(texIndex, mTextures[texIndex], GL_TEXTURE_2D, 0);
-    }
-
-    EXPECT_GL_NO_ERROR();
-
-    glDeleteProgram(program);
-}
-
 // Same as above but adds a state change from a program with different masks after a clear.
 TEST_P(DrawBuffersWebGL2Test, TwoProgramsWithDifferentOutputsAndClear)
 {
@@ -463,7 +416,7 @@ TEST_P(DrawBuffersWebGL2Test, TwoProgramsWithDifferentOutputsAndClear)
     verifyAttachment2DColor(3, mTextures[3], GL_TEXTURE_2D, 0, GLColor::green);
 
     // Draw with MRT program.
-    setDrawBuffers(kMaxBuffers, allBufs);
+    setDrawBuffers(kMaxBuffers, someBufs);
     drawQuad(program, positionAttrib(), 0.5, 1.0f, true);
     ASSERT_GL_NO_ERROR();
 
@@ -473,6 +426,11 @@ TEST_P(DrawBuffersWebGL2Test, TwoProgramsWithDifferentOutputsAndClear)
     verifyAttachment2D(2, mTextures[2], GL_TEXTURE_2D, 0);
     verifyAttachment2D(3, mTextures[3], GL_TEXTURE_2D, 0);
 
+    // Active draw buffers with no fragment output is not allowed.
+    setDrawBuffers(kMaxBuffers, allBufs);
+    drawQuad(program, positionAttrib(), 0.5, 1.0f, true);
+    ASSERT_GL_ERROR(GL_INVALID_OPERATION);
+
     // Clear again. All attachments should be cleared.
     glClear(GL_COLOR_BUFFER_BIT);
     verifyAttachment2DColor(0, mTextures[0], GL_TEXTURE_2D, 0, GLColor::green);

src/tests/gl_tests/MultiviewDrawTest.cpp

@@ -462,8 +462,9 @@ TEST_P(MultiviewDrawValidationTest, IndirectDraw)
         "#version 300 es\n"
         "#extension GL_OVR_multiview2 : require\n"
         "precision mediump float;\n"
+        "out vec4 color;\n"
         "void main()\n"
-        "{}\n";
+        "{color = vec4(1);}\n";
 
     GLVertexArray vao;
     GLBuffer vertexBuffer;
@@ -544,8 +545,9 @@ TEST_P(MultiviewDrawValidationTest, NumViewsMismatch)
         "#version 300 es\n"
         "#extension GL_OVR_multiview2 : require\n"
         "precision mediump float;\n"
+        "out vec4 color;\n"
         "void main()\n"
-        "{}\n";
+        "{color = vec4(1);}\n";
     ANGLE_GL_PROGRAM(program, kVS, kFS);
     glUseProgram(program);
 
@@ -684,6 +686,9 @@ void main()
     GLTexture tex2DArray;
     initOnePixelColorTexture2DMultiLayered(tex2DArray);
 
+    GLenum bufs[] = {GL_NONE};
+    glDrawBuffers(1, bufs);
+
     // Check that drawArrays generates an error when there is an active transform feedback object
     // and the number of views in the draw framebuffer is greater than 1.
     {
@@ -777,6 +782,9 @@ TEST_P(MultiviewDrawValidationTest, ActiveTimeElapsedQuery)
     GLTexture tex2DArr;
     initOnePixelColorTexture2DMultiLayered(tex2DArr);
 
+    GLenum bufs[] = {GL_NONE};
+    glDrawBuffers(1, bufs);
+
     // Check first case.
     {
         glUseProgram(dualViewProgram);

src/tests/gl_tests/WebGLCompatibilityTest.cpp

@@ -4065,7 +4065,7 @@ TEST_P(WebGLCompatibilityTest, FramebufferAttachmentQuery)
     EXPECT_GL_ERROR(GL_INVALID_ENUM);
 }
 
-// Tests the WebGL removal of undefined behavior when attachments aren't written to.
+// Tests WebGL reports INVALID_OPERATION for mismatch of drawbuffers and fragment output
 TEST_P(WebGLCompatibilityTest, DrawBuffers)
 {
     // Make sure we can use at least 4 attachments for the tests.
@@ -4152,12 +4152,12 @@ TEST_P(WebGLCompatibilityTest, DrawBuffers)
 
     GLenum halfDrawBuffers[] = {
         GL_NONE,
+        GL_COLOR_ATTACHMENT1,
         GL_NONE,
-        GL_COLOR_ATTACHMENT2,
         GL_COLOR_ATTACHMENT3,
     };
 
-    // Test that when using gl_FragColor, only the first attachment is written to.
+    // Test that when using gl_FragColor with no-array
     const char *fragESSL1 =
         R"(precision highp float;
 void main()
@@ -4167,28 +4167,10 @@ void main()
     ANGLE_GL_PROGRAM(programESSL1, essl1_shaders::vs::Simple(), fragESSL1);
 
     {
-        ClearEverythingToRed(renderbuffers);
-
         glBindFramebuffer(GL_FRAMEBUFFER, drawFBO);
         DrawBuffers(useEXT, 4, allDrawBuffers);
         drawQuad(programESSL1, essl1_shaders::PositionAttrib(), 0.5, 1.0, true);
-        ASSERT_GL_NO_ERROR();
-
-        CheckColors(renderbuffers, 0b0001, GLColor::green);
-        CheckColors(renderbuffers, 0b1110, GLColor::red);
-    }
-
-    // Test that when using gl_FragColor, but the first draw buffer is 0, then no attachment is
-    // written to.
-    {
-        ClearEverythingToRed(renderbuffers);
-
-        glBindFramebuffer(GL_FRAMEBUFFER, drawFBO);
-        DrawBuffers(useEXT, 4, halfDrawBuffers);
-        drawQuad(programESSL1, essl1_shaders::PositionAttrib(), 0.5, 1.0, true);
-        ASSERT_GL_NO_ERROR();
-
-        CheckColors(renderbuffers, 0b1111, GLColor::red);
+        EXPECT_GL_ERROR(GL_INVALID_OPERATION);
     }
 
     // Test what happens when rendering to a subset of the outputs. There is a behavior difference
@@ -4199,11 +4181,8 @@ void main()
     const char *positionAttrib;
     const char *writeOddOutputsVert;
     const char *writeOddOutputsFrag;
-    GLColor unwrittenColor;
     if (useEXT)
     {
-        // In the extension, when an attachment isn't written to, it should get 0's
-        unwrittenColor      = GLColor(0, 0, 0, 0);
         positionAttrib      = essl1_shaders::PositionAttrib();
         writeOddOutputsVert = essl1_shaders::vs::Simple();
         writeOddOutputsFrag =
@@ -4217,9 +4196,6 @@ void main()
     }
     else
     {
-        // In ES3 if an attachment isn't declared, it shouldn't get written and should be red
-        // because of the preceding clears.
-        unwrittenColor      = GLColor::red;
         positionAttrib      = essl3_shaders::PositionAttrib();
         writeOddOutputsVert = essl3_shaders::vs::Simple();
         writeOddOutputsFrag =
@@ -4235,21 +4211,30 @@ void main()
     }
     ANGLE_GL_PROGRAM(writeOddOutputsProgram, writeOddOutputsVert, writeOddOutputsFrag);
 
-    // Test that attachments not written to get the "unwritten" color
+    // Test that attachments not written to get the "unwritten" color (useEXT)
+    // Or INVALID_OPERATION is generated if there's active draw buffer receive no output
     {
         ClearEverythingToRed(renderbuffers);
 
         glBindFramebuffer(GL_FRAMEBUFFER, drawFBO);
         DrawBuffers(useEXT, 4, allDrawBuffers);
         drawQuad(writeOddOutputsProgram, positionAttrib, 0.5, 1.0, true);
-        ASSERT_GL_NO_ERROR();
 
-        CheckColors(renderbuffers, 0b1010, GLColor::green);
-        CheckColors(renderbuffers, 0b0101, unwrittenColor);
+        if (useEXT)
+        {
+            ASSERT_GL_NO_ERROR();
+            CheckColors(renderbuffers, 0b1010, GLColor::green);
+            // In the extension, when an attachment isn't written to, it should get 0's
+            CheckColors(renderbuffers, 0b0101, GLColor(0, 0, 0, 0));
+        }
+        else
+        {
+            EXPECT_GL_ERROR(GL_INVALID_OPERATION);
+        }
     }
 
-    // Test that attachments not written to get the "unwritten" color but that even when the
-    // extension is used, disabled attachments are not written at all and stay red.
+    // Test that attachments written to get the correct color from shader output but that even when
+    // the extension is used, disabled attachments are not written at all and stay red.
     {
         ClearEverythingToRed(renderbuffers);
 
@@ -4258,9 +4243,8 @@ void main()
         drawQuad(writeOddOutputsProgram, positionAttrib, 0.5, 1.0, true);
         ASSERT_GL_NO_ERROR();
 
-        CheckColors(renderbuffers, 0b1000, GLColor::green);
-        CheckColors(renderbuffers, 0b0100, unwrittenColor);
-        CheckColors(renderbuffers, 0b0011, GLColor::red);
+        CheckColors(renderbuffers, 0b1010, GLColor::green);
+        CheckColors(renderbuffers, 0b0101, GLColor::red);
     }
 }