From 042184128efea90552495d5054c3ae17b84cb3c3 Mon Sep 17 00:00:00 2001 From: Stephen White Date: Mon, 7 Dec 2020 19:31:03 +0000 Subject: [PATCH] Widen color state support on ES. Indexed draw buffer (color state) support on ES is not core until 3.2. Previously, we were failing (asserting) if color state was changed for a non-zero attachment. This CL compares the state, and only asserts if the color state actually differs per-attachment. It also implements a disable_indexed_draw_buffers toggle, which allows a test to check for the functionality in a platform-independent manner. BUG=dawn:580, dawn:582 Change-Id: I11c67b0dd72f73e7302c06cad24e8a268fb37a76 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/34981 Commit-Queue: Stephen White Reviewed-by: Corentin Wallez --- src/dawn_native/Toggles.cpp | 5 ++ src/dawn_native/Toggles.h | 1 + src/dawn_native/opengl/DeviceGL.cpp | 4 + src/dawn_native/opengl/RenderPipelineGL.cpp | 95 ++++++++++++--------- src/tests/end2end/ColorStateTests.cpp | 2 + 5 files changed, 69 insertions(+), 38 deletions(-) diff --git a/src/dawn_native/Toggles.cpp b/src/dawn_native/Toggles.cpp index f48a27454f..bc4d55b047 100644 --- a/src/dawn_native/Toggles.cpp +++ b/src/dawn_native/Toggles.cpp @@ -120,6 +120,11 @@ namespace dawn_native { "Disables the use of non-zero base instance which is unsupported on some " "platforms.", "https://crbug.com/dawn/343"}}, + {Toggle::DisableIndexedDrawBuffers, + {"disable_indexed_draw_buffers", + "Disables the use of indexed draw buffer state which is unsupported on some " + "platforms.", + "https://crbug.com/dawn/582"}}, {Toggle::UseD3D12SmallShaderVisibleHeapForTesting, {"use_d3d12_small_shader_visible_heap", "Enable use of a small D3D12 shader visible heap, instead of using a large one by " diff --git a/src/dawn_native/Toggles.h b/src/dawn_native/Toggles.h index 880d4734df..d2f70608e3 100644 --- a/src/dawn_native/Toggles.h +++ b/src/dawn_native/Toggles.h @@ -39,6 +39,7 @@ namespace dawn_native { MetalUseSharedModeForCounterSampleBuffer, DisableBaseVertex, DisableBaseInstance, + DisableIndexedDrawBuffers, UseD3D12SmallShaderVisibleHeapForTesting, UseDXC, DisableRobustness, diff --git a/src/dawn_native/opengl/DeviceGL.cpp b/src/dawn_native/opengl/DeviceGL.cpp index 9099ae32a7..5ae56b8784 100644 --- a/src/dawn_native/opengl/DeviceGL.cpp +++ b/src/dawn_native/opengl/DeviceGL.cpp @@ -65,6 +65,9 @@ namespace dawn_native { namespace opengl { bool supportsBaseInstance = gl.IsAtLeastGLES(3, 2) || gl.IsAtLeastGL(4, 2); + // TODO(crbug.com/dawn/582): Use OES_draw_buffers_indexed where available. + bool supportsIndexedDrawBuffers = gl.IsAtLeastGLES(3, 2) || gl.IsAtLeastGL(3, 0); + // TODO(crbug.com/dawn/343): We can support the extension variants, but need to load the EXT // procs without the extension suffix. // We'll also need emulation of shader builtins gl_BaseVertex and gl_BaseInstance. @@ -82,6 +85,7 @@ namespace dawn_native { namespace opengl { // TODO(crbug.com/dawn/343): Investigate emulation. SetToggle(Toggle::DisableBaseVertex, !supportsBaseVertex); SetToggle(Toggle::DisableBaseInstance, !supportsBaseInstance); + SetToggle(Toggle::DisableIndexedDrawBuffers, !supportsIndexedDrawBuffers); } const GLFormat& Device::GetGLFormat(const Format& format) { diff --git a/src/dawn_native/opengl/RenderPipelineGL.cpp b/src/dawn_native/opengl/RenderPipelineGL.cpp index edabc71cdd..9a5da5c282 100644 --- a/src/dawn_native/opengl/RenderPipelineGL.cpp +++ b/src/dawn_native/opengl/RenderPipelineGL.cpp @@ -106,44 +106,46 @@ namespace dawn_native { namespace opengl { ColorAttachmentIndex attachment, const ColorStateDescriptor* descriptor) { GLuint colorBuffer = static_cast(static_cast(attachment)); - if (gl.IsAtLeastGL(3, 0) || gl.IsAtLeastGLES(3, 2)) { - if (BlendEnabled(descriptor)) { - gl.Enablei(GL_BLEND, colorBuffer); - gl.BlendEquationSeparatei(colorBuffer, - GLBlendMode(descriptor->colorBlend.operation), - GLBlendMode(descriptor->alphaBlend.operation)); - gl.BlendFuncSeparatei(colorBuffer, - GLBlendFactor(descriptor->colorBlend.srcFactor, false), - GLBlendFactor(descriptor->colorBlend.dstFactor, false), - GLBlendFactor(descriptor->alphaBlend.srcFactor, true), - GLBlendFactor(descriptor->alphaBlend.dstFactor, true)); - } else { - gl.Disablei(GL_BLEND, colorBuffer); - } - gl.ColorMaski(colorBuffer, descriptor->writeMask & wgpu::ColorWriteMask::Red, - descriptor->writeMask & wgpu::ColorWriteMask::Green, - descriptor->writeMask & wgpu::ColorWriteMask::Blue, - descriptor->writeMask & wgpu::ColorWriteMask::Alpha); + if (BlendEnabled(descriptor)) { + gl.Enablei(GL_BLEND, colorBuffer); + gl.BlendEquationSeparatei(colorBuffer, + GLBlendMode(descriptor->colorBlend.operation), + GLBlendMode(descriptor->alphaBlend.operation)); + gl.BlendFuncSeparatei(colorBuffer, + GLBlendFactor(descriptor->colorBlend.srcFactor, false), + GLBlendFactor(descriptor->colorBlend.dstFactor, false), + GLBlendFactor(descriptor->alphaBlend.srcFactor, true), + GLBlendFactor(descriptor->alphaBlend.dstFactor, true)); } else { - // TODO(crbug.com/dawn/582): Add validation to prevent this as it is not supported - // on GLES < 3.2. - DAWN_ASSERT(colorBuffer == 0); - if (BlendEnabled(descriptor)) { - gl.Enable(GL_BLEND); - gl.BlendEquationSeparate(GLBlendMode(descriptor->colorBlend.operation), - GLBlendMode(descriptor->alphaBlend.operation)); - gl.BlendFuncSeparate(GLBlendFactor(descriptor->colorBlend.srcFactor, false), - GLBlendFactor(descriptor->colorBlend.dstFactor, false), - GLBlendFactor(descriptor->alphaBlend.srcFactor, true), - GLBlendFactor(descriptor->alphaBlend.dstFactor, true)); - } else { - gl.Disable(GL_BLEND); - } - gl.ColorMask(descriptor->writeMask & wgpu::ColorWriteMask::Red, - descriptor->writeMask & wgpu::ColorWriteMask::Green, - descriptor->writeMask & wgpu::ColorWriteMask::Blue, - descriptor->writeMask & wgpu::ColorWriteMask::Alpha); + gl.Disablei(GL_BLEND, colorBuffer); } + gl.ColorMaski(colorBuffer, descriptor->writeMask & wgpu::ColorWriteMask::Red, + descriptor->writeMask & wgpu::ColorWriteMask::Green, + descriptor->writeMask & wgpu::ColorWriteMask::Blue, + descriptor->writeMask & wgpu::ColorWriteMask::Alpha); + } + + void ApplyColorState(const OpenGLFunctions& gl, const ColorStateDescriptor* descriptor) { + if (BlendEnabled(descriptor)) { + gl.Enable(GL_BLEND); + gl.BlendEquationSeparate(GLBlendMode(descriptor->colorBlend.operation), + GLBlendMode(descriptor->alphaBlend.operation)); + gl.BlendFuncSeparate(GLBlendFactor(descriptor->colorBlend.srcFactor, false), + GLBlendFactor(descriptor->colorBlend.dstFactor, false), + GLBlendFactor(descriptor->alphaBlend.srcFactor, true), + GLBlendFactor(descriptor->alphaBlend.dstFactor, true)); + } else { + gl.Disable(GL_BLEND); + } + gl.ColorMask(descriptor->writeMask & wgpu::ColorWriteMask::Red, + descriptor->writeMask & wgpu::ColorWriteMask::Green, + descriptor->writeMask & wgpu::ColorWriteMask::Blue, + descriptor->writeMask & wgpu::ColorWriteMask::Alpha); + } + + bool Equal(const BlendDescriptor& lhs, const BlendDescriptor& rhs) { + return lhs.operation == rhs.operation && lhs.srcFactor == rhs.srcFactor && + lhs.dstFactor == rhs.dstFactor; } GLuint OpenGLStencilOperation(wgpu::StencilOperation stencilOperation) { @@ -298,8 +300,25 @@ namespace dawn_native { namespace opengl { gl.Disable(GL_POLYGON_OFFSET_FILL); } - for (ColorAttachmentIndex attachmentSlot : IterateBitSet(GetColorAttachmentsMask())) { - ApplyColorState(gl, attachmentSlot, GetColorStateDescriptor(attachmentSlot)); + if (!GetDevice()->IsToggleEnabled(Toggle::DisableIndexedDrawBuffers)) { + for (ColorAttachmentIndex attachmentSlot : IterateBitSet(GetColorAttachmentsMask())) { + ApplyColorState(gl, attachmentSlot, GetColorStateDescriptor(attachmentSlot)); + } + } else { + const ColorStateDescriptor* prevDescriptor = nullptr; + for (ColorAttachmentIndex attachmentSlot : IterateBitSet(GetColorAttachmentsMask())) { + const ColorStateDescriptor* descriptor = GetColorStateDescriptor(attachmentSlot); + if (!prevDescriptor) { + ApplyColorState(gl, descriptor); + prevDescriptor = descriptor; + } else if (!Equal(descriptor->alphaBlend, prevDescriptor->alphaBlend) || + !Equal(descriptor->colorBlend, prevDescriptor->colorBlend) || + descriptor->writeMask != prevDescriptor->writeMask) { + // TODO(crbug.com/dawn/582): Add validation to prevent this as it is not + // supported on GLES < 3.2. + ASSERT(false); + } + } } } diff --git a/src/tests/end2end/ColorStateTests.cpp b/src/tests/end2end/ColorStateTests.cpp index 766015d4e1..aee4145f6b 100644 --- a/src/tests/end2end/ColorStateTests.cpp +++ b/src/tests/end2end/ColorStateTests.cpp @@ -747,6 +747,8 @@ TEST_P(ColorStateTest, ColorWriteMaskBlendingDisabled) { TEST_P(ColorStateTest, IndependentColorState) { DAWN_SKIP_TEST_IF(IsWindows() && IsVulkan() && IsIntel()); + DAWN_SKIP_TEST_IF(HasToggleEnabled("disable_indexed_draw_buffers")); + std::array renderTargets; std::array renderTargetViews;