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

PRESUBMIT.py

@@ -60,7 +60,7 @@ def _CheckCommitMessageFormatting(input_api, output_api):
     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

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

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;

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;

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;

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
 {

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;

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);
 }

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;

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

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);
 }
 

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();

src/libANGLE/renderer/d3d/FramebufferD3D.cpp

@@ -362,19 +362,18 @@ const gl::AttachmentList &FramebufferD3D::getColorAttachmentsForRender(const gl:
             // it to be attached to a new binding point.
             if (mDummyAttachment.isAttached())
             {
-                mDummyAttachment.detach(context);
+                mDummyAttachment.detach(context, Serial());
             }
 
             gl::Texture *dummyTex = nullptr;
-            // TODO(Jamie): Handle error if dummy texture can't be created.
+            // TODO(jmadill): Handle error if dummy texture can't be created.
             (void)mRenderer->getIncompleteTexture(context, gl::TextureType::_2D, &dummyTex);
             if (dummyTex)
             {
-
                 gl::ImageIndex index = gl::ImageIndex::Make2D(0);
                 mDummyAttachment     = gl::FramebufferAttachment(
                     context, GL_TEXTURE, GL_COLOR_ATTACHMENT0_EXT + activeProgramLocation, index,
-                    dummyTex);
+                    dummyTex, Serial());
                 colorAttachmentsForRender.push_back(&mDummyAttachment);
             }
         }
@@ -390,7 +389,7 @@ void FramebufferD3D::destroy(const gl::Context *context)
 {
     if (mDummyAttachment.isAttached())
     {
-        mDummyAttachment.detach(context);
+        mDummyAttachment.detach(context, Serial());
     }
 }
 

src/libANGLE/validationES.cpp

@@ -3005,7 +3005,7 @@ const char *ValidateDrawStates(const Context *context)
             }
 
             // Detect rendering feedback loops for WebGL.
-            if (framebuffer->hasRenderingFeedbackLoop())
+            if (framebuffer->formsRenderingFeedbackLoopWith(context))
             {
                 return kFeedbackLoop;
             }

src/tests/gl_tests/WebGLCompatibilityTest.cpp

@@ -5070,6 +5070,13 @@ TEST_P(WebGL2CompatibilityTest, UniformVariablesReturnTypes)
     EXPECT_GL_ERROR(GL_INVALID_ENUM);
 }
 
+// Tests an error case to ensure we don't crash.
+TEST_P(WebGLCompatibilityTest, DrawWithNoProgram)
+{
+    glDrawArrays(GL_TRIANGLES, 0, 6);
+    EXPECT_GL_ERROR(GL_INVALID_OPERATION);
+}
+
 // Use this to select which configurations (e.g. which renderer, which GLES major version) these
 // tests should be run against.
 ANGLE_INSTANTIATE_TEST_ES2_AND_ES3(WebGLCompatibilityTest);