GNOME Bugzilla – Bug 796106
video: Support alternate interlaced video
Last modified: 2018-08-29 16:50:25 UTC
Until now, GStreamer has until now only supported mixed and interleaved types of interlaved video buffers. These patches add supporting API and format for interlaced video buffers, where each buffer carries only one of the two fields in an alternating manner. I'm working on adding actual support for this in interlace and deinterlace elements, which rely on this new API/formats. I wanted to get input on the API/format already so submitting these patches for review but if they are good, we should wait before merging until I provide a way to test this in action.
Created attachment 372015 [details] [review] video: Add format for single fields of interlaced video in a buffer
Created attachment 372016 [details] [review] video: Add GST_VIDEO_INTERLACE_MODE_ALTERNATE Add a new interlace mode enum to represent buffers containing a single field of an interlaced video in a buffer. The name is based on the equivalent video format in the V4L2 API, V4L2_FIELD_ALTERNATE: https://01.org/linuxgraphics/gfx-docs/drm/media/uapi/v4l/field-order.html Since caps fields are optional, we also introduce a new caps feature, "format:Interlaced" that always goes with "alternate" interlace mode to ensure that caps for this incompatible format are incompatible with other interlaced and progressive video caps.
Review of attachment 372015 [details] [review]: I think we should modify gst_video_frame_map_id() to add the buffer flag -> frame flag conversion as it exists for the other flags. ::: gst-libs/gst/video/video-frame.h @@ +58,3 @@ + GST_VIDEO_FRAME_FLAG_TF = GST_VIDEO_FRAME_FLAG_TFF | + GST_VIDEO_FRAME_FLAG_ONEFIELD, + GST_VIDEO_FRAME_FLAG_BF = GST_VIDEO_FRAME_FLAG_ONEFIELD, Shouldn't we add extra GST_VIDEO_FRAME_IS_* macros for those?
Review of attachment 372015 [details] [review]: ::: gst-libs/gst/video/video-frame.h @@ +58,3 @@ + GST_VIDEO_FRAME_FLAG_TF = GST_VIDEO_FRAME_FLAG_TFF | + GST_VIDEO_FRAME_FLAG_ONEFIELD, + GST_VIDEO_FRAME_FLAG_BF = GST_VIDEO_FRAME_FLAG_ONEFIELD, Good catch, yeah I'd think so. Also the gst_video_frame_map_id() should be taken care of, yes.
Review of attachment 372015 [details] [review]: ::: gst-libs/gst/video/video-frame.h @@ +58,3 @@ + GST_VIDEO_FRAME_FLAG_TF = GST_VIDEO_FRAME_FLAG_TFF | + GST_VIDEO_FRAME_FLAG_ONEFIELD, + GST_VIDEO_FRAME_FLAG_BF = GST_VIDEO_FRAME_FLAG_ONEFIELD, Actually, the gst_video_frame_map_id() doesn't require any changes since both buffer and frame flags here are just re-using existing flags.
Created attachment 372507 [details] [review] video: Add format for single fields of interlaced video in a buffer Addressed comments from Guillaume.
Created attachment 372508 [details] [review] video: Add GST_VIDEO_INTERLACE_MODE_ALTERNATE Just re-attaching to keep patches in order.
(In reply to Zeeshan Ali (Khattak) from comment #5) > Actually, the gst_video_frame_map_id() doesn't require any changes since > both buffer and frame flags here are just re-using existing flags. Right, good point. Maybe add a comment so it's clearer?
Review of attachment 372507 [details] [review]: ::: gst-libs/gst/video/video-frame.h @@ +44,3 @@ + * @GST_VIDEO_FRAME_FLAG_BF: The video frame has the bottom field only. This is + * the same as GST_VIDEO_FRAME_FLAG_ONEFIELD + * (GST_VIDEO_FRAME_FLAG_TFF flag unset). Missing Since marker. @@ +58,3 @@ + GST_VIDEO_FRAME_FLAG_TF = GST_VIDEO_FRAME_FLAG_TFF | + GST_VIDEO_FRAME_FLAG_ONEFIELD, + GST_VIDEO_FRAME_FLAG_BF = GST_VIDEO_FRAME_FLAG_ONEFIELD, May I suggest naming these just _TOP and _BOTTOM ? @@ +125,3 @@ #define GST_VIDEO_FRAME_IS_ONEFIELD(f) (GST_VIDEO_FRAME_FLAG_IS_SET(f, GST_VIDEO_FRAME_FLAG_ONEFIELD)) +#define GST_VIDEO_FRAME_IS_TF(f) (GST_VIDEO_FRAME_FLAG_IS_SET(f, GST_VIDEO_FRAME_FLAG_TF)) +#define GST_VIDEO_FRAME_IS_BF(f) (GST_VIDEO_FRAME_FLAG_IS_SET(f, GST_VIDEO_FRAME_FLAG_BF)) Then IS_TOP, IS_BOTTOM. @@ +174,3 @@ + * @GST_VIDEO_BUFFER_FLAG_BF: The video frame has the bottom field only. This is + * the same as GST_VIDEO_BUFFER_FLAG_ONEFIELD + * (GST_VIDEO_BUFFER_FLAG_TFF flag unset). Same. @@ +195,3 @@ + GST_VIDEO_BUFFER_FLAG_ONEFIELD, + GST_VIDEO_BUFFER_FLAG_BF = GST_VIDEO_BUFFER_FLAG_ONEFIELD, + Same.
Review of attachment 372508 [details] [review]: This looks sensible to me. ::: gst-libs/gst/video/video-info.h @@ +35,3 @@ + * + * Name of the caps feature for indicating the stream is interlaced. Currently + * it is only used for video. Need a Since marker.
Review of attachment 372507 [details] [review]: ::: gst-libs/gst/video/video-frame.h @@ +58,3 @@ + GST_VIDEO_FRAME_FLAG_TF = GST_VIDEO_FRAME_FLAG_TFF | + GST_VIDEO_FRAME_FLAG_ONEFIELD, + GST_VIDEO_FRAME_FLAG_BF = GST_VIDEO_FRAME_FLAG_ONEFIELD, or _TOP_FIELD and _BOTTOM_FIELD
Or that, I'm just looking for code that I won't need to pause and translate the acronym while reading. I really don't mind the extra chars and TOP_FIELD/BOTTOM_FIELD is really clear.
Should we add GST_VIDEO_INFO_FIELD_HEIGHT() and GST_VIDEO_INFO_FIELD_SIZE() to save elements implementing this interlacing mode to do the math themselves?
Sounds useful, yes
We'll also have to change existing code as we are breaking the assumption 'info.size == buffer-size'. Like in gst_video_frame_map() : https://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst-libs/gst/video/video-frame.c#n120 or video_buffer_pool_set_config : https://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst-libs/gst/video/gstvideopool.c#n143 I'm sure there are more of those. The one frame / one buffer model will go away with bug #660770 as well so it may be interesting to think about the proper general solution we'd want.
(In reply to Guillaume Desmottes from comment #15) > We'll also have to change existing code as we are breaking the assumption > 'info.size == buffer-size'. No, we won't break that. The plan is to add a gst_video_info_set_interlace_format(), that does the same as gst_video_info_set_format() but also gets passed interlace mode, so for ALTERNATE interlace mode, it'll ensure that size canculation takes into account the half height.
(In reply to Guillaume Desmottes from comment #13) > Should we add GST_VIDEO_INFO_FIELD_HEIGHT() and GST_VIDEO_INFO_FIELD_SIZE() The former is on my TODO already, the latter not cause Nicolas and I agreed some weeks ago that info.size will hold the actual buffer size and that we add the said helper (in comment#16) to ensure that's the case.
I agree that info.size being the field/buffer size makes sense. But I find it strange and confusing that some part of GstVideoInfo refers to the full frame (height, width) and some to the field. Also, if we have an odd frame height what should GST_VIDEO_INFO_FIELD_HEIGHT() be?
(In reply to Guillaume Desmottes from comment #18) > I agree that info.size being the field/buffer size makes sense. But I find > it strange and confusing that some part of GstVideoInfo refers to the full > frame (height, width) and some to the field. Which ones? info.size is just the buffer size and GST_VIDEO_INFO_FIELD_HEIGHT() will just be a helper on top. The struct itself just represents the caps. > Also, if we have an odd frame height what should > GST_VIDEO_INFO_FIELD_HEIGHT() be? That's good question. I guess the last line will (and should?) be ignored? Maybe it's enough (for now) to just add a note about that in the appropriate places?
So we agreed that GstVideoInfo.height should be the full frame height. But what about GstVideoMeta.height? Are we keeping the same logic and use the frame height rather than the buffer/field height?
(In reply to Guillaume Desmottes from comment #20) > So we agreed that GstVideoInfo.height should be the full frame height. But > what about GstVideoMeta.height? Are we keeping the same logic and use the > frame height rather than the buffer/field height? Well in case of VideoInfo, it's expected to be height of the video (not frame) and same as the height in caps. Doesn't VideoMeta represent the buffer/frame rather than the video?
Hard to say, VideoMeta is attached to a buffer, so there will be two GstVideoMeta per frame in that context.
Our current WIP implementation is using the frame height for the video meta; that seemed the easier way.
Created attachment 373400 [details] [review] video: Add format for single fields of interlaced video in a buffer
Created attachment 373401 [details] [review] video: Add GST_VIDEO_INTERLACE_MODE_ALTERNATE Add a new interlace mode enum to represent buffers containing a single field of an interlaced video in a buffer. The name is based on the equivalent video format in the V4L2 API, V4L2_FIELD_ALTERNATE: https://01.org/linuxgraphics/gfx-docs/drm/media/uapi/v4l/field-order.html Since caps fields are optional, we also introduce a new caps feature, "format:Interlaced" that always goes with "alternate" interlace mode to ensure that caps for this incompatible format are incompatible with other interlaced and progressive video caps.
Created attachment 373402 [details] [review] video: Add gst_video_info_set_interlaced_format() Add a helper to set the interlacing mode while creating the GstVideoInfo in addition to format and resolution. Using this helper will ensure that size is correctly calculated for split-field interlacing mode.
Created attachment 373403 [details] [review] test: Ensure gst_video_info_set_format() calls succeed
Created attachment 373404 [details] [review] video: Add GST_VIDEO_INFO_FIELD_HEIGHT() macro Add a new macro that gives you the height of a field. It returns the height of the full frame unless split-field (alternate) interlacing is in use. Also GST_VIDEO_INFO_COMP_HEIGHT macro now uses this new macro to get the height for its calculation.
Created attachment 373405 [details] [review] video: Add gst_video_decoder_set_interlaced_output_state() Add a variant of gst_video_decoder_set_output_state() that allows the user to pass an interlacing mode as well. This is needed to ensure that gst_video_info_set_interlaced_format() is used instead so that GstVideoInfo.size is correctly initialized.
Created attachment 373406 [details] [review] video: Add GST_VIDEO_INFO_FIELD_RATE_N() macro Add a new macro that gives you the rate of the fields, which is the numerator of the field-rate for ALTERNATE interlacing video and FPS for progressive and other interlacing formats.
Review of attachment 373400 [details] [review]: ::: gst-libs/gst/video/video-frame.h @@ +41,3 @@ * in a set of corresponding views provided as sequential frames. + * @GST_VIDEO_FRAME_FLAG_TOP_FIELD: The video frame has the top field only. This + * is the same as GST_VIDEO_FRAME_FLAG_TFF | GST_VIDEO_FRAME_FLAG_ONEFIELD. (Since 1.16) @@ +44,3 @@ + * @GST_VIDEO_FRAME_FLAG_BOTTOM_FIELD: The video frame has the bottom field + * only. This is the same as GST_VIDEO_FRAME_FLAG_ONEFIELD + * (GST_VIDEO_FRAME_FLAG_TFF flag unset). (Since 1.16) @@ +174,3 @@ + * @GST_VIDEO_BUFFER_FLAG_BOTTOM_FIELD: The video frame has the bottom field only. This is + * the same as GST_VIDEO_BUFFER_FLAG_ONEFIELD + * (GST_VIDEO_BUFFER_FLAG_TFF flag unset). (Since 1.16)
Review of attachment 373401 [details] [review]: ::: gst-libs/gst/video/video-info.c @@ +698,3 @@ + height = (gsize) info->height / 2; + else + height = (gsize) info->height; Maybe move up the patch adding GST_VIDEO_INFO_FIELD_HEIGHT() and use it here ? ::: gst-libs/gst/video/video-info.h @@ +34,3 @@ + * GST_CAPS_FEATURE_FORMAT_INTERLACED: + * + * Name of the caps feature for indicating the stream is interlaced. Currently What about "Name of the caps feature indicating that the stream is interlaced." ? @@ +35,3 @@ + * + * Name of the caps feature for indicating the stream is interlaced. Currently + * it is only used for video. Since: 1.16 @@ +56,3 @@ + * @GST_VIDEO_BUFFER_FLAG_TF or @GST_VIDEO_BUFFER_FLAG_BF indicates if + * the buffer is carrying the top or bottom field, respectively. The top and + * bottom buffers are expected to alternate in the pipeline, with this mode. (Since 1.16)
Review of attachment 373402 [details] [review]: Lgtm.
Review of attachment 373403 [details] [review]: Lgtm.
Review of attachment 373404 [details] [review]: ::: gst-libs/gst/video/video-info.h @@ +349,3 @@ #define GST_VIDEO_INFO_WIDTH(i) ((i)->width) #define GST_VIDEO_INFO_HEIGHT(i) ((i)->height) +#define GST_VIDEO_INFO_FIELD_HEIGHT(i) ((i)->interlace_mode == GST_VIDEO_INTERLACE_MODE_ALTERNATE? (i)->height / 2 : (i)->height) A bit sad none of these is documented, but clearly not related to this change, so I'm fine. Maybe document this one, just for the sake of adding the Since: 1.16 marker ?
Review of attachment 373405 [details] [review]: ::: gst-libs/gst/video/gstvideodecoder.c @@ +3471,2 @@ * * Returns: (transfer full): the newly configured output state. this is the new one, should have a Since: 1.16
Review of attachment 373406 [details] [review]: lgtm
Created attachment 373474 [details] [review] video: Add format for single fields of interlaced video in a buffer
Created attachment 373475 [details] [review] video: Add GST_VIDEO_INTERLACE_MODE_ALTERNATE Add a new interlace mode enum to represent buffers containing a single field of an interlaced video in a buffer. The name is based on the equivalent video format in the V4L2 API, V4L2_FIELD_ALTERNATE: https://01.org/linuxgraphics/gfx-docs/drm/media/uapi/v4l/field-order.html Since caps fields are optional, we also introduce a new caps feature, "format:Interlaced" that always goes with "alternate" interlace mode to ensure that caps for this incompatible format are incompatible with other interlaced and progressive video caps.
Created attachment 373476 [details] [review] video: Add gst_video_info_set_interlaced_format() Add a helper to set the interlacing mode while creating the GstVideoInfo in addition to format and resolution. Using this helper will ensure that size is correctly calculated for split-field interlacing mode.
Created attachment 373477 [details] [review] test: Ensure gst_video_info_set_format() calls succeed
Created attachment 373478 [details] [review] video: Add GST_VIDEO_INFO_FIELD_HEIGHT() macro Add a new macro that gives you the height of a field. It returns the height of the full frame unless split-field (alternate) interlacing is in use. Also GST_VIDEO_INFO_COMP_HEIGHT macro now uses this new macro to get the height for its calculation.
Created attachment 373479 [details] [review] video: Add gst_video_decoder_set_interlaced_output_state() Add a variant of gst_video_decoder_set_output_state() that allows the user to pass an interlacing mode as well. This is needed to ensure that gst_video_info_set_interlaced_format() is used instead so that GstVideoInfo.size is correctly initialized.
Created attachment 373480 [details] [review] video: Add GST_VIDEO_INFO_FIELD_RATE_N() macro Add a new macro that gives you the rate of the fields, which is the numerator of the field-rate for ALTERNATE interlacing video and FPS for progressive and other interlacing formats.
Review of attachment 373474 [details] [review]: .
Review of attachment 373476 [details] [review]: .
Review of attachment 373477 [details] [review]: .
Review of attachment 373478 [details] [review]: .
Review of attachment 373475 [details] [review]: .
Review of attachment 373479 [details] [review]: .
Review of attachment 373480 [details] [review]: .
Attachment 373474 [details] pushed as 8fa9fda - video: Add format for single fields of interlaced video in a buffer Attachment 373475 [details] pushed as bd9c7b3 - video: Add GST_VIDEO_INTERLACE_MODE_ALTERNATE Attachment 373476 [details] pushed as 0fbe463 - video: Add gst_video_info_set_interlaced_format() Attachment 373477 [details] pushed as 0c034f3 - test: Ensure gst_video_info_set_format() calls succeed Attachment 373478 [details] pushed as 4879983 - video: Add GST_VIDEO_INFO_FIELD_HEIGHT() macro Attachment 373479 [details] pushed as be6c840 - video: Add gst_video_decoder_set_interlaced_output_state() Attachment 373480 [details] pushed as 3880eb8 - video: Add GST_VIDEO_INFO_FIELD_RATE_N() macro