GNOME Bugzilla – Bug 773433
video-format: add API to check if a video format is "sparse"
Last modified: 2016-11-23 21:32:19 UTC
There is no simple way to know if a given GstVideoFormatInfo describes a sparse format.
Well, there is, fps_d = 0, fps_n = 1. Would you like a macro to check that ?
Created attachment 338362 [details] [review] add a IS_SPARSE flag to GstVideoFormatInfo
Comment on attachment 338362 [details] [review] add a IS_SPARSE flag to GstVideoFormatInfo What is the actual definition of "sparse" here? Is RGB also sparse for widths%4!=0, are formats in general sparse if there is some row-end-padding or plane padding? (Also you break ABI here, you have to add the flag at the end and not change the value of any existing ones)
Or most of our 10 bit formats, which store 10 bits in 16 bit integers?
Hmm, ok, let's forget about the term sparse, we need another word. Also, let's not merge this until we have code using it. I'd like if we can agree this is useful information before adding yet more code here.
The word sparse comes from video-format.h itself: * @GST_VIDEO_FORMAT_RGBx: sparse rgb packed into 32 bit, space last * @GST_VIDEO_FORMAT_BGRx: sparse reverse rgb packed into 32 bit, space last * @GST_VIDEO_FORMAT_xRGB: sparse rgb packed into 32 bit, space first * @GST_VIDEO_FORMAT_xBGR: sparse reverse rgb packed into 32 bit, space first About ABI : I preferred to add the new flag in a "correct" position and not at the end. But I can move it at the end if it is critical. Not sure it is as you are not supposed to access the bit field directly but use macros for that.
(In reply to Philippe Renon from comment #6) > About ABI : I preferred to add the new flag in a "correct" position and not > at the end. But I can move it at the end if it is critical. Not sure it is > as you are not supposed to access the bit field directly but use macros for > that. It's critical, yes :) As it's macros and not functions, the actual values of the enum are used by the compiler. If you change it in the header, recompilation of all code using those values is needed as now suddenly e.g. 2 means Y instead of X as before.
I will fix the ABI issue. As I do that I can also change the controversial use of "sparse" to "padded" throughout. Opinions ?
Created attachment 340033 [details] [review] video-format: add API to check if a video format is "sparse" fixed ABI
See https://bugzilla.gnome.org/show_bug.cgi?id=774576 for a use case of the API addition.
In the OpenCV case it probably makes more sense to explicitly support specific formats and handle them that way. Or work with the pixel stride (pstride) and the component offsets. Especially in your OpenCV patch you handle the "x" component as an additional channel, which it is not. It possibly contains uninitialized data even. My main problem here is still that sparse or padded is not well defined
Just for some context, here is the complete list of image types: #define CV_8UC1 CV_MAKETYPE(CV_8U,1) #define CV_8UC2 CV_MAKETYPE(CV_8U,2) #define CV_8UC3 CV_MAKETYPE(CV_8U,3) #define CV_8UC4 CV_MAKETYPE(CV_8U,4) #define CV_8UC(n) CV_MAKETYPE(CV_8U,(n)) #define CV_8SC1 CV_MAKETYPE(CV_8S,1) #define CV_8SC2 CV_MAKETYPE(CV_8S,2) #define CV_8SC3 CV_MAKETYPE(CV_8S,3) #define CV_8SC4 CV_MAKETYPE(CV_8S,4) #define CV_8SC(n) CV_MAKETYPE(CV_8S,(n)) #define CV_16UC1 CV_MAKETYPE(CV_16U,1) #define CV_16UC2 CV_MAKETYPE(CV_16U,2) #define CV_16UC3 CV_MAKETYPE(CV_16U,3) #define CV_16UC4 CV_MAKETYPE(CV_16U,4) #define CV_16UC(n) CV_MAKETYPE(CV_16U,(n)) #define CV_16SC1 CV_MAKETYPE(CV_16S,1) #define CV_16SC2 CV_MAKETYPE(CV_16S,2) #define CV_16SC3 CV_MAKETYPE(CV_16S,3) #define CV_16SC4 CV_MAKETYPE(CV_16S,4) #define CV_16SC(n) CV_MAKETYPE(CV_16S,(n)) #define CV_32SC1 CV_MAKETYPE(CV_32S,1) #define CV_32SC2 CV_MAKETYPE(CV_32S,2) #define CV_32SC3 CV_MAKETYPE(CV_32S,3) #define CV_32SC4 CV_MAKETYPE(CV_32S,4) #define CV_32SC(n) CV_MAKETYPE(CV_32S,(n)) #define CV_32FC1 CV_MAKETYPE(CV_32F,1) #define CV_32FC2 CV_MAKETYPE(CV_32F,2) #define CV_32FC3 CV_MAKETYPE(CV_32F,3) #define CV_32FC4 CV_MAKETYPE(CV_32F,4) #define CV_32FC(n) CV_MAKETYPE(CV_32F,(n)) #define CV_64FC1 CV_MAKETYPE(CV_64F,1) #define CV_64FC2 CV_MAKETYPE(CV_64F,2) #define CV_64FC3 CV_MAKETYPE(CV_64F,3) #define CV_64FC4 CV_MAKETYPE(CV_64F,4) #define CV_64FC(n) CV_MAKETYPE(CV_64F,(n)) First parameter to CV_MAKETYPE is the channel "depth" and second is the number of channels. In case of BGRx you need to specify 4 channels to opencv. If opencv is told that there are 3 channels then you see ghosting artefacts typical of stride issues. I think most opencv algorithm will ignore the x channel (or A for that matter). But it will probably fail if the x channel is not last. I need to check on that. That said, the proposed fix is not great (and probably not universal) but it does fix existing issues with BGRx.
BGRA (and RGBA) work ok because GST_VIDEO_INFO_N_COMPONENTS reports 4 channels for these formats. Again if the alpha channel is not the last then things probably start to fail. The proposed fix simply tries to do the same for BGRx (i.e. bring support of BGRx to the level of support found for BGRA).
PS: in the opencv context it can help to look at x as a channel that should be ignored...
Silently giving the "x" channel to OpenCV as if it contains something meaningful does not seem like a good idea. All this should probably be done on a per-element basis (when the element knows that it's fine to pass the "x" component there), and in a generic way per format or something in the helper functions. Note that you can get the same information right now by getting the number of components for the format (3) and the pixel stride (4). And by getting the component offsets you know where the "x" or "A" components would be. I also assume that for some filters it matters if it's RGBA or BGRA (i.e. they do different things with R and B). So there generally seems to be some more fundamental problem here.
Yes, I totally agree. The elements I looked at eventually do a cvCvtColor( img, norm_img, CV_BGR2GRAY ); And it works well with BGRx and RGB (or so it seems) so I need to find out why and what would happen with other formats (xBGR, etc...). For now, I do as suggested : i.e. explicit switch/case on the gst format... I have a working fix that uses the other suggestion (using pixel stride) but I don't like it... Looks like this: is_sparse = FALSE; for (i = 0; i < GST_VIDEO_INFO_N_COMPONENTS (&info); i++) { depth += GST_VIDEO_INFO_COMP_DEPTH (&info, i); if (GST_VIDEO_INFO_COMP_PSTRIDE (&info, i) > (gint) GST_VIDEO_INFO_N_COMPONENTS (&info)) is_sparse = TRUE; }
With the given input I found a simpler approach. See https://bugzilla.gnome.org/show_bug.cgi?id=774576. Please close this issue :)