GNOME Bugzilla – Bug 619500
It is not possible to "recycle" buffers without subclassing GstBuffer object
Last modified: 2012-05-18 08:03:32 UTC
Created attachment 161849 [details] [review] Patch which adds weak reference handling to GstMiniObject Imagine a following situation: there is a video source element which wants to use buffer pool to recycle same buffers over and over again (HW requires it) AND use gst_pad_alloc_buffer() to pre-allocate the buffers from downstream (video sink) element to enable optimized (zero-copy) video rendering. But, this is not possible, because source element must create its own GstBuffer subclass in order to overwrite GstMiniObject::finalize() function for resurrecting the rendered and unref'ed video buffers. There is no other way to recycle the used buffers than this. And using subclassed GstBuffer prevents the use of gst_pad_alloc_buffer() call. There would be one hackish way do this: call pad_alloc(), store the obtained GstBuffer pointer somewhere and steal its GST_BUFFER_DATA pointer. However, video sink's zero-copy operation requires the pushed GstBuffers to be the same ones which were allocated with pad_alloc(), so this wouldn't work. I tried to solve this by adding a weak reference handling mechanism to GstMiniObject. This way source element could just allocate the buffers with gst_pad_alloc_buffer() and add a weak references to them. When sink unrefs a buffer, weak reference notification callback would then resurrect the buffer and queue it again to video device. However, this turned out not to be reliable. Since these video buffers are subclassed by sink element, it also has ability to resurrect them in finalize() function. This leads into problems when source element's weak ref notification has already been called, src doesn't want to resurrect the buffer, but sink element decides to resurrect it. Now source element believes that buffer has been destroyed, even it is still alive. Another problem is that if also video sink has a pool for video buffers, then rendered buffer will end up into this pool instead of freeing. If source element gets notification about this and re-queues, captures and pushes a new video frame by using this buffer, video sink might get confused because it receives a buffer which should be still in its internal pool of unused buffers. Attached is a patch which adds weak reference handling to GstMiniObject. It is still WIP and contains some unnecessary debugs to keep track what's going on there. I am not sure if this even is the right approach to try to solve this problem, but let's consider it as a beginning of discussion.
With current code, it looks like the first problem could happen indeed. On the one hand, that need not be a real problem (might be suboptimal to believe buffer has been destroyed while it has not, but it should not mess up stuff). On the other hand, that approach basically means buffers are snatched from the originating (sink) pool and sort of collected into a source pool. As for the second problem, I am not clear on where this would occur, since the current code effectively gives "first turn" to the weak notification (before the actual _finalize gets a chance to decide on recycling or not). As an alternative approach, rather than trying to overload _finalize by all sorts of recycling tricks behind-the-scenes that then compete with each other, we could make this all more explicit. That is, we could introduce a GstBuffer subclass, say GstPooledBuffer, which could then be used as a basetype for the pooled buffer implementations in e.g. xvimagesink and/or v4l2src. It would provide certain common 'services', most notably arranging for these buffers to be recycled into a (say) GstBufferPool (= basically wrapping a collection/list of buffers and whatever (e.g. locks) are needed). Callbacks can arrange for subclass specific actions/notifications (e.g. when a buffer is being recycled or being really destroyed). In particular, this allows the owning pool to really release (finalize) the buffer if needed (without interference) and thereby merely informing others that they should "clean up any references". Alternatively, if it does not mind recycling, the notification could allow others to "claim" the buffer and carry on using it (without the owning pool then ending up surprised by this).
We had buffer pools in the public GStreamer API in 0.8 but removed them for 0.10 for various reasons. I don't remember those reasons though, I just remember us thinking that subclassing is a better idea.
(In reply to comment #0) > there is a video source element which wants to use buffer pool to recycle same > buffers over and over again (HW requires it) AND use gst_pad_alloc_buffer() to > pre-allocate the buffers from downstream (video sink) element to enable > optimized (zero-copy) video rendering. > While I'm not convinced that is a good idea, you could achieve this today by just keeping a ref to the buffers you allocated using pad_alloc() in your pool and then reusing them when GST_MINI_OBJECT_REFCOUNT_VALUE() returns 1. That'd basically be the same thing as your approach, just without requiring new API.
(In reply to comment #3) > While I'm not convinced that is a good idea, you could achieve this today by > just keeping a ref to the buffers you allocated using pad_alloc() in your pool > and then reusing them when GST_MINI_OBJECT_REFCOUNT_VALUE() returns 1. > That'd basically be the same thing as your approach, just without requiring new > API. But then there should be an active loop which polls the refcounts. The problem is that when source element has pushed all its buffers out it can't do anything until a free buffer is available. So it needs to wait for the buffer to get released. If we had a weak ref notification callback, I wouldn't need to write a busy loop for it.
Oh gosh, yeah, that is nasty. Could the decoder and sink elements cooperatively handle the buffer pool maybe?
Review of attachment 161849 [details] [review]: Good idea and basic implementations. Needs a bit of cleanup and some +1 from someone else. ::: gst/gstminiobject.c @@ +158,3 @@ +gst_mini_object_weakref_notify (gpointer data, gpointer user_data) +{ + GstMOWeakRef *wref; I would spell this out GstMiniObjectWeakRef @@ +693,3 @@ + g_return_if_fail (notify != NULL); + + wref = g_new0 (GstMOWeakRef, 1); please use g_slice and no need to zero it, if we fully initialize it. @@ +700,3 @@ + "weak ref for %p added (data %p notify %p)", obj, data, notify); + + obj->wrefs = g_slist_append (obj->wrefs, wref); as the order of the weak refs does not matter, I'd suggest g_slist_prepend() - see notes on g_slist_append in api docs. ::: gst/gstminiobject.h @@ +148,2 @@ /*< private >*/ + GSList *wrefs; irks, this is the only pointer we have left. May be we need to use that for GstMiniObjectPrivate and put the list there.
(In reply to comment #6) > @@ +148,2 @@ > /*< private >*/ > + GSList *wrefs; > > irks, this is the only pointer we have left. May be we need to use that for > GstMiniObjectPrivate and put the list there. Yes, definitely. Use the GType instance private mechanism here and only store a pointer to that struct. There will be more things we want to put into GstBuffer ;)
It's quite easy to have a pool of underlying memory, as opposed to a pool of buffers. Create a buffer: - select a free chunk from the pool, mark as non-free - create an appropriate buffer for that memory - set buffer->free and buffer->malloc_data When the buffer is unreffed 1->0, buffer->free is called: - mark chunk as free Also, > And using subclassed GstBuffer prevents the use of gst_pad_alloc_buffer() call. This is not correct, as xvimagesink subclasses GstBuffer and provides buffers to peers.
FWIW, there is also a per-buffer freefunction.
(In reply to comment #8) > It's quite easy to have a pool of underlying memory, as opposed to a pool of > buffers. > > Create a buffer: > > - select a free chunk from the pool, mark as non-free > - create an appropriate buffer for that memory > - set buffer->free and buffer->malloc_data > > When the buffer is unreffed 1->0, buffer->free is called: > > - mark chunk as free But the point is that my element doesn't own that underlying memory; it is being owned and pooled by other element in downstream pipeline, for example xvimagesink. However, my element would still like to have a pool of buffers, which I want to allocate from that downstream element (from hardware). > > And using subclassed GstBuffer prevents the use of gst_pad_alloc_buffer() call. > > This is not correct, as xvimagesink subclasses GstBuffer and provides buffers > to peers. What I ment that if my own element wants to create a subclass of GstBuffer (and create a pool of those), then it cannot allocate those subclassed buffers from downstream without hacks. (In reply to comment #9) > FWIW, there is also a per-buffer freefunction. Yes, I know. But if the one who gave me the buffer (e.g. xvimagesink) pools those buffers, then this freefunction isn't called, since the buffer isn't physically freed (buffer is just put back to pool). So, ultimately what I'd like to achieve: 1) My (source) element wants to pre-allocate a pool of buffers from downstream 2) The element wants to know when the pushed buffer has been rendered, so that it can revive it back to the pool To solve (1), the element must use gst_pad_alloc_buffer(). To solve (2), the element must get a signal that buffer has been freed (by the sink). But this is currently impossible.
(In reply to comment #10) <snip> > Yes, I know. But if the one who gave me the buffer (e.g. xvimagesink) pools > those buffers, then this freefunction isn't called, since the buffer isn't > physically freed (buffer is just put back to pool). > > So, ultimately what I'd like to achieve: > > 1) My (source) element wants to pre-allocate a pool of buffers from downstream > 2) The element wants to know when the pushed buffer has been rendered, so that > it can revive it back to the pool what is the point of (2)? when the buffer is rendered it will go back to the owner of the pool (xvimagesink) why would a downstream element be interested in when that happens? > > To solve (1), the element must use gst_pad_alloc_buffer(). > To solve (2), the element must get a signal that buffer has been freed (by the > sink). But this is currently impossible.
(In reply to comment #11) > > So, ultimately what I'd like to achieve: > > > > 1) My (source) element wants to pre-allocate a pool of buffers from downstream > > 2) The element wants to know when the pushed buffer has been rendered, so that > > it can revive it back to the pool > > what is the point of (2)? when the buffer is rendered it will go back to the > owner of the pool (xvimagesink) why would a downstream element be interested in > when that happens? The point is that this source element wants to get that pre-allocated buffer back to its own pool. It doesn't want it to go into the owner's pool. For example, if you run v4l2src ! xvimagesink pipeline, there will be one memcpy() per frame, when xvimagesink (or X window system in general) copies the video frame to video hardware memory. How can we eliminate that memcpy() operation?
miniobject has weak refs in 0.10 and in 0.11 we have a bufferpool ref in the buffer object. closing.