WebGL compatibility: remove UB for draw buffers in the GL backend.

src/libANGLE/Framebuffer.cpp

@@ -176,7 +176,7 @@ bool CheckAttachmentSampleCompleteness(const Context *context,
 FramebufferState::FramebufferState()
     : mLabel(),
       mColorAttachments(1),
-      mDrawBufferStates(IMPLEMENTATION_MAX_DRAW_BUFFERS, GL_NONE),
+      mDrawBufferStates(1, GL_BACK),
       mReadBufferState(GL_BACK),
       mDefaultWidth(0),
       mDefaultHeight(0),
@@ -185,7 +185,6 @@ FramebufferState::FramebufferState()
       mWebGLDepthStencilConsistent(true)
 {
     ASSERT(mDrawBufferStates.size() > 0);
-    mDrawBufferStates[0] = GL_BACK;
     mEnabledDrawBuffers.set(0);
 }
 
@@ -202,6 +201,7 @@ FramebufferState::FramebufferState(const Caps &caps)
 {
     ASSERT(mDrawBufferStates.size() > 0);
     mDrawBufferStates[0] = GL_COLOR_ATTACHMENT0_EXT;
+    mEnabledDrawBuffers.set(0);
 }
 
 FramebufferState::~FramebufferState()
@@ -1243,6 +1243,8 @@ void Framebuffer::setAttachmentImpl(GLenum type,
                              &mDirtyColorAttachmentBindings[colorIndex], type, binding,
                              textureIndex, resource);
 
+            // TODO(jmadill): ASSERT instead of checking the attachment exists in
+            // formsRenderingFeedbackLoopWith
             bool enabled = (type != GL_NONE && getDrawBufferState(colorIndex) != GL_NONE);
             mState.mEnabledDrawBuffers.set(colorIndex, enabled);
         }

src/libANGLE/Framebuffer.h

@@ -71,6 +71,7 @@ class FramebufferState final : angle::NonCopyable
     const FramebufferAttachment *getDepthStencilAttachment() const;
 
     const std::vector<GLenum> &getDrawBufferStates() const { return mDrawBufferStates; }
