[Bug 611157] video: API to signal stereoscopic and multiview video

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #68 from sreerenj <bsreerenj at gmail.com> 2013-04-30 07:33:49 UTC ---
(In reply to comment #60)
> Review of attachment 241092 [details]:
> 
> frame_type_to_string() maybe?
> 
> Also everything should be gst_video_* and GstVideo*

I was thinking that SteroVideo is giving more readability :). Will change this.
> 
> @@ +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?

*NO*. Because the frame_type is the integer parsed from the encoded data. The
Enum value is unique for gstreamer for each type. So these APIs are for
demuxers, which is intended to parse the data and call this api. Because
different Schemes have different values for the FrameType.
for eg:
in GST_VIDEO_STEREO_SCHEME_ISO_IEC_23000_11, 0x00 indicates
packed_side_by_side.
in  GST_VIDEO_STEREO_SCHEME_ISO_IEC_ISO_IEC_14496_10, 0x00 indicates
check_bord_interleaving.
> 
> @@ +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?

same explanation :)
> 
> @@ +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?

same explanation :)
> 
> ::: 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?

by default it is mono. May be it can change like this:
GST_VIDEO_CHANNEL_LAYOUT_STEREO = 1
GST_VIDEO_CHANNEL_LAYOUT_MONO_STEREO = 2

any objection?

> 
> @@ +54,3 @@
> +  GST_VIDEO_CHANNEL_LAYOUT_STEREO,
> +  GST_VIDEO_CHANNEL_LAYOUT_MONO_STEREO,
> +  GST_VIDEO_CHANNEL_LAYOUT_UNKNOWN
> 
> And multiview?

I had a multiview flag before and then removed since we are only thinking about
the stereo video at the moment.
But no objection to add it if necessary.

> @@ +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

Okay. :)

> 
> @@ +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?

I just added it for completion: I added a comment also in the
video-stereo.h.may be you missed it:
/* Caps doesn't have field for flipping flags */

I only spec which supports this at the moement is
GST_VIDEO_STEREO_SCHEME_ISO_IEC_ISO_IEC_14496_10 and we won't get this info
from demuxer . Will get it only when parsing SEI headers. 

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