[gstreamer-bugs] [Bug 614872] [tag] Add basic exif support

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #3 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2010-04-06 09:46:16 UTC ---
Review of attachment 157971:
 --> (https://bugzilla.gnome.org/review?bug=614872&attachment=157971)

::: gst-libs/gst/tag/gstexiftag.c
@@ +72,3 @@
+static const GstExifTagMatch tag_map[] = {
+  {GST_TAG_COPYRIGHT, 0x8298, EXIF_TYPE_ASCII},
+  {NULL, 0, 0}

We could save the NULL-terminator entry and use G_N_ELEMENTS to iterate the
table.

@@ +112,3 @@
+      return i;
+    }
+  }

This is going to loop endlessly if tag != tag_map[0].gst_tag, isn't it? I'd
suggest a for-loop with G_N_ELEMENTS (tag_map).

@@ +131,3 @@
+      return i;
+    }
+  }

Same as above: endless loop?

@@ +313,3 @@
+ * Since: 0.10.29
+ */
+/* FIXME How to add the next IFD address? */

What about this then? :)

@@ +316,3 @@
+GstBuffer *
+gst_tag_list_to_exif_buffer (const GstTagList * taglist, gint byte_order,
+    guint32 base_offset)

byte_order: can't we just always write everything in little endian or big
endian (or host order)? I mean, does the caller really *need* to specify the
byte order in some cases?

base_offset: maybe make normal guint?

@@ +389,3 @@
+GstTagList *
+gst_tag_list_from_exif_buffer (const GstBuffer * buffer, gint byte_order,
+    guint32 base_offset)

Can we somehow figure out the byte_order ourselves? Having to pass in the byte
order seems a bit awkard. Where does this info come from?

I would make base_offset a normal integer, even if it's limited to 32-bit in
the format, just seems nicer. Others may disagree of course :)

-- 
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.