GNOME Bugzilla – Bug 645167
[xmp] Add a new XmpConfig interface
Last modified: 2011-03-31 20:50:46 UTC
We need a new interface to configure which schemas should be used when serializing xmp tags. The attached patches propose a new interface that have functions to add/remove schemas. The added schemas are picked for serialization. One bit that I'm not happy about is that each gst tag has to be searched over the gst -> xmp tags mapping, now there is an extra action to iterate over a schemas list to check if that tag is on an 'allowed' schema. Anyway, this is a first attempt, reviews and suggestions are welcome.
Created attachment 183737 [details] [review] tag: xmp: Add function to list the available schemas Adds a function to list the available schemas in our xmp lib
Created attachment 183738 [details] [review] xmpconfig: Adds a new XmpConfig interface The XmpConfig interface is to be implemented on elements that provide xmp serialization. It allows users to select which xmp schemas should be used on serialization. API: GstXmpConfig
Created attachment 183940 [details] [review] xmpconfig: Adds a new XmpConfig interface Updated version of the xmpconfig interface, this fixes a bug on registering its GType
- not so keen on making libgstinterfaces depend on libgsttag. That kind of defeats the purpose of libgstinterfaces. Why not just add this to libgsttag directly? It's also more discoverable there with the rest of the XMP stuff, IMHO. We have interfaces in other libraries too (e.g. GstRtspExtension in libgstrtsp). - I'm not fully convinced that the hash table in GstXmpConfigData is the right data structure for the job, but since it's 100% internal it doesn't hurt, I guess - there's no interface vfunc to get a list of supported schemas for that particular object. Am I correct in assuming that's not needed because all XMP writers will always support all schemas supported by libgsttag? (ie. it's not that some schemas don't make sense for certain elements) - not sure what to think about the name (GstXmpConfig), if maybe GstTagXmpWriter or so wouldn't be nicer. - dislike the GList * argument in: gst_tag_list_to_xmp_buffer_full (const GstTagList * list, gboolean read_only, GList * schemas); why not a const gchar ** or even a simple const gchar * (e.g. comma-separated list of schemas to use)? (see below) - The config API per se looks ok, but I was wondering if it actually reflects the envisaged use cases. In case of an application using this interface to configure the xmp writer to only use certain schemas: does the app usually know in advance which schemas it wants (whitelist), or does it know which ones it wants to disable (blacklist), or does it just build things via add/remove from an empty slate or the default list? If it's the first, some simple set_schemas (writer, "schema1, schema2"); might almost be sufficient :)
(In reply to comment #4) > - not so keen on making libgstinterfaces depend on libgsttag. That > kind of defeats the purpose of libgstinterfaces. Why not just add > this to libgsttag directly? It's also more discoverable there with > the rest of the XMP stuff, IMHO. We have interfaces in other > libraries too (e.g. GstRtspExtension in libgstrtsp). Agreed, moving there. > > - I'm not fully convinced that the hash table in GstXmpConfigData > is the right data structure for the job, but since it's 100% internal > it doesn't hurt, I guess I can't think of something else now, but we can always change this later. > > - there's no interface vfunc to get a list of supported schemas for > that particular object. Am I correct in assuming that's not needed > because all XMP writers will always support all schemas supported > by libgsttag? (ie. it's not that some schemas don't make sense for > certain elements) AFAIK there are no restrictions, the same schemas can be used for all formats. We can add a new function in the future and default to all if it isn't implemented. > > - not sure what to think about the name (GstXmpConfig), if maybe > GstTagXmpWriter or so wouldn't be nicer. Ok. > > - dislike the GList * argument in: > > gst_tag_list_to_xmp_buffer_full (const GstTagList * list, > gboolean read_only, GList * schemas); > > why not a const gchar ** or even a simple const gchar * > (e.g. comma-separated list of schemas to use)? (see below) GList* is not binding friendly, right? I prefer the const gchar**, but a single gchar* is good, too. > > - The config API per se looks ok, but I was wondering if it > actually reflects the envisaged use cases. In case of an > application using this interface to configure the xmp > writer to only use certain schemas: does the app usually > know in advance which schemas it wants (whitelist), or > does it know which ones it wants to disable (blacklist), > or does it just build things via add/remove from an empty > slate or the default list? If it's the first, some simple > set_schemas (writer, "schema1, schema2"); might > almost be sufficient :) The use case I have is to be able to disable some undesired schema, so the blacklist mode makes sense to me. But then I don't know about all the possible use cases, one might want only a specific schema.
> > [hash table] > I can't think of something else now, but we can always change this later. A plain GList or array, for example. > > why not a const gchar ** or even a simple const gchar * > > (e.g. comma-separated list of schemas to use)? (see below) > > GList* is not binding friendly, right? Not so much, or at least it requires more markup. But it seemed to me that the GList * was chosen mostly because that's what g_hash_table_get_keys() returns. I'd keep it simple here. Let's make it a const gchar ** then - that's in line with what _list_schemas() returns. > The use case I have is to be able to disable some undesired schema, so the > blacklist mode makes sense to me. But then I don't know about all the possible > use cases, one might want only a specific schema. I think we can just keep it as it is then, that allows for all options.
Created attachment 184500 [details] [review] tagxmpwriter: Adds a new GstTagXmpWriter interface The GstTagXmpWriter interface is to be implemented on elements that provide xmp serialization. It allows users to select which xmp schemas should be used on serialization. API: GstTagXmpWriter
Created attachment 184501 [details] [review] tagxmpwriter: Add check tests
Updated patches according to comments.
Review of attachment 184500 [details] [review]: Looks good to me in general. ::: gst-libs/gst/tag/xmpwriter.c @@ +111,3 @@ + + if (data->schemas) + g_slist_free_full (data->schemas, g_free); g_[s]list_free_full() is new since GLib 2.28. And you can call it on NULL too (NULL is a valid GList/GSList)
commit b5246da456bdd079ccc65800359604c13abb8c98 Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk> Date: Mon Mar 21 15:34:09 2011 -0300 tagxmpwriter: Add check tests https://bugzilla.gnome.org/show_bug.cgi?id=645167 commit 34ba387d0bab06eded02cba411e7ea8c30abfd46 Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk> Date: Thu Mar 17 15:42:28 2011 -0300 tagxmpwriter: Adds a new GstTagXmpWriter interface The GstTagXmpWriter interface is to be implemented on elements that provide xmp serialization. It allows users to select which xmp schemas should be used on serialization. API: GstTagXmpWriter https://bugzilla.gnome.org/show_bug.cgi?id=645167 commit 6cc96a67ec8c9e49b761ac0d4e79e226bdd88c21 Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk> Date: Fri Mar 18 09:28:23 2011 -0300 tag: xmp: Add function to list the available schemas Adds a function to list the available schemas in our xmp lib https://bugzilla.gnome.org/show_bug.cgi?id=645167
Isn't this what we discussed in bug #623799?