After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 351768 - Unwanted concurent buffer modifications
Unwanted concurent buffer modifications
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal major
: 0.10.10
Assigned To: Wim Taymans
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2006-08-17 15:27 UTC by Michal Benes
Modified: 2006-08-21 09:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix wrong GST_BUFFER_FLAG_READONLY and set buffer metadata (846 bytes, patch)
2006-08-17 16:25 UTC, Michal Benes
none Details | Review
LIke the previous patch but set READONLY flag allways (761 bytes, patch)
2006-08-17 16:44 UTC, Michal Benes
none Details | Review
updated patch (3.21 KB, patch)
2006-08-18 08:56 UTC, Wim Taymans
committed Details | Review

Description Michal Benes 2006-08-17 15:27:47 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.
Comment 1 Michal Benes 2006-08-17 15:34:09 UTC
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.
Comment 2 Jan Schmidt 2006-08-17 16:00:41 UTC
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.
Comment 3 Michal Benes 2006-08-17 16:25:22 UTC
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
Comment 4 Michal Benes 2006-08-17 16:44:08 UTC
Created attachment 71090 [details] [review]
LIke the previous patch but set READONLY flag allways
Comment 5 Michal Benes 2006-08-17 16:45:00 UTC
(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.
Comment 6 Wim Taymans 2006-08-18 08:56:07 UTC
Created attachment 71130 [details] [review]
updated patch

How about this patch? moves the end/duration copying to create_sub.
Comment 7 Michal Benes 2006-08-21 07:22:18 UTC
To Wim: I like your patch, end/duration/caps copying makes more sense in create_sub.
Comment 8 Wim Taymans 2006-08-21 09:30:35 UTC
        * 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.