Commit 8413faba by shrekshao Committed by Commit Bot

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/+/1553963Reviewed-by: 's avatarJamie Madill <jmadill@chromium.org> Reviewed-by: 's avatarGeoff Lang <geofflang@chromium.org> Commit-Queue: Shrek Shao <shrekshao@google.com>
parent bfc5df16
...@@ -1839,10 +1839,12 @@ void Framebuffer::setAttachmentImpl(const Context *context, ...@@ -1839,10 +1839,12 @@ void Framebuffer::setAttachmentImpl(const Context *context,
if (!resource) if (!resource)
{ {
mColorAttachmentBits.reset(colorIndex);
mFloat32ColorAttachmentBits.reset(colorIndex); mFloat32ColorAttachmentBits.reset(colorIndex);
} }
else else
{ {
mColorAttachmentBits.set(colorIndex);
updateFloat32ColorAttachmentBits( updateFloat32ColorAttachmentBits(
colorIndex, resource->getAttachmentFormat(binding, textureIndex).info); colorIndex, resource->getAttachmentFormat(binding, textureIndex).info);
} }
...@@ -1944,8 +1946,9 @@ FramebufferAttachment *Framebuffer::getAttachmentFromSubjectIndex(angle::Subject ...@@ -1944,8 +1946,9 @@ FramebufferAttachment *Framebuffer::getAttachmentFromSubjectIndex(angle::Subject
} }
} }
bool Framebuffer::formsRenderingFeedbackLoopWith(const State &state) const bool Framebuffer::formsRenderingFeedbackLoopWith(const Context *context) const
{ {
const State &state = context->getState();
const Program *program = state.getProgram(); const Program *program = state.getProgram();
// TODO(jmadill): Default framebuffer feedback loops. // TODO(jmadill): Default framebuffer feedback loops.
...@@ -1954,39 +1957,53 @@ bool Framebuffer::formsRenderingFeedbackLoopWith(const State &state) const ...@@ -1954,39 +1957,53 @@ bool Framebuffer::formsRenderingFeedbackLoopWith(const State &state) const
return false; return false;
} }
// The bitset will skip inactive draw buffers. const FramebufferAttachment *depth = getDepthbuffer();
for (size_t drawIndex : mState.mEnabledDrawBuffers) 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]; Texture *texture = textures[textureUnit];
ASSERT(attachment.isAttached());
if (attachment.type() == GL_TEXTURE) if (texture == nullptr)
{
continue;
}
// Depth and stencil attachment form feedback loops
// Regardless of if enabled or masked.
if (checkDepth)
{ {
// Validate the feedback loop. if (texture->id() == depth->id())
if (program->samplesFromTexture(state, attachment.id()))
{ {
return true; return true;
} }
} }
}
// Validate depth-stencil feedback loop. This is independent of Depth/Stencil state. if (checkStencil)
const FramebufferAttachment *depth = getDepthbuffer();
if (depth && depth->type() == GL_TEXTURE)
{
if (program->samplesFromTexture(state, depth->id()))
{ {
return true; if (texture->id() == stencil->id())
{
return true;
}
} }
}
const FramebufferAttachment *stencil = getStencilbuffer(); // Check if any color attachment forms a feedback loop.
if (stencil && stencil->type() == GL_TEXTURE) for (size_t drawIndex : mColorAttachmentBits)
{
// Skip the feedback loop check if depth/stencil point to the same resource.
if (!depth || *stencil != *depth)
{ {
if (program->samplesFromTexture(state, stencil->id())) const FramebufferAttachment &attachment = mState.mColorAttachments[drawIndex];
ASSERT(attachment.isAttached());
if (attachment.isTextureWithId(texture->id()))
{ {
// TODO(jmadill): Check for appropriate overlap.
return true; return true;
} }
} }
......
...@@ -351,7 +351,7 @@ class Framebuffer final : public angle::ObserverInterface, ...@@ -351,7 +351,7 @@ class Framebuffer final : public angle::ObserverInterface,
angle::SubjectIndex index, angle::SubjectIndex index,
angle::SubjectMessage message) override; angle::SubjectMessage message) override;
bool formsRenderingFeedbackLoopWith(const State &state) const; bool formsRenderingFeedbackLoopWith(const Context *context) const;
bool formsCopyingFeedbackLoopWith(GLuint copyTextureID, bool formsCopyingFeedbackLoopWith(GLuint copyTextureID,
GLint copyTextureLevel, GLint copyTextureLevel,
GLint copyTextureLayer) const; GLint copyTextureLayer) const;
...@@ -439,6 +439,7 @@ class Framebuffer final : public angle::ObserverInterface, ...@@ -439,6 +439,7 @@ class Framebuffer final : public angle::ObserverInterface,
DirtyBits mDirtyBits; DirtyBits mDirtyBits;
DrawBufferMask mFloat32ColorAttachmentBits; DrawBufferMask mFloat32ColorAttachmentBits;
DrawBufferMask mColorAttachmentBits;
// The dirty bits guard is checked when we get a dependent state change message. We verify that // 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. // we don't set a dirty bit that isn't already set, when inside the dirty bits syncState.
......
...@@ -4234,29 +4234,6 @@ void Program::getUniformInternal(const Context *context, ...@@ -4234,29 +4234,6 @@ void Program::getUniformInternal(const Context *context,
} }
} }
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) angle::Result Program::syncState(const Context *context)
{ {
if (mDirtyBits.any()) if (mDirtyBits.any())
......
...@@ -783,7 +783,6 @@ class Program final : angle::NonCopyable, public LabeledObject ...@@ -783,7 +783,6 @@ class Program final : angle::NonCopyable, public LabeledObject
} }
bool isValidated() const; bool isValidated() const;
bool samplesFromTexture(const State &state, GLuint textureID) const;
const AttributesMask &getActiveAttribLocationsMask() const const AttributesMask &getActiveAttribLocationsMask() const
{ {
......
...@@ -2746,7 +2746,7 @@ const char *ValidateDrawStates(Context *context) ...@@ -2746,7 +2746,7 @@ const char *ValidateDrawStates(Context *context)
} }
// Detect rendering feedback loops for WebGL. // Detect rendering feedback loops for WebGL.
if (framebuffer->formsRenderingFeedbackLoopWith(state)) if (framebuffer->formsRenderingFeedbackLoopWith(context))
{ {
return kFeedbackLoop; return kFeedbackLoop;
} }
......
...@@ -2441,7 +2441,9 @@ void main() { ...@@ -2441,7 +2441,9 @@ void main() {
GL_INVALID_OPERATION); GL_INVALID_OPERATION);
drawBuffersEXTFeedbackLoop(program.get(), {{GL_COLOR_ATTACHMENT0, GL_COLOR_ATTACHMENT1}}, drawBuffersEXTFeedbackLoop(program.get(), {{GL_COLOR_ATTACHMENT0, GL_COLOR_ATTACHMENT1}},
GL_INVALID_OPERATION); 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. // Test tests that texture copying feedback loops are properly rejected in WebGL.
...@@ -3541,7 +3543,8 @@ void main() { ...@@ -3541,7 +3543,8 @@ void main() {
drawBuffersFeedbackLoop(program.get(), {{GL_NONE, GL_COLOR_ATTACHMENT1}}, GL_INVALID_OPERATION); drawBuffersFeedbackLoop(program.get(), {{GL_NONE, GL_COLOR_ATTACHMENT1}}, GL_INVALID_OPERATION);
drawBuffersFeedbackLoop(program.get(), {{GL_COLOR_ATTACHMENT0, GL_COLOR_ATTACHMENT1}}, drawBuffersFeedbackLoop(program.get(), {{GL_COLOR_ATTACHMENT0, GL_COLOR_ATTACHMENT1}},
GL_INVALID_OPERATION); 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. // This test covers detection of rendering feedback loops between the FBO and a depth Texture.
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment