[gstreamer-bugs] [Bug 613690] [xmp] refactoring to 1-n tag mappings

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=613690
  GStreamer | gst-plugins-base | git

Stefan Kost (gstreamer, gtkdoc dev) <ensonic> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #156860|none                        |needs-work
             status|                            |

--- Comment #6 from Stefan Kost (gstreamer, gtkdoc dev) <ensonic at sonicpulse.de> 2010-03-24 13:22:04 UTC ---
Review of attachment 156860:
 --> (https://bugzilla.gnome.org/review?bug=613690&attachment=156860)

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" ?

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.