GNOME Bugzilla – Bug 613690
[xmp] refactoring to 1-n tag mappings
Last modified: 2010-03-24 18:58:05 UTC
xmp helper lib needs to be refactored in order to be able to support exif GPS location tags. The problems to solve are: 1) Be able to serialize doubles into the exif GPSCoordinate format, which is 'degrees','minutes','seconds'D, where D is a char (N, S, W, E) indicating the direction. - The solution here is to add a serialization/deserialization pair of functions to each tag mapping so they can handle with the tags in the way it is requested. If NULL, defaults to the trivial serialization. 2) _ELEVATION tag is mapped into exif:GPSAltitude and exif:GPSAltitudeRef *at the same time*, so we need a way to have 1-n mappings and be able to serialize/deserialize it properly. Patches follow.
Created attachment 156860 [details] [review] tag: xmp: Refactor mappings storage This commit is only refactoring, no fetaures added. Do not store tags in flexible arrays as it doesn't allow us to use nested flexible arrays. This is going to be needed in the following commits to map gst tags that are stored into 2 separate tags in xmp (Not that they are alternatives, but they are complimentary). For example, GST_TAG_ELEVATION is represented in the exif schema with 2 fields: the absolute altitude and an integer to indicate if it is above or below sea level. The previous mappings storage wouldn't allow us to express it. Also store a serialization and a deserialization function for each xmp tag as some of them require some non-trivial convertion to its string form. Fixes #613690
Created attachment 156861 [details] [review] tag: xmp: Refactor buffer parsing When parsing the xmp buffer into the gst taglist store the found tags into a list to be parsed only after finding all tags on the buffer. This allows the parser function to search this list for complimentary tags that should be parsed together Fixes #613690
Created attachment 156862 [details] [review] tag: xmp: Adds mappings for LATITUDE and LONGITUDE Adds the mappings for those tags and tests for tags serialization. Fixes #613690
Created attachment 156863 [details] [review] tag: xmp: Fix off by one Avoid ignoring single char tags, like exif:GPSAltitudeRef Fixes #613690
Created attachment 156864 [details] [review] tag: xmp: Add Elevation tag mapping Adds a mapping to the _ELEVATION tag, this is a different mapping as it has to be mapped into exif:GPSAltitude and exif:GPSAltitudeRef at the same time. So we needed to refactor a little more to be able to deserialize it properly. Now, when parsing a xmp buffer into a taglist all tags are added to a list before being parsed so that when one of the altitude tags are found the deserialization function can search for its complimentary tag to do the correct parsing Fixes #613690
Review of attachment 156860 [details] [review]: Looks good in general. Small comments below. ::: gst-libs/gst/tag/gstxmptag.c @@ +89,3 @@ + key = g_quark_from_string (gst_tag); + + xmpinfo = g_new0 (XmpTag, 1); maybe use g_slice here. @@ +105,3 @@ + +/* FIXME + * Do we need to return a shallow copy here? */ We would need the copy, if someone could add more mapping while parsing buffers. As of now adding mappings is not public, so it can't happen, right? @@ +113,3 @@ + XMP_TAG_MAP_LOCK; + ret = g_slist_copy ((GSList *) g_hash_table_lookup (__xmp_tag_map, + GUINT_TO_POINTER (g_quark_from_string (gst_tag)))); move the g_quark_from_string (gst_tag) to key = g_quark_from_string (gst_tag) out of the lock (for readability). @@ +559,3 @@ + xmptaglist = _xmp_tag_get_mapping (tag); + if (xmptaglist) { + /* FIXME - se always chose the first tag mapped on the list */ typo "se" -> "we" ?
Review of attachment 156861 [details] [review]: ::: gst-libs/gst/tag/gstxmptag.c @@ +432,3 @@ + PendingXmpTag *ptag; + + ptag = g_new0 (PendingXmpTag, 1); g_slice for efficiency, maybe a gpointerarray is even more useful (if you don't want to reorder things).
Review of attachment 156862 [details] [review]: Looks good. Thanks!
Review of attachment 156863 [details] [review]: Hard to see from the context. I'll trust you :)
Review of attachment 156864 [details] [review]: If you switch to g_slice, it would also be used here. Please also use g_slice_new and not g_slice_new0 as you set all fields anyway. ::: gst-libs/gst/tag/gstxmptag.c @@ +876,3 @@ XmpTag *xmp_tag; + extra newline
Created attachment 157000 [details] [review] tag: xmp: Refactor mappings storage Updated patch Fixes #613690
Created attachment 157001 [details] [review] tag: xmp: Refactor buffer parsing Updated patch.
Module: gst-plugins-base Branch: master Commit: e82414643caf876f094f78a8df2293068fe11f13 URL: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=e82414643caf876f094f78a8df2293068fe11f13 Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk> Date: Sat Mar 20 11:17:38 2010 -0300 tag: xmp: Refactor mappings storage This commit is only refactoring, no fetaures added. Do not store tags in flexible arrays as it doesn't allow us to use nested flexible arrays. This is going to be needed in the following commits to map gst tags that are stored into 2 separate tags in xmp (Not that they are alternatives, but they are complementary). For example, GST_TAG_ELEVATION is represented in the exif schema with 2 fields: the absolute altitude and an integer to indicate if it is above or below sea level. The previous mappings storage wouldn't allow us to express it. Also store a serialization and a deserialization function for each xmp tag as some of them require some non-trivial convertion to its string form. Fixes #613690 Module: gst-plugins-base Branch: master Commit: fe1f3e35957ddae12ae662e8e950bd8eb2358520 URL: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=fe1f3e35957ddae12ae662e8e950bd8eb2358520 Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk> Date: Mon Mar 22 22:03:09 2010 -0300 tag: xmp: Refactor buffer parsing When parsing the xmp buffer into the gst taglist store the found tags into a list to be parsed only after finding all tags on the buffer. This allows the parser function to search this list for complimentary tags that should be parsed together Fixes #613690 Module: gst-plugins-base Branch: master Commit: 7ebbfbd3a5571c0e7518c5de9e710b9b9915f91e URL: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=7ebbfbd3a5571c0e7518c5de9e710b9b9915f91e Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk> Date: Mon Mar 22 15:18:28 2010 -0300 tag: xmp: Adds mappings for LATITUDE and LONGITUDE Adds the mappings for those tags and tests for tags serialization. Fixes #613690 Module: gst-plugins-base Branch: master Commit: e207463582cc227b73922e8d6a70a721b75baf45 URL: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=e207463582cc227b73922e8d6a70a721b75baf45 Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk> Date: Tue Mar 23 09:48:19 2010 -0300 tag: xmp: Fix off by one Avoid ignoring single char tags, like exif:GPSAltitudeRef Fixes #613690 Module: gst-plugins-base Branch: master Commit: 007bf4fe7cef2c911a280a8f2c5caa27f9f25c14 URL: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=007bf4fe7cef2c911a280a8f2c5caa27f9f25c14 Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk> Date: Tue Mar 23 09:32:40 2010 -0300 tag: xmp: Add Elevation tag mapping Adds a mapping to the _ELEVATION tag, this is a different mapping as it has to be mapped into exif:GPSAltitude and exif:GPSAltitudeRef at the same time. So we needed to refactor a little more to be able to deserialize it properly. Now, when parsing a xmp buffer into a taglist all tags are added to a list before being parsed so that when one of the altitude tags are found the deserialization function can search for its complementary tag to do the correct parsing Fixes #613690
Comment on attachment 156864 [details] [review] tag: xmp: Add Elevation tag mapping Obsolete this, updated version commited
Comment on attachment 157000 [details] [review] tag: xmp: Refactor mappings storage Obsolete too, updated version committed