After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 614872 - [tag] Add basic exif support
[tag] Add basic exif support
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other All
: Normal enhancement
: 0.10.30
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-04-05 13:21 UTC by Thiago Sousa Santos
Modified: 2010-06-10 11:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tag: Adds basic exif tags support (16.58 KB, patch)
2010-04-05 13:22 UTC, Thiago Sousa Santos
none Details | Review
tests: tag: Adds unit tests for exif helper lib (3.63 KB, patch)
2010-04-05 13:23 UTC, Thiago Sousa Santos
none Details | Review
tag: Adds basic exif tags support (16.54 KB, patch)
2010-04-06 11:34 UTC, Thiago Sousa Santos
none Details | Review
tag: Adds basic exif tags support (22.24 KB, patch)
2010-05-04 17:22 UTC, Thiago Sousa Santos
none Details | Review
tests: tag: Adds unit tests for exif helper lib (3.96 KB, patch)
2010-05-04 17:23 UTC, Thiago Sousa Santos
none Details | Review
tag: Adds basic exif tags support (36.45 KB, patch)
2010-05-06 19:03 UTC, Thiago Sousa Santos
none Details | Review
tests: tag: Adds unit tests for exif helper lib (4.67 KB, patch)
2010-05-06 19:04 UTC, Thiago Sousa Santos
none Details | Review
tag: Adds basic exif tags support (36.45 KB, patch)
2010-05-17 18:33 UTC, Thiago Sousa Santos
needs-work Details | Review
tests: tag: Adds unit tests for exif helper lib (4.67 KB, patch)
2010-05-17 18:34 UTC, Thiago Sousa Santos
committed Details | Review
tag: exif: Adds new geo-location tag mappings (38.45 KB, patch)
2010-05-17 18:35 UTC, Thiago Sousa Santos
committed Details | Review
jpegformat: Add exif support (6.12 KB, patch)
2010-05-17 19:14 UTC, Thiago Sousa Santos
none Details | Review
tests: jifmux: Adds jifmux exif tags tests (7.86 KB, patch)
2010-05-17 19:14 UTC, Thiago Sousa Santos
committed Details | Review

Description Thiago Sousa Santos 2010-04-05 13:21:28 UTC
Exif spec describe an image format that can contain raw or jpeg encoded images, it also specs a tag storage scheme that can be embedded into jpeg/tiff/wav and avi (that I know). We should have a helper tag library for exif as we have recently added for xmp.

Spec found at: http://www.exif.org/

Exif uses TIFF's IFD structures to store the tags, the IFD entries store the tag id, type, size and offset to where it is located. The current patch I'm attaching here will only handle the case where the tag entries and the tag data are placed together on the file (as the spec seems to recommend). If you feel other methods of storing the tags are common and important, please comment here.

Bug #486659 is related.
Comment 1 Thiago Sousa Santos 2010-04-05 13:22:09 UTC
Created attachment 157971 [details] [review]
tag: Adds basic exif tags support

Adds exif helper lib functions to parse exif buffers from/to
taglists. Exif is tipically used in jpeg images, but it can
also be embedded into TIFF, AVI and WAV formats.

API: gst_tag_list_to_exif_buffer
API: gst_tag_list_from_exif_buffer

Fixes #614872
Comment 2 Thiago Sousa Santos 2010-04-05 13:23:17 UTC
Created attachment 157972 [details] [review]
tests: tag: Adds unit tests for exif helper lib

Adds some simple unit tests for exif helper lib functions

Fixes #614872
Comment 3 Tim-Philipp Müller 2010-04-06 09:46:16 UTC
Review of attachment 157971 [details] [review]:

::: 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 :)
Comment 4 Thiago Sousa Santos 2010-04-06 11:16:55 UTC
(In reply to comment #3)
> Review of attachment 157971 [details] [review]:
> 
> ::: 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.

Done.

> 
> @@ +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).

Erm, stupid mistake :)

> 
> @@ +131,3 @@
> +      return i;
> +    }
> +  }
> 
> Same as above: endless loop?

Yes, fixed.

> 
> @@ +313,3 @@
> + * Since: 0.10.29
> + */
> +/* FIXME How to add the next IFD address? */
> 
> What about this then? :)

Brief explanation:
Each IFD ends with a pointer to the next one and we have no idea on what it is or where it is located. We could add a new parameter for setting that, or just set to 0 and add to the docs that the last 4 bytes should be overridden by the element, or don't add it and tell that in the docs.

> 
> @@ +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?

The problem here is that this is defined in the TIFF header (that is put in both jpeg and TIFF), we need to follow what the app has already set there.

