From 91d3647325f7334f0fc960dd774af400379f3a8b Mon Sep 17 00:00:00 2001 From: Shahbaz Youssefi Date: Mon, 8 Nov 2021 16:27:20 -0500 Subject: [PATCH] Fix invalidation of GL_FRAMEBUFFER invalidating READ FBO Per spec, GL_FRAMEBUFFER means GL_DRAW_FRAMEBUFFER for glInvalidateFramebuffer. Bug: chromium:1267424 Change-Id: I8c9ab61ecdbd4ccee4262dc8559b2feb02b4837c Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/3266176 Reviewed-by: Jamie Madill Commit-Queue: Shahbaz Youssefi --- src/libANGLE/Context.cpp | 18 +++++++------ src/tests/gl_tests/FramebufferTest.cpp | 36 ++++++++++++++++++++++++++ src/tests/gl_tests/StateChangeTest.cpp | 21 +++++++++++++++ 3 files changed, 67 insertions(+), 8 deletions(-) diff --git a/src/libANGLE/Context.cpp b/src/libANGLE/Context.cpp index a7146ffd9..b6b763f03 100644 --- a/src/libANGLE/Context.cpp +++ b/src/libANGLE/Context.cpp @@ -4144,8 +4144,14 @@ ANGLE_INLINE angle::Result Context::prepareForDispatch() angle::Result Context::prepareForInvalidate(GLenum target) { - // Only sync the FBO that's being invalidated - ANGLE_TRY(mState.syncDirtyObject(this, target)); + // Only sync the FBO that's being invalidated. Per the GLES3 spec, GL_FRAMEBUFFER is equivalent + // to GL_DRAW_FRAMEBUFFER for the purposes of invalidation. + GLenum effectiveTarget = target; + if (effectiveTarget == GL_FRAMEBUFFER) + { + effectiveTarget = GL_DRAW_FRAMEBUFFER; + } + ANGLE_TRY(mState.syncDirtyObject(this, effectiveTarget)); return syncDirtyBits(mInvalidateDirtyBits, Command::Invalidate); } @@ -4790,13 +4796,9 @@ void Context::readBuffer(GLenum mode) void Context::discardFramebuffer(GLenum target, GLsizei numAttachments, const GLenum *attachments) { - Framebuffer *framebuffer = mState.getTargetFramebuffer(target); - ASSERT(framebuffer); - // The specification isn't clear what should be done when the framebuffer isn't complete. - // We leave it up to the framebuffer implementation to decide what to do. - ANGLE_CONTEXT_TRY(prepareForInvalidate(target)); - ANGLE_CONTEXT_TRY(framebuffer->discard(this, numAttachments, attachments)); + // We threat it the same way as GLES3 glInvalidateFramebuffer. + invalidateFramebuffer(target, numAttachments, attachments); } void Context::invalidateFramebuffer(GLenum target, diff --git a/src/tests/gl_tests/FramebufferTest.cpp b/src/tests/gl_tests/FramebufferTest.cpp index 0173c8a64..19b7c0aaf 100644 --- a/src/tests/gl_tests/FramebufferTest.cpp +++ b/src/tests/gl_tests/FramebufferTest.cpp @@ -744,6 +744,42 @@ TEST_P(FramebufferTest_ES3, AttachmentWith3DLayers) EXPECT_GL_NO_ERROR(); } +// Check that invalid layer is detected in framebuffer completeness check. +TEST_P(FramebufferTest_ES3, 3DAttachmentInvalidLayer) +{ + GLTexture tex; + glBindTexture(GL_TEXTURE_3D, tex); + glTexImage3D(GL_TEXTURE_3D, 0, GL_RGBA8, 4, 4, 2, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr); + + GLFramebuffer framebuffer; + glBindFramebuffer(GL_FRAMEBUFFER, framebuffer); + glFramebufferTextureLayer(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, tex, 0, 2); + EXPECT_GLENUM_EQ(GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT, + glCheckFramebufferStatus(GL_FRAMEBUFFER)); + + glFramebufferTextureLayer(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, tex, 0, 1); + EXPECT_GL_FRAMEBUFFER_COMPLETE(GL_FRAMEBUFFER); + EXPECT_GL_NO_ERROR(); +} + +// Check that invalid layer is detected in framebuffer completeness check. +TEST_P(FramebufferTest_ES3, 2DArrayInvalidLayer) +{ + GLTexture tex; + glBindTexture(GL_TEXTURE_2D_ARRAY, tex); + glTexImage3D(GL_TEXTURE_2D_ARRAY, 0, GL_RGBA8, 4, 4, 2, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr); + + GLFramebuffer framebuffer; + glBindFramebuffer(GL_FRAMEBUFFER, framebuffer); + glFramebufferTextureLayer(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, tex, 0, 2); + EXPECT_GLENUM_EQ(GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT, + glCheckFramebufferStatus(GL_FRAMEBUFFER)); + + glFramebufferTextureLayer(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, tex, 0, 1); + EXPECT_GL_FRAMEBUFFER_COMPLETE(GL_FRAMEBUFFER); + EXPECT_GL_NO_ERROR(); +} + // Test that clearing the stencil buffer when the framebuffer only has a color attachment does not // crash. TEST_P(FramebufferTest_ES3, ClearNonexistentStencil) diff --git a/src/tests/gl_tests/StateChangeTest.cpp b/src/tests/gl_tests/StateChangeTest.cpp index 5f6efe947..267077f37 100644 --- a/src/tests/gl_tests/StateChangeTest.cpp +++ b/src/tests/gl_tests/StateChangeTest.cpp @@ -7208,6 +7208,27 @@ TEST_P(SimpleStateChangeTestES3, ClearColorInvalidateDepthClearColor) glClear(GL_COLOR_BUFFER_BIT); ASSERT_GL_NO_ERROR(); } + +// Regression test for a bug where glInvalidateFramebuffer(GL_FRAMEBUFFER, ...) was invalidating +// both the draw and read framebuffers. +TEST_P(SimpleStateChangeTestES3, InvalidateFramebufferShouldntInvalidateReadFramebuffer) +{ + // Create an invalid read framebuffer. + GLFramebuffer fbo; + glBindFramebuffer(GL_READ_FRAMEBUFFER, fbo); + + GLTexture tex; + glBindTexture(GL_TEXTURE_2D_ARRAY, tex); + glTexStorage3D(GL_TEXTURE_2D_ARRAY, 1, GL_R8, 1, 1, 1); + glFramebufferTextureLayer(GL_READ_FRAMEBUFFER, GL_STENCIL_ATTACHMENT, tex, 0, 5); + + // Invalidate using GL_FRAMEBUFFER. If GL_READ_FRAMEBUFFER was used, validation would fail due + // to the framebuffer not being complete. A bug here was attempting to invalidate the read + // framebuffer given GL_FRAMEBUFFER anyway. + constexpr std::array kAttachments = {GL_DEPTH, GL_STENCIL}; + glInvalidateFramebuffer(GL_FRAMEBUFFER, 2, kAttachments.data()); + EXPECT_GL_NO_ERROR(); +} } // anonymous namespace ANGLE_INSTANTIATE_TEST_ES2(StateChangeTest);