GNOME Bugzilla – Bug 726337
omxbufferpool: leak buffers when stopped
Last modified: 2014-07-23 08:18:09 UTC
Created attachment 271881 [details] [review] omxbufferpool: fix memory leak if used on output port When using GstOMXBufferPool on an output port, it internally uses a GPtrArray to manage the GstBuffers instead of the default queue from the GstBufferPool base class. In this case GstBufferPool::default_free_buffer is not called when the pool is stopped. So we have to explicitly call gst_omx_buffer_pool_free_buffer on each buffer contained in the GPtrArray.
Review of attachment 271881 [details] [review]: I was already wondering how to fix this problem, thanks. I've just tested the patch and it seems to be doing what it should, maybe I can't judge but the code looks good as well.
Review of attachment 271881 [details] [review]: ::: omx/gstomxbufferpool.c @@ +220,3 @@ + g_ptr_array_foreach (pool->buffers, + gst_omx_buffer_pool_free_buffer_foreach_func, bpool); any reason to not do it here, would save quite a few lines: for (i = 0; i < pool->buffers->len; i++) gst_omx_buffer_pool_free_buffer (pool, g_ptr_array_index (buffers,i)); Otherwise it looks good to me too.
Comment on attachment 271881 [details] [review] omxbufferpool: fix memory leak if used on output port Looks good
Created attachment 272127 [details] [review] omxbufferpool: fix memory leak if used on output port (In reply to comment #2) > Review of attachment 271881 [details] [review]: > > ::: omx/gstomxbufferpool.c > @@ +220,3 @@ > > + g_ptr_array_foreach (pool->buffers, > + gst_omx_buffer_pool_free_buffer_foreach_func, bpool); > > any reason to not do it here, would save quite a few lines: > > for (i = 0; i < pool->buffers->len; i++) > gst_omx_buffer_pool_free_buffer (pool, g_ptr_array_index (buffers,i)); > > Otherwise it looks good to me too. No particular reason, and I prefer your suggestion :) Thx
I'm going to push it, any objection ?
Comment on attachment 272127 [details] [review] omxbufferpool: fix memory leak if used on output port commit e8ca74c6f862e5f1e0bdc5057dc836a71902244e Author: Julien Isorce <julien.isorce@collabora.co.uk> Date: Mon Mar 17 09:57:11 2014 +0000 omxbufferpool: fix memory leak if used on output port When using GstOMXBufferPool on an output port, it internally uses a GPtrArray to manage the GstBuffers instead of the default queue from the GstBufferPool base class. In this case GstBufferPool::default_free_buffer is not called when the pool is stopped. Because the queue is empty. So explicitely call gst_omx_buffer_pool_free_buffer on each buffer contained in the GPtrArray. https://bugzilla.gnome.org/show_bug.cgi?id=726337
The change isn't fully correct and introduced a segfault when pipeline goes from PAUSED to READY.
Created attachment 272807 [details] [review] Patch that fixes the issue This patch fixes the issue.
Review of attachment 272807 [details] [review]: I was not able to reproduce the crash. I may use a different gst version than you so maybe I do not reach your exact case. But I confirm this is better to push back in the internal parent queue so that the parent can call free itself.
commit 100e9f998d6f986838f9e27be1ef1d511a9638e3 Author: Josep Torra <n770galaxy@gmail.com> Date: Mon Mar 24 17:49:59 2014 +0100 omxbufferpool: return buffers to the pool instead of freeing them We have to return the buffers back to the pool in when stopping to not mess with the GstBufferPool accounting. The OMX buffers will be freed when those won't be in charge of the pool in the chained up call to 'stop'. Fixes segfaults on finalize and pool not being properly deactivated. https://bugzilla.gnome.org/show_bug.cgi?id=726337