GNOME Bugzilla – Bug 516395
gst_buffer_create_sub does not copy flags
Last modified: 2008-02-15 12:31:59 UTC
It should copy the flags like it does when copying buffers. E.g. when creating a subbuffer of a buffer that has silence (_GAP), also the subbuffer has silence.
Created attachment 105215 [details] [review] copy the flags
It should probably copy the GAP flag but not always the others.
(I started typing this before Wim's comment, but since it's more detailed I might just as well post it anyway) The only flag that looks like it should be copied for sure is the GAP flag IMO. - READONLY: not sure, could be copied I guess, although it shouldn't really be necessary since sub-buffers are never writable (I think) - PREROLL: not used, is it? - DISCONT shouldn't be copied, I think (unless offset is 0 maybe) - IN_CAPS: shouldn't be copied (which you seem to agree with) - GAP: should be copied (as you do). - DELTA_UNIT: not sure about this one, I think the semantics are only really clear for the normal case where one would only look at the buffer as a whole (so probably shouldn't be copied) I think it would be best to just copy the flags we know make sense to copy, rather than just copy all flags unconditionally (even unknown ones of subclasses, which we don't know much about).
(I also started typing at the same time. :/ ) It currently does the same as in gst_buffer_make_metadata_writable() where it creates a sub_buffer too. If we can conclude I like to remove the special case handling in gst_buffer_make_metadata_writable(). GST_BUFFER_FLAG_READONLY should be set and is set in subbuffer_init already GST_BUFFER_FLAG_PREROLL should be copied too, if we peel off some bytes and send the subbuffer further it should keep the flag GST_BUFFER_FLAG_DISCONT same here, keep the flag GST_BUFFER_FLAG_IN_CAPS it would be good to explain in the docs, what the use case is here. I cleared the flag in the patch as its cleared in _make_metadata_writable() already for a reason. GST_BUFFER_FLAG_GAP should be copied as a subbuffer inherits this property GST_BUFFER_FLAG_DELTA_UNIT copy too, if the unit can not be decoded independently, how should it not be the same for even a subbuffer? Btw. the (core) test suite runs fine with the patch. So if its not good to do it, then we don't cover those cases yet.
READONLY is set in _init, so you can't overwrite this flag. IN_CAPS should be cleared. You can't decide on DISCONT. Take this example: 1 parent buffer of size 100 with DISCONT flag - create sub 0, 50 -> copy DISCONT - create sub 50, 100 -> ? You can't copy the discont flag on the second buffer, because if you send the first subbufer downstream, it's not DISCONT with that buffer. If you however drop the first subbuffer, then the DISCONT flag should be on the second buffer. DELTA_UNIT refers to the first byte in the buffer, it's possible that a subbuffer starts on a keyunit fine so you can't blindly copy the DELTA_UNIT flag. I would start with a patch that keeps the GAP and (possibly) the PREROLL flag of the parent buffer for now. If we find a good case for copying the others we'll add it but I would not do this right now.
Yes, I was thinking more about it and its up to the elemnts that create subbuffers to decide on DISCONT and DELTA. Also the code in gst_buffer_make_metadata_writable() should be keept as it is, because this is a special case where the whole buffer is subbuffered to make the metadata writable (as the name says). Now I just wonder if I should just move that to gst_buffer_create_sub() so that when a subbuffer is created with no offset and same length it copies the flags, otherwise it only copys PREROLL|GAP. It would be good to spend some further thoughts on the offset=0 case later on: find . -name "*.c" -exec grep -Hn "gst_buffer_create_sub" {} \; | grep ", 0," gives me 27 hits.
Created attachment 105272 [details] [review] copy only GAP and PREROLL
2008-02-15 Stefan Kost <ensonic@users.sf.net> * gst/gstbuffer.c: Copy selected buffer-flags when creating subbuffers. Fixes #516395.