GNOME Bugzilla – Bug 614872
[tag] Add basic exif support
Last modified: 2010-06-10 11:28:44 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.
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
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
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 :)
(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 :)
Created attachment 158036 [details] [review] tag: Adds basic exif tags support Updated patch.
(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.
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
Created attachment 160278 [details] [review] tests: tag: Adds unit tests for exif helper lib Updated tests.
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.
Created attachment 160456 [details] [review] tests: tag: Adds unit tests for exif helper lib
Created attachment 161247 [details] [review] tag: Adds basic exif tags support Updated.
Created attachment 161248 [details] [review] tests: tag: Adds unit tests for exif helper lib Updated.
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.
Created attachment 161258 [details] [review] jpegformat: Add exif support Adds exif writing support to jifmux. Adds parsing support to jpegparse.
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.
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); +
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).
Review of attachment 161249 [details] [review]: This looks good.
Review of attachment 161248 [details] [review]: This looks good.
Comment on attachment 161247 [details] [review] tag: Adds basic exif tags support Updated according to comments and pushed.
Comment on attachment 161258 [details] [review] jpegformat: Add exif support Pushed a different version
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