GNOME Bugzilla – Bug 611962
tags: Move tags from metadata into -base
Last modified: 2010-08-10 15:00:27 UTC
Created attachment 155392 [details] [review] Trivial patch that just copies metatags.c into -base Metadata plugin in -bad has lots of tags that could go into -base tag library, or maybe some of them could go into -core (the more generic ones). Most of the tags in metadatatags.[ch] are about photography/image tagging.
Created attachment 155562 [details] [review] tags: Move the tags from metadata to tag library Updated patch that removes some useless tags (not registered) and adds FIXMEs to the items I'm reviewing. Mostly they are tags that use ints and are in fact enums. But it seems we don't use enum tags around. So if you have an idea on how to help here, it'd be really welcome.
Created attachment 155640 [details] [review] tags: Move the tags from metadata to tag library Updated patch using enums for some tags
Review of attachment 155640 [details] [review]: Had a quick glance at this, comments below. Most importantly I think this should be changed to use glib-mkenums. ::: gst-libs/gst/tag/gstimagetags.c @@ +23,3 @@ + * + * Alternatively, the contents of this file may be used under the + * GNU Lesser General Public License Version 2.1 (the "LGPL"), in Here it says LGPL 2.1, but below "version 2 or later". Please make sure it is "version 2 or later" like the rest of GStreamer. @@ +82,3 @@ + } + return g_define_type_id__volatile; +} glib-mkenum should do all this for us :) @@ +128,3 @@ + static const GEnumValue values[] = { + {GST_TAG_CAPTURE_EXPOSURE_PROGRAM_NOT_DEFINED, + "GST_TAG_CAPTURE_EXPOSURE_PROGRAM_NOT_DEFINED", "not-defined"}, Or UNDEFINED? @@ +427,3 @@ + gst_tag_register (GST_TAG_CAPTURE_CUSTOM_RENDERED, GST_TAG_FLAG_META, + G_TYPE_BOOLEAN, GST_TAG_CAPTURE_CUSTOM_RENDERED, + "Indicates the use of special processing on image data", NULL); Example? @@ +620,3 @@ + gst_tag_register (GST_TAG_DATE_TIME_DIGITIZED, GST_TAG_FLAG_META, + G_TYPE_STRING, GST_TAG_DATE_TIME_DIGITIZED, + "Date/Time of image digitized", NULL); Maybe we should also provide utility functions to parse these strings? Also see bug against core for generic date/time tag @@ +668,3 @@ +static void +gst_image_tags_xmp_register (void) +{ Do we really want multiple _register() functions? (Ideally we should think of some system where these aren't needed at all, some kind of tag factory so that core knows what plugin to load to get certain tags registered automatically when they're first used or something..) ::: gst-libs/gst/tag/gstimagetags.h @@ +50,3 @@ + +#include <gst/gst.h> +#include <gst/base/gstadapter.h> I don't think we need to include GstAdapter here? @@ +62,3 @@ + METADATA_TAG_MAP_INDIVIDUALS = 1 << 0, + METADATA_TAG_MAP_WHOLECHUNK = 1 << 1 +} MetadataTagMapping; Is this used anywhere? @@ +84,3 @@ + GST_TAG_CAPTURE_EXPOSURE_MODE_AUTO = 0, + GST_TAG_CAPTURE_EXPOSURE_MODE_MANUAL, + GST_TAG_CAPTURE_EXPOSURE_MODE_AUTO_BRACKET, Should get rid of all the trailing commas, some compilers don't like those IIRC (e.g. old gcc versions). @@ +87,3 @@ +} GstTagCaptureExposureMode; +GType gst_tag_capture_exposure_mode_get_type (void); +#define GST_TAG_CAPTURE_EXPOSURE_MODE_TYPE \ Shouldn't this be GST_TYPE_TAG_CAPTURE_EXPOSURE_MODE? (But in any case, we should be using glib-mkenums, which should create these for us) @@ +188,3 @@ + 24- ISO studio tungsten + 255- other light source + Other = reserved These should be turned into proper gtk-doc chunks. Where do these values come from? Are they generic or relating to a particular driver/camera? @@ +229,3 @@ +/* individual tags */ + +#define GST_TAG_CAPTURE_APERTURE "capture-aperture" These should all have gtk-doc chunks one day, with the type mentioned :) All the comments above the gst_tag_register() should be moved into the gtk-doc chunks as well really.. @@ +239,3 @@ +#define GST_TAG_CAPTURE_EXPOSURE_TIME "capture-exposure-time" +#define GST_TAG_CAPTURE_FLASH "capture-flash" +#define GST_TAG_CAPTURE_FNUMBER "capture-fnumber" Maybe _FOCAL_RATIO would be nicer? (not sure) @@ +240,3 @@ +#define GST_TAG_CAPTURE_FLASH "capture-flash" +#define GST_TAG_CAPTURE_FNUMBER "capture-fnumber" +#define GST_TAG_CAPTURE_FOCAL_LEN "capture-focal-len" Maybe this should be FOCAL_LENGTH? @@ +270,3 @@ +#define GST_TAG_XMP_GEO_LOCATION_SUBLOCATION "geo-location-sublocation" + +/* *INDENT-ON* */ Header files are not indented by gst-indent, these indent-on/off markers aren't needed.
Created attachment 155730 [details] [review] tags: Move the tags from metadata to tag library Updated patch with Tim's comments and removed the _GEO_* tags as those are in another bug now: #612410
Created attachment 155740 [details] [review] tags: Move the tags from metadata to tag library Same as before, forgot to add a file to the patch
Review of attachment 155740 [details] [review]: Thanks for cleaning this up. A few more small comments below: ::: gst-libs/gst/tag/gstimagetags.c @@ +49,3 @@ + * + * This module register tags need for image metadata. + * typo: register_s_ @@ +68,3 @@ + * extern functions implementations + */ + remove above comment. no externs here @@ +229,3 @@ + * Other = reserved + */ + /* FIXME: this seems complex */ Can a GFlag be used here (like a GEnum below)? ::: gst-libs/gst/tag/gstimagetags.h @@ +64,3 @@ + * The whole data in an EXIF tag, as stored in a file + */ +#define GST_TAG_EXIF "exif" These doc-comments (this and below) are a bit weird. Could you please make them e.g.: /* * GST_TAG_EXIF: * * The whole data of an EXIF section as a #GstBuffer. */ ::: gst-libs/gst/tag/gstimagetagsdefs.h @@ +158,3 @@ + GST_TAG_CAPTURE_WHITE_BALANCE_FLASH, + GST_TAG_CAPTURE_WHITE_BALANCE_HORIZON /* sun on the horizon */ +} GstTagCaptureWhiteBalance; move comment to doc blob @@ +184,3 @@ + * @GST_TAG_CAPTURE_LIGHT_SOURCE_OTHER_LIGHT_SOURCE: + */ +/* Some notes that might help in the doc could you move those to the above doc comment?
I'll mark these as obsolete as it seems to be easier to open bugs for smaller groups of tags (easier for reviewing and integrating in the xmp/exif libs). I've been doing that successfully on the past days.