Re-land "Feedback Loop Redesign 2/3: Track bound FBOs in Texture."

null77 · Commit Bot · commit 185d9d08bc78 · 2020-08-20T01:24:10.000Z
Re-land fixes the crash when drawing with no bound Program executable. Currently we track feedback loops by counting the times a Texture is bound as a sampler or image in a particular context. This is a bit tricky because Texture bindings change frequently. Relative to the number of times we need to check for a feedback loop this causes excess overhead. Usually Framebuffers have a low number of Textures bound (in many cases just 1). And Textures aren't usually bound to many different FBOs. So instead of counting the number of times a Texture is bound as a sampler or image we will track the Framebuffers that the Texture is bound to. This CL adds a small vector class to gl::Texture which tracks all the Framebufer Serials of its bound Framebuffers. We can use this set to quickly check if there's any potential feedback loop between the a FBO and this Texture. We also update the feedback loop check to use this new method. We will be able to remove the old counting method when we switch the Vulkan feedback loop handling to use the new tracking in this CL. Bug: angleproject:4500 Bug: angleproject:4959 Change-Id: If2bd25b08298a99f5e64b4055137f9154b0f0860 Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/2365595 Reviewed-by: Jamie Madill <jmadill@chromium.org> Commit-Queue: Jamie Madill <jmadill@chromium.org>
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
@@ -60,7 +60,7 @@ def _SplitIntoMultipleCommits(description_text):
     def _CheckTabInCommit(lines):
         return all([line.find("\t") == -1 for line in lines])
 
-    whitelist_strings = ['Revert "', 'Roll ', 'Reland ']
+    whitelist_strings = ['Revert "', 'Roll ', 'Reland ', 'Re-land ']
     summary_linelength_warning_lower_limit = 65
     summary_linelength_warning_upper_limit = 70
     description_linelength_limit = 72
diff --git a/src/libANGLE/Framebuffer.cpp b/src/libANGLE/Framebuffer.cpp
@@ -796,19 +796,19 @@ void Framebuffer::onDestroy(const Context *context)
 {
     if (isDefault())
     {
-        mState.mDefaultFramebufferReadAttachment.detach(context);
+        mState.mDefaultFramebufferReadAttachment.detach(context, mState.mFramebufferSerial);
         mState.mDefaultFramebufferReadAttachmentInitialized = false;
     }
 
     for (auto &attachment : mState.mColorAttachments)
     {
-        attachment.detach(context);
+        attachment.detach(context, mState.mFramebufferSerial);
     }
-    mState.mDepthAttachment.detach(context);
-    mState.mStencilAttachment.detach(context);
-    mState.mWebGLDepthAttachment.detach(context);
-    mState.mWebGLStencilAttachment.detach(context);
-    mState.mWebGLDepthStencilAttachment.detach(context);
+    mState.mDepthAttachment.detach(context, mState.mFramebufferSerial);
+    mState.mStencilAttachment.detach(context, mState.mFramebufferSerial);
+    mState.mWebGLDepthAttachment.detach(context, mState.mFramebufferSerial);
+    mState.mWebGLStencilAttachment.detach(context, mState.mFramebufferSerial);
+    mState.mWebGLDepthStencilAttachment.detach(context, mState.mFramebufferSerial);
 
     mImpl->destroy(context);
 }
@@ -819,7 +819,7 @@ void Framebuffer::setReadSurface(const Context *context, egl::Surface *readSurfa
     mState.mDefaultFramebufferReadAttachment.attach(
         context, GL_FRAMEBUFFER_DEFAULT, GL_BACK, ImageIndex(), readSurface,
         FramebufferAttachment::kDefaultNumViews, FramebufferAttachment::kDefaultBaseViewIndex,
-        false, FramebufferAttachment::kDefaultRenderToTextureSamples);
+        false, FramebufferAttachment::kDefaultRenderToTextureSamples, mState.mFramebufferSerial);
     mDirtyBits.set(DIRTY_BIT_READ_BUFFER);
 }
 