> 
> base_offset: maybe make normal guint?

I'd prefer having it as a guint32 as it is the 'real' type, not a strong opinion, though.

> 
> @@ +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?

As the opposite of writing, this needs to be told to the function as it was read from the TIFF header.

> 
> 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 :)

Already answered.


Thanks for the review :)
Comment 5 Thiago Sousa Santos 2010-04-06 11:34:19 UTC
Created attachment 158036 [details] [review]
tag: Adds basic exif tags support

Updated patch.
Comment 6 Stefan Sauer (gstreamer, gtkdoc dev) 2010-04-09 08:48:49 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > Review of attachment 157971 [details] [review] [details]:

> > 
> > @@ +313,3 @@
> > + * Since: 0.10.29
> > + */
> > +/* FIXME How to add the next IFD address? */
> > 
> > What about this then? :)
> 
> Brief explanation:
> Each IFD ends with a pointer to the next one and we have no idea on what it is
> or where it is located. We could add a new parameter for setting that, or just
> set to 0 and add to the docs that the last 4 bytes should be overridden by the
> element, or don't add it and tell that in the docs.
> 

Maybe document it. I think this is something that the caller has to handle depending on which container format the chunks are getting store to.
Comment 7 Thiago Sousa Santos 2010-05-04 17:22:55 UTC
Created attachment 160277 [details] [review]
tag: Adds basic exif tags support

Updated and providing new functions to help embedding in APP1 for jpeg.

Fixes #614872
Comment 8 Thiago Sousa Santos 2010-05-04 17:23:37 UTC
Created attachment 160278 [details] [review]
tests: tag: Adds unit tests for exif helper lib

Updated tests.
Comment 9 Thiago Sousa Santos 2010-05-06 19:03:33 UTC
Created attachment 160455 [details] [review]
tag: Adds basic exif tags support

Lots of refactoring to add some more tricky mappings,
including exif gps tags that needed tags pointing to
other IFDs support.
Comment 10 Thiago Sousa Santos 2010-05-06 19:04:22 UTC
Created attachment 160456 [details] [review]
tests: tag: Adds unit tests for exif helper lib
Comment 11 Thiago Sousa Santos 2010-05-17 18:33:33 UTC
Created attachment 161247 [details] [review]
tag: Adds basic exif tags support

Updated.
Comment 12 Thiago Sousa Santos 2010-05-17 18:34:06 UTC
Created attachment 161248 [details] [review]
tests: tag: Adds unit tests for exif helper lib

Updated.
Comment 13 Thiago Sousa Santos 2010-05-17 18:35:08 UTC
Created attachment 161249 [details] [review]
tag: exif: Adds new geo-location tag mappings

New patch that adds some new tag mappings and does lots of
refactoring to the code.

Adds mappings for:
GST_TAG_GEO_LOCATION_CAPTURE_DIRECTION
GST_TAG_GEO_LOCATION_MOVEMENT_DIRECTION
GST_TAG_GEO_LOCATION_MOVEMENT_SPEED
GST_TAG_GEO_LOCATION_ELEVATION

Does some refactoring in the code to reduce number of parameters
passed to functions

Tests included.
Comment 14 Thiago Sousa Santos 2010-05-17 19:14:11 UTC
Created attachment 161258 [details] [review]
jpegformat: Add exif support

Adds exif writing support to jifmux.
Adds parsing support to jpegparse.
Comment 15 Thiago Sousa Santos 2010-05-17 19:14:48 UTC
Created attachment 161259 [details] [review]
tests: jifmux: Adds jifmux exif tags tests

Adds a basic unit test for jifmux to test that exif tags
are writen properly.

This test uses libexif.
Comment 16 Thiago Sousa Santos 2010-06-07 10:57:09 UTC
Anyone willing to review this? It has automated tests and they pass. So this works at some point :)

The critical part to review here is the API, which is fairly simple:

+/* functions related to exif */
+GstBuffer *             gst_tag_list_to_exif_buffer (const GstTagList * list,
+                                                     gint byte_order,
+                                                     guint32 base_offset);
+
+GstBuffer *             gst_tag_list_to_exif_buffer_with_tiff_header (const GstTagList * taglist);
+
+GstTagList *            gst_tag_list_from_exif_buffer (const GstBuffer * buffer,
+                                                       gint byte_order,
+                                                       guint32 base_offset);
+
+GstTagList *            gst_tag_list_from_exif_buffer_with_tiff_header (
+                                                      const GstBuffer * buffer);
+
Comment 17 Stefan Sauer (gstreamer, gtkdoc dev) 2010-06-09 15:58:47 UTC
Review of attachment 161247 [details] [review]:

