GNOME Bugzilla – Bug 742385
video/audio encoders/decoders: need API to determine when to copy over GstMetas and when to drop them
Last modified: 2015-08-16 13:40:36 UTC
I made my custom version of multifilesrc which loads multiple jpg files each into a separate GstBuffer. I also add my custom metadata structure. However when I decode the jpg files using jpegdec; my metadata is systematically removed. jpegdec should not remove the custom metadata.
Can you share some of the code? That way I can reproduce the bug easily and potentially investigate it.
Created attachment 293825 [details] source code to make meta api library
That's a lot of code to share... I attached some (very simplified) source code for my metadata library. then in the source plugin I use gst_buffer_add_meci_meta(buffer, timestamp, idx) etc... I hope this helps you on your way...
Do you have a self-contained example that would trigger this bug? I can't run this very simplified source code you attached, and it doesn't tell me much. Do you think there is a bug in jpegdec? Could you write some code that directly triggers this bug? (without custom multifilesrc or metadata structures).
nope that is not possible, since I add my custom metadata. When using jpegenc however the metadata is not removed. By having a quick look at the code, I suppose the cause is that jpegenc just allocates new memory (not a new buffer), while jpegdec allocates a whole new buffer; thus removing the custom metadata.
Just to clarify, when you say it is not possible, you are answering to my question about writing a self-contained example to trigger this bug? So is the bug in your custom metadata code or in jpegdec? Unless you point out to a bug in some upstream code in gst-plugins-good, I can't help :( and in that case the best approach would be to ask in the devel mailing list [1] how to build this around GStreamer. [1] http://gstreamer.freedesktop.org/lists/
I don't think there's anything jpegdec-specific about this, if any GstMetas are copied over to the output through a decoder, that's probably mostly by accident at the moment :)
I agree with Tim. Having that in mind, Chris, what did you mean by "jpegdec should not remove the custom metadata."? Want to clarify that before I close this bug, and not leave you hanging.
Ok, I'll start over and try to specify everything. I am describing a bug in jpegdec ; not in my own code. First I created my own metadata following this example: http://gstreamer.freedesktop.org/data/doc/gstreamer/head/pwg/html/section-allocation-meta.html (and I joined that source code). It is fully functional; have been using this for over a year now. So if I do: gst-launch-1.0 filesrc ! addmeta ! printmeta ! fakesink (with "addmeta" and "printmeta" two fictional; custom-made plugins. addmeta will add my metadata structure and printmeta will print out whether my metadata is present or not and what is inside). This pipeline will work correctly (thus printing out my metadata for each buffer). If I do: gst-launch-1.0 filesrc ! addmeta ! jpegenc ! printmeta ! fakesink then my metadata will still be printed out without problem. Now the bug: if I do: gst-launch-1.0 filesrc ! addmeta ! jpegdec ! printmeta ! fakesink my metadata will not be printed since jpegdec removed it.
Sorry this issue of yours has evolved to this level of discussion about GstMetas. From the resource you mention, "With the GstMeta system you can add arbitrary structures on buffers. These structures describe extra properties of the buffer such as cropping, stride, region of interest etc." My understanding of what this means is that GstMetas are meant to be a solution to pass extra information related to the buffer that the elements handling this buffer (output/input) should know about. I think what Tim is trying to say, using your example as a basis, is that jpegenc wasn't intended to copy over metadata of filesrc's video/x-raw buffer into the image/jpeg buffer. Since this wasn't designed to be and is happening by accident. It can't be expected to happen in other elements, or for that matter, probably not permanently in jpegenc. Can you give me an example of when it would be a benefitial feature to copy the metadata from one GstBuffer in the input side of an element to the GstBuffer generated on the other side?
A few months ago I discussed this on the #gstreamer channer on IRC, and the developers opinion there was that the new GstMeta had been designed so that the metadata should be retained across all plugins, and that if jpegdec didn't; this was a bug. In my case: I have stored jpegs in which I encoded the camera parameters (exposure time and gain). I wish to replay these in a gstreamer chain. My version of multifilesrc opens these jpegs and adds these camera parameters as metadata to the buffers. Then I decode the jpeg and I lose all that metadata; while they remain important for that buffer. The way the buffer is encoded doesn't change its camera parameters. By the way it is not only jpegenc that preserves the metadata: I didn't test many of them, but videoconvert preserves them as well (and most plugins that don't really change the buffer like tee, queue,...)
To be sure, I think IMHO it's entirely reasonable to expect that certain GstMetas get copied over through the encoding/decoding process, I'm just saying that I don't think we handle that yet and that we have to agree on conventions how to signal whether a certain meta should be dropped when encoding/decoding or not.
You are using the GstMeta for EXIF data. I agree that makes sense. Tim, should I write a patch that copies along the GstMetas in jpegenc for now or wait until there is a system/convention to communicate if this needs to be copied or not?
I think we should first figure out how to handle this in general. Passing through all metas now and then stopping to copy over some metas later will be a regression otherwise ;) Btw, just for your information: gst-launch-1.0 filesrc location=photo.jpg ! jpegparse ! fakesink silent=false -v | grep -i tag | more GstTagList-stream, taglist=(taglist)"taglist\,\ device-manufacturer\=\( string\)Nokia\,\ device-model\=\(string\)N900\,\ image-orientation\=\(string\)rotate-0\,\ image-horizontal-ppi\=\(double\)300\,\ image-vertical-ppi\=\(double\)300\,\ datetime\=\(datetime\ )2014-03-14T16:51:15Z\,\ capturing-shutter-speed\=\(fraction\)9/500\,\ capturing-focal-ratio\=\(double\)2.7999999999999998\,\ capturing-exposure-program\=\(string\)undefined\,\ capturing- flash-fired\=\(boolean\)false\,\ capturing-focal-length\=\(double\)5.2000000000000002\,.... This will extract (mapped) EXIF data into GstTagLists, and those should be passed through by the decoder/encoder. You'll get a new tag event for every frame then of course, but that would work fine already. (This is not to make a statement about whether GstMetas make more sense here or not, just offering another option in case you weren't aware of it.)
There are the tags in the meta API and the transform functions to define which metas can be copied when, and which need to be transformed how.
http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/gstreamer-GstMeta.html GST_META_FLAG_LOCKED, GST_META_TRANSFORM_IS_COPY(type), and typedef struct GstMetaTransformCopy. Looks like it. I can look tomorrow into how to have jpegenc use this, and if there is any further work to do of what Tim mentioned.
Take a look at what basetransform does with the tags and transform functions, IIRC audiofilter/videofilter base classes also do something more on top.
Cool! Thanks for the tip.
just a small remark: as far as I'm concerned jpegenc is ok, jpegdec is not; so if you plan to adapt jpegenc to have some kind of flag marking metadata to be NOT removed, please do so as well for jpegdec...
Luis, any progress on this?
I wonder if we should just add a series of new "transforms" to meta, something like a TRANSFORM_ENCODE, TRANSFORM_DECODER and TRANFORM_ADPATER. The last one is particularly useful for metas to survive their trip through parsers, for example, in GstVaapi... everything goes through the parse functions, that would let the metas through.
Actually, forget that, the adapter should just do a copy of the METAs from the first input buffer used to create the output I think.
From the first or from all? Or should it somehow give the caller a way to decide about that based on the actual meta? I think a bit more is needed for GstAdapter here than that.
Created attachment 306283 [details] [review] videoencoder: Add transform_meta() vfunc with default implementation The default implementation copies all metadata without tags, and metadata with only the video tag. Same behaviour as in GstVideoFilter.
(In reply to Sebastian Dröge (slomo) from comment #24) > Created attachment 306283 [details] [review] [review] > videoencoder: Add transform_meta() vfunc with default implementation > > The default implementation copies all metadata without tags, and metadata > with only the video tag. Same behaviour as in GstVideoFilter. Here's the most simple case, videoencoder. Will do videodecoder too now, then audio decoder/encoder which probably involves GstAdapter. Then baseparse.
Created attachment 306289 [details] [review] videoencoder: Add transform_meta() vfunc with default implementation The default implementation copies all metadata without tags, and metadata with only the video tag. Same behaviour as in GstVideoFilter.
Created attachment 306290 [details] [review] videodecoder: Add transform_meta() vfunc with default implementation The default implementation copies all metadata without tags, and metadata with only the video tag. Same behaviour as in GstVideoFilter. This currently does not work if the ::parse() vfunc is implemented as all metas are getting lost inside GstAdapter.
Created attachment 306300 [details] [review] adapter: Copy over GstMeta from the input buffers to the output All functions that return a GstBuffer or a list of them will now copy all GstMeta from the input buffers except for meta with GST_META_FLAG_POOLED flag or "memory" tag. This is similar to the existing behaviour that the caller can't assume anything about the buffer flags, timestamps or other metadata.
Created attachment 306301 [details] [review] adapter: Copy over GstMeta from the input buffers to the output All functions that return a GstBuffer or a list of them will now copy all GstMeta from the input buffers except for meta with GST_META_FLAG_POOLED flag or "memory" tag. This is similar to the existing behaviour that the caller can't assume anything about the buffer flags, timestamps or other metadata. And it's also the same that gst_adapter_take_buffer_fast() did before, and what gst_adapter_take_buffer() did if part of the first buffer or the complete first buffer was requested.
Created attachment 306302 [details] [review] adapter: Copy over GstMeta from the input buffers to the output All functions that return a GstBuffer or a list of them will now copy all GstMeta from the input buffers except for meta with GST_META_FLAG_POOLED flag or "memory" tag. This is similar to the existing behaviour that the caller can't assume anything about the buffer flags, timestamps or other metadata. And it's also the same that gst_adapter_take_buffer_fast() did before, and what gst_adapter_take_buffer() did if part of the first buffer or the complete first buffer was requested.
Created attachment 306303 [details] [review] audioencoder: Add transform_meta() vfunc with default implementation The default implementation copies all metadata without tags, and metadata with only the audio tag. Same behaviour as in GstAudioFilter.
Sebastian, Thanks for the patches. Looking good. Sorry I never finished this task.
Created attachment 306306 [details] [review] audiodecoder: Add transform_meta() vfunc with default implementation The default implementation copies all metadata without tags, and metadata with only the audio tag. Same behaviour as in GstAudioFilter.
baseparse mostly works because of the adapter changes, unless a subclass does special things with buffers.
Created attachment 306377 [details] [review] adapter: Add get variants of the buffer based take functions Main difference to gst_adapter_map() for all practical purposes is that GstMeta of the buffers will be preserved.
Created attachment 306378 [details] [review] baseparse: Use new gst_adapter_get_buffer() API instead of gst_adapter_map() This preserves GstMeta properly unless the subclass does special things. It's enough to make h264parse's stream-format/alignment conversion pass through metas as needed.
qtdemux and matroskademux also passes through meta from source to downstream in push mode, the adapter changes seemed to have handled many cases.
Created attachment 306389 [details] [review] opus: Copy metadata in the (de)payloader, but only the relevant ones The payloader didn't copy anything so far, the depayloader copied every possible meta. Let's make it consistent and just copy all non-memory, non-pooled metas without tags or with only the audio tag. Also see https://bugzilla.gnome.org/show_bug.cgi?id=751712
Created attachment 306414 [details] [review] rtp/vp8: Copy metadata in the (de)payloader, but only the relevant ones The payloader didn't copy anything so far, the depayloader copied every possible meta. Let's make it consistent and just copy all non-memory, non-pooled metas without tags or with only the video tag. Also see https://bugzilla.gnome.org/show_bug.cgi?id=751712
(In reply to Sebastian Dröge (slomo) from comment #39) > Created attachment 306414 [details] [review] [review] > rtp/vp8: Copy metadata in the (de)payloader, but only the relevant ones This change will have to be done for all payloaders and depayloaders, and should probably be moved into some helper function if this is considered a sensible change.
Created attachment 306423 [details] [review] rtp/h264: Copy metadata in the (de)payloader, but only the relevant ones The payloader didn't copy anything so far, the depayloader copied every possible meta. Let's make it consistent and just copy all non-memory, non-pooled metas without tags or with only the video tag. Also see https://bugzilla.gnome.org/show_bug.cgi?id=751712
I'll push the patches for decoder/encoder/parser/adapter in a bit if nobody complains as those just implement behaviour that is elsewhere already and make it consistent. For the RTP changes, I'll refactor that a bit and put the meta handling into some helper function, and then modify all (de)payloaders accordingly. Which should be a matter of adding <10 lines then for each.
Created attachment 306477 [details] [review] rtp/{vp8,h264}: Copy metadata in the (de)payloader, but only the relevant ones The payloader didn't copy anything so far, the depayloader copied every possible meta. Let's make it consistent and just copy all non-memory, non-pooled metas without tags or with only the video tag.
Created attachment 306478 [details] [review] opus: Copy metadata in the (de)payloader, but only the relevant ones The payloader didn't copy anything so far, the depayloader copied every possible meta. Let's make it consistent and just copy all metas without tags or with only the audio tag.
Created attachment 306479 [details] [review] rtp/{vp8,h264}: Copy metadata in the (de)payloader, but only the relevant ones The payloader didn't copy anything so far, the depayloader copied every possible meta. Let's make it consistent and just copy all metas without tags or with only the video tag.
Let's close this one and talk about the RTP stuff in another bug: bug 751774