@@ -1674,19 +1674,21 @@ void Framebuffer::setAttachment(const Context *context,
     {
         case GL_DEPTH_STENCIL:
         case GL_DEPTH_STENCIL_ATTACHMENT:
-            mState.mWebGLDepthStencilAttachment.attach(context, type, binding, textureIndex,
-                                                       resource, numViews, baseViewIndex,
-                                                       isMultiview, samples);
+            mState.mWebGLDepthStencilAttachment.attach(
+                context, type, binding, textureIndex, resource, numViews, baseViewIndex,
+                isMultiview, samples, mState.mFramebufferSerial);
             break;
         case GL_DEPTH:
         case GL_DEPTH_ATTACHMENT:
             mState.mWebGLDepthAttachment.attach(context, type, binding, textureIndex, resource,
-                                                numViews, baseViewIndex, isMultiview, samples);
+                                                numViews, baseViewIndex, isMultiview, samples,
+                                                mState.mFramebufferSerial);
             break;
         case GL_STENCIL:
         case GL_STENCIL_ATTACHMENT:
             mState.mWebGLStencilAttachment.attach(context, type, binding, textureIndex, resource,
-                                                  numViews, baseViewIndex, isMultiview, samples);
+                                                  numViews, baseViewIndex, isMultiview, samples,
+                                                  mState.mFramebufferSerial);
             break;
         default:
             setAttachmentImpl(context, type, binding, textureIndex, resource, numViews,
@@ -1869,7 +1871,7 @@ void Framebuffer::updateAttachment(const Context *context,
                                    GLsizei samples)
 {
     attachment->attach(context, type, binding, textureIndex, resource, numViews, baseViewIndex,
-                       isMultiview, samples);
+                       isMultiview, samples, mState.mFramebufferSerial);
     mDirtyBits.set(dirtyBit);
     mState.mResourceNeedsInit.set(dirtyBit, attachment->initState() == InitState::MayNeedInit);
     onDirtyBinding->bind(resource);
@@ -1967,6 +1969,34 @@ FramebufferAttachment *Framebuffer::getAttachmentFromSubjectIndex(angle::Subject
     }
 }
 
+bool Framebuffer::formsRenderingFeedbackLoopWith(const Context *context) const
+{
+    const State &glState                = context->getState();
+    const ProgramExecutable *executable = glState.getProgramExecutable();
+
+    // In some error cases there may be no bound program or executable.
+    if (!executable)
+        return false;
+
+    const ActiveTextureMask &activeTextures    = executable->getActiveSamplersMask();
+    const ActiveTextureTypeArray &textureTypes = executable->getActiveSamplerTypes();
+
+    for (size_t textureIndex : activeTextures)
+    {
+        unsigned int uintIndex = static_cast<unsigned int>(textureIndex);
+        Texture *texture       = glState.getSamplerTexture(uintIndex, textureTypes[textureIndex]);
+        const Sampler *sampler = glState.getSampler(uintIndex);
+        if (texture && texture->isSamplerComplete(context, sampler) &&
+            texture->isBoundToFramebuffer(mState.mFramebufferSerial))
+        {
+            // TODO(jmadill): Subresource check. http://anglebug.com/4500
+            return true;
+        }
+    }
+
+    return false;
+}
+
 bool Framebuffer::formsCopyingFeedbackLoopWith(TextureID copyTextureID,
                                                GLint copyTextureLevel,
                                                GLint copyTextureLayer) const
diff --git a/src/libANGLE/Framebuffer.h b/src/libANGLE/Framebuffer.h
@@ -171,6 +171,7 @@ class FramebufferState final : angle::NonCopyable
     bool mWebGLDepthStencilConsistent;
 
     // Tracks rendering feedback loops.
+    // TODO(jmadill): Remove. http://anglebug.com/4500
     DrawBufferMask mDrawBufferFeedbackLoops;
     bool mDepthBufferFeedbackLoop;
     bool mStencilBufferFeedbackLoop;
@@ -409,7 +410,10 @@ class Framebuffer final : public angle::ObserverInterface,
     // Observer implementation
     void onSubjectStateChange(angle::SubjectIndex index, angle::SubjectMessage message) override;
 
+    // TODO(jmadill): Remove. http://anglebug.com/4500
     bool hasRenderingFeedbackLoop() const { return mState.mHasRenderingFeedbackLoop; }