+    DrawBufferMask getEnabledDrawBuffers() const { return mEnabledDrawBuffers; }
     GLenum getReadBufferState() const { return mReadBufferState; }
     const std::vector<FramebufferAttachment> &getColorAttachments() const
     {
@@ -99,7 +100,7 @@ class FramebufferState final : angle::NonCopyable
 
     std::vector<GLenum> mDrawBufferStates;
     GLenum mReadBufferState;
-    angle::BitSet<IMPLEMENTATION_MAX_DRAW_BUFFERS> mEnabledDrawBuffers;
+    DrawBufferMask mEnabledDrawBuffers;
 
     GLint mDefaultWidth;
     GLint mDefaultHeight;

src/libANGLE/Program.cpp

@@ -769,6 +769,7 @@ void Program::unlink()
     mState.mOutputVariables.clear();
     mState.mOutputLocations.clear();
     mState.mOutputVariableTypes.clear();
+    mState.mActiveOutputVariables.reset();
     mState.mComputeShaderLocalSize.fill(1);
     mState.mSamplerBindings.clear();
 
@@ -943,6 +944,9 @@ Error Program::loadBinary(const Context *context,
     {
         mState.mOutputVariableTypes.push_back(stream.readInt<GLenum>());
     }
+    static_assert(IMPLEMENTATION_MAX_DRAW_BUFFERS < 8 * sizeof(uint32_t),
+                  "All bits of DrawBufferMask can be contained in an uint32_t");
+    mState.mActiveOutputVariables = stream.readInt<uint32_t>();
 
     stream.readInt(&mState.mSamplerUniformRange.start);
     stream.readInt(&mState.mSamplerUniformRange.end);
@@ -1082,6 +1086,9 @@ Error Program::saveBinary(const Context *context,
     {
         stream.writeInt(outputVariableType);
     }
+    static_assert(IMPLEMENTATION_MAX_DRAW_BUFFERS < 8 * sizeof(uint32_t),
+                  "All bits of DrawBufferMask can be contained in an uint32_t");
+    stream.writeInt(static_cast<uint32_t>(mState.mActiveOutputVariables.to_ulong()));
 
     stream.writeInt(mState.mSamplerUniformRange.start);
     stream.writeInt(mState.mSamplerUniformRange.end);
@@ -2677,6 +2684,7 @@ void Program::linkOutputVariables()
     ASSERT(fragmentShader != nullptr);
 
     ASSERT(mState.mOutputVariableTypes.empty());
+    ASSERT(mState.mActiveOutputVariables.none());
 
     // Gather output variable types
     for (const auto &outputVariable : fragmentShader->getActiveOutputVariables())
@@ -2698,6 +2706,8 @@ void Program::linkOutputVariables()
             {
                 mState.mOutputVariableTypes.resize(location + 1, GL_NONE);
             }
+            ASSERT(location < mState.mActiveOutputVariables.size());
+            mState.mActiveOutputVariables.set(location);
             mState.mOutputVariableTypes[location] = VariableComponentType(outputVariable.type);
         }
     }

src/libANGLE/Program.h

@@ -228,6 +228,7 @@ class ProgramState final : angle::NonCopyable
     {
         return mActiveAttribLocationsMask;
     }
+    DrawBufferMask getActiveOutputVariables() const { return mActiveOutputVariables; }
     const std::map<int, VariableLocation> &getOutputLocations() const { return mOutputLocations; }
     const std::vector<LinkedUniform> &getUniforms() const { return mUniforms; }
     const std::vector<VariableLocation> &getUniformLocations() const { return mUniformLocations; }
@@ -278,6 +279,7 @@ class ProgramState final : angle::NonCopyable
     std::vector<sh::OutputVariable> mOutputVariables;
     // TODO(jmadill): use unordered/hash map when available
     std::map<int, VariableLocation> mOutputLocations;
+    DrawBufferMask mActiveOutputVariables;
 
     // Fragment output variable base types: FLOAT, INT, or UINT.  Ordered by location.
     std::vector<GLenum> mOutputVariableTypes;

src/libANGLE/angletypes.h

@@ -275,9 +275,12 @@ struct PixelPackState : PixelStoreStateBase
 // Used in Program and VertexArray.
 using AttributesMask = angle::BitSet<MAX_VERTEX_ATTRIBS>;
 
-// Use in Program
+// Used in Program
 using UniformBlockBindingMask = angle::BitSet<IMPLEMENTATION_MAX_COMBINED_SHADER_UNIFORM_BUFFERS>;
 
+// Used in Framebuffer
+using DrawBufferMask = angle::BitSet<IMPLEMENTATION_MAX_DRAW_BUFFERS>;
+
 // A map of GL objects indexed by object ID. The specific map implementation may change.
 // Client code should treat it as a std::map.
 template <class ResourceT>

src/libANGLE/renderer/gl/ContextGL.cpp

@@ -51,7 +51,8 @@ CompilerImpl *ContextGL::createCompiler()
 
 ShaderImpl *ContextGL::createShader(const gl::ShaderState &data)
 {
-    return new ShaderGL(data, getFunctions(), getWorkaroundsGL());
+    return new ShaderGL(data, getFunctions(), getWorkaroundsGL(),
+                        getExtensions().webglCompatibility);
 }
 
 ProgramImpl *ContextGL::createProgram(const gl::ProgramState &data)

src/libANGLE/renderer/gl/FramebufferGL.cpp

@@ -44,7 +44,8 @@ FramebufferGL::FramebufferGL(const FramebufferState &state,
       mWorkarounds(workarounds),
       mBlitter(blitter),
       mFramebufferID(0),
-      mIsDefault(isDefault)
+      mIsDefault(isDefault),
+      mAppliedEnabledDrawBuffers(1)
 {
     if (!mIsDefault)
     {
@@ -64,7 +65,8 @@ FramebufferGL::FramebufferGL(GLuint id,
       mWorkarounds(workarounds),
       mBlitter(blitter),
       mFramebufferID(id),
-      mIsDefault(true)
+      mIsDefault(true),
+      mAppliedEnabledDrawBuffers(1)
 {
 }
 
@@ -438,6 +440,7 @@ void FramebufferGL::syncState(const gl::Context *context, const Framebuffer::Dir
                 const auto &drawBuffers = mState.getDrawBufferStates();
                 mFunctions->drawBuffers(static_cast<GLsizei>(drawBuffers.size()),
                                         drawBuffers.data());
+                mAppliedEnabledDrawBuffers = mState.getEnabledDrawBuffers();
                 break;
             }
             case Framebuffer::DIRTY_BIT_READ_BUFFER:
@@ -485,6 +488,28 @@ bool FramebufferGL::isDefault() const
     return mIsDefault;
 }
 
+void FramebufferGL::maskOutInactiveOutputDrawBuffers(DrawBufferMask maxSet)
+{
+    auto targetAppliedDrawBuffers = mState.getEnabledDrawBuffers() & maxSet;
+    if (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;
+        }
+
+        mStateManager->bindFramebuffer(GL_FRAMEBUFFER, mFramebufferID);
+        mFunctions->drawBuffers(drawBufferCount, drawBuffers);
+    }
+}
+
 void FramebufferGL::syncClearState(const gl::Context *context, GLbitfield mask)
 {
     if (mFunctions->standard == STANDARD_GL_DESKTOP)

src/libANGLE/renderer/gl/FramebufferGL.h

@@ -88,6 +88,8 @@ class FramebufferGL : public FramebufferImpl
     GLuint getFramebufferID() const;
     bool isDefault() const;
 
+    void maskOutInactiveOutputDrawBuffers(gl::DrawBufferMask maxSet);
+
   private:
     void syncClearState(const gl::Context *context, GLbitfield mask);
     void syncClearBufferState(const gl::Context *context, GLenum buffer, GLint drawBuffer);
@@ -116,6 +118,8 @@ class FramebufferGL : public FramebufferImpl
 
     GLuint mFramebufferID;
     bool mIsDefault;
+
+    gl::DrawBufferMask mAppliedEnabledDrawBuffers;
 };
 }
 

src/libANGLE/renderer/gl/ShaderGL.cpp

@@ -21,8 +21,13 @@ namespace rx
 
 ShaderGL::ShaderGL(const gl::ShaderState &data,
                    const FunctionsGL *functions,
-                   const WorkaroundsGL &workarounds)
-    : ShaderImpl(data), mFunctions(functions), mWorkarounds(workarounds), mShaderID(0)
+                   const WorkaroundsGL &workarounds,
+                   bool isWebGL)
+    : ShaderImpl(data),
+      mFunctions(functions),
+      mWorkarounds(workarounds),
+      mShaderID(0),
+      mIsWebGL(isWebGL)
 {
     ASSERT(mFunctions);
 }
@@ -50,6 +55,11 @@ ShCompileOptions ShaderGL::prepareSourceAndReturnOptions(std::stringstream *sour
 
     ShCompileOptions options = SH_INIT_GL_POSITION;
 
+    if (mIsWebGL)
+    {
+        options |= SH_INIT_OUTPUT_VARIABLES;
+    }
+
     if (mWorkarounds.doWhileGLSLCausesGPUHang)
     {
         options |= SH_REWRITE_DO_WHILE_LOOPS;

src/libANGLE/renderer/gl/ShaderGL.h

@@ -21,7 +21,8 @@ class ShaderGL : public ShaderImpl
   public:
     ShaderGL(const gl::ShaderState &data,
              const FunctionsGL *functions,
-             const WorkaroundsGL &workarounds);
+             const WorkaroundsGL &workarounds,
+             bool isWebGL);
     ~ShaderGL() override;
 
     // ShaderImpl implementation
@@ -37,6 +38,7 @@ class ShaderGL : public ShaderImpl
     const WorkaroundsGL &mWorkarounds;
 
     GLuint mShaderID;
+    bool mIsWebGL;
 };
 
 }

