GNOME Bugzilla – Bug 728045
omxvideodec: support cropping information
Last modified: 2018-05-01 08:31:51 UTC
Some video decoder components could provide cropping information in order to remove padding inserted by the encoder. For instance when the video size is not a multiple of macroblocks. Since I have an OMX video decoder which doesn't remove padding, I propose to have omxvideodec support cropping information. I intend to implement it in this way: - Add a structure to store crop information: GstOMXVideoCrop. - Get output port crop information at port reconfiguration time using the index OMX_IndexConfigCommonOutputCrop and set the GstOMXVideoCrop structure accordingly. - During allocation query/answer, we can check if downstream elements supports GstVideoCropMeta (for instance when chained with a videocrop or v4l2sink). If so omxvideodec will attach GstVideoCropMeta to buffer and let these elements do the job. Otherwise, we may do the crop in omxvideodec element, for instance when filling the output buffer.
Created attachment 274494 [details] [review] omxvideodec: implement decoder cropping information support This is a preliminary work. It just query the output crop config at reconfigure time and attach GstVideoCropMeta to buffer if downstream element support videometa. I don't like two things in this 'patch': - I add a dependency to gstomxvideodec in gstomxbufferpool because I add cropping info to GstOMXVideoDec structure. - I attach GstVideoCropMeta to buffer if downstream element support video meta. For the first point, I may add the cropping info in GstOMXBufferPool and move cropping structure declaration to gstomx.c (it will just define a rect). For the second point, we currently know that downstream element support crop meta API when we parse the allocation query. However we don't have a specific buffer pool option like for GST_VIDEO_META_API_TYPE. To solve this, we can define a new option in gstvideopool.h (gst-plugins-base) or add a boolean to GstOMXVideoDec to tell we want crop info and set associated flag in GstOMVBufferPool in gst_omx_video_dec_allocate_output_buffers().
Created attachment 274604 [details] [review] omxvideodec: implement decoder cropping information support I added boolean to know if downstream element support crop meta, I rename GstOMXVideoDecCrop in GstOMXVideoCrop and move it to gstomxvideo.c as it is only meaningful for video. I also modify gstomxbufferpool in consequences.
Review of attachment 274494 [details] [review]: I had a long discussion about cropping yesterday and I think we made a conclusion that this is not right. Here's why: decoder -> (buffer crop x=10,y=10) -> converter -> videosink You'll endup 10 pixels garbage on the right and bottom borders. The reason is that the converter will convert up to width * bpp (rather then up to stride), reducing the amount of processing when there is large padding. So instead of that, you need to adjust the offset of you image, so the data pointer you get from frame_map() points to the right place. This can be achieved easily filling the GstVideoAlignment structure, and calling gst_video_info_align() before copying the offset and stride array to the GstVideoMeta. VideoMeta need to be supported downstream obviously.
Review of attachment 274604 [details] [review]: Same comment.
This should be implemented the same way it is done in e.g. theoradec. You use the GstVideoCropMeta conditionally if downstream supports it, otherwise you have to do the cropping yourself.
I don't agree, Using video crop meta may confuse certain pipeline. As OMX do allocates it's own buffer, I think it's preferred play with the offset (see GstVideoAlignement and gst_video_info_align). For the reference: dec (crop meta with x,y positive, same size) ! effect ! displaysink It is likely that the effect will only be applied from from the portion starting at coordinate (0,0) till (width,height), instead of (x,y) till (width+x, height+y). CropMeta works well if the crop region is within (0,0) an (width,height), which might not always be the case.
A pipeline that is confused by the crop meta is buggy, and the faulty elements should be fixed. But yes, videoalignment solves a similar problem. But which of the two is supported downstream, or which of the two can be applied to OpenMAX has to be checked. They are not the same thing.
(In reply to comment #7) > But yes, videoalignment solves a similar problem. But which of the two is > supported downstream, or which of the two can be applied to OpenMAX has to be > checked. They are not the same thing. To clarify a bit (I know you know Sebastian, it's for others). Both requires VideoMeta to be supported downstream. VideoCrop require downstream to support it. VideoAlignement only require downstream support it if downstream pool is being used (currently only supported by ximagsink and xvimagsink). In OMX, it is likely that downstream pool won't be used, hence VideoAlignment being a good choice. Note that using this you can then use gst_video_frame_copy() to copy to a buffer with default alignment (if there is not way around).
(In reply to comment #8) > To clarify a bit (I know you know Sebastian, it's for others). Both requires > VideoMeta to be supported downstream. VideoCrop require downstream to support > it. VideoAlignement only require downstream support it if downstream pool is > being used (currently only supported by ximagsink and xvimagsink). In OMX, it > is likely that downstream pool won't be used, hence VideoAlignment being a good > choice. Note that using this you can then use gst_video_frame_copy() to copy to > a buffer with default alignment (if there is not way around). Ok, it could be easier for others elements to play with VideoAlignment or frame stride/offset to handle crop rectangle. So in which cases should we use VideoCrop meta ? At this moment, in current gst-omx implementation, it always use its internal bufferpool, but in the future, we could want to use an external pool with the OMX UseBuffer method. For information, in my case, I plan to use: omxvideodec --> v4l2sink io-mode=userptr to achieve a zero copy rendering path.
(In reply to Aurélien Zanelli from comment #9) > At this moment, in current gst-omx implementation, it always use its > internal bufferpool, but in the future, we could want to use an external > pool with the OMX UseBuffer method. Done here https://bugzilla.gnome.org/show_bug.cgi?id=784069 . Please someone fill free to sum-up the situation and tell if this cropping support is still something needed, thx. Marking as NEEDINFO for now.
If i remember correctly, with OMXUseBuffer, we could negotiate a bufferpool with downstream and configure alignment correctly to handle decoder cropping information (which are likely alignment). On my side, I don't need this anymore, so feel free to close this issue if no one else is interested.
Closing as per last comment