+
+    bool formsRenderingFeedbackLoopWith(const Context *context) const;
     bool formsCopyingFeedbackLoopWith(TextureID copyTextureID,
                                       GLint copyTextureLevel,
                                       GLint copyTextureLayer) const;
diff --git a/src/libANGLE/FramebufferAttachment.cpp b/src/libANGLE/FramebufferAttachment.cpp
@@ -60,11 +60,12 @@ FramebufferAttachment::FramebufferAttachment(const Context *context,
                                              GLenum type,
                                              GLenum binding,
                                              const ImageIndex &textureIndex,
-                                             FramebufferAttachmentObject *resource)
+                                             FramebufferAttachmentObject *resource,
+                                             rx::Serial framebufferSerial)
     : mResource(nullptr)
 {
     attach(context, type, binding, textureIndex, resource, kDefaultNumViews, kDefaultBaseViewIndex,
-           false, kDefaultRenderToTextureSamples);
+           false, kDefaultRenderToTextureSamples, framebufferSerial);
 }
 
 FramebufferAttachment::FramebufferAttachment(FramebufferAttachment &&other)
@@ -90,12 +91,12 @@ FramebufferAttachment::~FramebufferAttachment()
     ASSERT(!isAttached());
 }
 
-void FramebufferAttachment::detach(const Context *context)
+void FramebufferAttachment::detach(const Context *context, rx::Serial framebufferSerial)
 {
     mType = GL_NONE;
     if (mResource != nullptr)
     {
-        mResource->onDetach(context);
+        mResource->onDetach(context, framebufferSerial);
         mResource = nullptr;
     }
     mNumViews      = kDefaultNumViews;
@@ -114,11 +115,12 @@ void FramebufferAttachment::attach(const Context *context,
                                    GLsizei numViews,
                                    GLuint baseViewIndex,
                                    bool isMultiview,
-                                   GLsizei samples)
+                                   GLsizei samples,
+                                   rx::Serial framebufferSerial)
 {
     if (resource == nullptr)
     {
-        detach(context);
+        detach(context, framebufferSerial);
         return;
     }
 
@@ -128,11 +130,11 @@ void FramebufferAttachment::attach(const Context *context,
     mBaseViewIndex          = baseViewIndex;
     mIsMultiview            = isMultiview;
     mRenderToTextureSamples = samples;
-    resource->onAttach(context);
+    resource->onAttach(context, framebufferSerial);
 
     if (mResource != nullptr)
     {
-        mResource->onDetach(context);
+        mResource->onDetach(context, framebufferSerial);
     }
 
     mResource = resource;
diff --git a/src/libANGLE/FramebufferAttachment.h b/src/libANGLE/FramebufferAttachment.h
@@ -64,14 +64,15 @@ class FramebufferAttachment final
                           GLenum type,
                           GLenum binding,
                           const ImageIndex &textureIndex,
-                          FramebufferAttachmentObject *resource);
+                          FramebufferAttachmentObject *resource,
+                          rx::Serial framebufferSerial);
 
     FramebufferAttachment(FramebufferAttachment &&other);
     FramebufferAttachment &operator=(FramebufferAttachment &&other);
 
     ~FramebufferAttachment();
 
-    void detach(const Context *context);
+    void detach(const Context *context, rx::Serial framebufferSerial);
     void attach(const Context *context,
                 GLenum type,
                 GLenum binding,
@@ -80,7 +81,8 @@ class FramebufferAttachment final
                 GLsizei numViews,
                 GLuint baseViewIndex,
                 bool isMultiview,
-                GLsizei samples);
+                GLsizei samples,
+                rx::Serial framebufferSerial);
 
     // Helper methods
     GLuint getRedSize() const;
@@ -208,9 +210,9 @@ class FramebufferAttachmentObject : public angle::Subject, public angle::Observe
                               GLenum binding,
                               const ImageIndex &imageIndex) const                          = 0;
 
-    virtual void onAttach(const Context *context) = 0;
-    virtual void onDetach(const Context *context) = 0;
-    virtual GLuint getId() const                  = 0;
+    virtual void onAttach(const Context *context, rx::Serial framebufferSerial) = 0;
+    virtual void onDetach(const Context *context, rx::Serial framebufferSerial) = 0;
+    virtual GLuint getId() const                                                = 0;
 
     // These are used for robust resource initialization.
     virtual InitState initState(const ImageIndex &imageIndex) const              = 0;