src/libANGLE/renderer/gl/StateManagerGL.cpp

@@ -856,8 +856,8 @@ gl::Error StateManagerGL::setGenericDrawState(const gl::ContextState &data)
 
     const gl::State &state = data.getState();
 
-    const gl::Framebuffer *framebuffer = state.getDrawFramebuffer();
-    const FramebufferGL *framebufferGL = GetImplAs<FramebufferGL>(framebuffer);
+    gl::Framebuffer *framebuffer = state.getDrawFramebuffer();
+    FramebufferGL *framebufferGL = GetImplAs<FramebufferGL>(framebuffer);
     bindFramebuffer(GL_DRAW_FRAMEBUFFER, framebufferGL->getFramebufferID());
 
     // Set the current transform feedback state
@@ -878,6 +878,12 @@ gl::Error StateManagerGL::setGenericDrawState(const gl::ContextState &data)
         mPrevDrawTransformFeedback = nullptr;
     }
 
+    if (data.getExtensions().webglCompatibility)
+    {
+        auto activeOutputs = state.getProgram()->getState().getActiveOutputVariables();
+        framebufferGL->maskOutInactiveOutputDrawBuffers(activeOutputs);
+    }
+
     return gl::NoError();
 }
 

src/tests/gl_tests/WebGLCompatibilityTest.cpp

