[Bug 611157] [RFC] more buffer flags and caps fields in gst-video for 3d video

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #60 from Sebastian Dröge <slomo at circular-chaos.org> 2013-04-24 13:07:14 UTC ---
Review of attachment 241092:
 --> (https://bugzilla.gnome.org/review?bug=611157&attachment=241092)

::: gst-libs/gst/video/video-stereo.c
@@ +36,3 @@
+} StereoFrameType;
+
+static StereoFrameType sf_types[] = {

constify

@@ +61,3 @@
+
+const gchar *
+get_stereo_frame_string_from_type (GstStereoVideoFrameType type)

frame_type_to_string() maybe?

Also everything should be gst_video_* and GstVideo*

@@ +85,3 @@
+const gchar *
+gst_stereo_video_frame_get_type_string (GstStereoVideoScheme scheme,
+    guint frame_type)

Shouldn't the frame_type be some enum type?

@@ +168,3 @@
+const gchar *
+gst_stereo_video_frame_get_layout_string (GstStereoVideoScheme scheme,
+    guint frame_layout)

Shouldn't the frame_layout be some enum type?

@@ +212,3 @@
+gst_stereo_video_caps_set_stereo_info (GstCaps * caps,
+    GstStereoVideoScheme scheme, GstVideoChannelLayout channel_layout,
+    guint frame_type, guint frame_layout)

Shouldn't the frame_type and frame_layout be some enum type?

::: gst-libs/gst/video/video-stereo.h
@@ +41,3 @@
+  GST_STEREO_VIDEO_SCHEME_ISO_IEC_13818_2,
+  GST_STEREO_VIDEO_SCHEME_UNKNOWN
+} GstStereoVideoScheme;

GstVideo* and gst_video_* everywhere

@@ +53,3 @@
+typedef enum {
+  GST_VIDEO_CHANNEL_LAYOUT_STEREO,
+  GST_VIDEO_CHANNEL_LAYOUT_MONO_STEREO,

Maybe another value for MONO here?

@@ +54,3 @@
+  GST_VIDEO_CHANNEL_LAYOUT_STEREO,
+  GST_VIDEO_CHANNEL_LAYOUT_MONO_STEREO,
+  GST_VIDEO_CHANNEL_LAYOUT_UNKNOWN

And multiview?

@@ +76,3 @@
+ *     packed over-under in a single frame.
+ * @GST_STEREO_VIDEO_FRAME_TYPE_PACKED_CHECK_BOARD_INTERLEAVED: 2 views are
+ *     packed in a single frame as check-board interleaved (quincunx
sampling).

Maybe call the enum value quincunx then, in case there are other similar
check-board-like patterns in the future

@@ +109,3 @@
+  GST_STEREO_VIDEO_FRAME_LAYOUT_LEFT_VIEW_FIRST      = (1 << 0),
+  GST_STEREO_VIDEO_FRAME_LAYOUT_HORIZONTALLY_FLIPPED = (1 << 2),
+  GST_STEREO_VIDEO_FRAME_LAYOUT_VERTICALLY_FLIPPED   = (1 << 3),

These last two flags are also proposed as buffer flags, and here they're for
the caps. Why two places? Can it change frame-by-frame?

@@ +110,3 @@
+  GST_STEREO_VIDEO_FRAME_LAYOUT_HORIZONTALLY_FLIPPED = (1 << 2),
+  GST_STEREO_VIDEO_FRAME_LAYOUT_VERTICALLY_FLIPPED   = (1 << 3),
+  GST_STEREO_VIDEO_FRAME_LAYOUT_UNKNOWN          = (1 << 4)

Is this a flags type or a enum? For flags UNKNOWN should be 0

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