From 7c811715508175171303b305bc6891f8be9b0567 Mon Sep 17 00:00:00 2001 From: Shahbaz Youssefi Date: Wed, 25 Sep 2024 11:09:44 -0400 Subject: [PATCH] Vulkan: fix crash when clearing stencil with ClearBuffer Follow up to [1] which fixed a crash with glClear, but the bug remained with glClearBufferiv. This change refactors the "is stencil write masked out" query to always take the framebuffer's stencil bit count into account (practically always 8), which also happens to make the rest of the code checking this query more accurate in the presence of nonsense masks where the bottom 8 bits are 0. [1]: https://chromium-review.googlesource.com/c/angle/angle/+/3315158 Bug: chromium:40207259 Bug: angleproject:42266334 Change-Id: I68a6b0b75c67ed2cdc8c4d03b243efe5495efce1 Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/5889788 Reviewed-by: Geoff Lang Auto-Submit: Shahbaz Youssefi Commit-Queue: Geoff Lang --- src/libANGLE/Context.cpp | 17 +++---- src/libANGLE/Context.h | 4 +- src/libANGLE/Framebuffer.cpp | 44 ++++++------------- src/libANGLE/Framebuffer.h | 12 ++--- src/libANGLE/State.h | 10 +++-- src/libANGLE/angletypes.cpp | 19 +++++--- src/libANGLE/angletypes.h | 6 +-- src/libANGLE/renderer/vulkan/ContextVk.cpp | 18 +++++--- .../renderer/vulkan/FramebufferVk.cpp | 2 +- .../HardwareBufferImageSiblingVkAndroid.cpp | 2 +- src/libANGLE/renderer/vulkan/vk_helpers.cpp | 4 +- src/libANGLE/renderer/vulkan/vk_helpers.h | 1 + src/tests/gl_tests/ClearTest.cpp | 8 ++++ 13 files changed, 80 insertions(+), 67 deletions(-) diff --git a/src/libANGLE/Context.cpp b/src/libANGLE/Context.cpp index 88db6ba8fec..c147c5f406f 100644 --- a/src/libANGLE/Context.cpp +++ b/src/libANGLE/Context.cpp @@ -4714,10 +4714,8 @@ void Context::clear(GLbitfield mask) } // If all stencil bits are masked, don't attempt to clear stencil. - if (mState.getDrawFramebuffer()->getStencilAttachment() == nullptr || - (angle::BitMask( - mState.getDrawFramebuffer()->getStencilAttachment()->getStencilSize()) & - mState.getDepthStencilState().stencilWritemask) == 0) + if (mState.getDepthStencilState().isStencilMaskedOut( + mState.getDrawFramebuffer()->getStencilBitCount())) { mask &= ~GL_STENCIL_BUFFER_BIT; } @@ -4733,7 +4731,9 @@ void Context::clear(GLbitfield mask) ANGLE_CONTEXT_TRY(mState.getDrawFramebuffer()->clear(this, mask)); } -bool Context::isClearBufferMaskedOut(GLenum buffer, GLint drawbuffer) const +bool Context::isClearBufferMaskedOut(GLenum buffer, + GLint drawbuffer, + GLuint framebufferStencilSize) const { switch (buffer) { @@ -4742,10 +4742,10 @@ bool Context::isClearBufferMaskedOut(GLenum buffer, GLint drawbuffer) const case GL_DEPTH: return mState.getDepthStencilState().isDepthMaskedOut(); case GL_STENCIL: - return mState.getDepthStencilState().isStencilMaskedOut(); + return mState.getDepthStencilState().isStencilMaskedOut(framebufferStencilSize); case GL_DEPTH_STENCIL: return mState.getDepthStencilState().isDepthMaskedOut() && - mState.getDepthStencilState().isStencilMaskedOut(); + mState.getDepthStencilState().isStencilMaskedOut(framebufferStencilSize); default: UNREACHABLE(); return true; @@ -4757,7 +4757,8 @@ bool Context::noopClearBuffer(GLenum buffer, GLint drawbuffer) const Framebuffer *framebufferObject = mState.getDrawFramebuffer(); return !IsClearBufferEnabled(framebufferObject->getState(), buffer, drawbuffer) || - mState.isRasterizerDiscardEnabled() || isClearBufferMaskedOut(buffer, drawbuffer); + mState.isRasterizerDiscardEnabled() || + isClearBufferMaskedOut(buffer, drawbuffer, framebufferObject->getStencilBitCount()); } void Context::clearBufferfv(GLenum buffer, GLint drawbuffer, const GLfloat *values) diff --git a/src/libANGLE/Context.h b/src/libANGLE/Context.h index d1ef247cc5e..c62f4cebda3 100644 --- a/src/libANGLE/Context.h +++ b/src/libANGLE/Context.h @@ -801,7 +801,9 @@ class Context final : public egl::LabeledObject, angle::NonCopyable, public angl bool noopDrawInstanced(PrimitiveMode mode, GLsizei count, GLsizei instanceCount) const; bool noopMultiDraw(GLsizei drawcount) const; - bool isClearBufferMaskedOut(GLenum buffer, GLint drawbuffer) const; + bool isClearBufferMaskedOut(GLenum buffer, + GLint drawbuffer, + GLuint framebufferStencilSize) const; bool noopClearBuffer(GLenum buffer, GLint drawbuffer) const; void addRef() const { mRefCount++; } diff --git a/src/libANGLE/Framebuffer.cpp b/src/libANGLE/Framebuffer.cpp index 4e393d951fe..858003abc47 100644 --- a/src/libANGLE/Framebuffer.cpp +++ b/src/libANGLE/Framebuffer.cpp @@ -740,25 +740,30 @@ bool FramebufferState::colorAttachmentsAreUniqueImages() const bool FramebufferState::hasDepth() const { - return (mDepthAttachment.isAttached() && mDepthAttachment.getDepthSize() > 0); + return mDepthAttachment.isAttached() && mDepthAttachment.getDepthSize() > 0; } bool FramebufferState::hasStencil() const { - return (mStencilAttachment.isAttached() && mStencilAttachment.getStencilSize() > 0); + return mStencilAttachment.isAttached() && mStencilAttachment.getStencilSize() > 0; +} + +GLuint FramebufferState::getStencilBitCount() const +{ + return mStencilAttachment.isAttached() ? mStencilAttachment.getStencilSize() : 0; } bool FramebufferState::hasExternalTextureAttachment() const { // External textures can only be bound to color attachment 0 - return (mColorAttachments[0].isAttached() && mColorAttachments[0].isExternalTexture()); + return mColorAttachments[0].isAttached() && mColorAttachments[0].isExternalTexture(); } bool FramebufferState::hasYUVAttachment() const { // The only attachments that can be YUV are external textures and surfaces, both are attached at // color attachment 0. - return (mColorAttachments[0].isAttached() && mColorAttachments[0].isYUV()); + return mColorAttachments[0].isAttached() && mColorAttachments[0].isYUV(); } bool FramebufferState::isMultiview() const @@ -1248,31 +1253,6 @@ void Framebuffer::setReadBuffer(GLenum buffer) } } -size_t Framebuffer::getNumColorAttachments() const -{ - return mState.mColorAttachments.size(); -} - -bool Framebuffer::hasDepth() const -{ - return mState.hasDepth(); -} - -bool Framebuffer::hasStencil() const -{ - return mState.hasStencil(); -} - -bool Framebuffer::hasExternalTextureAttachment() const -{ - return mState.hasExternalTextureAttachment(); -} - -bool Framebuffer::hasYUVAttachment() const -{ - return mState.hasYUVAttachment(); -} - void Framebuffer::invalidateCompletenessCache() { if (!isDefault()) @@ -2495,7 +2475,8 @@ angle::Result Framebuffer::ensureClearAttachmentsInitialized(const Context *cont bool color = (mask & GL_COLOR_BUFFER_BIT) != 0 && !glState.allActiveDrawBufferChannelsMasked(); bool depth = (mask & GL_DEPTH_BUFFER_BIT) != 0 && !depthStencil.isDepthMaskedOut(); - bool stencil = (mask & GL_STENCIL_BUFFER_BIT) != 0 && !depthStencil.isStencilMaskedOut(); + bool stencil = (mask & GL_STENCIL_BUFFER_BIT) != 0 && + !depthStencil.isStencilMaskedOut(getStencilBitCount()); if (!color && !depth && !stencil) { @@ -2523,7 +2504,8 @@ angle::Result Framebuffer::ensureClearBufferAttachmentsInitialized(const Context { if (!context->isRobustResourceInitEnabled() || context->getState().isRasterizerDiscardEnabled() || - context->isClearBufferMaskedOut(buffer, drawbuffer) || mState.mResourceNeedsInit.none()) + context->isClearBufferMaskedOut(buffer, drawbuffer, getStencilBitCount()) || + mState.mResourceNeedsInit.none()) { return angle::Result::Continue; } diff --git a/src/libANGLE/Framebuffer.h b/src/libANGLE/Framebuffer.h index 79b196cf353..c9c3df0decb 100644 --- a/src/libANGLE/Framebuffer.h +++ b/src/libANGLE/Framebuffer.h @@ -111,6 +111,7 @@ class FramebufferState final : angle::NonCopyable bool hasDepth() const; bool hasStencil() const; + GLuint getStencilBitCount() const; bool hasExternalTextureAttachment() const; bool hasYUVAttachment() const; @@ -285,12 +286,13 @@ class Framebuffer final : public angle::ObserverInterface, GLenum getReadBufferState() const; void setReadBuffer(GLenum buffer); - size_t getNumColorAttachments() const; - bool hasDepth() const; - bool hasStencil() const; + size_t getNumColorAttachments() const { return mState.mColorAttachments.size(); } + bool hasDepth() const { return mState.hasDepth(); } + bool hasStencil() const { return mState.hasStencil(); } + GLuint getStencilBitCount() const { return mState.getStencilBitCount(); } - bool hasExternalTextureAttachment() const; - bool hasYUVAttachment() const; + bool hasExternalTextureAttachment() const { return mState.hasExternalTextureAttachment(); } + bool hasYUVAttachment() const { return mState.hasYUVAttachment(); } // This method calls checkStatus. int getSamples(const Context *context) const; diff --git a/src/libANGLE/State.h b/src/libANGLE/State.h index 7d9a6046395..193f927c48b 100644 --- a/src/libANGLE/State.h +++ b/src/libANGLE/State.h @@ -334,10 +334,11 @@ class PrivateState : angle::NonCopyable // Stencil state maniupulation bool isStencilTestEnabled() const { return mDepthStencil.stencilTest; } - bool isStencilWriteEnabled() const + bool isStencilWriteEnabled(GLuint framebufferStencilSize) const { return mDepthStencil.stencilTest && - !(mDepthStencil.isStencilNoOp() && mDepthStencil.isStencilBackNoOp()); + !(mDepthStencil.isStencilNoOp(framebufferStencilSize) && + mDepthStencil.isStencilBackNoOp(framebufferStencilSize)); } void setStencilTest(bool enabled); void setStencilParams(GLenum stencilFunc, GLint stencilRef, GLuint stencilMask); @@ -1305,7 +1306,10 @@ class State : angle::NonCopyable { return mPrivateState.isBlendAdvancedCoherentEnabled(); } - bool isStencilWriteEnabled() const { return mPrivateState.isStencilWriteEnabled(); } + bool isStencilWriteEnabled(GLuint framebufferStencilSize) const + { + return mPrivateState.isStencilWriteEnabled(framebufferStencilSize); + } GLint getStencilRef() const { return mPrivateState.getStencilRef(); } GLint getStencilBackRef() const { return mPrivateState.getStencilBackRef(); } PolygonMode getPolygonMode() const { return mPrivateState.getPolygonMode(); } diff --git a/src/libANGLE/angletypes.cpp b/src/libANGLE/angletypes.cpp index 9a0671d4e23..172f1cdcf05 100644 --- a/src/libANGLE/angletypes.cpp +++ b/src/libANGLE/angletypes.cpp @@ -21,6 +21,12 @@ namespace gl { namespace { +bool IsStencilWriteMaskedOut(GLuint stencilWritemask, GLuint framebufferStencilSize) +{ + const GLuint framebufferMask = angle::BitMask(framebufferStencilSize); + return (stencilWritemask & framebufferMask) == 0; +} + bool IsStencilNoOp(GLenum stencilFunc, GLenum stencilFail, GLenum stencilPassDepthFail, @@ -165,21 +171,20 @@ bool DepthStencilState::isDepthMaskedOut() const return !depthMask; } -bool DepthStencilState::isStencilMaskedOut() const +bool DepthStencilState::isStencilMaskedOut(GLuint framebufferStencilSize) const { - return stencilWritemask == 0; + return IsStencilWriteMaskedOut(stencilWritemask, framebufferStencilSize); } -bool DepthStencilState::isStencilNoOp() const +bool DepthStencilState::isStencilNoOp(GLuint framebufferStencilSize) const { - return isStencilMaskedOut() || + return isStencilMaskedOut(framebufferStencilSize) || IsStencilNoOp(stencilFunc, stencilFail, stencilPassDepthFail, stencilPassDepthPass); } -bool DepthStencilState::isStencilBackNoOp() const +bool DepthStencilState::isStencilBackNoOp(GLuint framebufferStencilSize) const { - const bool isStencilBackMaskedOut = stencilBackWritemask == 0; - return isStencilBackMaskedOut || + return IsStencilWriteMaskedOut(stencilBackWritemask, framebufferStencilSize) || IsStencilNoOp(stencilBackFunc, stencilBackFail, stencilBackPassDepthFail, stencilBackPassDepthPass); } diff --git a/src/libANGLE/angletypes.h b/src/libANGLE/angletypes.h index 58d3d3608e6..9756ee531c9 100644 --- a/src/libANGLE/angletypes.h +++ b/src/libANGLE/angletypes.h @@ -301,9 +301,9 @@ struct DepthStencilState final DepthStencilState &operator=(const DepthStencilState &other); bool isDepthMaskedOut() const; - bool isStencilMaskedOut() const; - bool isStencilNoOp() const; - bool isStencilBackNoOp() const; + bool isStencilMaskedOut(GLuint framebufferStencilSize) const; + bool isStencilNoOp(GLuint framebufferStencilSize) const; + bool isStencilBackNoOp(GLuint framebufferStencilSize) const; bool depthTest; GLenum depthFunc; diff --git a/src/libANGLE/renderer/vulkan/ContextVk.cpp b/src/libANGLE/renderer/vulkan/ContextVk.cpp index 337052bf123..0db24956b47 100644 --- a/src/libANGLE/renderer/vulkan/ContextVk.cpp +++ b/src/libANGLE/renderer/vulkan/ContextVk.cpp @@ -329,6 +329,7 @@ vk::ResourceAccess GetDepthAccess(const gl::DepthStencilState &dsState, } vk::ResourceAccess GetStencilAccess(const gl::DepthStencilState &dsState, + GLuint framebufferStencilSize, UpdateDepthFeedbackLoopReason reason) { // Skip if depth/stencil not actually accessed. @@ -344,8 +345,10 @@ vk::ResourceAccess GetStencilAccess(const gl::DepthStencilState &dsState, return vk::ResourceAccess::Unused; } - return dsState.isStencilNoOp() && dsState.isStencilBackNoOp() ? vk::ResourceAccess::ReadOnly - : vk::ResourceAccess::ReadWrite; + return dsState.isStencilNoOp(framebufferStencilSize) && + dsState.isStencilBackNoOp(framebufferStencilSize) + ? vk::ResourceAccess::ReadOnly + : vk::ResourceAccess::ReadWrite; } egl::ContextPriority GetContextPriority(const gl::State &state) @@ -2304,7 +2307,8 @@ angle::Result ContextVk::switchOutReadOnlyDepthStencilMode( const gl::DepthStencilState &dsState = mState.getDepthStencilState(); vk::ResourceAccess depthAccess = GetDepthAccess(dsState, depthReason); - vk::ResourceAccess stencilAccess = GetStencilAccess(dsState, stencilReason); + vk::ResourceAccess stencilAccess = + GetStencilAccess(dsState, mState.getDrawFramebuffer()->getStencilBitCount(), stencilReason); if ((HasResourceWriteAccess(depthAccess) && mDepthStencilAttachmentFlags[vk::RenderPassUsage::DepthReadOnlyAttachment]) || @@ -2450,7 +2454,8 @@ angle::Result ContextVk::handleDirtyGraphicsDepthStencilAccess( const gl::DepthStencilState &dsState = mState.getDepthStencilState(); vk::ResourceAccess depthAccess = GetDepthAccess(dsState, UpdateDepthFeedbackLoopReason::Draw); vk::ResourceAccess stencilAccess = - GetStencilAccess(dsState, UpdateDepthFeedbackLoopReason::Draw); + GetStencilAccess(dsState, mState.getDrawFramebuffer()->getStencilBitCount(), + UpdateDepthFeedbackLoopReason::Draw); mRenderPassCommands->onDepthAccess(depthAccess); mRenderPassCommands->onStencilAccess(stencilAccess); @@ -4552,7 +4557,8 @@ angle::Result ContextVk::optimizeRenderPassForPresent(vk::ImageViewHelper *color mRenderPassCommands->invalidateRenderPassDepthAttachment( dsState, mRenderPassCommands->getRenderArea()); mRenderPassCommands->invalidateRenderPassStencilAttachment( - dsState, mRenderPassCommands->getRenderArea()); + dsState, mState.getDrawFramebuffer()->getStencilBitCount(), + mRenderPassCommands->getRenderArea()); } // Use finalLayout instead of extra barrier for layout change to present @@ -8569,7 +8575,7 @@ angle::Result ContextVk::switchToReadOnlyDepthStencilMode(gl::Texture *texture, if (isStencilTexture) { - if (mState.isStencilWriteEnabled()) + if (mState.isStencilWriteEnabled(mState.getDrawFramebuffer()->getStencilBitCount())) { // This looks like a feedback loop, but we don't issue a warning because the application // may have correctly used BASE and MAX levels to avoid it. ANGLE doesn't track that. diff --git a/src/libANGLE/renderer/vulkan/FramebufferVk.cpp b/src/libANGLE/renderer/vulkan/FramebufferVk.cpp index 71ded475085..4c4f50caaab 100644 --- a/src/libANGLE/renderer/vulkan/FramebufferVk.cpp +++ b/src/libANGLE/renderer/vulkan/FramebufferVk.cpp @@ -2217,7 +2217,7 @@ angle::Result FramebufferVk::invalidateImpl(ContextVk *contextVk, if (invalidateStencilBuffer) { contextVk->getStartedRenderPassCommands().invalidateRenderPassStencilAttachment( - dsState, invalidateArea); + dsState, mState.getStencilBitCount(), invalidateArea); } } } diff --git a/src/libANGLE/renderer/vulkan/android/HardwareBufferImageSiblingVkAndroid.cpp b/src/libANGLE/renderer/vulkan/android/HardwareBufferImageSiblingVkAndroid.cpp index 3c28935a398..07abd77794d 100644 --- a/src/libANGLE/renderer/vulkan/android/HardwareBufferImageSiblingVkAndroid.cpp +++ b/src/libANGLE/renderer/vulkan/android/HardwareBufferImageSiblingVkAndroid.cpp @@ -217,7 +217,7 @@ egl::Error HardwareBufferImageSiblingVkAndroid::ValidateHardwareBuffer( { return egl::EglBadAccess() << "EGL_PROTECTED_CONTENT_EXT attribute does not match protected state " - "of EGLCleintBuffer."; + "of EGLClientBuffer."; } } diff --git a/src/libANGLE/renderer/vulkan/vk_helpers.cpp b/src/libANGLE/renderer/vulkan/vk_helpers.cpp index 09b81b4303b..8683228f44c 100644 --- a/src/libANGLE/renderer/vulkan/vk_helpers.cpp +++ b/src/libANGLE/renderer/vulkan/vk_helpers.cpp @@ -3045,10 +3045,12 @@ void RenderPassCommandBufferHelper::invalidateRenderPassDepthAttachment( void RenderPassCommandBufferHelper::invalidateRenderPassStencilAttachment( const gl::DepthStencilState &dsState, + GLuint framebufferStencilSize, const gl::Rectangle &invalidateArea) { const bool isStencilWriteEnabled = - dsState.stencilTest && (!dsState.isStencilNoOp() || !dsState.isStencilBackNoOp()); + dsState.stencilTest && (!dsState.isStencilNoOp(framebufferStencilSize) || + !dsState.isStencilBackNoOp(framebufferStencilSize)); mStencilAttachment.invalidate(invalidateArea, isStencilWriteEnabled, getRenderPassWriteCommandCount()); } diff --git a/src/libANGLE/renderer/vulkan/vk_helpers.h b/src/libANGLE/renderer/vulkan/vk_helpers.h index 211041929f1..83b9ab7da0d 100644 --- a/src/libANGLE/renderer/vulkan/vk_helpers.h +++ b/src/libANGLE/renderer/vulkan/vk_helpers.h @@ -1833,6 +1833,7 @@ class RenderPassCommandBufferHelper final : public CommandBufferHelperCommon void invalidateRenderPassDepthAttachment(const gl::DepthStencilState &dsState, const gl::Rectangle &invalidateArea); void invalidateRenderPassStencilAttachment(const gl::DepthStencilState &dsState, + GLuint framebufferStencilSize, const gl::Rectangle &invalidateArea); void updateRenderPassColorClear(PackedAttachmentIndex colorIndexVk, diff --git a/src/tests/gl_tests/ClearTest.cpp b/src/tests/gl_tests/ClearTest.cpp index 0e2cce59986..e17c2b0c7f8 100644 --- a/src/tests/gl_tests/ClearTest.cpp +++ b/src/tests/gl_tests/ClearTest.cpp @@ -3006,6 +3006,14 @@ TEST_P(ClearTestES3, ClearStencilZeroFirstByteMask) glClear(GL_STENCIL_BUFFER_BIT); } +// Same test as ClearStencilZeroFirstByteMask, but using glClearBufferiv. +TEST_P(ClearTestES3, ClearBufferStencilZeroFirstByteMask) +{ + glStencilMask(0xe7d6a900); + const GLint kStencilClearValue = 0x55; + glClearBufferiv(GL_STENCIL, 0, &kStencilClearValue); +} + // Test that mid render pass clear after draw sets the render pass size correctly. TEST_P(ClearTestES3, ScissoredDrawThenFullClear) {