Reland "Metal: Skip disabled draw buffers"

This is a reland of commit 4b084310d7 Do not try to create a command encoder with no attachments since it may crash some Metal drivers. Do not reset pipeline descriptors. Original change's description: > Metal: Skip disabled draw buffers > > When creating render pass descriptors, do not > assign textures to disabled color attachments. > > When creating pipeline descriptors, reset > pixel formats of disabled color attachments. > > Exit early when MTLRenderCommandEncoder is not created. > > Added: > * DrawBuffersTest.None > * DrawBuffersTest.NoneWithDepth > * DrawBuffersTest.NoneWithStencil > * DrawBuffersTestES3.DrawWithDisabledIncompatibleAttachment > > Fixes: > * conformance2/rendering/fs-color-type-mismatch-color-buffer-type.html > > Bug: angleproject:6430 > Change-Id: I7f650c761f757985b027388c350c01340a83fd51 > Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4003180 > Reviewed-by: Kenneth Russell <kbr@chromium.org> > Reviewed-by: Geoff Lang <geofflang@chromium.org> > Commit-Queue: Alexey Knyazev <lexa.knyazev@gmail.com> Bug: angleproject:6430 Change-Id: I13977bd7ef32c4c85420706215b4f4d3a65629ca Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4030310 Commit-Queue: Alexey Knyazev <lexa.knyazev@gmail.com> Reviewed-by: Geoff Lang <geofflang@chromium.org>
2022-11-04 00:00:00 +00:00 · 2022-11-04 00:00:00 +00:00 · 81e9dc5640
--- a/src/libANGLE/renderer/metal/FrameBufferMtl.mm
+++ b/src/libANGLE/renderer/metal/FrameBufferMtl.mm
@ -1048,6 +1048,8 @@ angle::Result FramebufferMtl::getReadableViewForRenderTarget(
 angle::Result FramebufferMtl::prepareRenderPass(const gl::Context *context,
                                                mtl::RenderPassDesc *pDescOut)
 {
+    const gl::DrawBufferMask enabledDrawBuffers = getState().getEnabledDrawBuffers();
+
    mtl::RenderPassDesc &desc = *pDescOut;

    mRenderPassFirstColorAttachmentFormat = nullptr;
@ -1062,7 +1064,10 @@ angle::Result FramebufferMtl::prepareRenderPass(const gl::Context *context,
        mtl::RenderPassColorAttachmentDesc &colorAttachment = desc.colorAttachments[colorIndexGL];
        const RenderTargetMtl *colorRenderTarget            = mColorRenderTargets[colorIndexGL];

-        if (colorRenderTarget)
+        // GL allows data types of fragment shader color outputs to be incompatible with disabled
+        // color attachments. To prevent various Metal validation issues, assign textures only to
+        // enabled attachments.
+        if (colorRenderTarget && enabledDrawBuffers.test(colorIndexGL))
        {
            colorRenderTarget->toRenderPassAttachmentDesc(&colorAttachment);

--- a/src/libANGLE/renderer/metal/mtl_command_buffer.h
+++ b/src/libANGLE/renderer/metal/mtl_command_buffer.h
@ -513,7 +513,7 @@ class RenderCommandEncoder final : public CommandEncoder
    void initAttachmentWriteDependencyAndScissorRect(const RenderPassAttachmentDesc &attachment);
    void initWriteDependency(const TextureRef &texture);

-    void finalizeLoadStoreAction(MTLRenderPassAttachmentDescriptor *objCRenderPassAttachment);
+    bool finalizeLoadStoreAction(MTLRenderPassAttachmentDescriptor *objCRenderPassAttachment);

    void encodeMetalEncoder();
    void simulateDiscardFramebuffer();
--- a/src/libANGLE/renderer/metal/mtl_command_buffer.mm
+++ b/src/libANGLE/renderer/metal/mtl_command_buffer.mm
@ -1117,7 +1117,7 @@ void RenderCommandEncoder::reset()
    mCommands.clear();
 }

-void RenderCommandEncoder::finalizeLoadStoreAction(
+bool RenderCommandEncoder::finalizeLoadStoreAction(
    MTLRenderPassAttachmentDescriptor *objCRenderPassAttachment)
 {
    if (!objCRenderPassAttachment.texture)
@ -1125,7 +1125,7 @@ void RenderCommandEncoder::finalizeLoadStoreAction(
        objCRenderPassAttachment.loadAction     = MTLLoadActionDontCare;
        objCRenderPassAttachment.storeAction    = MTLStoreActionDontCare;
        objCRenderPassAttachment.resolveTexture = nil;
-        return;
+        return false;
    }

    if (objCRenderPassAttachment.resolveTexture)
@ -1152,6 +1152,8 @@ void RenderCommandEncoder::finalizeLoadStoreAction(
        // If storeAction hasn't been set for this attachment, we set to dontcare.
        objCRenderPassAttachment.storeAction = MTLStoreActionDontCare;
    }
+
+    return true;
 }

 void RenderCommandEncoder::endEncoding()
@ -1164,6 +1166,8 @@ void RenderCommandEncoder::endEncodingImpl(bool considerDiscardSimulation)
    if (!valid())
        return;

+    bool hasAttachment = false;
+
    // Last minute correcting the store options.
    MTLRenderPassDescriptor *objCRenderPassDesc = mCachedRenderPassDescObjC.get();
    for (uint32_t i = 0; i < mRenderPassDesc.numColorAttachments; ++i)
@ -1171,17 +1175,20 @@ void RenderCommandEncoder::endEncodingImpl(bool considerDiscardSimulation)
        // Update store action set between restart() and endEncoding()
        objCRenderPassDesc.colorAttachments[i].storeAction =
            mRenderPassDesc.colorAttachments[i].storeAction;
-        finalizeLoadStoreAction(objCRenderPassDesc.colorAttachments[i]);
+        if (finalizeLoadStoreAction(objCRenderPassDesc.colorAttachments[i]))
+            hasAttachment = true;
    }

    // Update store action set between restart() and endEncoding()
    objCRenderPassDesc.depthAttachment.storeAction = mRenderPassDesc.depthAttachment.storeAction;
-    finalizeLoadStoreAction(objCRenderPassDesc.depthAttachment);
+    if (finalizeLoadStoreAction(objCRenderPassDesc.depthAttachment))
+        hasAttachment = true;

    // Update store action set between restart() and endEncoding()
    objCRenderPassDesc.stencilAttachment.storeAction =
        mRenderPassDesc.stencilAttachment.storeAction;
-    finalizeLoadStoreAction(objCRenderPassDesc.stencilAttachment);
+    if (finalizeLoadStoreAction(objCRenderPassDesc.stencilAttachment))
+        hasAttachment = true;

    // Set visibility result buffer
    if (mOcclusionQueryPool.getNumRenderPassAllocatedQueries())
@ -1194,8 +1201,15 @@ void RenderCommandEncoder::endEncodingImpl(bool considerDiscardSimulation)
        objCRenderPassDesc.visibilityResultBuffer = nil;
    }

-    // Encode the actual encoder
-    encodeMetalEncoder();
+    // Encode the actual encoder. It will not be created when there are no attachments.
+    if (hasAttachment)
+    {
+        encodeMetalEncoder();
+    }
+    else
+    {
+        mCommands.clear();
+    }

    CommandEncoder::endEncoding();

--- a/src/tests/gl_tests/DrawBuffersTest.cpp
+++ b/src/tests/gl_tests/DrawBuffersTest.cpp
@ -572,6 +572,8 @@ TEST_P(DrawBuffersTest, FirstHalfNULL)
    bool flags[8]  = {false};
    GLenum bufs[8] = {GL_NONE};

+    ASSERT_GT(mMaxDrawBuffers, 0);
+    ASSERT_LE(mMaxDrawBuffers, 8);
    GLuint halfMaxDrawBuffers = static_cast<GLuint>(mMaxDrawBuffers) / 2;

    for (GLuint texIndex = 0; texIndex < halfMaxDrawBuffers; texIndex++)
@ -613,6 +615,179 @@ TEST_P(DrawBuffersTest, DefaultFramebufferDrawBufferQuery)
    EXPECT_EQ(GL_NONE, drawbuffer);
 }

+// Test that drawing with all color buffers disabled works.
+TEST_P(DrawBuffersTest, None)
+{
+    ANGLE_SKIP_TEST_IF(!setupTest());
+
+    bool flags[8]  = {false};
+    GLenum bufs[8] = {GL_NONE};
+    GLTexture textures[8];
+
+    ASSERT_GT(mMaxDrawBuffers, 0);
+    ASSERT_LE(mMaxDrawBuffers, 8);
+    for (GLint texIndex = 0; texIndex < mMaxDrawBuffers; ++texIndex)
+    {
+        glBindTexture(GL_TEXTURE_2D, textures[texIndex]);
+        glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, getWindowWidth(), getWindowHeight(), 0, GL_RGBA,
+                     GL_UNSIGNED_BYTE, nullptr);
+        glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0 + texIndex, GL_TEXTURE_2D,
+                               textures[texIndex], 0);
+        flags[texIndex] = true;
+        bufs[texIndex]  = GL_COLOR_ATTACHMENT0 + texIndex;
+    }
+
+    GLuint program;
+    setupMRTProgram(flags, &program);
+
+    setDrawBuffers(mMaxDrawBuffers, bufs);
+    glClearColor(0.5, 0.5, 0.5, 1.0);
+    glClear(GL_COLOR_BUFFER_BIT);
+
+    for (GLint texIndex = 0; texIndex < mMaxDrawBuffers; ++texIndex)
+    {
+        bufs[texIndex] = GL_NONE;
+    }
+
+    setDrawBuffers(mMaxDrawBuffers, bufs);
+    drawQuad(program, positionAttrib(), 0.5);
+
+    ASSERT_GL_NO_ERROR();
+
+    for (GLint texIndex = 0; texIndex < mMaxDrawBuffers; ++texIndex)
+    {
+        glFramebufferTexture2D(GL_READ_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D,
+                               textures[texIndex], 0);
+        EXPECT_PIXEL_NEAR(getWindowWidth() / 2, getWindowHeight() / 2, 127, 127, 127, 255, 1);
+    }
+
+    glDeleteProgram(program);
+}
+
+// Test that drawing with a color buffer disabled and a depth buffer enabled works.
+TEST_P(DrawBuffersTest, NoneWithDepth)
+{
+    ANGLE_SKIP_TEST_IF(!setupTest());
+
+    bool flags[8]  = {true, false, false, false, false, false, false, false};
+    GLenum bufs[8] = {
+        GL_COLOR_ATTACHMENT0, GL_NONE, GL_NONE, GL_NONE, GL_NONE, GL_NONE, GL_NONE, GL_NONE};
+
+    GLTexture texture;
+    glBindTexture(GL_TEXTURE_2D, texture);
+    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, getWindowWidth(), getWindowHeight(), 0, GL_RGBA,
+                 GL_UNSIGNED_BYTE, nullptr);
+    glFramebufferTexture2D(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture, 0);
+    glFramebufferTexture2D(GL_READ_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture, 0);
+
+    GLRenderbuffer rb;
+    glBindRenderbuffer(GL_RENDERBUFFER, rb);
+    glRenderbufferStorage(GL_RENDERBUFFER, GL_DEPTH_COMPONENT16, getWindowWidth(),
+                          getWindowHeight());
+    glFramebufferRenderbuffer(GL_DRAW_FRAMEBUFFER, GL_DEPTH_ATTACHMENT, GL_RENDERBUFFER, rb);
+
+    EXPECT_GL_NO_ERROR();
+    EXPECT_GL_FRAMEBUFFER_COMPLETE(GL_DRAW_FRAMEBUFFER);
+
+    GLuint program;
+    setupMRTProgram(flags, &program);
+
+    ASSERT_GT(mMaxDrawBuffers, 0);
+    ASSERT_LE(mMaxDrawBuffers, 8);
+    setDrawBuffers(mMaxDrawBuffers, bufs);
+
+    glClearColor(0.5, 0.5, 0.5, 1.0);
+    glClearDepthf(0.0);
+    glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
+    EXPECT_PIXEL_NEAR(getWindowWidth() / 2, getWindowHeight() / 2, 127, 127, 127, 255, 1);
+
+    // Color buffer must remain untouched, depth buffer must be set to 1.0
+    bufs[0] = GL_NONE;
+    setDrawBuffers(mMaxDrawBuffers, bufs);
+    glEnable(GL_DEPTH_TEST);
+    glDepthFunc(GL_ALWAYS);
+    drawQuad(program, positionAttrib(), 1.0);
+    EXPECT_GL_NO_ERROR();
+    EXPECT_PIXEL_NEAR(getWindowWidth() / 2, getWindowHeight() / 2, 127, 127, 127, 255, 1);
+
+    // Draw with the color buffer and depth test enabled.
+    // Depth test must fail and the color buffer must remain unchanged.
+    bufs[0] = GL_COLOR_ATTACHMENT0;
+    setDrawBuffers(mMaxDrawBuffers, bufs);
+    glDepthFunc(GL_LESS);
+    drawQuad(program, positionAttrib(), 1.0);
+    EXPECT_PIXEL_NEAR(getWindowWidth() / 2, getWindowHeight() / 2, 127, 127, 127, 255, 1);
+
+    // Draw with another Z value.
+    // Depth test must pass and the color buffer must be updated.
+    drawQuad(program, positionAttrib(), 0.0);
+    EXPECT_PIXEL_COLOR_EQ(getWindowWidth() / 2, getWindowHeight() / 2, GLColor::red);
+
+    glDeleteProgram(program);
+}
+
+// Test that drawing with a color buffer disabled and a stencil buffer enabled works.
+TEST_P(DrawBuffersTest, NoneWithStencil)
+{
+    ANGLE_SKIP_TEST_IF(!setupTest());
+
+    bool flags[8]  = {true, false, false, false, false, false, false, false};
+    GLenum bufs[8] = {
+        GL_COLOR_ATTACHMENT0, GL_NONE, GL_NONE, GL_NONE, GL_NONE, GL_NONE, GL_NONE, GL_NONE};
+
+    GLTexture texture;
+    glBindTexture(GL_TEXTURE_2D, texture);
+    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, getWindowWidth(), getWindowHeight(), 0, GL_RGBA,
+                 GL_UNSIGNED_BYTE, nullptr);
+    glFramebufferTexture2D(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture, 0);
+    glFramebufferTexture2D(GL_READ_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture, 0);
+
+    GLRenderbuffer rb;
+    glBindRenderbuffer(GL_RENDERBUFFER, rb);
+    glRenderbufferStorage(GL_RENDERBUFFER, GL_STENCIL_INDEX8, getWindowWidth(), getWindowHeight());
+    glFramebufferRenderbuffer(GL_DRAW_FRAMEBUFFER, GL_STENCIL_ATTACHMENT, GL_RENDERBUFFER, rb);
+
+    EXPECT_GL_NO_ERROR();
+    EXPECT_GL_FRAMEBUFFER_COMPLETE(GL_DRAW_FRAMEBUFFER);
+
+    GLuint program;
+    setupMRTProgram(flags, &program);
+
+    ASSERT_GT(mMaxDrawBuffers, 0);
+    ASSERT_LE(mMaxDrawBuffers, 8);
+    setDrawBuffers(mMaxDrawBuffers, bufs);
+
+    glClearColor(0.5, 0.5, 0.5, 1.0);
+    glClear(GL_COLOR_BUFFER_BIT | GL_STENCIL_BUFFER_BIT);
+    EXPECT_PIXEL_NEAR(getWindowWidth() / 2, getWindowHeight() / 2, 127, 127, 127, 255, 1);
+
+    // Color buffer must remain untouched, stencil test must pass and stencil buffer must be
+    // incremented to 1.
+    bufs[0] = GL_NONE;
+    setDrawBuffers(mMaxDrawBuffers, bufs);
+    glEnable(GL_STENCIL_TEST);
+    glStencilOp(GL_KEEP, GL_INCR, GL_INCR);
+    drawQuad(program, positionAttrib(), 1.0);
+    EXPECT_GL_NO_ERROR();
+    EXPECT_PIXEL_NEAR(getWindowWidth() / 2, getWindowHeight() / 2, 127, 127, 127, 255, 1);
+
+    // Draw with the color buffer enabled and stencil test expecting 0.
+    // Stencil test must fail, and both the color and the stencil buffers must remain unchanged.
+    bufs[0] = GL_COLOR_ATTACHMENT0;
+    setDrawBuffers(mMaxDrawBuffers, bufs);
+    glStencilFunc(GL_EQUAL, 0, 255);
+    drawQuad(program, positionAttrib(), 1.0);
+    EXPECT_PIXEL_NEAR(getWindowWidth() / 2, getWindowHeight() / 2, 127, 127, 127, 255, 1);
+
+    // Draw with stencil ref value matching the stored stencil buffer value.
+    // Stencil test must pass and the color buffer must be updated.
+    glStencilFunc(GL_EQUAL, 1, 255);
+    drawQuad(program, positionAttrib(), 1.0);
+    EXPECT_PIXEL_COLOR_EQ(getWindowWidth() / 2, getWindowHeight() / 2, GLColor::red);
+
+    glDeleteProgram(program);
+}
+
 // Test that draws to every buffer and verifies that every buffer was drawn to.
 TEST_P(DrawBuffersTest, AllRGBA8)
 {
@ -622,6 +797,8 @@ TEST_P(DrawBuffersTest, AllRGBA8)
    GLenum bufs[8] = {GL_NONE};
    GLTexture textures[8];

+    ASSERT_GT(mMaxDrawBuffers, 0);
+    ASSERT_LE(mMaxDrawBuffers, 8);
    for (GLint texIndex = 0; texIndex < mMaxDrawBuffers; ++texIndex)
    {
        glBindTexture(GL_TEXTURE_2D, textures[texIndex]);
@ -1266,6 +1443,45 @@ void main()
    EXPECT_PIXEL_NEAR(0, 0, 143, 175, 159, 143, 1);
 }

+// Test that a disabled color attachment incompatible with a fragment output
+// is correctly ignored and does not affect other attachments.
+TEST_P(DrawBuffersTestES3, DrawWithDisabledIncompatibleAttachment)
+{
+    ANGLE_SKIP_TEST_IF(!setupTest());
+
+    ASSERT_GE(mMaxDrawBuffers, 4);
+    for (GLuint texIndex = 0; texIndex < 4; texIndex++)
+    {
+        glBindTexture(GL_TEXTURE_2D, mTextures[texIndex]);
+        if (texIndex == 1)
+        {
+            glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8UI, getWindowWidth(), getWindowHeight(), 0,
+                         GL_RGBA_INTEGER, GL_UNSIGNED_BYTE, nullptr);
+        }
+        glFramebufferTexture2D(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0 + texIndex, GL_TEXTURE_2D,
+                               mTextures[texIndex], 0);
+    }
+    ASSERT_GL_NO_ERROR();
+    EXPECT_GL_FRAMEBUFFER_COMPLETE(GL_DRAW_FRAMEBUFFER);
+
+    const GLenum bufs[] = {GL_COLOR_ATTACHMENT0, GL_NONE, GL_COLOR_ATTACHMENT2,
+                           GL_COLOR_ATTACHMENT3};
+    setDrawBuffers(4, bufs);
+
+    bool flags[8] = {true, true, true, true};
+    GLuint program;
+    setupMRTProgram(flags, &program);
+
+    drawQuad(program, positionAttrib(), 0.5);
+    EXPECT_GL_NO_ERROR();
+
+    verifyAttachment2D(0, mTextures[0], GL_TEXTURE_2D, 0);
+    verifyAttachment2D(2, mTextures[2], GL_TEXTURE_2D, 0);
+    verifyAttachment2D(3, mTextures[3], GL_TEXTURE_2D, 0);
+
+    glDeleteProgram(program);
+}
+
 // Vulkan backend is setting per buffer color mask to false for draw buffers that set to GL_NONE.
 // These set of tests are to test draw buffer change followed by draw/clear/blit and followed by
 // draw buffer change are behaving correctly.