GNOME Bugzilla – Bug 706340
videofilter: it might make sense to implement the "transform_meta" virtual method
Last modified: 2013-09-18 16:48:33 UTC
We've got a problem in GES with buffer metas not being copied when they go through a filter (e.g. agingtv) A fix is to implement transform_meta and return TRUE in each and every one of the effects we might use, but I wonder if it might not make more sense to do so directly in videofilter ? IRC : <__tim> Mathieu_Du, any metas in particular? it's a bit tricky to generically copy metas if you don't know what they represent <Mathieu_Du> Hm well this is a meta used to carry alpha information from our track elements to the global videomixer <Mathieu_Du> That is internal to ges __tim> I'm not sure if you can just assume that they will still apply to the new transformed buffer <__tim> e.g imagine a meta marking a face, now some effects warps the image, and the meta now marks .. something <Mathieu_Du> Well that should be the role of the transform_func of the meta to determine that may'be ? <Mathieu_Du> I don't really know I have to say :) <Mathieu_Du> I see the problem now though <Mathieu_Du> The fact still remains that we *need* these meta to get copied :)
I think this has to be implemented separately in every videofilter subclass, depending on what that specific filter does.
But then you'll always drop custom metas that an element doesn't know about, that's not really right either and makes metas much less useful. I think there should be some kind of hint or flag that indicates that the meta applies to the buffer and should be maintained even if the content is transformed (but the chunking/timing remains unchanged). Likewise there should be a hint that the meta is related to the memory, etc. There might already be something somewhere, I remember having had this discussion before.
The tags with which you register a meta tell you on which aspects of a buffer this meta applies. That can be used to generically handle unknown metas to some degree.
(In reply to comment #3) > The tags with which you register a meta tell you on which aspects of a buffer > this meta applies. That can be used to generically handle unknown metas to some > degree. That's the idea, the tags themselves are not very well defined but we currently have: - memory: metadata describes memory layout and size etc, if memory layout changes, metadata becomes invalid - colorspace: metadata describes colorspace, if colorspace changes, metadata needs to be removed. videoconvert removes this meta when it does a transform - size: metadata is related to video size, if the size changes, remove meta There are also transform functions defined for, for example, scaling. A metadata that depends on the size can implement a scale transform and update the metadata accordingly. A filter that changes the size needs to either remove all size dependent metadata or call the transform with the scale quark and extra info.
OK so after discussion on IRC, the resulting decisions and observations are : The Metadata transform mechanism is not yet completely fixed. A problem that can and will arise with the copying of metadata when transforming buffers is that these metadata might not be relevant anymore. Evident example : you put metadata on video buffers to indicate the position of something, but a downstream element modifies the shape or size of the image. For now, the current behaviour is that these metadata get dropped, even though they are correctly tagged with "orientation" or "size". The ideal situation would be in the future to let the meta update itself when needed, with each individual filter calling the transform_func of the meta with new state informations. For now, it has been agreed that when a metadata has no tags, then it means it can be copied without asking the subclass, and furthermore in videofilter, if the metadata only has the "video" tag, it can be copied as well. To decide that, a new API was needed in gstmeta, namely gst_meta_get_tags. The current implementation didn't allow to get all tags. To make sure not to modify existing behaviour, I had a look at all existing metas and saw that one of them didn't ha
sorry .. One of them (GstAudioDownSampleMeta) didn't have any tags, I added two there. I've got other patches which, as agreed with slomo, implement #defines for the metadata strings used for now. This in my opinion will make the system slightly more robust and well-defined, but I can provide other patches if that's not correct / desirable. The only place where I didn't set a #define is in libs/gst/net/gstnetaddressmeta.c, as I didn't know how to describe the "origin" metadata. I'd rather link my branches here, I'll provide single patches if needed : -core : https://github.com/MathieuDuponchelle/gstreamer/commits/metadata -good : https://github.com/MathieuDuponchelle/gst-plugins-good/commits/metadata -base : https://github.com/MathieuDuponchelle/gst-plugins-base/commits/metadata (also contains a trivial fix in gstvideometa.h)
Added some comments there but generally looks good :)
I just pushed back my branches. I did so with -f, so your previous comments will be erased, problem is recommiting on top would have made strange and unreadable commits. I anyway fixed everything you mentioned, the erasing of comments is kind of workflow breaking I have to say, with thibault the only solution we found is to do the review separately with no line notes :(
Aaaaand added some comments, we're getting there :)
Aaaaaaaand we're there I guess :)
Looks good to me, I have some problems with the defines for the tags, "memory" -> GST_MEMORY_METADATA "video" -> GST_VIDEO_METADATA etc. To me it's not metadata but metadata *tags* so that should be included in there. It's also all about the tags so that should go first, like: "memory" -> GST_META_TAG_MEMORY "video" -> GST_META_TAG_VIDEO etc
ack
GST_META_TAG_MEMORY already exists as a GQuark, what should we do ?
And that's there since 1.0.0 already. That's a bit unfortunate as all API takes strings instead of quarks. Wim, what names would you prefer for the defines, or what should be done here instead?
You only need strings to register because that was easiest, we don't have defines for that currently so what about GST_META_TAG_MEMORY_STR or so? It's all not ideal maybe you would have wanted to register the api with a GQuark array instead and reuse the existing defines.
Pushed all the #define changes to the relevant metadata branches.
Any objection about merging that now ?
Needs another review and then can probably be merged, hopefully I get to that today.
It's still a bit inconstistent in the naming. I would suggest: GST_META_TAG_<subsys>[_<feature>]_STR thus: GST_META_TAG_MEMORY_STR "memory" GST_META_TAG_AUDIO_STR GST_META_TAG_AUDIO_CHANNELS_STR GST_META_TAG_VIDEO_STR GST_META_TAG_VIDEO_ORIENTATION_STR GST_META_TAG_VIDEO_SIZE_STR GST_META_TAG_VIDEO_COLORSPACE_STR With optionally, defines that return the GQuark (either using a static variable, g_quark_from_string() or a method to do the GOnce registration of the quark): GST_META_TAG_MEMORY (_gst_meta_tag_memory) GST_META_TAG_AUDIO (g_quark_from_string(GST_META_TAG_AUDIO_STR)) GST_META_TAG_AUDIO_CHANNELS (gst_audio_channels_get_meta_tag())
I agree with Wim, otherwise this looks good. Personally I prefer the global variables. Also see the single comment I added for basetransform. Oh and ideally have the STR and quark define for the tag at the same place in the header :) For audio the samplerate would also be a good candidate for a tag, information can be sample rate specific (like filter coefficients).
Pushed changes to : https://github.com/MathieuDuponchelle/gstreamer/commits/metadata https://github.com/MathieuDuponchelle/gst-plugins-good/commits/metadata https://github.com/MathieuDuponchelle/gst-plugins-base/commits/metadata I don't understand why we want to add new GQuark #defines though ?
It would be great if we could get that stuff merged now. I have a bug branch in GES that actually relies on that set of patches. Anything else is still needed?