GNOME Bugzilla – Bug 572285
Buffer Lists
Last modified: 2009-05-12 13:34:43 UTC
Please describe the problem: 1) Be able to push many buffers in one go in elements that spilt one buffer to many 2) Avoid memcpy of original buffer when inserting headers between chunks of payload data Steps to reproduce: 1. 2. 3. Actual results: Expected results: Does this happen every time? Other information:
Created attachment 128990 [details] [review] buffer arrays in core Adds GstBufferArray type and support for pushing arrays in GstPad.
Missing feature: gst_buffer_array_make_writeable() How can we implement it?
<slomo> jij: looks like it could cause alignment problems... you store the length as guint before the array of GstBufferArrayData, if sizeof(guint)!=sizeof(gpointer) you store a pointer (data field of GstBufferArrayData) not pointer aligned <jij> slomo, darn, I knew that would cause some problem <slomo> jij: look at libgstfft and grep for STRUCT_ALIGN or ALIGN_STRUCT <jij> slomo, why didn't someone add a size field for the free func in GstBuffer :) <jij> slomo, thx <slomo> jij: essentially you have to allocate 2*sizeof(size_t) for the size field and put the array afterwards <jij> slomo, ok Also it would make sense to use GstMiniObject as base class here, it would give you refcounting and all that.
Created attachment 129072 [details] [review] updated patch Updated patch: - fix docs - clean up weird pointer math - clean up method names - clean up pad code
It's still no GstMiniObject though ;)
The problem with gst_buffer_array_make_writeable() is that if the buffer array contain pointers into a "parent buffer" (e.g. array with RTP payloaded H.264 points into a H.264 buffer), the only way to check if that data is writeable is to check that refcount of the parent buffer equals the number of refs the array has to that parent buffer. Another problem is that calling gst_buffer_make_writeable() on a group buffer will return the same buffer if refcount is 1 on the group buffer even if the group data points to data in a parent buffer with refcount >1.
I've been meaning to add disjoint buffers to GStreamer for years. Thank you for actually moving on the issue. My plan was to split GstBuffer into 2 layers, the lower layer being a simple wrapper of a contiguous memory region into a GstMiniObject, with reference counting but no metadata. The upper layer would contain the metadata and point to one or more MemoryRegions to hold the data, including an offset and size to specify a subregion of the MemoryRegion. This rearrangement would subsume subbuffers and most buffer subclasses. It might even take over GstAdapter, since adapters are an attempt to get around the disjoint buffer limitation. It would allow buffers to share underlying data while having different (writeable) metadata. Obviously, all this requires an ABI change, so would be for 0.11 at least, assuming someone does it. I wouldn't be opposed to having something like this in 0.10. An actual comment on the patch: Couldn't this be better solved by extending (ahem, hacking to death) GstBuffer in a compatible way?
Maybe we still need a subclass of the buffers in the array so that we can, for example, implement a working copy function. If that's the case we could expose the group methods on this new type. It all seems a bit too much for what we can reasonably implement right now. If you want to transfer some GstBufferArrayData element to another buffer, there is not really a way to increment the refcount of whatever object actually owns that data pointer unless you also add a _ref method to the Data structure. I would think that for 0.10 we want to create the buffer array with its special buffers inside but that's it. Modifications to these arrays seems too much to handle without doing some serious GstBuffer redesigns.
Created attachment 129655 [details] [review] updated patch with _make_data_writable() Added gst_buffer_array_make_data_writable() and a debug category for buffer array.
Created attachment 129892 [details] [review] Updated patch using mini object * GstBufferArray extends GstMiniObject * Added copy-function (mini objects need it) * Updated docs sections
Created attachment 130390 [details] [review] rtph264 payloader with buffer arrays and unit tests
Created attachment 130391 [details] [review] basertp payloader and gstrtpbuffer with buffer arrays including unit tests
Created attachment 130392 [details] [review] basesink with buffer arrays
Created attachment 130398 [details] [review] multiudpsink with buffer arrays, unit tests also included
Created attachment 133027 [details] [review] buffer list in core Updated core patch using linked list and iterator instead of array.
ah, looks neater indeed. I'm wondering if the copy function should really do a deep copy instead of a shallow one. The make writable on the list could mean that it's ok to change the list. But to actually change the buffer contents one would explicitly make the buffer writable first. Is there a function to replace a buffer too?
Created attachment 133453 [details] [review] updated buffer list in core Made copy function shallow and added functions for removing and replacing buffers.
How does one append a buffer to a group?
With the current API one would find the correct position using _next_group() and _next() (returns NULL at the end of the group) and then _add().
Commited into git.