Very nice patch. I only have some formalities - please commit when taken care of them. (When applying this also need to update the ./win32/common/libgsttag.def.)

::: docs/libs/gst-plugins-base-libs-sections.txt
@@ +1688,3 @@
+gst_tag_list_from_exif_buffer
+<SUBSECTION Standard>
+</SECTION>

should this also contain gst_tag_list_*_with_tiff_header?

::: gst-libs/gst/tag/gstexiftag.c
@@ +27,3 @@
+ *
+ */
+

This needs at least 1 sentence of description.

@@ +144,3 @@
+  {GST_TAG_GEO_LOCATION_LONGITUDE, 0x4, EXIF_TYPE_RATIONAL, 0x3,
+      serialize_geo_coordinate, deserialize_geo_coordinate},
+#if 0

IUs this disabled because we lack the GST_TAG_XX defines? Didn't you files a bug about it? Maybe add a FIXME: comment and point to the bug.

@@ +391,3 @@
+        break;
+    } else {
+      GST_WARNING ("Unexpected endianness");

maybe check this when setting up the writer/reader. There are already other places in the code above without the warning.

@@ +546,3 @@
+  /* Add the next IFD offset, we just set it to 0 because
+   * there is no easy way to predict what it is going to be.
+   * The user might rewrite the value if needed */

Maybe this is something that can be explained in the section docs (and then you can remove the FIXME).
Comment 18 Stefan Sauer (gstreamer, gtkdoc dev) 2010-06-09 16:02:37 UTC
Review of attachment 161249 [details] [review]:

This looks good.
Comment 19 Stefan Sauer (gstreamer, gtkdoc dev) 2010-06-09 16:02:57 UTC
Review of attachment 161248 [details] [review]:

This looks good.
Comment 20 Thiago Sousa Santos 2010-06-09 19:32:58 UTC
Comment on attachment 161247 [details] [review]
tag: Adds basic exif tags support

Updated according to comments and pushed.
Comment 21 Thiago Sousa Santos 2010-06-10 11:25:49 UTC
Comment on attachment 161258 [details] [review]
jpegformat: Add exif support

Pushed a different version
Comment 22 Thiago Sousa Santos 2010-06-10 11:28:44 UTC
Fixed.

In -base:

commit 491d02553028194518d532e2c2be30c2377091a8
Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk>
Date:   Mon May 10 14:01:46 2010 -0300

    tag: exif: Adds new geo-location tag mappings
    
    Adds mappings for:
    GST_TAG_GEO_LOCATION_CAPTURE_DIRECTION
    GST_TAG_GEO_LOCATION_MOVEMENT_DIRECTION
    GST_TAG_GEO_LOCATION_MOVEMENT_SPEED
    GST_TAG_GEO_LOCATION_ELEVATION
    
    Does some refactoring in the code to reduce number of parameters
    passed to functions
    
    Tests included.

commit 4418dc9cdf9137d2aad27585da84de57b388f9c7
Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk>
Date:   Sun Apr 4 22:25:24 2010 -0300

    tests: tag: Adds unit tests for exif helper lib
    
    Adds some simple unit tests for exif helper lib functions
    
    Fixes #614872

commit 6b6a4e85ad01f1c25d7b77c1980e2a0c6080eb54
Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk>
Date:   Sat Apr 3 23:02:57 2010 -0300

    tag: Adds basic exif tags support
    
    Adds exif helper lib functions to parse exif buffers from/to
    taglists. Exif is tipically used in jpeg images, but it can
    also be embedded into TIFF, AVI and WAV formats.
    
    Adds a couple function to handle exif in tiff header structures, that is how
    exif is embedded in jpeg and (obviously) in tiff.
    
    API: gst_tag_list_to_exif_buffer
    API: gst_tag_list_to_exif_buffer_with_tiff_header
    API: gst_tag_list_from_exif_buffer
    API: gst_tag_list_from_exif_buffer_with_tiff_header
    
    Fixes #614872


In -bad:

commit c54b5325cb956da964be0ee0166fb506185c14ae
Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk>
Date:   Mon May 10 10:47:06 2010 -0300

    tests: jifmux: Adds jifmux exif tags tests
    
    Adds a basic unit test for jifmux to test that exif tags
    are writen properly.
    
    This test uses libexif.
    
    Fixes #614872

commit 00897e21a99011884c07c4def20a6d658cc0e97b
Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk>
Date:   Wed Apr 28 23:52:12 2010 -0300

    jpegformat: Add exif support
    
    Adds exif writing support to jifmux.
    Adds parsing support to jpegparse.
    
    Fixes #614872