diff --git a/src/libANGLE/Image.cpp b/src/libANGLE/Image.cpp
@@ -193,9 +193,9 @@ bool ExternalImageSibling::isTextureable(const gl::Context *context) const
     return mImplementation->isTexturable(context);
 }
 
-void ExternalImageSibling::onAttach(const gl::Context *context) {}
+void ExternalImageSibling::onAttach(const gl::Context *context, rx::Serial framebufferSerial) {}
 
-void ExternalImageSibling::onDetach(const gl::Context *context) {}
+void ExternalImageSibling::onDetach(const gl::Context *context, rx::Serial framebufferSerial) {}
 
 GLuint ExternalImageSibling::getId() const
 {
diff --git a/src/libANGLE/Image.h b/src/libANGLE/Image.h
@@ -98,8 +98,8 @@ class ExternalImageSibling : public ImageSibling
                       const gl::ImageIndex &imageIndex) const override;
     bool isTextureable(const gl::Context *context) const;
 
-    void onAttach(const gl::Context *context) override;
-    void onDetach(const gl::Context *context) override;
+    void onAttach(const gl::Context *context, rx::Serial framebufferSerial) override;
+    void onDetach(const gl::Context *context, rx::Serial framebufferSerial) override;
     GLuint getId() const override;
 
     gl::InitState initState(const gl::ImageIndex &imageIndex) const override;
diff --git a/src/libANGLE/Renderbuffer.cpp b/src/libANGLE/Renderbuffer.cpp
@@ -234,12 +234,12 @@ GLint Renderbuffer::getMemorySize() const
     return size.ValueOrDefault(std::numeric_limits<GLint>::max());
 }
 
-void Renderbuffer::onAttach(const Context *context)
+void Renderbuffer::onAttach(const Context *context, rx::Serial framebufferSerial)
 {
     addRef();
 }
 
-void Renderbuffer::onDetach(const Context *context)
+void Renderbuffer::onDetach(const Context *context, rx::Serial framebufferSerial)
 {
     release(context);
 }
diff --git a/src/libANGLE/Renderbuffer.h b/src/libANGLE/Renderbuffer.h
@@ -110,8 +110,8 @@ class Renderbuffer final : public RefCountObject<RenderbufferID>,
                       GLenum binding,
                       const ImageIndex &imageIndex) const override;
 
-    void onAttach(const Context *context) override;
-    void onDetach(const Context *context) override;
+    void onAttach(const Context *context, rx::Serial framebufferSerial) override;
+    void onDetach(const Context *context, rx::Serial framebufferSerial) override;
     GLuint getId() const override;
 
     InitState initState(const ImageIndex &imageIndex) const override;
diff --git a/src/libANGLE/Surface.h b/src/libANGLE/Surface.h
@@ -143,8 +143,8 @@ class Surface : public LabeledObject, public gl::FramebufferAttachmentObject
                       GLenum binding,
                       const gl::ImageIndex &imageIndex) const override;
 
-    void onAttach(const gl::Context *context) override {}
-    void onDetach(const gl::Context *context) override {}
+    void onAttach(const gl::Context *context, rx::Serial framebufferSerial) override {}
+    void onDetach(const gl::Context *context, rx::Serial framebufferSerial) override {}
     GLuint getId() const override;
 
     bool flexibleSurfaceCompatibilityRequested() const
diff --git a/src/libANGLE/Texture.cpp b/src/libANGLE/Texture.cpp
@@ -1774,13 +1774,20 @@ GLenum Texture::getGenerateMipmapHint() const
     return mState.getGenerateMipmapHint();
 }
 
-void Texture::onAttach(const Context *context)
+void Texture::onAttach(const Context *context, rx::Serial framebufferSerial)
 {
     addRef();
+
+    // Duplicates allowed for multiple attachment points. See the comment in the header.
+    mBoundFramebufferSerials.push_back(framebufferSerial);
 }
 