@@ -2561,6 +2561,223 @@ TEST_P(WebGL2CompatibilityTest, VertexShaderAttributeTypeMissmatch)
     EXPECT_GL_NO_ERROR();
 }
 
+// Tests the WebGL removal of undefined behavior when attachments aren't written to.
+TEST_P(WebGLCompatibilityTest, DrawBuffers)
+{
+    if (IsD3D9() || IsD3D11())
+    {
+        std::cout << "Test skipped on " << GetParam() << std::endl;
+        return;
+    }
+
+    // Make sure we can use at least 4 attachments for the tests.
+    bool useEXT = false;
+    if (getClientMajorVersion() < 3)
+    {
+        if (!extensionRequestable("GL_EXT_draw_buffers"))
+        {
+            std::cout << "Test skipped because draw buffers are not available" << std::endl;
+            return;
+        }
+
+        glRequestExtensionANGLE("GL_EXT_draw_buffers");
+        useEXT = true;
+        EXPECT_GL_NO_ERROR();
+    }
+
+    GLint maxDrawBuffers = 0;
+    glGetIntegerv(GL_MAX_DRAW_BUFFERS, &maxDrawBuffers);
+    if (maxDrawBuffers < 4)
+    {
+        std::cout << "Test skipped because MAX_DRAW_BUFFERS is too small." << std::endl;
+        return;
+    }
+
+    // Clears all the renderbuffers to red.
+    auto ClearEverythingToRed = [](GLRenderbuffer *renderbuffers) {
+        GLFramebuffer clearFBO;
+        glBindFramebuffer(GL_DRAW_FRAMEBUFFER, clearFBO);
+
+        glClearColor(1, 0, 0, 1);
+        for (int i = 0; i < 4; ++i)
+        {
+            glFramebufferRenderbuffer(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_RENDERBUFFER,
+                                      renderbuffers[i]);
+            glClear(GL_COLOR_BUFFER_BIT);
+        }
+        ASSERT_GL_NO_ERROR();
+    };
+
+    // Checks that the renderbuffers specified by mask have the correct color
+    auto CheckColors = [](GLRenderbuffer *renderbuffers, int mask, GLColor color) {
+        GLFramebuffer readFBO;
+        glBindFramebuffer(GL_READ_FRAMEBUFFER, readFBO);
+
+        for (int i = 0; i < 4; ++i)
+        {
+            if (mask & (1 << i))
+            {
+                glFramebufferRenderbuffer(GL_READ_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
+                                          GL_RENDERBUFFER, renderbuffers[i]);
+                EXPECT_PIXEL_COLOR_EQ(0, 0, color);
+            }
+        }
+        ASSERT_GL_NO_ERROR();
+    };
+
+    // Depending on whether we are using the extension or ES3, a different entrypoint must be called
+    auto DrawBuffers = [](bool useEXT, int numBuffers, GLenum *buffers) {
+        if (useEXT)
+        {
+            glDrawBuffersEXT(numBuffers, buffers);
+        }
+        else
+        {
+            glDrawBuffers(numBuffers, buffers);
+        }
+    };
+
+    // Initialized the test framebuffer
+    GLFramebuffer drawFBO;
+    glBindFramebuffer(GL_DRAW_FRAMEBUFFER, drawFBO);
+
+    GLRenderbuffer renderbuffers[4];
+    for (int i = 0; i < 4; ++i)
+    {
+        glBindRenderbuffer(GL_RENDERBUFFER, renderbuffers[i]);
+        glRenderbufferStorage(GL_RENDERBUFFER, GL_RGBA8, 1, 1);
+        glFramebufferRenderbuffer(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0 + i, GL_RENDERBUFFER,
+                                  renderbuffers[i]);
+    }
+
+    ASSERT_GL_NO_ERROR();
+
+    const char *vertESSL1 =
+        "attribute vec4 a_pos;\n"
+        "void main()\n"
+        "{\n"
+        "    gl_Position = a_pos;\n"
+        "}\n";
+    const char *vertESSL3 =
+        "#version 300 es\n"
+        "in vec4 a_pos;\n"
+        "void main()\n"
+        "{\n"
+        "    gl_Position = a_pos;\n"
+        "}\n";
+
+    GLenum allDrawBuffers[] = {
+        GL_COLOR_ATTACHMENT0, GL_COLOR_ATTACHMENT1, GL_COLOR_ATTACHMENT2, GL_COLOR_ATTACHMENT3,
+    };
+
+    GLenum halfDrawBuffers[] = {
+        GL_NONE, GL_NONE, GL_COLOR_ATTACHMENT2, GL_COLOR_ATTACHMENT3,
+    };
+
+    // Test that when using gl_FragColor, only the first attachment is written to.
+    const char *fragESSL1 =
+        "precision highp float;\n"
+        "void main()\n"
+        "{\n"
+        "    gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0);\n"
+        "}\n";
+    ANGLE_GL_PROGRAM(programESSL1, vertESSL1, fragESSL1);
+
+    {
+        ClearEverythingToRed(renderbuffers);
+
+        glBindFramebuffer(GL_DRAW_FRAMEBUFFER, drawFBO);
+        DrawBuffers(useEXT, 4, allDrawBuffers);
+        drawQuad(programESSL1, "a_pos", 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_DRAW_FRAMEBUFFER, drawFBO);
+        DrawBuffers(useEXT, 4, halfDrawBuffers);
+        drawQuad(programESSL1, "a_pos", 0.5, 1.0, true);
+        ASSERT_GL_NO_ERROR();
+
+        CheckColors(renderbuffers, 0b1111, GLColor::red);
+    }
+
+    // Test what happens when rendering to a subset of the outputs. There is a behavior difference
+    // between the extension and ES3. In the extension gl_FragData is implicitly declared as an
+    // array of size MAX_DRAW_BUFFERS, so the WebGL spec stipulates that elements not written to
+    // should default to 0. On the contrary, in ES3 outputs are specified one by one, so
+    // attachments not declared in the shader should not be written to.
+    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);
+        writeOddOutputsVert = vertESSL1;
+        writeOddOutputsFrag =
+            "#extension GL_EXT_draw_buffers : require\n"
+            "precision highp float;\n"
+            "void main()\n"
+            "{\n"
+            "    gl_FragData[1] = vec4(0.0, 1.0, 0.0, 1.0);\n"
+            "    gl_FragData[3] = vec4(0.0, 1.0, 0.0, 1.0);\n"
+            "}\n";
+    }
+    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;
+        writeOddOutputsVert = vertESSL3;
+        writeOddOutputsFrag =
+            "#version 300 es\n"
+            "precision highp float;\n"
+            "layout(location = 1) out vec4 output1;"
+            "layout(location = 3) out vec4 output2;"
+            "void main()\n"
+            "{\n"
+            "    output1 = vec4(0.0, 1.0, 0.0, 1.0);\n"
+            "    output2 = vec4(0.0, 1.0, 0.0, 1.0);\n"
+            "}\n";
+    }
+    ANGLE_GL_PROGRAM(writeOddOutputsProgram, writeOddOutputsVert, writeOddOutputsFrag);
+
+    // Test that attachments not written to get the "unwritten" color
+    {
+        ClearEverythingToRed(renderbuffers);
+
+        glBindFramebuffer(GL_DRAW_FRAMEBUFFER, drawFBO);
+        DrawBuffers(useEXT, 4, allDrawBuffers);
+        drawQuad(writeOddOutputsProgram, "a_pos", 0.5, 1.0, true);
+        ASSERT_GL_NO_ERROR();
+
+        CheckColors(renderbuffers, 0b1010, GLColor::green);
+        CheckColors(renderbuffers, 0b0101, unwrittenColor);
+    }
+
+    // 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.
+    {
+        ClearEverythingToRed(renderbuffers);
+
+        glBindFramebuffer(GL_DRAW_FRAMEBUFFER, drawFBO);
+        DrawBuffers(useEXT, 4, halfDrawBuffers);
+        drawQuad(writeOddOutputsProgram, "a_pos", 0.5, 1.0, true);
+        ASSERT_GL_NO_ERROR();
+
+        CheckColors(renderbuffers, 0b1000, GLColor::green);
+        CheckColors(renderbuffers, 0b0100, unwrittenColor);
+        CheckColors(renderbuffers, 0b0011, GLColor::red);
+    }
+}
+
 // Use this to select which configurations (e.g. which renderer, which GLES major version) these
 // tests should be run against.
 ANGLE_INSTANTIATE_TEST(WebGLCompatibilityTest,