Fix leak with binding Framebuffers directly.
Using BindFramebuffer(1) then GenFramebuffers would return 1.
This leads to a memory leak and was something that was obscuring
debugging a bug in my ReadPixels fix for ES3.
This also fixes a bug where running the texture tests along produces
some random failures.
BUG=angleproject:1290
BUG=angleproject:1299
Change-Id: If11e8c743d2ddde725b12749ac012f670cd290e1
Reviewed-on: https://chromium-review.googlesource.com/324820
Reviewed-by: Geoff Lang <geofflang@chromium.org>
Commit-Queue: Jamie Madill <jmadill@chromium.org>
Tested-by: Jamie Madill <jmadill@chromium.org>
diff --git a/src/libANGLE/Context.cpp b/src/libANGLE/Context.cpp
index 58f39a6..b835c1a 100644
--- a/src/libANGLE/Context.cpp
+++ b/src/libANGLE/Context.cpp
@@ -686,24 +686,16 @@
mState.setSamplerTexture(target, texture);
}
-void Context::bindReadFramebuffer(GLuint framebuffer)
+void Context::bindReadFramebuffer(GLuint framebufferHandle)
{
- if (!getFramebuffer(framebuffer))
- {
- mFramebufferMap[framebuffer] = new Framebuffer(mCaps, mRenderer, framebuffer);
- }
-
- mState.setReadFramebufferBinding(getFramebuffer(framebuffer));
+ Framebuffer *framebuffer = checkFramebufferAllocation(framebufferHandle);
+ mState.setReadFramebufferBinding(framebuffer);
}
-void Context::bindDrawFramebuffer(GLuint framebuffer)
+void Context::bindDrawFramebuffer(GLuint framebufferHandle)
{
- if (!getFramebuffer(framebuffer))
- {
- mFramebufferMap[framebuffer] = new Framebuffer(mCaps, mRenderer, framebuffer);
- }
-
- mState.setDrawFramebufferBinding(getFramebuffer(framebuffer));
+ Framebuffer *framebuffer = checkFramebufferAllocation(framebufferHandle);
+ mState.setDrawFramebufferBinding(framebuffer);
}
void Context::bindRenderbuffer(GLuint renderbuffer)
@@ -887,16 +879,8 @@
Framebuffer *Context::getFramebuffer(unsigned int handle) const
{
- FramebufferMap::const_iterator framebuffer = mFramebufferMap.find(handle);
-
- if (framebuffer == mFramebufferMap.end())
- {
- return NULL;
- }
- else
- {
- return framebuffer->second;
- }
+ auto framebufferIt = mFramebufferMap.find(handle);
+ return ((framebufferIt == mFramebufferMap.end()) ? nullptr : framebufferIt->second);
}
FenceNV *Context::getFenceNV(unsigned int handle)
@@ -1685,6 +1669,7 @@
void Context::checkVertexArrayAllocation(GLuint vertexArray)
{
+ // Only called after a prior call to Gen.
if (!getVertexArray(vertexArray))
{
VertexArray *vertexArrayObject =
@@ -1695,6 +1680,7 @@
void Context::checkTransformFeedbackAllocation(GLuint transformFeedback)
{
+ // Only called after a prior call to Gen.
if (!getTransformFeedback(transformFeedback))
{
TransformFeedback *transformFeedbackObject =
@@ -1704,6 +1690,27 @@
}
}
+Framebuffer *Context::checkFramebufferAllocation(GLuint framebuffer)
+{
+ // Can be called from Bind without a prior call to Gen.
+ auto framebufferIt = mFramebufferMap.find(framebuffer);
+ bool neverCreated = framebufferIt == mFramebufferMap.end();
+ if (neverCreated || framebufferIt->second == nullptr)
+ {
+ Framebuffer *newFBO = new Framebuffer(mCaps, mRenderer, framebuffer);
+ if (neverCreated)
+ {
+ mFramebufferHandleAllocator.reserve(framebuffer);
+ mFramebufferMap[framebuffer] = newFBO;
+ return newFBO;
+ }
+
+ framebufferIt->second = newFBO;
+ }
+
+ return framebufferIt->second;
+}
+
bool Context::isVertexArrayGenerated(GLuint vertexArray)
{
return mVertexArrayMap.find(vertexArray) != mVertexArrayMap.end();
diff --git a/src/libANGLE/Context.h b/src/libANGLE/Context.h
index b15d948..6b82eb7 100644
--- a/src/libANGLE/Context.h
+++ b/src/libANGLE/Context.h
@@ -111,8 +111,8 @@
void bindArrayBuffer(GLuint buffer);
void bindElementArrayBuffer(GLuint buffer);
void bindTexture(GLenum target, GLuint handle);
- void bindReadFramebuffer(GLuint framebuffer);
- void bindDrawFramebuffer(GLuint framebuffer);
+ void bindReadFramebuffer(GLuint framebufferHandle);
+ void bindDrawFramebuffer(GLuint framebufferHandle);
void bindRenderbuffer(GLuint renderbuffer);
void bindVertexArray(GLuint vertexArray);
void bindSampler(GLuint textureUnit, GLuint sampler);
@@ -319,6 +319,7 @@
private:
void checkVertexArrayAllocation(GLuint vertexArray);
void checkTransformFeedbackAllocation(GLuint transformFeedback);
+ Framebuffer *checkFramebufferAllocation(GLuint framebufferHandle);
void detachBuffer(GLuint buffer);
void detachTexture(GLuint texture);
@@ -391,6 +392,6 @@
ResourceManager *mResourceManager;
};
-}
+} // namespace gl
#endif // LIBANGLE_CONTEXT_H_
diff --git a/src/libANGLE/HandleAllocator.cpp b/src/libANGLE/HandleAllocator.cpp
index 16365dc..4815855 100644
--- a/src/libANGLE/HandleAllocator.cpp
+++ b/src/libANGLE/HandleAllocator.cpp
@@ -126,8 +126,9 @@
}
if (begin != handle)
{
+ ASSERT(begin < handle);
mUnallocatedList.insert(placementIt, HandleRange(begin, handle));
}
}
-}
+} // namespace gl
diff --git a/src/libANGLE/HandleAllocator.h b/src/libANGLE/HandleAllocator.h
index c22f2ba..1888d57 100644
--- a/src/libANGLE/HandleAllocator.h
+++ b/src/libANGLE/HandleAllocator.h
@@ -58,6 +58,6 @@
std::vector<GLuint> mReleasedList;
};
-}
+} // namespace gl
#endif // LIBANGLE_HANDLEALLOCATOR_H_
diff --git a/src/tests/angle_end2end_tests.gypi b/src/tests/angle_end2end_tests.gypi
index 9480a0b..6f50b0f 100644
--- a/src/tests/angle_end2end_tests.gypi
+++ b/src/tests/angle_end2end_tests.gypi
@@ -46,6 +46,7 @@
'<(angle_path)/src/tests/gl_tests/PBOExtensionTest.cpp',
'<(angle_path)/src/tests/gl_tests/PointSpritesTest.cpp',
'<(angle_path)/src/tests/gl_tests/ProvokingVertexTest.cpp',
+ '<(angle_path)/src/tests/gl_tests/ObjectAllocationTest.cpp',
'<(angle_path)/src/tests/gl_tests/OcclusionQueriesTest.cpp',
'<(angle_path)/src/tests/gl_tests/ProgramBinaryTest.cpp',
'<(angle_path)/src/tests/gl_tests/ReadPixelsTest.cpp',
diff --git a/src/tests/gl_tests/ObjectAllocationTest.cpp b/src/tests/gl_tests/ObjectAllocationTest.cpp
new file mode 100644
index 0000000..b0e59f7
--- /dev/null
+++ b/src/tests/gl_tests/ObjectAllocationTest.cpp
@@ -0,0 +1,54 @@
+//
+// Copyright 2015 The ANGLE Project Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// ObjectAllocationTest
+// Tests for object allocations and lifetimes.
+//
+
+#include "test_utils/ANGLETest.h"
+
+using namespace angle;
+
+namespace
+{
+
+class ObjectAllocationTest : public ANGLETest
+{
+ protected:
+ ObjectAllocationTest() {}
+};
+
+// Test that we don't re-allocate a bound framebuffer ID.
+TEST_P(ObjectAllocationTest, BindFramebufferBeforeGen)
+{
+ glBindFramebuffer(GL_FRAMEBUFFER, 1);
+ GLuint fbo = 0;
+ glGenFramebuffers(1, &fbo);
+ EXPECT_NE(1u, fbo);
+ glDeleteFramebuffers(1, &fbo);
+ EXPECT_GL_NO_ERROR();
+}
+
+// Test that we don't re-allocate a bound framebuffer ID, other pattern.
+TEST_P(ObjectAllocationTest, BindFramebufferAfterGen)
+{
+ GLuint firstFBO = 0;
+ glGenFramebuffers(1, &firstFBO);
+ glBindFramebuffer(GL_FRAMEBUFFER, 1);
+ glDeleteFramebuffers(1, &firstFBO);
+
+ glBindFramebuffer(GL_FRAMEBUFFER, 2);
+ GLuint secondFBOs[2] = {0};
+ glGenFramebuffers(2, secondFBOs);
+ EXPECT_NE(2u, secondFBOs[0]);
+ EXPECT_NE(2u, secondFBOs[1]);
+ glDeleteFramebuffers(2, secondFBOs);
+
+ EXPECT_GL_NO_ERROR();
+}
+
+} // anonymous namespace
+
+ANGLE_INSTANTIATE_TEST(ObjectAllocationTest, ES3_OPENGL(), ES3_D3D11());
\ No newline at end of file