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 742385 - video/audio encoders/decoders: need API to determine when to copy over GstMetas and when to drop them
video/audio encoders/decoders: need API to determine when to copy over GstMet...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal enhancement
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 751712
Blocks: 751647
 
 
Reported: 2015-01-05 14:57 UTC by Chris Maes
Modified: 2015-08-16 13:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
source code to make meta api library (1.69 KB, application/x-compressed-tar)
2015-01-05 15:20 UTC, Chris Maes
  Details
videoencoder: Add transform_meta() vfunc with default implementation (6.05 KB, patch)
2015-06-29 12:00 UTC, Sebastian Dröge (slomo)
none Details | Review
videoencoder: Add transform_meta() vfunc with default implementation (6.04 KB, patch)
2015-06-29 13:59 UTC, Sebastian Dröge (slomo)
committed Details | Review
videodecoder: Add transform_meta() vfunc with default implementation (6.55 KB, patch)
2015-06-29 13:59 UTC, Sebastian Dröge (slomo)
committed Details | Review
adapter: Copy over GstMeta from the input buffers to the output (3.08 KB, patch)
2015-06-29 15:12 UTC, Sebastian Dröge (slomo)
none Details | Review
adapter: Copy over GstMeta from the input buffers to the output (3.70 KB, patch)
2015-06-29 15:24 UTC, Sebastian Dröge (slomo)
none Details | Review
adapter: Copy over GstMeta from the input buffers to the output (3.55 KB, patch)
2015-06-29 15:25 UTC, Sebastian Dröge (slomo)
committed Details | Review
audioencoder: Add transform_meta() vfunc with default implementation (7.47 KB, patch)
2015-06-29 15:39 UTC, Sebastian Dröge (slomo)
committed Details | Review
audiodecoder: Add transform_meta() vfunc with default implementation (7.04 KB, patch)
2015-06-29 16:00 UTC, Sebastian Dröge (slomo)
committed Details | Review
adapter: Add get variants of the buffer based take functions (15.15 KB, patch)
2015-06-30 09:12 UTC, Sebastian Dröge (slomo)
committed Details | Review
baseparse: Use new gst_adapter_get_buffer() API instead of gst_adapter_map() (2.09 KB, patch)
2015-06-30 09:19 UTC, Sebastian Dröge (slomo)
committed Details | Review
opus: Copy metadata in the (de)payloader, but only the relevant ones (4.44 KB, patch)
2015-06-30 11:54 UTC, Sebastian Dröge (slomo)
none Details | Review
rtp/vp8: Copy metadata in the (de)payloader, but only the relevant ones (4.54 KB, patch)
2015-06-30 14:38 UTC, Sebastian Dröge (slomo)
none Details | Review
rtp/h264: Copy metadata in the (de)payloader, but only the relevant ones (9.86 KB, patch)
2015-06-30 16:12 UTC, Sebastian Dröge (slomo)
none Details | Review
rtp/{vp8,h264}: Copy metadata in the (de)payloader, but only the relevant ones (12.94 KB, patch)
2015-07-01 09:59 UTC, Sebastian Dröge (slomo)
none Details | Review
opus: Copy metadata in the (de)payloader, but only the relevant ones (4.13 KB, patch)
2015-07-01 10:06 UTC, Sebastian Dröge (slomo)
none Details | Review
rtp/{vp8,h264}: Copy metadata in the (de)payloader, but only the relevant ones (12.92 KB, patch)
2015-07-01 10:07 UTC, Sebastian Dröge (slomo)
none Details | Review

