GNOME Bugzilla – Bug 113180
Some GstBuffer comments
Last modified: 2004-12-22 21:47:04 UTC
I'm going over the gstbuffer.h API currently, and have got some questions/comments on struct GstBuffer. 1) size is a guint, maxsize is guint64. Why? This makes little sense. The size should always be optionally as large as maxsize (wy else is maxsize of any interest?). I suppose both should be a gulong or guint. guint64 is not interesting, since we're not dealing with whole *files* here, but just *buffers. If a buffer is larger than 4 gig, something is wrong upstream. Making it a guint64 is fine with me, but making maxsize/size a different type is quite weird. 2) offset should be documented. Is it offset in the file, offset in the ring buffer, ...? It says media-specific, does it mean I can use it for anything? Please document it. Inside the .h file, that is. ;). 3) there is no 'duration' (GstClockTime). This is interesting for VBR audio and subtitles.
1) maxsize and size should be the same format _and_ maxsize should actually be set. The core doesn't initialize it afaik. We should make it so that buf->size <= buf->maxsize is true everywhere. 2) Afaik offset is supposed to be byteoffset. I think only filesrc sets it. I'd prefer if buffers would carry GstFormat/guint64 tuples, just like discont events. 3) The tuples idea goes for buffer size, too. I don't know how fast that would be though, but optimization should come last anyway. :)
1) not sure why the maxsize could be used for, I think it was added to indicate the size of the allocated data so that a plugin could use the extra allocated space when it is smaller than size. 2) offset is media specific (frames for video, samples for audio, bytes for mpeg/mp3, ...) 3) I don't see a need for multiple offset/format pairs currently, we don't even use the single offset value. I do see the need for an indication of how long this buffer is (in time probably)
Created attachment 16595 [details] [review] core changes
With this, the plugins still need to make actual use of it, but it's a start. Can I apply this?
- GST_BUFFER_MAXSIZE (copy) = GST_BUFFER_MAXSIZE (buffer); + GST_BUFFER_MAXSIZE (copy) = GST_BUFFER_SIZE (buffer) yes, but after this is fixed (it was ok)
'duration' would be very useful in gst-sci. 'offset' is used as a byte offset in subbuffers compared to the parent buffer, so it probably isn't supposed to be media-dependent. While you're at it, GstData::free needs a closure gpointer and/or GstData needs a 'gpointer private' member.
yes, we need an additional mediaoffset field then. Not sure what you mean with the private gpointer; do you want to store a pointer to private data in the event? isn't that best done by subclassing the data structure?
Wim, I think that's actually correct. Look at this: GST_BUFFER_DATA (copy) = g_memdup (GST_BUFFER_DATA (buffer), GST_BUFFER_SIZE (buffer)); The *new* buffer doesn't have the size of the old buffer (maxsize), but the size of the *used* memory (size). Or does g_memdup() do something else? It seemed like a potential bug to me.
On second tought, the offset field should be made media specific. The offset field is currently updated when creating a subbuffer but any plugin can change it afterwards. Should we still update the offset field or let the plugin update it?
Ronald, yes, sorry, your code is right. I was confused :)
The offset field needs to be initialized to an INVALID value when creating/copying a buffer. You may initialize it to the same value as the original buffer in the special case where a subbuffers starts at the beginning of its parent. And if we do a media specific offset field (I still like bytes more) we need to define what offset we use for every media format. That would require getting caps docs going. Another thing: Would it make sense to restrict events (seek, discont) and buffers to 3 formats (TIME, BYTE, media-specific aka UNITS) and allow getting other formats only by querying pads/elements?
first attempt commited to HEAD
This is in HEAD now...