Fix formsRenderingFeedbackLoopWith check
To make it pass the following webgl conformance test
https://github.com/KhronosGroup/WebGL/blob/master/sdk/tests/conformance/rendering/rendering-sampling-feedback-loop.html
It used to fail due to
1. Didn't check if texture unit is sampler complete
2. Only checked active drawbuffers. But drawbuffer settings shouldn't be
taken into account when checking drawing feedback loop.
On top of applying these 2 functional fixes, I also tried to do some
optimization by unwrapping the nested for loop for program sampler
bindings and texture unit in `Program::samplesFromTexture` and putting
them outside of the draw buffer loop according to the old comment by
Antonie @piman.
https://codereview.chromium.org/2461973002/
> ... turning it around (for each texture check if it's
also an attachment, instead of for each attachment check if it's a bound
texture). In particular, we have an upper bound on the number of
attachments, so we can look them up outside the loop into a fixed size
buffer on the stack - and the very common case will be to only have 1 of
them making the inner loop cheap.
But this unwraps sort of breaks the code structure. An alternative way
would be passed in a framebuffer pointer into `Program::samplesFromTexture`
but that would ends up in tight class coupling. I am a bit unsure here.
Would like to hear if think this change is okay in terms of code style.
In addition to further speed up this check (as it runs for every draw
validation) I added a cache mLastColorAttachmentId indicating the last i
of GL_COLORATTACHMENTi that is not GL_NONE to shorten this inner loop.
In most scenario we won't have up to max number of color attachments.
A side note: this is still failing
https://github.com/KhronosGroup/WebGL/blob/master/sdk/tests/conformance2/rendering/depth-stencil-feedback-loop.html
But it's because the test case doesn't fit the spec at this moment.
I will update this test later.
Bug: chromium:660844
Change-Id: I6d718dada71a5d989caac04de03f2454f2377612
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/1553963
Reviewed-by: Jamie Madill <jmadill@chromium.org>
Reviewed-by: Geoff Lang <geofflang@chromium.org>
Commit-Queue: Shrek Shao <shrekshao@google.com>
diff --git a/src/libANGLE/Framebuffer.cpp b/src/libANGLE/Framebuffer.cpp
index 2520684..43045f7 100644
--- a/src/libANGLE/Framebuffer.cpp
+++ b/src/libANGLE/Framebuffer.cpp
@@ -1839,10 +1839,12 @@
if (!resource)
{
+ mColorAttachmentBits.reset(colorIndex);
mFloat32ColorAttachmentBits.reset(colorIndex);
}
else
{
+ mColorAttachmentBits.set(colorIndex);
updateFloat32ColorAttachmentBits(
colorIndex, resource->getAttachmentFormat(binding, textureIndex).info);
}
@@ -1944,8 +1946,9 @@
}
}
-bool Framebuffer::formsRenderingFeedbackLoopWith(const State &state) const
+bool Framebuffer::formsRenderingFeedbackLoopWith(const Context *context) const
{
+ const State &state = context->getState();
const Program *program = state.getProgram();
// TODO(jmadill): Default framebuffer feedback loops.
@@ -1954,42 +1957,56 @@
return false;
}
- // The bitset will skip inactive draw buffers.
- for (size_t drawIndex : mState.mEnabledDrawBuffers)
+ const FramebufferAttachment *depth = getDepthbuffer();
+ const FramebufferAttachment *stencil = getStencilbuffer();
+
+ const bool checkDepth = depth && depth->type() == GL_TEXTURE;
+ // Skip the feedback loop check for stencil if depth/stencil point to the same resource.
+ const bool checkStencil =
+ (stencil && stencil->type() == GL_TEXTURE) && (!depth || *stencil != *depth);
+
+ const gl::ActiveTextureMask &activeTextures = program->getActiveSamplersMask();
+ const gl::ActiveTexturePointerArray &textures = state.getActiveTexturesCache();
+
+ for (size_t textureUnit : activeTextures)
{
- const FramebufferAttachment &attachment = mState.mColorAttachments[drawIndex];
- ASSERT(attachment.isAttached());
- if (attachment.type() == GL_TEXTURE)
+ Texture *texture = textures[textureUnit];
+
+ if (texture == nullptr)
{
- // Validate the feedback loop.
- if (program->samplesFromTexture(state, attachment.id()))
+ continue;
+ }
+
+ // Depth and stencil attachment form feedback loops
+ // Regardless of if enabled or masked.
+ if (checkDepth)
+ {
+ if (texture->id() == depth->id())
{
return true;
}
}
- }
- // Validate depth-stencil feedback loop. This is independent of Depth/Stencil state.
- const FramebufferAttachment *depth = getDepthbuffer();
- if (depth && depth->type() == GL_TEXTURE)
- {
- if (program->samplesFromTexture(state, depth->id()))
+ if (checkStencil)
{
- return true;
- }
- }
-
- const FramebufferAttachment *stencil = getStencilbuffer();
- if (stencil && stencil->type() == GL_TEXTURE)
- {
- // Skip the feedback loop check if depth/stencil point to the same resource.
- if (!depth || *stencil != *depth)
- {
- if (program->samplesFromTexture(state, stencil->id()))
+ if (texture->id() == stencil->id())
{
return true;
}
}
+
+ // Check if any color attachment forms a feedback loop.
+ for (size_t drawIndex : mColorAttachmentBits)
+ {
+ const FramebufferAttachment &attachment = mState.mColorAttachments[drawIndex];
+ ASSERT(attachment.isAttached());
+
+ if (attachment.isTextureWithId(texture->id()))
+ {
+ // TODO(jmadill): Check for appropriate overlap.
+ return true;
+ }
+ }
}
return false;
diff --git a/src/libANGLE/Framebuffer.h b/src/libANGLE/Framebuffer.h
index 219384b..df5d8d4 100644
--- a/src/libANGLE/Framebuffer.h
+++ b/src/libANGLE/Framebuffer.h
@@ -351,7 +351,7 @@
angle::SubjectIndex index,
angle::SubjectMessage message) override;
- bool formsRenderingFeedbackLoopWith(const State &state) const;
+ bool formsRenderingFeedbackLoopWith(const Context *context) const;
bool formsCopyingFeedbackLoopWith(GLuint copyTextureID,
GLint copyTextureLevel,
GLint copyTextureLayer) const;
@@ -439,6 +439,7 @@
DirtyBits mDirtyBits;
DrawBufferMask mFloat32ColorAttachmentBits;
+ DrawBufferMask mColorAttachmentBits;
// The dirty bits guard is checked when we get a dependent state change message. We verify that
// we don't set a dirty bit that isn't already set, when inside the dirty bits syncState.
diff --git a/src/libANGLE/Program.cpp b/src/libANGLE/Program.cpp
index 514c375..c675165 100644
--- a/src/libANGLE/Program.cpp
+++ b/src/libANGLE/Program.cpp
@@ -4234,29 +4234,6 @@
}
}
-bool Program::samplesFromTexture(const gl::State &state, GLuint textureID) const
-{
- ASSERT(mLinkResolved);
- // Must be called after samplers are validated.
- ASSERT(mCachedValidateSamplersResult.valid() && mCachedValidateSamplersResult.value());
-
- for (const auto &binding : mState.mSamplerBindings)
- {
- TextureType textureType = binding.textureType;
- for (const auto &unit : binding.boundTextureUnits)
- {
- GLenum programTextureID = state.getSamplerTextureId(unit, textureType);
- if (programTextureID == textureID)
- {
- // TODO(jmadill): Check for appropriate overlap.
- return true;
- }
- }
- }
-
- return false;
-}
-
angle::Result Program::syncState(const Context *context)
{
if (mDirtyBits.any())
diff --git a/src/libANGLE/Program.h b/src/libANGLE/Program.h
index 79bcb21..80d25fe 100644
--- a/src/libANGLE/Program.h
+++ b/src/libANGLE/Program.h
@@ -783,7 +783,6 @@
}
bool isValidated() const;
- bool samplesFromTexture(const State &state, GLuint textureID) const;
const AttributesMask &getActiveAttribLocationsMask() const
{
diff --git a/src/libANGLE/validationES.cpp b/src/libANGLE/validationES.cpp
index 80e743a..4e9a85b 100644
--- a/src/libANGLE/validationES.cpp
+++ b/src/libANGLE/validationES.cpp
@@ -2746,7 +2746,7 @@
}
// Detect rendering feedback loops for WebGL.
- if (framebuffer->formsRenderingFeedbackLoopWith(state))
+ if (framebuffer->formsRenderingFeedbackLoopWith(context))
{
return kFeedbackLoop;
}
diff --git a/src/tests/gl_tests/WebGLCompatibilityTest.cpp b/src/tests/gl_tests/WebGLCompatibilityTest.cpp
index e401ed1..adcf182 100644
--- a/src/tests/gl_tests/WebGLCompatibilityTest.cpp
+++ b/src/tests/gl_tests/WebGLCompatibilityTest.cpp
@@ -2441,7 +2441,9 @@
GL_INVALID_OPERATION);
drawBuffersEXTFeedbackLoop(program.get(), {{GL_COLOR_ATTACHMENT0, GL_COLOR_ATTACHMENT1}},
GL_INVALID_OPERATION);
- drawBuffersEXTFeedbackLoop(program.get(), {{GL_COLOR_ATTACHMENT0, GL_NONE}}, GL_NO_ERROR);
+ // A feedback loop is formed regardless of drawBuffers settings.
+ drawBuffersEXTFeedbackLoop(program.get(), {{GL_COLOR_ATTACHMENT0, GL_NONE}},
+ GL_INVALID_OPERATION);
}
// Test tests that texture copying feedback loops are properly rejected in WebGL.
@@ -3541,7 +3543,8 @@
drawBuffersFeedbackLoop(program.get(), {{GL_NONE, GL_COLOR_ATTACHMENT1}}, GL_INVALID_OPERATION);
drawBuffersFeedbackLoop(program.get(), {{GL_COLOR_ATTACHMENT0, GL_COLOR_ATTACHMENT1}},
GL_INVALID_OPERATION);
- drawBuffersFeedbackLoop(program.get(), {{GL_COLOR_ATTACHMENT0, GL_NONE}}, GL_NO_ERROR);
+ // A feedback loop is formed regardless of drawBuffers settings.
+ drawBuffersFeedbackLoop(program.get(), {{GL_COLOR_ATTACHMENT0, GL_NONE}}, GL_INVALID_OPERATION);
}
// This test covers detection of rendering feedback loops between the FBO and a depth Texture.
