GNOME Bugzilla – Bug 351768
Unwanted concurent buffer modifications
Last modified: 2006-08-21 09:30:35 UTC
I have a pipeline +-- retimestamp -- audioconvert tee --| +-- retimestamp -- audioconvert My retimestamp plugin only calls gst_buffer_make_metadata_writable and changes timestamp The problem is that gst_buffer_make_metadata_writable is implemented as gst_buffer_create_sub internally. The created subbuffer has only one reference and hence it is writable (is it possible that it is writable even if the parent has READONLY flag set?!). Now, both the duplicated buffers created by tee are writable and point to the same memory, this is a clear problem when audioconvert chooses to the the converion in place. I think, that the correct solution would be to mark all subbuffers as READONLY.
Allright, this does not belong exactly here, but should not gst_buffer_make_metadata_writable copy the buffer duration and caps too? Currently it only copies timestamp and offset. I could open an enhancement "bug" for this but it would be probably only waste of one bugnumber.
You're quite right. Ordinarily, creating a subbuffer sets the READONLY flag, but make_metadata_writable is overwriting it by copying the flags directly off the parent buffer. In fact, make_metadata_writable could check whether the buffer is writable (using gst_buffer_is_writable) before subbuffering and set the READONLY flag on the subbuffer accordingly - if we subbuffer a writable buffer and then drop the reference, the child is still writable, because there will still only be one reference to the parent. Also, yes, I think DURATION, OFFSET_END and CAPS should be copied too. This could possibly be done as a generic thing if making a subbuffer that is the entire original buffer.
Created attachment 71089 [details] [review] Fix wrong GST_BUFFER_FLAG_READONLY and set buffer metadata Oh, at last I see the whole picture (thanks Jan). I could not find where the subbuffers are made READONLY, the real problem is GST_BUFFER_FLAGS (ret) = GST_BUFFER_FLAGS (buf); Some of the code could be moved to gst_buffer_create_sub
Created attachment 71090 [details] [review] LIke the previous patch but set READONLY flag allways
(In reply to comment #2) > if we subbuffer a writable buffer and then drop the > reference, the child is still writable, because there will still only be one > reference to the parent. But the parent buffer will still be refenced by the child and if somebody makes child's subbuffer, than the child2 will reference parent as its parent and not child. Thus child will remain writable but it will point to the same data as chil2.
Created attachment 71130 [details] [review] updated patch How about this patch? moves the end/duration copying to create_sub.
To Wim: I like your patch, end/duration/caps copying makes more sense in create_sub.
* gst/gstbuffer.c: (gst_buffer_make_metadata_writable), (gst_buffer_create_sub): Copy duration/offset_end/caps when creating a subbuffer of the complete parent. Make the subbuffer read-only when we make the metadata writable for now. Fixes #351768. * tests/check/gst/gstbuffer.c: (GST_START_TEST): Added check for metadata copy when creating subbuffers.