After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 619500 - It is not possible to "recycle" buffers without subclassing GstBuffer object
It is not possible to "recycle" buffers without subclassing GstBuffer object
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
0.10.29
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-05-24 11:16 UTC by Tommi Myöhänen
Modified: 2012-05-18 08:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch which adds weak reference handling to GstMiniObject (5.26 KB, patch)
2010-05-24 11:16 UTC, Tommi Myöhänen
needs-work Details | Review

Description Tommi Myöhänen 2010-05-24 11:16:33 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.
Comment 1 Mark Nauwelaerts 2010-05-25 09:57:27 UTC
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).
Comment 2 Benjamin Otte (Company) 2010-05-25 11:51:15 UTC
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.
Comment 3 Benjamin Otte (Company) 2010-05-25 12:00:43 UTC
(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.
Comment 4 Tommi Myöhänen 2010-05-25 12:11:26 UTC
(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.
Comment 5 Benjamin Otte (Company) 2010-05-25 12:27:28 UTC
Oh gosh, yeah, that is nasty.
Could the decoder and sink elements cooperatively handle the buffer pool maybe?
Comment 6 Stefan Sauer (gstreamer, gtkdoc dev) 2010-09-01 15:09:21 UTC
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.
Comment 7 Sebastian Dröge (slomo) 2010-09-01 15:17:16 UTC
(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 ;)
Comment 8 David Schleef 2010-12-04 22:05:22 UTC
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.
Comment 9 Wim Taymans 2011-02-14 17:16:29 UTC
FWIW, there is also a per-buffer freefunction.
Comment 10 Tommi Myöhänen 2011-02-22 14:21:22 UTC
(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.
Comment 11 Wim Taymans 2011-02-22 14:50:12 UTC
(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.
Comment 12 Tommi Myöhänen 2011-02-22 15:19:07 UTC
(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?
Comment 13 Wim Taymans 2012-05-18 08:03:32 UTC
miniobject has weak refs in 0.10 and in 0.11 we have a bufferpool ref in the buffer object. closing.