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 791415 - buffer: store sequence number for metas
buffer: store sequence number for metas
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other All
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 790470 791918
 
 
Reported: 2017-12-09 12:32 UTC by Tim-Philipp Müller
Modified: 2018-11-03 12:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
buffer: store sequence number for metas (6.34 KB, patch)
2017-12-09 12:32 UTC, Tim-Philipp Müller
accepted-commit_now Details | Review

Description Tim-Philipp Müller 2017-12-09 12:32:18 UTC
Created attachment 365288 [details] [review]
buffer: store sequence number for metas

For metas where order might be significant if multiple metas are
attached to the same buffer, store a sequence number with the
meta when adding it to the buffer. This allows users of the meta
to make sure metas are processed in the right order.

We need a 64-bit integer for the sequence number here in the API,
a 32-bit one might overflow too easily with high packet/buffer
rates. We could do it rtp-seqnum style of course, but that's a
bit of a pain.
    
We could also make it so that gst_buffer_add_meta() just keeps metas in
order or rely on the order we add the metas in, but that seemed too
fragile overall, when buffers (incl. metas) get merged or split.
    
Also add a compare function for easier sorting.
    
We store the seqnum in the MetaItem struct here and not in the
GstMeta struct since there's no padding in the GstMeta struct.
We could add a private struct to GstMeta before the start of
GstMeta, but that's what MetaItem effectively is implementation-
wise. We can still change this later if we want, since it's all
private.
Comment 1 Nicolas Dufresne (ndufresne) 2017-12-09 20:49:01 UTC
Interesting proposition I didn't check the code yet. Do you have sample code to demonstrate its use. It does not solve everything though. An hypothetical pipeline like this (assume that rotate and textoverlay can add meta, and also apply the meta, could also be other element)

.. ! videocrop ! rotate ! rotate ! videocrop ! ..


Event if the second rotate, which will apply the hypothetical meta knows there is other meta before, it does not know if it is correct to apply it or if it should fait the pipeline. In this case you can see the they are miss ordered.
Comment 2 Nicolas Dufresne (ndufresne) 2017-12-09 20:51:43 UTC
Maybe we need a tag for meta that represents a transformation of the buffer.  And then we can notify the dev if something is out of order or missing handler.
Comment 3 Tim-Philipp Müller 2017-12-09 21:00:35 UTC
I'm not claiming that this will solve  all use cases or those use cases you describe :)

I have used this in various scenarios where metadata is received e.g. in RTP header extensions or otherwise attached to buffers and either transported through the pipeline or saved to file to be replayed and streamed out again.

In those cases it was important to preserve the global ordering of the metas as they are attached to the buffers in the various places, and across buffer transformations.
Comment 4 Sebastian Dröge (slomo) 2018-01-03 13:27:11 UTC
Review of attachment 365288 [details] [review]:

Looks good to me

::: gst/gstbuffer.c
@@ +134,3 @@
 GType _gst_buffer_type = 0;
 
+/* FIXME: -sizeof(GstMeta) as it is already included in the info size? */

Should just solve this FIXME before merging (in a separate commit) :)
Comment 5 GStreamer system administrator 2018-11-03 12:43:39 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/262.