Fix error report when active color buffer has no fs output
Also modify or remove some tests to sync up with the expected behavior
stated in spec.
Related to https://github.com/KhronosGroup/WebGL/pull/2780
If any draw buffer with an attachment does not have a defined fragment shader output,
draws generate INVALID_OPERATION.
Also remove Framebuffer masking for inactive outputs.
This workaround is no longer necessary as the WebGL spec has changed.
It also was never fully working and had bugs with certain orders of
calls.
Bug: angleproject:2872
Bug: chromium:927908
Bug: chromium:943538
Change-Id: I73715a6ab851ae3db7096f49ea0a9fdd6f576703
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/1530018
Commit-Queue: Shrek Shao <shrekshao@google.com>
Reviewed-by: Jonah Ryan-Davis <jonahr@google.com>
diff --git a/src/libANGLE/ErrorStrings.h b/src/libANGLE/ErrorStrings.h
index 3e681ce..a2322c0 100644
--- a/src/libANGLE/ErrorStrings.h
+++ b/src/libANGLE/ErrorStrings.h
@@ -84,6 +84,7 @@
MSG kDestinationTextureTooSmall = "Destination texture too small.";
MSG kDimensionsMustBePow2 = "Texture dimensions must be power-of-two.";
MSG kDispatchIndirectBufferNotBound = "Dispatch indirect buffer must be bound.";
+MSG kDrawBufferMaskMismatch = "Active draw buffers with missing fragment shader outputs.";
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.";
diff --git a/src/libANGLE/renderer/gl/ContextGL.cpp b/src/libANGLE/renderer/gl/ContextGL.cpp
index bb3b4aa..3e0dbcb 100644
--- a/src/libANGLE/renderer/gl/ContextGL.cpp
+++ b/src/libANGLE/renderer/gl/ContextGL.cpp
@@ -200,13 +200,6 @@
count, instanceCount));
}
- if (context->getExtensions().webglCompatibility)
- {
- const gl::State &glState = context->getState();
- FramebufferGL *framebufferGL = GetImplAs<FramebufferGL>(glState.getDrawFramebuffer());
- framebufferGL->maskOutInactiveOutputDrawBuffers(context);
- }
-
return angle::Result::Continue;
}
@@ -236,24 +229,6 @@
*outIndices = indices;
}
- if (context->getExtensions().webglCompatibility)
- {
- FramebufferGL *framebufferGL = GetImplAs<FramebufferGL>(glState.getDrawFramebuffer());
- framebufferGL->maskOutInactiveOutputDrawBuffers(context);
- }
-
- return angle::Result::Continue;
-}
-
-ANGLE_INLINE angle::Result ContextGL::setDrawIndirectState(const gl::Context *context)
-{
- if (context->getExtensions().webglCompatibility)
- {
- const gl::State &glState = context->getState();
- FramebufferGL *framebufferGL = GetImplAs<FramebufferGL>(glState.getDrawFramebuffer());
- framebufferGL->maskOutInactiveOutputDrawBuffers(context);
- }
-
return angle::Result::Continue;
}
@@ -375,7 +350,6 @@
gl::PrimitiveMode mode,
const void *indirect)
{
- ANGLE_TRY(setDrawIndirectState(context));
getFunctions()->drawArraysIndirect(ToGLenum(mode), indirect);
return angle::Result::Continue;
}
@@ -385,7 +359,6 @@
gl::DrawElementsType type,
const void *indirect)
{
- ANGLE_TRY(setDrawIndirectState(context));
getFunctions()->drawElementsIndirect(ToGLenum(mode), ToGLenum(type), indirect);
return angle::Result::Continue;
}
diff --git a/src/libANGLE/renderer/gl/ContextGL.h b/src/libANGLE/renderer/gl/ContextGL.h
index eebc5e2..0b08b3c 100644
--- a/src/libANGLE/renderer/gl/ContextGL.h
+++ b/src/libANGLE/renderer/gl/ContextGL.h
@@ -231,8 +231,6 @@
GLsizei instanceCount,
const void **outIndices);
- angle::Result setDrawIndirectState(const gl::Context *context);
-
protected:
std::shared_ptr<RendererGL> mRenderer;
};
diff --git a/src/libANGLE/renderer/gl/FramebufferGL.cpp b/src/libANGLE/renderer/gl/FramebufferGL.cpp
index e1db56a..0185f21 100644
--- a/src/libANGLE/renderer/gl/FramebufferGL.cpp
+++ b/src/libANGLE/renderer/gl/FramebufferGL.cpp
@@ -726,30 +726,6 @@
return mIsDefault;
}
-void FramebufferGL::maskOutInactiveOutputDrawBuffersImpl(const gl::Context *context,
- DrawBufferMask targetAppliedDrawBuffers)
-{
-
- ASSERT(mAppliedEnabledDrawBuffers != targetAppliedDrawBuffers);
- mAppliedEnabledDrawBuffers = targetAppliedDrawBuffers;
-
- const auto &stateDrawBuffers = mState.getDrawBufferStates();
- GLsizei drawBufferCount = static_cast<GLsizei>(stateDrawBuffers.size());
- ASSERT(drawBufferCount <= IMPLEMENTATION_MAX_DRAW_BUFFERS);
-
- GLenum drawBuffers[IMPLEMENTATION_MAX_DRAW_BUFFERS];
- for (GLenum i = 0; static_cast<int>(i) < drawBufferCount; ++i)
- {
- drawBuffers[i] = targetAppliedDrawBuffers[i] ? stateDrawBuffers[i] : GL_NONE;
- }
-
- const FunctionsGL *functions = GetFunctionsGL(context);
- StateManagerGL *stateManager = GetStateManagerGL(context);
-
- ASSERT(stateManager->getFramebufferID(angle::FramebufferBindingDraw) == mFramebufferID);
- functions->drawBuffers(drawBufferCount, drawBuffers);
-}
-
void FramebufferGL::syncClearState(const gl::Context *context, GLbitfield mask)
{
const FunctionsGL *functions = GetFunctionsGL(context);
diff --git a/src/libANGLE/renderer/gl/FramebufferGL.h b/src/libANGLE/renderer/gl/FramebufferGL.h
index 6b1017d..1ae9885 100644
--- a/src/libANGLE/renderer/gl/FramebufferGL.h
+++ b/src/libANGLE/renderer/gl/FramebufferGL.h
@@ -86,20 +86,6 @@
GLuint getFramebufferID() const;
bool isDefault() const;
- ANGLE_INLINE void maskOutInactiveOutputDrawBuffers(const gl::Context *context)
- {
- ASSERT(context->getExtensions().webglCompatibility);
-
- const gl::DrawBufferMask &maxSet =
- context->getState().getProgram()->getActiveOutputVariables();
-
- gl::DrawBufferMask targetAppliedDrawBuffers = mState.getEnabledDrawBuffers() & maxSet;
- if (mAppliedEnabledDrawBuffers != targetAppliedDrawBuffers)
- {
- maskOutInactiveOutputDrawBuffersImpl(context, targetAppliedDrawBuffers);
- }
- }
-
private:
void syncClearState(const gl::Context *context, GLbitfield mask);
void syncClearBufferState(const gl::Context *context, GLenum buffer, GLint drawBuffer);
diff --git a/src/libANGLE/validationES.cpp b/src/libANGLE/validationES.cpp
index 68749a9..54c54a8 100644
--- a/src/libANGLE/validationES.cpp
+++ b/src/libANGLE/validationES.cpp
@@ -362,6 +362,17 @@
return true;
}
+bool ValidateFragmentShaderColorBufferMaskMatch(Context *context)
+{
+ const Program *program = context->getState().getLinkedProgram(context);
+ const Framebuffer *framebuffer = context->getState().getDrawFramebuffer();
+
+ auto drawBufferMask = framebuffer->getDrawBufferMask().to_ulong();
+ auto fragmentOutputMask = program->getActiveOutputVariables().to_ulong();
+
+ return drawBufferMask == (drawBufferMask & fragmentOutputMask);
+}
+
bool ValidateFragmentShaderColorBufferTypeMatch(Context *context)
{
const Program *program = context->getState().getLinkedProgram(context);
@@ -2767,6 +2778,12 @@
return kVertexShaderTypeMismatch;
}
+ // Detect that if there's active color buffer without fragment shader output
+ if (!ValidateFragmentShaderColorBufferMaskMatch(context))
+ {
+ return kDrawBufferMaskMismatch;
+ }
+
// Detect that the color buffer types match the fragment shader output types
if (!ValidateFragmentShaderColorBufferTypeMatch(context))
{
diff --git a/src/tests/gl_tests/DrawBuffersTest.cpp b/src/tests/gl_tests/DrawBuffersTest.cpp
index d4b2bb3..7553bbf 100644
--- a/src/tests/gl_tests/DrawBuffersTest.cpp
+++ b/src/tests/gl_tests/DrawBuffersTest.cpp
@@ -357,53 +357,6 @@
EXPECT_EQ(GL_NONE, drawbuffer);
}
-// Tests masking out some of the draw buffers by not writing to them in the program.
-TEST_P(DrawBuffersWebGL2Test, SomeProgramOutputsDisabled)
-{
- ANGLE_SKIP_TEST_IF(!setupTest());
-
- // TODO(ynovikov): Investigate the failure (https://anglebug.com/1533)
- ANGLE_SKIP_TEST_IF(IsWindows() && IsAMD() && IsDesktopOpenGL());
-
- bool flags[8] = {false};
- GLenum bufs[4] = {GL_NONE};
-
- constexpr GLuint kMaxBuffers = 4;
- constexpr GLuint kHalfMaxBuffers = 2;
-
- // Enable all draw buffers.
- for (GLuint texIndex = 0; texIndex < kMaxBuffers; texIndex++)
- {
- glBindTexture(GL_TEXTURE_2D, mTextures[texIndex]);
- glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0 + texIndex, GL_TEXTURE_2D,
- mTextures[texIndex], 0);
- bufs[texIndex] = GL_COLOR_ATTACHMENT0 + texIndex;
-
- // Mask out the first two buffers.
- flags[texIndex] = texIndex >= kHalfMaxBuffers;
- }
-
- GLuint program;
- setupMRTProgram(flags, &program);
-
- setDrawBuffers(kMaxBuffers, bufs);
- drawQuad(program, positionAttrib(), 0.5, 1.0f, true);
-
- for (GLuint texIndex = 0; texIndex < kHalfMaxBuffers; texIndex++)
- {
- verifyAttachment2DUnwritten(texIndex, mTextures[texIndex], GL_TEXTURE_2D, 0);
- }
-
- for (GLuint texIndex = kHalfMaxBuffers; texIndex < kMaxBuffers; texIndex++)
- {
- verifyAttachment2D(texIndex, mTextures[texIndex], GL_TEXTURE_2D, 0);
- }
-
- EXPECT_GL_NO_ERROR();
-
- glDeleteProgram(program);
-}
-
// Same as above but adds a state change from a program with different masks after a clear.
TEST_P(DrawBuffersWebGL2Test, TwoProgramsWithDifferentOutputsAndClear)
{
@@ -463,7 +416,7 @@
verifyAttachment2DColor(3, mTextures[3], GL_TEXTURE_2D, 0, GLColor::green);
// Draw with MRT program.
- setDrawBuffers(kMaxBuffers, allBufs);
+ setDrawBuffers(kMaxBuffers, someBufs);
drawQuad(program, positionAttrib(), 0.5, 1.0f, true);
ASSERT_GL_NO_ERROR();
@@ -473,6 +426,11 @@
verifyAttachment2D(2, mTextures[2], GL_TEXTURE_2D, 0);
verifyAttachment2D(3, mTextures[3], GL_TEXTURE_2D, 0);
+ // Active draw buffers with no fragment output is not allowed.
+ setDrawBuffers(kMaxBuffers, allBufs);
+ drawQuad(program, positionAttrib(), 0.5, 1.0f, true);
+ ASSERT_GL_ERROR(GL_INVALID_OPERATION);
+
// Clear again. All attachments should be cleared.
glClear(GL_COLOR_BUFFER_BIT);
verifyAttachment2DColor(0, mTextures[0], GL_TEXTURE_2D, 0, GLColor::green);
diff --git a/src/tests/gl_tests/MultiviewDrawTest.cpp b/src/tests/gl_tests/MultiviewDrawTest.cpp
index 3a71ea9..27026a1 100644
--- a/src/tests/gl_tests/MultiviewDrawTest.cpp
+++ b/src/tests/gl_tests/MultiviewDrawTest.cpp
@@ -462,8 +462,9 @@
"#version 300 es\n"
"#extension GL_OVR_multiview2 : require\n"
"precision mediump float;\n"
+ "out vec4 color;\n"
"void main()\n"
- "{}\n";
+ "{color = vec4(1);}\n";
GLVertexArray vao;
GLBuffer vertexBuffer;
@@ -544,8 +545,9 @@
"#version 300 es\n"
"#extension GL_OVR_multiview2 : require\n"
"precision mediump float;\n"
+ "out vec4 color;\n"
"void main()\n"
- "{}\n";
+ "{color = vec4(1);}\n";
ANGLE_GL_PROGRAM(program, kVS, kFS);
glUseProgram(program);
@@ -684,6 +686,9 @@
GLTexture tex2DArray;
initOnePixelColorTexture2DMultiLayered(tex2DArray);
+ GLenum bufs[] = {GL_NONE};
+ glDrawBuffers(1, bufs);
+
// Check that drawArrays generates an error when there is an active transform feedback object
// and the number of views in the draw framebuffer is greater than 1.
{
@@ -777,6 +782,9 @@
GLTexture tex2DArr;
initOnePixelColorTexture2DMultiLayered(tex2DArr);
+ GLenum bufs[] = {GL_NONE};
+ glDrawBuffers(1, bufs);
+
// Check first case.
{
glUseProgram(dualViewProgram);
diff --git a/src/tests/gl_tests/WebGLCompatibilityTest.cpp b/src/tests/gl_tests/WebGLCompatibilityTest.cpp
index adcf182..5f31c5c 100644
--- a/src/tests/gl_tests/WebGLCompatibilityTest.cpp
+++ b/src/tests/gl_tests/WebGLCompatibilityTest.cpp
@@ -4065,7 +4065,7 @@
EXPECT_GL_ERROR(GL_INVALID_ENUM);
}
-// Tests the WebGL removal of undefined behavior when attachments aren't written to.
+// Tests WebGL reports INVALID_OPERATION for mismatch of drawbuffers and fragment output
TEST_P(WebGLCompatibilityTest, DrawBuffers)
{
// Make sure we can use at least 4 attachments for the tests.
@@ -4152,12 +4152,12 @@
GLenum halfDrawBuffers[] = {
GL_NONE,
+ GL_COLOR_ATTACHMENT1,
GL_NONE,
- GL_COLOR_ATTACHMENT2,
GL_COLOR_ATTACHMENT3,
};
- // Test that when using gl_FragColor, only the first attachment is written to.
+ // Test that when using gl_FragColor with no-array
const char *fragESSL1 =
R"(precision highp float;
void main()
@@ -4167,28 +4167,10 @@
ANGLE_GL_PROGRAM(programESSL1, essl1_shaders::vs::Simple(), fragESSL1);
{
- ClearEverythingToRed(renderbuffers);
-
glBindFramebuffer(GL_FRAMEBUFFER, drawFBO);
DrawBuffers(useEXT, 4, allDrawBuffers);
drawQuad(programESSL1, essl1_shaders::PositionAttrib(), 0.5, 1.0, true);
- ASSERT_GL_NO_ERROR();
-
- CheckColors(renderbuffers, 0b0001, GLColor::green);
- CheckColors(renderbuffers, 0b1110, GLColor::red);
- }
-
- // Test that when using gl_FragColor, but the first draw buffer is 0, then no attachment is
- // written to.
- {
- ClearEverythingToRed(renderbuffers);
-
- glBindFramebuffer(GL_FRAMEBUFFER, drawFBO);
- DrawBuffers(useEXT, 4, halfDrawBuffers);
- drawQuad(programESSL1, essl1_shaders::PositionAttrib(), 0.5, 1.0, true);
- ASSERT_GL_NO_ERROR();
-
- CheckColors(renderbuffers, 0b1111, GLColor::red);
+ EXPECT_GL_ERROR(GL_INVALID_OPERATION);
}
// Test what happens when rendering to a subset of the outputs. There is a behavior difference
@@ -4199,11 +4181,8 @@
const char *positionAttrib;
const char *writeOddOutputsVert;
const char *writeOddOutputsFrag;
- GLColor unwrittenColor;
if (useEXT)
{
- // In the extension, when an attachment isn't written to, it should get 0's
- unwrittenColor = GLColor(0, 0, 0, 0);
positionAttrib = essl1_shaders::PositionAttrib();
writeOddOutputsVert = essl1_shaders::vs::Simple();
writeOddOutputsFrag =
@@ -4217,9 +4196,6 @@
}
else
{
- // In ES3 if an attachment isn't declared, it shouldn't get written and should be red
- // because of the preceding clears.
- unwrittenColor = GLColor::red;
positionAttrib = essl3_shaders::PositionAttrib();
writeOddOutputsVert = essl3_shaders::vs::Simple();
writeOddOutputsFrag =
@@ -4235,21 +4211,30 @@
}
ANGLE_GL_PROGRAM(writeOddOutputsProgram, writeOddOutputsVert, writeOddOutputsFrag);
- // Test that attachments not written to get the "unwritten" color
+ // Test that attachments not written to get the "unwritten" color (useEXT)
+ // Or INVALID_OPERATION is generated if there's active draw buffer receive no output
{
ClearEverythingToRed(renderbuffers);
glBindFramebuffer(GL_FRAMEBUFFER, drawFBO);
DrawBuffers(useEXT, 4, allDrawBuffers);
drawQuad(writeOddOutputsProgram, positionAttrib, 0.5, 1.0, true);
- ASSERT_GL_NO_ERROR();
- CheckColors(renderbuffers, 0b1010, GLColor::green);
- CheckColors(renderbuffers, 0b0101, unwrittenColor);
+ if (useEXT)
+ {
+ ASSERT_GL_NO_ERROR();
+ CheckColors(renderbuffers, 0b1010, GLColor::green);
+ // In the extension, when an attachment isn't written to, it should get 0's
+ CheckColors(renderbuffers, 0b0101, GLColor(0, 0, 0, 0));
+ }
+ else
+ {
+ EXPECT_GL_ERROR(GL_INVALID_OPERATION);
+ }
}
- // Test that attachments not written to get the "unwritten" color but that even when the
- // extension is used, disabled attachments are not written at all and stay red.
+ // Test that attachments written to get the correct color from shader output but that even when
+ // the extension is used, disabled attachments are not written at all and stay red.
{
ClearEverythingToRed(renderbuffers);
@@ -4258,9 +4243,8 @@
drawQuad(writeOddOutputsProgram, positionAttrib, 0.5, 1.0, true);
ASSERT_GL_NO_ERROR();
- CheckColors(renderbuffers, 0b1000, GLColor::green);
- CheckColors(renderbuffers, 0b0100, unwrittenColor);
- CheckColors(renderbuffers, 0b0011, GLColor::red);
+ CheckColors(renderbuffers, 0b1010, GLColor::green);
+ CheckColors(renderbuffers, 0b0101, GLColor::red);
}
}