GNOME Bugzilla – Bug 722127
v4l2: Add NV12_64Z32 support
Last modified: 2014-01-14 21:55:36 UTC
Add support for NV12_64Z32 into Video4Linux.
Created attachment 266189 [details] [review] Add NV12_64Z32 into V4L2
Review of attachment 266189 [details] [review]: Generally looks good but some weird things... ::: sys/v4l2/gstv4l2bufferpool.c @@ +58,3 @@ +#ifndef ALIGN +#define ALIGN(value,n) ((((value) + ((n) - 1)) & ~((n) - 1))) Maybe we should get an utility macro for that in gstutils.h @@ +448,3 @@ + tile_height = 1 << hs; + + x_tiles = obj->bytesperline[i] >> ws; Isn't the tilesize in components (or pixels?) while bytesperline is in bytes? You mix units here then without conversion @@ +450,3 @@ + x_tiles = obj->bytesperline[i] >> ws; + y_tiles = GST_VIDEO_FORMAT_INFO_SCALE_HEIGHT (finfo, i, + ALIGN (height, tile_height) >> hs); But not here But maybe all this needs rounding up instead of the rounding down division with >> ? @@ +568,3 @@ + if (GST_VIDEO_FORMAT_INFO_IS_TILED (obj->info.finfo)) { + stride = GST_VIDEO_TILE_X_TILES (stride) << + GST_VIDEO_FORMAT_INFO_TILE_WS ((obj->info.finfo)); And here any rounding needed or not?
(In reply to comment #2) > Review of attachment 266189 [details] [review]: > > Generally looks good but some weird things... > > ::: sys/v4l2/gstv4l2bufferpool.c > @@ +58,3 @@ > > +#ifndef ALIGN > +#define ALIGN(value,n) ((((value) + ((n) - 1)) & ~((n) - 1))) > > Maybe we should get an utility macro for that in gstutils.h I wasn't sure I wanted to expose this publicly as it's very limited to power of 2 without fallback. It's something available in the kernel in similar form though. I'll think about it. If not, I think I should give it a name-space, right ? > > @@ +448,3 @@ > + tile_height = 1 << hs; > + > + x_tiles = obj->bytesperline[i] >> ws; > > Isn't the tilesize in components (or pixels?) while bytesperline is in bytes? > You mix units here then without conversion No, tile_width is in bytes. By definition, the choice of tile size is memory related, and is made to provide a precise and uniform memory blocks to optimize the use of cache. Also, bytesperline is always a mutileple of tile_width, and it a multiple for 2 * tile_width for Z-flip-Z layout. > > @@ +450,3 @@ > + x_tiles = obj->bytesperline[i] >> ws; > + y_tiles = GST_VIDEO_FORMAT_INFO_SCALE_HEIGHT (finfo, i, > + ALIGN (height, tile_height) >> hs); > > But not here There is not pixel / bytes conversion here. V4L2, unlike Android, has no concept of lines per plane, which mean we need to extrapolate it from the information we have. In this case we do a roundup division "ALIGN (height, tile_height) >> hs" to get the number tiles in Y on the first plane, and then scale it (assuming as usual that plane number match a component number). > > > But maybe all this needs rounding up instead of the rounding down division with > >> ? The stride is aligned to tile size, otherwise it's a driver bug. > > @@ +568,3 @@ > + if (GST_VIDEO_FORMAT_INFO_IS_TILED (obj->info.finfo)) { > + stride = GST_VIDEO_TILE_X_TILES (stride) << > + GST_VIDEO_FORMAT_INFO_TILE_WS ((obj->info.finfo)); > > And here any rounding needed or not? Multiplication does not need rounding.
Created attachment 266299 [details] [review] Update with GST_ROUND_UP_N commit 4cffae36e3ca05e1ffc3a2db3c22eb7481b9b74d Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Tue Dec 10 14:29:55 2013 -0500 v4l2: Add NV12_64Z32 support https://bugzilla.gnome.org/show_bug.cgi?id=722127
Related commit, sorry as I forgot the bug reference this time: commit abddae152dd449f9d11d979ee7d4a9608e806978 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Tue Jan 14 16:15:02 2014 -0500 util: Add GST_ROUND_UP_N and GST_ROUND_DOWN_N These are generic rounding macro that works for any power of two.