From 8b0af482db7456077fb083a263fed85077e6b8cf Mon Sep 17 00:00:00 2001 From: Alexey Knyazev Date: Mon, 21 Aug 2023 00:00:00 +0000 Subject: [PATCH] Validate active draw buffers for dual-source blending Fail if more than MAX_DUAL_SOURCE_DRAW_BUFFERS_EXT draw buffers are enabled when dual-source blending is used. Drive-by: Do not invalidate draw state on changing blend equations if KHR_blend_equation_advanced is not enabled. Bug: angleproject:1085 Bug: angleproject:7177 Change-Id: Ieff80ce777c53b1d8183e1d0a52b7d2224347448 Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4823164 Reviewed-by: Shahbaz Youssefi Commit-Queue: Alexey Knyazev --- src/libANGLE/BlendStateExt_unittest.cpp | 2 + src/libANGLE/Context.h | 25 +++++- src/libANGLE/ErrorStrings.h | 1 + src/libANGLE/angletypes.cpp | 81 ++++++++++++++++---- src/libANGLE/angletypes.h | 15 +++- src/libANGLE/context_private_call_gles.cpp | 38 +++++++-- src/libANGLE/validationES.cpp | 13 ++++ src/tests/gl_tests/BlendFuncExtendedTest.cpp | 69 +++++++++++++++++ 8 files changed, 213 insertions(+), 31 deletions(-) diff --git a/src/libANGLE/BlendStateExt_unittest.cpp b/src/libANGLE/BlendStateExt_unittest.cpp index b5595b1a6..9d06e7469 100644 --- a/src/libANGLE/BlendStateExt_unittest.cpp +++ b/src/libANGLE/BlendStateExt_unittest.cpp @@ -51,6 +51,8 @@ TEST(BlendStateExt, Init) ASSERT_EQ(blendStateExt.getUsesAdvancedBlendEquationMask().to_ulong(), 0u); + ASSERT_EQ(blendStateExt.getUsesExtendedBlendFactorMask().to_ulong(), 0u); + ASSERT_EQ(blendStateExt.getSrcColorBits(), sourceColorAlpha[c]); ASSERT_EQ(blendStateExt.getSrcAlphaBits(), sourceColorAlpha[c]); ASSERT_EQ(blendStateExt.getDstColorBits(), 0u); diff --git a/src/libANGLE/Context.h b/src/libANGLE/Context.h index 21ef8cd9d..9faf05e90 100644 --- a/src/libANGLE/Context.h +++ b/src/libANGLE/Context.h @@ -170,8 +170,26 @@ class PrivateStateCache final : angle::NonCopyable void onCapChange() { mIsCachedBasicDrawStatesErrorValid = false; } void onColorMaskChange() { mIsCachedBasicDrawStatesErrorValid = false; } void onDefaultVertexAttributeChange() { mIsCachedBasicDrawStatesErrorValid = false; } - void onBlendFuncIndexedChange() { mIsCachedBasicDrawStatesErrorValid = false; } - void onBlendEquationChange() { mIsCachedBasicDrawStatesErrorValid = false; } + + // Blending updates invalidate draw + // state in the following cases: + // + // * Blend equations have been changed and the context + // supports KHR_blend_equation_advanced. The number + // of enabled draw buffers may need to be checked + // to not be greater than 1. + // + // * Blend funcs have been changed with indexed + // commands. The D3D11 backend cannot support + // constant color and alpha blend funcs together + // so a check is needed across all draw buffers. + // + // * Blend funcs have been changed and the context + // supports EXT_blend_func_extended. The number + // of enabled draw buffers may need to be checked + // against MAX_DUAL_SOURCE_DRAW_BUFFERS_EXT limit. + void onBlendEquationOrFuncChange() { mIsCachedBasicDrawStatesErrorValid = false; } + void onStencilStateChange() { mIsCachedBasicDrawStatesErrorValid = false; } bool isCachedBasicDrawStatesErrorValid() const { return mIsCachedBasicDrawStatesErrorValid; } @@ -237,8 +255,7 @@ class StateCache final : angle::NonCopyable // 2. onStencilStateChange. // 3. onDefaultVertexAttributeChange. // 4. onColorMaskChange. - // 5. onBlendFuncIndexedChange. - // 6. onBlendEquationChange. + // 5. onBlendEquationOrFuncChange. intptr_t getBasicDrawStatesErrorString(const Context *context, const PrivateStateCache *privateStateCache) const { diff --git a/src/libANGLE/ErrorStrings.h b/src/libANGLE/ErrorStrings.h index d586b5e2b..5b67e8258 100644 --- a/src/libANGLE/ErrorStrings.h +++ b/src/libANGLE/ErrorStrings.h @@ -102,6 +102,7 @@ MSG kES31OrDrawBuffersIndexedExtensionNotAvailable = "EXT/OES_draw_buffers_index 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."; +MSG kDualSourceBlendingDrawBuffersLimit = "Dual-source blending functions limit the number of supported draw buffers."; MSG kEGLImageCannotCreate2DMultisampled = "Cannot create a 2D texture from a multisampled EGL image."; MSG kEGLImageRenderbufferFormatNotSupported = "EGL image internal format is not supported as a renderbuffer."; MSG kEGLImageTextureFormatNotSupported = "EGL image internal format is not supported as a texture."; diff --git a/src/libANGLE/angletypes.cpp b/src/libANGLE/angletypes.cpp index 1998e578d..f05aaf237 100644 --- a/src/libANGLE/angletypes.cpp +++ b/src/libANGLE/angletypes.cpp @@ -42,6 +42,12 @@ bool IsAdvancedBlendEquation(gl::BlendEquationType blendEquation) return blendEquation >= gl::BlendEquationType::Multiply && blendEquation <= gl::BlendEquationType::HslLuminosity; } + +bool IsExtendedBlendFactor(gl::BlendFactorType blendFactor) +{ + return blendFactor >= gl::BlendFactorType::Src1Alpha && + blendFactor <= gl::BlendFactorType::OneMinusSrc1Alpha; +} } // anonymous namespace RasterizerState::RasterizerState() @@ -558,6 +564,12 @@ BlendStateExt::FactorStorage::Type BlendStateExt::expandFactorValue(const GLenum return FactorStorage::GetReplicatedValue(FromGLenum(func), mParameterMask); } +BlendStateExt::FactorStorage::Type BlendStateExt::expandFactorValue( + const gl::BlendFactorType func) const +{ + return FactorStorage::GetReplicatedValue(func, mParameterMask); +} + BlendStateExt::FactorStorage::Type BlendStateExt::expandSrcColorIndexed(const size_t index) const { ASSERT(index < mDrawBufferCount); @@ -591,10 +603,25 @@ void BlendStateExt::setFactors(const GLenum srcColor, const GLenum srcAlpha, const GLenum dstAlpha) { - mSrcColor = expandFactorValue(srcColor); - mDstColor = expandFactorValue(dstColor); - mSrcAlpha = expandFactorValue(srcAlpha); - mDstAlpha = expandFactorValue(dstAlpha); + const gl::BlendFactorType srcColorFactor = FromGLenum(srcColor); + const gl::BlendFactorType dstColorFactor = FromGLenum(dstColor); + const gl::BlendFactorType srcAlphaFactor = FromGLenum(srcAlpha); + const gl::BlendFactorType dstAlphaFactor = FromGLenum(dstAlpha); + + mSrcColor = expandFactorValue(srcColorFactor); + mDstColor = expandFactorValue(dstColorFactor); + mSrcAlpha = expandFactorValue(srcAlphaFactor); + mDstAlpha = expandFactorValue(dstAlphaFactor); + + if (IsExtendedBlendFactor(srcColorFactor) || IsExtendedBlendFactor(dstColorFactor) || + IsExtendedBlendFactor(srcAlphaFactor) || IsExtendedBlendFactor(dstAlphaFactor)) + { + mUsesExtendedBlendFactorMask = mAllEnabledMask; + } + else + { + mUsesExtendedBlendFactorMask.reset(); + } } void BlendStateExt::setFactorsIndexed(const size_t index, @@ -604,10 +631,21 @@ void BlendStateExt::setFactorsIndexed(const size_t index, const GLenum dstAlpha) { ASSERT(index < mDrawBufferCount); - FactorStorage::SetValueIndexed(index, FromGLenum(srcColor), &mSrcColor); - FactorStorage::SetValueIndexed(index, FromGLenum(dstColor), &mDstColor); - FactorStorage::SetValueIndexed(index, FromGLenum(srcAlpha), &mSrcAlpha); - FactorStorage::SetValueIndexed(index, FromGLenum(dstAlpha), &mDstAlpha); + + const gl::BlendFactorType srcColorFactor = FromGLenum(srcColor); + const gl::BlendFactorType dstColorFactor = FromGLenum(dstColor); + const gl::BlendFactorType srcAlphaFactor = FromGLenum(srcAlpha); + const gl::BlendFactorType dstAlphaFactor = FromGLenum(dstAlpha); + + FactorStorage::SetValueIndexed(index, srcColorFactor, &mSrcColor); + FactorStorage::SetValueIndexed(index, dstColorFactor, &mDstColor); + FactorStorage::SetValueIndexed(index, srcAlphaFactor, &mSrcAlpha); + FactorStorage::SetValueIndexed(index, dstAlphaFactor, &mDstAlpha); + + const bool isExtended = + IsExtendedBlendFactor(srcColorFactor) || IsExtendedBlendFactor(dstColorFactor) || + IsExtendedBlendFactor(srcAlphaFactor) || IsExtendedBlendFactor(dstAlphaFactor); + mUsesExtendedBlendFactorMask.set(index, isExtended); } void BlendStateExt::setFactorsIndexed(const size_t index, @@ -616,14 +654,25 @@ void BlendStateExt::setFactorsIndexed(const size_t index, { ASSERT(index < mDrawBufferCount); ASSERT(sourceIndex < source.mDrawBufferCount); - FactorStorage::SetValueIndexed( - index, FactorStorage::GetValueIndexed(sourceIndex, source.mSrcColor), &mSrcColor); - FactorStorage::SetValueIndexed( - index, FactorStorage::GetValueIndexed(sourceIndex, source.mDstColor), &mDstColor); - FactorStorage::SetValueIndexed( - index, FactorStorage::GetValueIndexed(sourceIndex, source.mSrcAlpha), &mSrcAlpha); - FactorStorage::SetValueIndexed( - index, FactorStorage::GetValueIndexed(sourceIndex, source.mDstAlpha), &mDstAlpha); + + const gl::BlendFactorType srcColorFactor = + FactorStorage::GetValueIndexed(sourceIndex, source.mSrcColor); + const gl::BlendFactorType dstColorFactor = + FactorStorage::GetValueIndexed(sourceIndex, source.mDstColor); + const gl::BlendFactorType srcAlphaFactor = + FactorStorage::GetValueIndexed(sourceIndex, source.mSrcAlpha); + const gl::BlendFactorType dstAlphaFactor = + FactorStorage::GetValueIndexed(sourceIndex, source.mDstAlpha); + + FactorStorage::SetValueIndexed(index, srcColorFactor, &mSrcColor); + FactorStorage::SetValueIndexed(index, dstColorFactor, &mDstColor); + FactorStorage::SetValueIndexed(index, srcAlphaFactor, &mSrcAlpha); + FactorStorage::SetValueIndexed(index, dstAlphaFactor, &mDstAlpha); + + const bool isExtended = + IsExtendedBlendFactor(srcColorFactor) || IsExtendedBlendFactor(dstColorFactor) || + IsExtendedBlendFactor(srcAlphaFactor) || IsExtendedBlendFactor(dstAlphaFactor); + mUsesExtendedBlendFactorMask.set(index, isExtended); } GLenum BlendStateExt::getSrcColorIndexed(size_t index) const diff --git a/src/libANGLE/angletypes.h b/src/libANGLE/angletypes.h index 66913939f..fa7cde4f9 100644 --- a/src/libANGLE/angletypes.h +++ b/src/libANGLE/angletypes.h @@ -729,6 +729,7 @@ class BlendStateExt final ///////// Blend Factors ///////// FactorStorage::Type expandFactorValue(const GLenum func) const; + FactorStorage::Type expandFactorValue(const gl::BlendFactorType func) const; FactorStorage::Type expandSrcColorIndexed(const size_t index) const; FactorStorage::Type expandDstColorIndexed(const size_t index) const; FactorStorage::Type expandSrcAlphaIndexed(const size_t index) const; @@ -775,6 +776,11 @@ class BlendStateExt final return mUsesAdvancedBlendEquationMask; } + constexpr DrawBufferMask getUsesExtendedBlendFactorMask() const + { + return mUsesExtendedBlendFactorMask; + } + constexpr uint8_t getDrawBufferCount() const { return mDrawBufferCount; } constexpr void setSrcColorBits(const FactorStorage::Type srcColor) { mSrcColor = srcColor; } @@ -819,17 +825,20 @@ class BlendStateExt final // Cache of whether the blend equation for each index is from KHR_blend_equation_advanced. DrawBufferMask mUsesAdvancedBlendEquationMask; + // Cache of whether the blend factor for each index is from EXT_blend_func_extended. + DrawBufferMask mUsesExtendedBlendFactorMask; + uint8_t mDrawBufferCount; - ANGLE_MAYBE_UNUSED_PRIVATE_FIELD uint32_t kUnused = 0; + ANGLE_MAYBE_UNUSED_PRIVATE_FIELD uint8_t kUnused[3] = {}; }; static_assert(sizeof(BlendStateExt) == sizeof(uint64_t) + (sizeof(BlendStateExt::FactorStorage::Type) * 4 + sizeof(BlendStateExt::EquationStorage::Type) * 2 + sizeof(BlendStateExt::ColorMaskStorage::Type) * 2 + - sizeof(DrawBufferMask) * 3 + sizeof(uint8_t)) + - sizeof(uint32_t), + sizeof(DrawBufferMask) * 4 + sizeof(uint8_t)) + + sizeof(uint8_t) * 3, "The BlendStateExt class must not contain gaps."); // Used in StateCache diff --git a/src/libANGLE/context_private_call_gles.cpp b/src/libANGLE/context_private_call_gles.cpp index 9682c2f1b..337fe32d1 100644 --- a/src/libANGLE/context_private_call_gles.cpp +++ b/src/libANGLE/context_private_call_gles.cpp @@ -495,7 +495,10 @@ void ContextPrivateBlendEquation(PrivateState *privateState, GLenum mode) { privateState->setBlendEquation(mode, mode); - privateStateCache->onBlendEquationChange(); + if (privateState->getExtensions().blendEquationAdvancedKHR) + { + privateStateCache->onBlendEquationOrFuncChange(); + } } void ContextPrivateBlendEquationi(PrivateState *privateState, @@ -504,7 +507,10 @@ void ContextPrivateBlendEquationi(PrivateState *privateState, GLenum mode) { privateState->setBlendEquationIndexed(mode, mode, buf); - privateStateCache->onBlendEquationChange(); + if (privateState->getExtensions().blendEquationAdvancedKHR) + { + privateStateCache->onBlendEquationOrFuncChange(); + } } void ContextPrivateBlendEquationSeparate(PrivateState *privateState, @@ -513,7 +519,10 @@ void ContextPrivateBlendEquationSeparate(PrivateState *privateState, GLenum modeAlpha) { privateState->setBlendEquation(modeRGB, modeAlpha); - privateStateCache->onBlendEquationChange(); + if (privateState->getExtensions().blendEquationAdvancedKHR) + { + privateStateCache->onBlendEquationOrFuncChange(); + } } void ContextPrivateBlendEquationSeparatei(PrivateState *privateState, @@ -523,7 +532,10 @@ void ContextPrivateBlendEquationSeparatei(PrivateState *privateState, GLenum modeAlpha) { privateState->setBlendEquationIndexed(modeRGB, modeAlpha, buf); - privateStateCache->onBlendEquationChange(); + if (privateState->getExtensions().blendEquationAdvancedKHR) + { + privateStateCache->onBlendEquationOrFuncChange(); + } } void ContextPrivateBlendFunc(PrivateState *privateState, @@ -532,6 +544,10 @@ void ContextPrivateBlendFunc(PrivateState *privateState, GLenum dfactor) { privateState->setBlendFactors(sfactor, dfactor, sfactor, dfactor); + if (privateState->getExtensions().blendFuncExtendedEXT) + { + privateStateCache->onBlendEquationOrFuncChange(); + } } void ContextPrivateBlendFunci(PrivateState *privateState, @@ -541,9 +557,10 @@ void ContextPrivateBlendFunci(PrivateState *privateState, GLenum dst) { privateState->setBlendFactorsIndexed(src, dst, src, dst, buf); - if (privateState->noSimultaneousConstantColorAndAlphaBlendFunc()) + if (privateState->noSimultaneousConstantColorAndAlphaBlendFunc() || + privateState->getExtensions().blendFuncExtendedEXT) { - privateStateCache->onBlendFuncIndexedChange(); + privateStateCache->onBlendEquationOrFuncChange(); } } @@ -555,6 +572,10 @@ void ContextPrivateBlendFuncSeparate(PrivateState *privateState, GLenum dstAlpha) { privateState->setBlendFactors(srcRGB, dstRGB, srcAlpha, dstAlpha); + if (privateState->getExtensions().blendFuncExtendedEXT) + { + privateStateCache->onBlendEquationOrFuncChange(); + } } void ContextPrivateBlendFuncSeparatei(PrivateState *privateState, @@ -566,9 +587,10 @@ void ContextPrivateBlendFuncSeparatei(PrivateState *privateState, GLenum dstAlpha) { privateState->setBlendFactorsIndexed(srcRGB, dstRGB, srcAlpha, dstAlpha, buf); - if (privateState->noSimultaneousConstantColorAndAlphaBlendFunc()) + if (privateState->noSimultaneousConstantColorAndAlphaBlendFunc() || + privateState->getExtensions().blendFuncExtendedEXT) { - privateStateCache->onBlendFuncIndexedChange(); + privateStateCache->onBlendEquationOrFuncChange(); } } diff --git a/src/libANGLE/validationES.cpp b/src/libANGLE/validationES.cpp index 4903df31c..da503c43f 100644 --- a/src/libANGLE/validationES.cpp +++ b/src/libANGLE/validationES.cpp @@ -4237,6 +4237,19 @@ const char *ValidateDrawStates(const Context *context, GLenum *outErrorCode) } } + // Dual-source blending functions limit the number of supported draw buffers. + if (blendStateExt.getUsesExtendedBlendFactorMask().any()) + { + // Imply the strictest spec interpretation to pass on all OpenGL drivers: + // dual-source blending is considered active if the blend state contains + // any SRC1 factor no matter what. + const DrawBufferMask bufferMask = framebuffer->getDrawBufferMask(); + if (bufferMask.any() && bufferMask.last() >= context->getCaps().maxDualSourceDrawBuffers) + { + return kDualSourceBlendingDrawBuffersLimit; + } + } + if (context->getStateCache().hasAnyEnabledClientAttrib()) { if (extensions.webglCompatibilityANGLE || !state.areClientArraysEnabled()) diff --git a/src/tests/gl_tests/BlendFuncExtendedTest.cpp b/src/tests/gl_tests/BlendFuncExtendedTest.cpp index 868e4d13d..a849c1268 100644 --- a/src/tests/gl_tests/BlendFuncExtendedTest.cpp +++ b/src/tests/gl_tests/BlendFuncExtendedTest.cpp @@ -380,6 +380,75 @@ TEST_P(EXTBlendFuncExtendedTest, TestMaxDualSourceDrawBuffers) ASSERT_GL_NO_ERROR(); } +// Test that SRC1 factors limit the number of allowed draw buffers. +TEST_P(EXTBlendFuncExtendedTest, MaxDualSourceDrawBuffersError) +{ + ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_blend_func_extended")); + ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_draw_buffers")); + ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_OES_rgb8_rgba8")); + + GLint maxDualSourceDrawBuffers = 0; + glGetIntegerv(GL_MAX_DUAL_SOURCE_DRAW_BUFFERS_EXT, &maxDualSourceDrawBuffers); + ANGLE_SKIP_TEST_IF(maxDualSourceDrawBuffers != 1); + + ANGLE_GL_PROGRAM(redProgram, essl1_shaders::vs::Simple(), essl1_shaders::fs::Red()); + + GLFramebuffer fbo; + glBindFramebuffer(GL_FRAMEBUFFER, fbo); + + GLRenderbuffer rb0; + glBindRenderbuffer(GL_RENDERBUFFER, rb0); + glRenderbufferStorage(GL_RENDERBUFFER, GL_RGBA8_OES, 1, 1); + glFramebufferRenderbuffer(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0_EXT, GL_RENDERBUFFER, rb0); + + GLRenderbuffer rb1; + glBindRenderbuffer(GL_RENDERBUFFER, rb1); + glRenderbufferStorage(GL_RENDERBUFFER, GL_RGBA8_OES, 1, 1); + glFramebufferRenderbuffer(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT1_EXT, GL_RENDERBUFFER, rb1); + + ASSERT_GL_FRAMEBUFFER_COMPLETE(GL_FRAMEBUFFER); + + const GLenum bufs[] = {GL_COLOR_ATTACHMENT0_EXT, GL_COLOR_ATTACHMENT1_EXT}; + glDrawBuffersEXT(2, bufs); + ASSERT_GL_NO_ERROR(); + + for (const GLenum func : {GL_SRC1_COLOR_EXT, GL_ONE_MINUS_SRC1_COLOR_EXT, GL_SRC1_ALPHA_EXT, + GL_ONE_MINUS_SRC1_ALPHA_EXT}) + { + for (size_t slot = 0; slot < 4; slot++) + { + switch (slot) + { + case 0: + glBlendFuncSeparate(func, GL_ONE, GL_ONE, GL_ONE); + break; + case 1: + glBlendFuncSeparate(GL_ONE, func, GL_ONE, GL_ONE); + break; + case 2: + glBlendFuncSeparate(GL_ONE, GL_ONE, func, GL_ONE); + break; + case 3: + glBlendFuncSeparate(GL_ONE, GL_ONE, GL_ONE, func); + break; + } + // Limit must be applied even with blending disabled + glDisable(GL_BLEND); + drawQuad(redProgram, essl1_shaders::PositionAttrib(), 0.0); + EXPECT_GL_ERROR(GL_INVALID_OPERATION); + + glEnable(GL_BLEND); + drawQuad(redProgram, essl1_shaders::PositionAttrib(), 0.0); + EXPECT_GL_ERROR(GL_INVALID_OPERATION); + + // Limit is not applied when non-SRC1 funcs are used + glBlendFunc(GL_ONE, GL_ONE); + drawQuad(redProgram, essl1_shaders::PositionAttrib(), 0.0); + EXPECT_GL_NO_ERROR(); + } + } +} + // Test a shader with EXT_blend_func_extended and gl_SecondaryFragColorEXT. // Outputs to primary color buffer using primary and secondary colors. TEST_P(EXTBlendFuncExtendedDrawTest, FragColor)