[Bug 722127] v4l2: Add NV12_64Z32 support

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=722127
  GStreamer | gst-plugins-good | git

--- Comment #3 from Nicolas Dufresne <nicolas.dufresne at collabora.co.uk> 2014-01-14 15:33:49 UTC ---
(In reply to comment #2)
> Review of attachment 266189 [details]:
> 
> 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.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.