After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 722127 - v4l2: Add NV12_64Z32 support
v4l2: Add NV12_64Z32 support
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 1.3.1
Assigned To: Nicolas Dufresne (ndufresne)
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-01-13 19:40 UTC by Nicolas Dufresne (ndufresne)
Modified: 2014-01-14 21:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add NV12_64Z32 into V4L2 (5.22 KB, patch)
2014-01-13 19:41 UTC, Nicolas Dufresne (ndufresne)
reviewed Details | Review
Update with GST_ROUND_UP_N (5.19 KB, patch)
2014-01-14 21:53 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Nicolas Dufresne (ndufresne) 2014-01-13 19:40:41 UTC
Add support for NV12_64Z32 into Video4Linux.
Comment 1 Nicolas Dufresne (ndufresne) 2014-01-13 19:41:37 UTC
Created attachment 266189 [details] [review]
Add NV12_64Z32 into V4L2
Comment 2 Sebastian Dröge (slomo) 2014-01-14 09:48:46 UTC
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?
Comment 3 Nicolas Dufresne (ndufresne) 2014-01-14 15:33:49 UTC
(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.
Comment 4 Nicolas Dufresne (ndufresne) 2014-01-14 21:53:35 UTC
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
Comment 5 Nicolas Dufresne (ndufresne) 2014-01-14 21:55:36 UTC
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.