-void Texture::onDetach(const Context *context)
+void Texture::onDetach(const Context *context, rx::Serial framebufferSerial)
 {
+    // Erase first instance. If there are multiple bindings, leave the others.
+    ASSERT(isBoundToFramebuffer(framebufferSerial));
+    mBoundFramebufferSerials.remove_and_permute(framebufferSerial);
+
     release(context);
 }
 
diff --git a/src/libANGLE/Texture.h b/src/libANGLE/Texture.h
@@ -146,6 +146,8 @@ class TextureState final : private angle::NonCopyable
     GLenum getUsage() const { return mUsage; }
     GLenum getDepthStencilTextureMode() const { return mDepthStencilTextureMode; }
     bool isStencilMode() const { return mDepthStencilTextureMode == GL_STENCIL_INDEX; }
+
+    // TODO(jmadill): Remove. http://anglebug.com/4500
     bool isBoundAsSamplerTexture(ContextID contextID) const
     {
         return getBindingCount(contextID).samplerBindingCount > 0;
@@ -545,8 +547,8 @@ class Texture final : public RefCountObject<TextureID>,
     void setGenerateMipmapHint(GLenum generate);
     GLenum getGenerateMipmapHint() const;
 
-    void onAttach(const Context *context) override;
-    void onDetach(const Context *context) override;
+    void onAttach(const Context *context, rx::Serial framebufferSerial) override;
+    void onDetach(const Context *context, rx::Serial framebufferSerial) override;
 
     // Used specifically for FramebufferAttachmentObject.
     GLuint getId() const override;
@@ -559,6 +561,17 @@ class Texture final : public RefCountObject<TextureID>,
     InitState initState() const { return mState.mInitState; }
     void setInitState(const ImageIndex &imageIndex, InitState initState) override;
 
+    bool isBoundToFramebuffer(rx::Serial framebufferSerial) const
+    {
+        for (size_t index = 0; index < mBoundFramebufferSerials.size(); ++index)
+        {
+            if (mBoundFramebufferSerials[index] == framebufferSerial)
+                return true;
+        }
+
+        return false;
+    }
+
     enum DirtyBitType
     {
         // Sampler state
@@ -643,6 +656,14 @@ class Texture final : public RefCountObject<TextureID>,
     egl::Surface *mBoundSurface;
     egl::Stream *mBoundStream;
 
+    // We track all the serials of the Framebuffers this texture is attached to. Note that this
+    // allows duplicates because different ranges of a Texture can be bound to the same Framebuffer.
+    // For the purposes of depth-stencil loops, a simple "isBound" check works fine. For color
+    // attachment Feedback Loop checks we then need to check further to see when a Texture is bound
+    // to mulitple bindings that the bindings don't overlap.
+    static constexpr uint32_t kFastFramebufferSerialCount = 8;
+    angle::FastVector<rx::Serial, kFastFramebufferSerialCount> mBoundFramebufferSerials;
+
     struct SamplerCompletenessCache
     {
         SamplerCompletenessCache();
diff --git a/src/libANGLE/renderer/d3d/FramebufferD3D.cpp b/src/libANGLE/renderer/d3d/FramebufferD3D.cpp
diff --git a/src/libANGLE/validationES.cpp b/src/libANGLE/validationES.cpp
diff --git a/src/tests/gl_tests/WebGLCompatibilityTest.cpp b/src/tests/gl_tests/WebGLCompatibilityTest.cpp

Original file line number	Diff line number	Diff line change
`@@ -60,11 +60,12 @@ FramebufferAttachment::FramebufferAttachment(const Context *context,`
`60`	`60`	`GLenum type,`
`61`	`61`	`GLenum binding,`
`62`	`62`	`const ImageIndex &textureIndex,`
`63`		`- FramebufferAttachmentObject *resource)`
	`63`	`+ FramebufferAttachmentObject *resource,`
	`64`	`+ rx::Serial framebufferSerial)`
`64`	`65`	`: mResource(nullptr)`
`65`	`66`	`{`
`66`	`67`	`attach(context, type, binding, textureIndex, resource, kDefaultNumViews, kDefaultBaseViewIndex,`
`67`		`- false, kDefaultRenderToTextureSamples);`
	`68`	`+ false, kDefaultRenderToTextureSamples, framebufferSerial);`
`68`	`69`	`}`
`69`	`70`
`70`	`71`	`FramebufferAttachment::FramebufferAttachment(FramebufferAttachment &&other)`
`@@ -90,12 +91,12 @@ FramebufferAttachment::~FramebufferAttachment()`
`90`	`91`	`ASSERT(!isAttached());`
`91`	`92`	`}`
`92`	`93`
`93`		`-void FramebufferAttachment::detach(const Context *context)`
	`94`	`+void FramebufferAttachment::detach(const Context *context, rx::Serial framebufferSerial)`
`94`	`95`	`{`
`95`	`96`	`mType = GL_NONE;`
`96`	`97`	`if (mResource != nullptr)`
`97`	`98`	`{`
`98`		`- mResource->onDetach(context);`
	`99`	`+ mResource->onDetach(context, framebufferSerial);`
`99`	`100`	`mResource = nullptr;`
`100`	`101`	`}`
`101`	`102`	`mNumViews = kDefaultNumViews;`
`@@ -114,11 +115,12 @@ void FramebufferAttachment::attach(const Context *context,`
`114`	`115`	`GLsizei numViews,`
`115`	`116`	`GLuint baseViewIndex,`
`116`	`117`	`bool isMultiview,`
`117`		`- GLsizei samples)`
	`118`	`+ GLsizei samples,`
	`119`	`+ rx::Serial framebufferSerial)`
`118`	`120`	`{`
`119`	`121`	`if (resource == nullptr)`
`120`	`122`	`{`
`121`		`- detach(context);`
	`123`	`+ detach(context, framebufferSerial);`
`122`	`124`	`return;`
`123`	`125`	`}`
`124`	`126`
`@@ -128,11 +130,11 @@ void FramebufferAttachment::attach(const Context *context,`
`128`	`130`	`mBaseViewIndex = baseViewIndex;`
`129`	`131`	`mIsMultiview = isMultiview;`
`130`	`132`	`mRenderToTextureSamples = samples;`
`131`		`- resource->onAttach(context);`
	`133`	`+ resource->onAttach(context, framebufferSerial);`
`132`	`134`
`133`	`135`	`if (mResource != nullptr)`
`134`	`136`	`{`
`135`		`- mResource->onDetach(context);`
	`137`	`+ mResource->onDetach(context, framebufferSerial);`
`136`	`138`	`}`
`137`	`139`
`138`	`140`	`mResource = resource;`
Original file line number	Diff line number	Diff line change
`@@ -193,9 +193,9 @@ bool ExternalImageSibling::isTextureable(const gl::Context *context) const`
`193`	`193`	`return mImplementation->isTexturable(context);`
`194`	`194`	`}`
`195`	`195`
`196`		`-void ExternalImageSibling::onAttach(const gl::Context *context) {}`
	`196`	`+void ExternalImageSibling::onAttach(const gl::Context *context, rx::Serial framebufferSerial) {}`
`197`	`197`
`198`		`-void ExternalImageSibling::onDetach(const gl::Context *context) {}`
	`198`	`+void ExternalImageSibling::onDetach(const gl::Context *context, rx::Serial framebufferSerial) {}`
`199`	`199`
`200`	`200`	`GLuint ExternalImageSibling::getId() const`
`201`	`201`	`{`
Original file line number	Diff line number	Diff line change
`@@ -234,12 +234,12 @@ GLint Renderbuffer::getMemorySize() const`
`234`	`234`	`return size.ValueOrDefault(std::numeric_limits<GLint>::max());`
`235`	`235`	`}`
`236`	`236`
`237`		`-void Renderbuffer::onAttach(const Context *context)`
	`237`	`+void Renderbuffer::onAttach(const Context *context, rx::Serial framebufferSerial)`
`238`	`238`	`{`
`239`	`239`	`addRef();`
`240`	`240`	`}`
`241`	`241`
`242`		`-void Renderbuffer::onDetach(const Context *context)`
	`242`	`+void Renderbuffer::onDetach(const Context *context, rx::Serial framebufferSerial)`
`243`	`243`	`{`
`244`	`244`	`release(context);`
`245`	`245`	`}`