Description Chris Maes 2015-01-05 14:57: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.
Comment 1 Luis de Bethencourt 2015-01-05 15:07:11 UTC
Can you share some of the code? That way I can reproduce the bug easily and potentially investigate it.
Comment 2 Chris Maes 2015-01-05 15:20:57 UTC
Created attachment 293825 [details]
source code to make meta api library
Comment 3 Chris Maes 2015-01-05 15:21:18 UTC
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...
Comment 4 Luis de Bethencourt 2015-01-05 15:32:51 UTC
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).
Comment 5 Chris Maes 2015-01-05 16:01:46 UTC
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.
Comment 6 Luis de Bethencourt 2015-01-05 16:10:31 UTC
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/
Comment 7 Tim-Philipp Müller 2015-01-05 16:19:06 UTC
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 :)
Comment 8 Luis de Bethencourt 2015-01-05 16:29:21 UTC
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.
Comment 9 Chris Maes 2015-01-05 17:11:23 UTC
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.
Comment 10 Luis de Bethencourt 2015-01-06 14:56:57 UTC
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?
Comment 11 Chris Maes 2015-01-07 07:21:32 UTC
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,...)
Comment 12 Tim-Philipp Müller 2015-01-07 10:57:56 UTC
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.
Comment 13 Luis de Bethencourt 2015-01-07 14:23:20 UTC
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?
Comment 14 Tim-Philipp Müller 2015-01-07 14:50:06 UTC
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.)
Comment 15 Sebastian Dröge (slomo) 2015-01-08 13:17:53 UTC
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.
Comment 16 Luis de Bethencourt 2015-01-08 17:19:23 UTC
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.
Comment 17 Sebastian Dröge (slomo) 2015-01-08 17:21:19 UTC
Take a look at what basetransform does with the tags and transform functions, IIRC audiofilter/videofilter base classes also do something more on top.
Comment 18 Luis de Bethencourt 2015-01-08 17:31:48 UTC
Cool! Thanks for the tip.
Comment 19 Chris Maes 2015-01-09 07:03:58 UTC
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...
Comment 20 Sebastian Dröge (slomo) 2015-03-17 11:36:47 UTC
Luis, any progress on this?
Comment 21 Olivier Crête 2015-04-23 23:42:41 UTC
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.
Comment 22 Olivier Crête 2015-04-23 23:49:42 UTC
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.
Comment 23 Sebastian Dröge (slomo) 2015-06-29 08:25:21 UTC
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.
Comment 24 Sebastian Dröge (slomo) 2015-06-29 12:00:23 UTC
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.
Comment 25 Sebastian Dröge (slomo) 2015-06-29 12:02:27 UTC
(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.
Comment 26 Sebastian Dröge (slomo) 2015-06-29 13:59:44 UTC
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.
Comment 27 Sebastian Dröge (slomo) 2015-06-29 13:59:51 UTC
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.
Comment 28 Sebastian Dröge (slomo) 2015-06-29 15:12:31 UTC
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.
Comment 29 Sebastian Dröge (slomo) 2015-06-29 15:24:12 UTC
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.
Comment 30 Sebastian Dröge (slomo) 2015-06-29 15:25:54 UTC
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.
Comment 31 Sebastian Dröge (slomo) 2015-06-29 15:39:24 UTC
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.
Comment 32 Luis de Bethencourt 2015-06-29 15:52:47 UTC
Sebastian,

Thanks for the patches. Looking good.
Sorry I never finished this task.
Comment 33 Sebastian Dröge (slomo) 2015-06-29 16:00:43 UTC
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.
Comment 34 Sebastian Dröge (slomo) 2015-06-30 08:50:22 UTC
baseparse mostly works because of the adapter changes, unless a subclass does special things with buffers.
Comment 35 Sebastian Dröge (slomo) 2015-06-30 09:12:56 UTC
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.
Comment 36 Sebastian Dröge (slomo) 2015-06-30 09:19:21 UTC
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.
Comment 37 Sebastian Dröge (slomo) 2015-06-30 09:50:24 UTC
qtdemux and matroskademux also passes through meta from source to downstream in push mode, the adapter changes seemed to have handled many cases.
Comment 38 Sebastian Dröge (slomo) 2015-06-30 11:54:24 UTC
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
Comment 39 Sebastian Dröge (slomo) 2015-06-30 14:38:02 UTC
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
Comment 40 Sebastian Dröge (slomo) 2015-06-30 14:39:06 UTC
(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.
Comment 41 Sebastian Dröge (slomo) 2015-06-30 16:12:34 UTC
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
Comment 42 Sebastian Dröge (slomo) 2015-06-30 16:14:32 UTC
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.
Comment 43 Sebastian Dröge (slomo) 2015-07-01 09:59:47 UTC
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.
Comment 44 Sebastian Dröge (slomo) 2015-07-01 10:06:44 UTC
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.
Comment 45 Sebastian Dröge (slomo) 2015-07-01 10:07:54 UTC
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.
Comment 46 Sebastian Dröge (slomo) 2015-07-01 10:10:29 UTC
Let's close this one and talk about the RTP stuff in another bug: bug 751774