[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 #4 from Thiago Sousa Santos <thiago.sousa.santos at collabora.co.uk> 2010-04-06 11:16:55 UTC ---
(In reply to comment #3)
> Review of attachment 157971 [details]:
> 
> ::: 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 :)

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