Make the ResourcePool hold FBOs as a per-context resource.
authorSteinar H. Gunderson <sgunderson@bigfoot.com>
Wed, 26 Mar 2014 00:02:20 +0000 (01:02 +0100)
committerSteinar H. Gunderson <sgunderson@bigfoot.com>
Wed, 26 Mar 2014 00:02:20 +0000 (01:02 +0100)
This is an attempt to get out of the FBO sharability mess (unfortunately
we can't just stop having persistent FBOs, due to NVidia performance).
We now require the client to tell us whenever a context is going away,
and we try to be more careful about not deleting them in the wrong context.

Also, we assumed FBO names were globally unique, which isn't necessarily
true, so re-key them.

For good measure, we were deleting FBOs off the freelist from the front,
not the back as we should have -- fixed.

effect_chain.cpp
effect_chain.h
resource_pool.cpp
resource_pool.h

index ca6905b..c6f1e89 100644 (file)
@@ -1493,8 +1493,7 @@ void EffectChain::execute_phase(Phase *phase, bool last_phase, map<Phase *, GLui
 
        // And now the output. (Already set up for us if it is the last phase.)
        if (!last_phase) {
-               void *context = get_gl_context_identifier();
-               fbo = resource_pool->create_fbo(context, (*output_textures)[phase]);
+               fbo = resource_pool->create_fbo((*output_textures)[phase]);
                glBindFramebuffer(GL_FRAMEBUFFER, fbo);
                glViewport(0, 0, phase->output_width, phase->output_height);
        }
index 6452d14..2b959bd 100644 (file)
 // but if so, the threads' contexts need to be set up to share resources, since
 // the EffectChain holds textures and other OpenGL objects that are tied to the
 // context.
+//
+// Memory management (only relevant if you use multiple contexts):
+// See corresponding comment in resource_pool.h. This holds even if you don't
+// allocate your own ResourcePool, but let EffectChain hold its own.
 
 #include <GL/glew.h>
 #include <stdio.h>
index 201b73c..19c26a0 100644 (file)
@@ -52,16 +52,29 @@ ResourcePool::~ResourcePool()
        assert(texture_formats.empty());
        assert(texture_freelist_bytes == 0);
 
-       for (list<GLuint>::const_iterator freelist_it = fbo_freelist.begin();
-            freelist_it != fbo_freelist.end();
-            ++freelist_it) {
-               GLuint free_fbo_num = *freelist_it;
-               assert(fbo_formats.count(free_fbo_num) != 0);
-               fbo_formats.erase(free_fbo_num);
-               // TODO: We currently leak due to FBO sharability issues.
-               // glDeleteFramebuffers(1, &free_fbo_num);
-               // check_error();
+       void *context = get_gl_context_identifier();
+       cleanup_unlinked_fbos(context);
+
+       for (map<void *, std::list<GLuint> >::iterator context_it = fbo_freelist.begin();
+            context_it != fbo_freelist.end();
+            ++context_it) {
+               if (context_it->first != context) {
+                       // If this does not hold, the client should have called clean_context() earlier.
+                       assert(context_it->second.empty());
+                       continue;
+               }
+               for (list<GLuint>::const_iterator freelist_it = context_it->second.begin();
+                    freelist_it != context_it->second.end();
+                    ++freelist_it) {
+                       pair<void *, GLuint> key(context, *freelist_it);
+                       GLuint free_fbo_num = *freelist_it;
+                       assert(fbo_formats.count(key) != 0);
+                       fbo_formats.erase(key);
+                       glDeleteFramebuffers(1, &free_fbo_num);
+                       check_error();
+               }
        }
+
        assert(fbo_formats.empty());
 }
 
@@ -256,40 +269,40 @@ void ResourcePool::release_2d_texture(GLuint texture_num)
                glDeleteTextures(1, &free_texture_num);
                check_error();
 
-               // Delete any FBO related to this texture.
-               for (list<GLuint>::iterator fbo_freelist_it = fbo_freelist.begin();
-                    fbo_freelist_it != fbo_freelist.end(); ) {
-                       GLuint fbo_num = *fbo_freelist_it;
-                       map<GLuint, FBO>::const_iterator format_it = fbo_formats.find(fbo_num);
-                       assert(format_it != fbo_formats.end());
+               // Unlink any lingering FBO related to this texture. We might
+               // not be in the right context, so don't delete it right away;
+               // the cleanup in release_fbo() (which calls cleanup_unlinked_fbos())
+               // will take care of actually doing that later.
+               for (map<pair<void *, GLuint>, FBO>::iterator format_it = fbo_formats.begin();
+                    format_it != fbo_formats.end();
+                    ++format_it) {
                        if (format_it->second.texture_num == free_texture_num) {
-                               fbo_formats.erase(fbo_num);
-                               // TODO: We currently leak due to FBO sharability issues.
-                               // glDeleteFramebuffers(1, &fbo_num);
-                               fbo_freelist.erase(fbo_freelist_it++);
-                       } else {
-                               ++fbo_freelist_it;
+                               format_it->second.texture_num = 0;
                        }
                }
        }
        pthread_mutex_unlock(&lock);
 }
 
-GLuint ResourcePool::create_fbo(void *context, GLuint texture_num)
+GLuint ResourcePool::create_fbo(GLuint texture_num)
 {
+       void *context = get_gl_context_identifier();
+
        pthread_mutex_lock(&lock);
-       // See if there's an FBO on the freelist we can use.
-       for (list<GLuint>::iterator freelist_it = fbo_freelist.begin();
-            freelist_it != fbo_freelist.end();
-            ++freelist_it) {
-               GLuint fbo_num = *freelist_it;
-               map<GLuint, FBO>::const_iterator format_it = fbo_formats.find(fbo_num);
-               assert(format_it != fbo_formats.end());
-               if (format_it->second.context == context &&
-                   format_it->second.texture_num == texture_num) {
-                       fbo_freelist.erase(freelist_it);
-                       pthread_mutex_unlock(&lock);
-                       return fbo_num;
+       if (fbo_freelist.count(context) != 0) {
+               // See if there's an FBO on the freelist we can use.
+               for (list<GLuint>::iterator freelist_it = fbo_freelist[context].begin();
+                    freelist_it != fbo_freelist[context].end();
+                    ++freelist_it) {
+                       GLuint fbo_num = *freelist_it;
+                       map<pair<void *, GLuint>, FBO>::const_iterator format_it =
+                               fbo_formats.find(make_pair(context, fbo_num));
+                       assert(format_it != fbo_formats.end());
+                       if (format_it->second.texture_num == texture_num) {
+                               fbo_freelist[context].erase(freelist_it);
+                               pthread_mutex_unlock(&lock);
+                               return fbo_num;
+                       }
                }
        }
 
@@ -312,10 +325,10 @@ GLuint ResourcePool::create_fbo(void *context, GLuint texture_num)
        check_error();
 
        FBO fbo_format;
-       fbo_format.context = context;
        fbo_format.texture_num = texture_num;
-       assert(fbo_formats.count(fbo_num) == 0);
-       fbo_formats.insert(make_pair(fbo_num, fbo_format));
+       pair<void *, GLuint> key(context, fbo_num);
+       assert(fbo_formats.count(key) == 0);
+       fbo_formats.insert(make_pair(key, fbo_format));
 
        pthread_mutex_unlock(&lock);
        return fbo_num;
@@ -323,22 +336,60 @@ GLuint ResourcePool::create_fbo(void *context, GLuint texture_num)
 
 void ResourcePool::release_fbo(GLuint fbo_num)
 {
+       void *context = get_gl_context_identifier();
+
        pthread_mutex_lock(&lock);
-       fbo_freelist.push_front(fbo_num);
-       assert(fbo_formats.count(fbo_num) != 0);
-
-       while (fbo_freelist.size() > fbo_freelist_max_length) {
-               GLuint free_fbo_num = fbo_freelist.front();
-               fbo_freelist.pop_front();
-               assert(fbo_formats.count(free_fbo_num) != 0);
-               fbo_formats.erase(free_fbo_num);
-               // TODO: We currently leak due to FBO sharability issues.
-               // glDeleteFramebuffers(1, &free_fbo_num);
-               // check_error();
-       }
+       fbo_freelist[context].push_front(fbo_num);
+       assert(fbo_formats.count(make_pair(context, fbo_num)) != 0);
+
+       // Now that we're in this context, free up any FBOs that are connected
+       // to deleted textures (in release_2d_texture).
+       cleanup_unlinked_fbos(context);
+
+       shrink_fbo_freelist(context, fbo_freelist_max_length);
        pthread_mutex_unlock(&lock);
 }
 
+void ResourcePool::clean_context()
+{
+       void *context = get_gl_context_identifier();
+
+       // Currently, we only need to worry about FBOs, as they are the only
+       // non-shareable resource we hold.
+       shrink_fbo_freelist(context, 0);
+       fbo_freelist.erase(context);
+}
+
+void ResourcePool::cleanup_unlinked_fbos(void *context)
+{
+       for (list<GLuint>::iterator freelist_it = fbo_freelist[context].begin();
+            freelist_it != fbo_freelist[context].end(); ) {
+               GLuint fbo_num = *freelist_it;
+               pair<void *, GLuint> key(context, fbo_num);
+               assert(fbo_formats.count(key) != 0);
+               if (fbo_formats[key].texture_num == 0) {
+                       glDeleteFramebuffers(1, &fbo_num);
+                       check_error();
+                       fbo_freelist[context].erase(freelist_it++);
+               } else {
+                       freelist_it++;
+               }
+       }
+}
+
+void ResourcePool::shrink_fbo_freelist(void *context, size_t max_length)
+{
+       while (fbo_freelist[context].size() > max_length) {
+               GLuint free_fbo_num = fbo_freelist[context].back();
+               pair<void *, GLuint> key(context, free_fbo_num);
+               fbo_freelist[context].pop_back();
+               assert(fbo_formats.count(key) != 0);
+               fbo_formats.erase(key);
+               glDeleteFramebuffers(1, &free_fbo_num);
+               check_error();
+       }
+}
+
 size_t ResourcePool::estimate_texture_size(const Texture2D &texture_format)
 {
        size_t bytes_per_pixel;
index 4ae4110..aad95c9 100644 (file)
 // Thread-safety: All functions except the constructor and destructor can be
 // safely called from multiple threads at the same time, provided they have
 // separate (but sharing) OpenGL contexts.
+//
+// Memory management (only relevant if you use multiple contexts): Some objects,
+// like FBOs, are not shareable across contexts, and can only be deleted from
+// the context they were created in. Thus, you will need to tell the
+// ResourcePool explicitly if you delete a context, or they will leak (and the
+// ResourcePool destructor will assert-fail). See clean_context().
 
 #include <GL/glew.h>
 #include <pthread.h>
@@ -41,7 +47,7 @@ public:
        // twice this estimate or more.
        ResourcePool(size_t program_freelist_max_length = 100,
                     size_t texture_freelist_max_bytes = 100 << 20,  // 100 MB.
-                    size_t fbo_freelist_max_length = 100);
+                    size_t fbo_freelist_max_length = 100);  // Per context.
        ~ResourcePool();
 
        // All remaining functions are intended for calls from EffectChain only.
@@ -63,21 +69,36 @@ public:
        // Allocate an FBO with the the given texture bound as a framebuffer attachment,
        // or fetch a previous used if possible. Unbinds GL_FRAMEBUFFER afterwards.
        // Keeps ownership of the FBO; you must call release_fbo() of deleting
-       // it when you no longer want it. You can get an appropriate context
-       // pointer from get_gl_context_identifier().
+       // it when you no longer want it.
        //
        // NOTE: In principle, the FBO doesn't have a resolution or pixel format;
        // you can bind almost whatever texture you want to it. However, changing
        // textures can have an adverse effect on performance due to validation,
        // in particular on NVidia cards. Also, keep in mind that FBOs are not
-       // shareable across contexts.
-       GLuint create_fbo(void *context, GLuint texture_num);
+       // shareable across contexts, so you must have the context that's supposed
+       // to own the FBO current when you create or release it.
+       GLuint create_fbo(GLuint texture_num);
        void release_fbo(GLuint fbo_num);
 
+       // Informs the ResourcePool that the current context is going away soon,
+       // and that any resources held for it in the freelist should be deleted.
+       //
+       // You do not need to do this for the last context; the regular destructor
+       // will take care of that. This means that if you only ever use one
+       // thread/context, you never need to call this function.
+       void clean_context();
+
 private:
        // Delete the given program and both its shaders.
        void delete_program(GLuint program_num);
 
+       // Deletes all FBOs for the given context that belong to deleted textures.
+       void cleanup_unlinked_fbos(void *context);
+
+       // Remove FBOs off the end of the freelist for <context>, until it
+       // is no more than <max_length> elements long.
+       void shrink_fbo_freelist(void *context, size_t max_length);
+
        // Protects all the other elements in the class.
        pthread_mutex_t lock;
 
@@ -118,19 +139,18 @@ private:
        size_t texture_freelist_bytes;
 
        struct FBO {
-               void *context;
-               GLuint texture_num;
+               GLuint texture_num;  // 0 means associated to a texture that has since been deleted.
        };
 
-       // A mapping from FBO number to format details. This is filled if the
-       // FBO is given out to a client or on the freelist, but not if it is
-       // deleted from the freelist.
-       std::map<GLuint, FBO> fbo_formats;
+       // For each context, a mapping from FBO number to format details. This is
+       // filled if the FBO is given out to a client or on the freelist, but
+       // not if it is deleted from the freelist.
+       std::map<std::pair<void *, GLuint>, FBO> fbo_formats;
 
-       // A list of all FBOs that are release but not freed (most recently freed
-       // first). Once this reaches <fbo_freelist_max_length>, the last element
-       // will be deleted.
-       std::list<GLuint> fbo_freelist;
+       // For each context, a list of all FBOs that are released but not freed
+       // (most recently freed first). Once this reaches <fbo_freelist_max_length>,
+       // the last element will be deleted.
+       std::map<void *, std::list<GLuint> > fbo_freelist;
 
        // See the caveats at the constructor.
        static size_t estimate_texture_size(const Texture2D &texture_format);