[gstreamer-bugs] [Bug 611962] tags: Move tags from metadata into -base

[GStreamer (bugzilla.gnome.org)]

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

Tim-Philipp Müller <t.i.m> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #155640|none                        |reviewed
             status|                            |

--- Comment #3 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2010-03-09 13:34:31 UTC ---
Review of attachment 155640:
 --> (https://bugzilla.gnome.org/review?bug=611962&attachment=155640)

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.

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