Created attachment 115627 [details] [review]
patch for gstreamer

This patch makes the SubBuffer type public and add's a GST_SUBBUFFER_PARENT macro available to access the parent of a subbuffer.

Created attachment 115629 [details] [review]
patch for gst-plugins-base

Use the parent of the subbuffer if this is a xvimagebuffer.

xvimagesink should not be providing buffers via its pad_alloc function with a refcount greater than 1.  This needs to be fixed if it's broken.

(In reply to comment #3)
> xvimagesink should not be providing buffers via its pad_alloc function with a
> refcount greater than 1.  This needs to be fixed if it's broken.
> 
It's not doing that, it is delivering buffers with refcount = 1 as expected.

For example:
The decoder has requested with pad_alloc a buffer and get's the xvimagebuffer (with refcount=1). he pushes it and identity is signalling this buffer in his transform_ip function. Playbin is listening to this signal and ref's the buffer. When then there is a discont in the stream another basetransform element (like ffmpegcolorspace) want to mark the buffer as DISCONT and tries to make the metadata writable by making a subbuffer of the xvimagebuffer. xvimagesink receives this information and handles it with the slow method while it is able to do the fast method for this frame.
 

Whoever's fault it is (I think basetransform, but it really doesn't matter), it seems to me like we're hitting a problem with the buffer design here and any sort of gst_buffer_get_parent_buffer() method would just be a hack to work around that.

IMO this needs to be sorted out via a ::make_metadata_writable and/or ::create_subbuffer vfunc or somesuch, so that GStreamer can do the right thing automagically.

Tim, I agree with you that we should have virtual functions for this. But I think you can't add new virtual functions without breaking the ABI. So it is not a solution for the 0.10 series. (or am I wrong in this?)

Option b) with the gst_buffer_get_parent_buffer is a hack because parent buffer does not exists in the instance of a GstBuffer. But I proposed it because it is the hack with the least impact. (only one extra function, easily removable when we fix this for real with the virtual function in 0.11)

I prefer option a) for making the subbuffer public, like in the attached patches. Because it does not change anything in the current behavior of gstreamer core. and It makes it pretty easy for the sink to recover the lost parent. I don't see a problem in making a class public that is already used and provided to the elements. Are there disadvantages at making this public ?

GstBuffer subclasses are pretty painfull for many reasons, exposing the fact that we have subbuffers as subclasses is not something that sounds very appealing. A little function to deref to the 'real' buffer object sounds sweeter.

Another option is to add the vmethods. There is only one free pointer in miniobject which can be used to by GstBuffer to store a pointer to an array of new vmethods that can be overridden by subclasses. not very nice but possible.

Any news on this? Is this something that we want/can fix for 0.10?

I have a patch to move the parent pointer into GstBuffer so that we can remove the subclasses for subbuffers. If that were commited, I would not much object to making a method to get the parent of a buffer to get the original type.

Commited this as part of a greater secret plan. Now we need to settle on some API to get back to the parent buffer type (and likely also the offset/size relative to it)

commit 67bd9529d0eaa169a2361fde91f4948ebaedbb98
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Wed Dec 2 19:47:46 2009 +0100

    buffer: remove subbuffer subclass
    
    Move the parent buffer pointer into the GstBuffer struct so that we can
    remove the subbuffer class and type. This is interesting because it allows us to
    more naturally implement methods to get the real type and parent
    of a subbuffer (See #545501).
    It should also be slightly faster because there is no extra object hierarchy to
    initialize and free.

heh, I just ran into exact same issue w/ v4l2sink.  And even in a pipeline without a tee, since gst_mini_object_free() increments the refcnt before calling finalize function (which puts buffer back on queue making it available to thread calling _buffer_alloc())

for now I just check:

    if (buf->parent &&
        (GST_BUFFER_DATA (buf) == GST_BUFFER_DATA (buf->parent)) &&
        (GST_BUFFER_SIZE (buf) == GST_BUFFER_SIZE (buf->parent))) {
      ....
    }

it's a bit ugly.. it might be nice to have a better API to tell me the same thing, that it's a sub-buf w/ offset of zero and size equal to original buffer.  Or even better if it was somehow transparent..

(In reply to comment #3)
> xvimagesink should not be providing buffers via its pad_alloc function with a
> refcount greater than 1.  This needs to be fixed if it's broken.

btw, I think it isn't always under the sink's control..  since the refcnt is incremented before the finalize function is called.  If the finalize function itself ref's the buffer and puts it in a pool of available buffers which another thread can pad_alloc from, depending on thread priorities and such, you could end up returning a buffer from pad_alloc w/ refcnt==2.

but it's also a legit use-case in pipelines with a tee, for example, or other cases.

I'm not quite sure what kind of semantics are needed. 

I guess you essentially want to know what the original type of the (parent) buffer is and if the data is still the same range in the subbuffer.

You would usually use this to get to the parent buffer and then cast that buffer to the right subclassed type and get your custom buffer fields from there.

something like

 GstBuffer * gst_buffer_cast (GstBuffer *buffer, GType type, guint *start_offset, guint *end_offset);

it takes a buffer, tries to cast it (or the parent) to @type and then fills in the start_offset and end_offset (when a subbuffer) and return a GstBuffer that you can then cast to GType or NULL. 

It doesn't quite look good but it would allow you to get a handle to a buffer of GType and fill in the offsets according to the parent. (0, 0) would mean that there are no offsets...

(In reply to comment #13)
> I'm not quite sure what kind of semantics are needed. 

What about some sort of clone() method, to allow creating a new instance of the same type of buffer..

so if gst_buffer_create_sub (buf, 0, GST_BUFFER_SIZE (buf)) could return a buffer that is the same GType as the original buffer, then it would be somewhat transparent to the receiver of the buffer that a new subbuffer was created.

(On the other hand, it might require any video sink that implements a buffer pool would have to add a new virtual method to their GstBuffer subclass.. or at least somehow handle the fact in it's finalize method that the buffer being finalized could be a clone.. so I guess that doesn't make it completely transparent.)

the title needs updating - GstBuffer has parent exposed nowadays.

Is this fixed in 0.11 already?

(In reply to comment #16)
> Is this fixed in 0.11 already?

There are no subbuffers with a parent like that in 0.11 anymore. You would use buffer metadata to store the extra fields you want and those are copied or duplicated correctly.