Created attachment 287859 [details] [review]
v4l2allocator: fix import_userptr() in single-planar API when

Patch proposal which assume that planes are contiguous.

Review of attachment 287859 [details] [review]:

::: sys/v4l2/gstv4l2allocator.c
@@ +1138,3 @@
+    if (V4L2_TYPE_IS_MULTIPLANAR (allocator->type)) {
+      /* TODO Support passing N plane from 1 memory to MPLANE v4l2 format */
+      goto n_mem_missmatch;

This though should really be supported.

@@ +1142,3 @@
+      /* With the single planar API, planes should be contiguous in memory.
+       * TODO: Check that planes are contiguous instead of assuming it's true ?
+       */

Feels like without this part in, it isn't safe to merge that. Please correct me if I'm wrong.

(In reply to comment #2)
> Review of attachment 287859 [details] [review]:
> 
> ::: sys/v4l2/gstv4l2allocator.c
> @@ +1138,3 @@
> +    if (V4L2_TYPE_IS_MULTIPLANAR (allocator->type)) {
> +      /* TODO Support passing N plane from 1 memory to MPLANE v4l2 format */
> +      goto n_mem_missmatch;
> 
> This though should really be supported.
I can try to look at this part, but I think it should be done in a different patch.

> 
> @@ +1142,3 @@
> +      /* With the single planar API, planes should be contiguous in memory.
> +       * TODO: Check that planes are contiguous instead of assuming it's true
> ?
> +       */
> 
> Feels like without this part in, it isn't safe to merge that. Please correct me
> if I'm wrong.
I think so, especially if data[i] points to different 'memory space', ie planes not contiguous in virtual memory. I try to do the following to check that:

for (i = 0; i < (n_planes - 1); i++) {
  gsize diff_data = ((guint8 *) data[i + 1]) - ((guint8 *) data[i]);
  gsize size = offset[i + 1] - offset[i];
  
  if (diff_data != size)
    goto non_contiguous_mem;
}

It works for contiguous data, but I'm not sure it will fails if it's not the case.

(In reply to comment #3)
> not contiguous in virtual memory. I try to do the following to check that:
> 
> for (i = 0; i < (n_planes - 1); i++) {
>   gsize diff_data = ((guint8 *) data[i + 1]) - ((guint8 *) data[i]);
>   gsize size = offset[i + 1] - offset[i];

This code may go pretty wrong if offset[i] > offset[i + 1]. Note that contiguous
isn't enough here. It has to be contiguous and of expected size (no extra line for padding). If there was extra line, you would have into redoing S_FMT, and using selection (or crop if first isn't supported).

(note, if you have reach allocator import, it's probably too late for trying to adapt the alignment).

Created attachment 294853 [details] [review]
v4l2allocator: fix import_userptr() in single-planar API when n_planes > 1

(In reply to comment #5)
> (note, if you have reach allocator import, it's probably too late for trying to
> adapt the alignment).
Yes, I think so, so in this case, I think we should just return in error, at least for now.

So, here is a new patch proposal to handle the issue and check that planes are contiguous and have expected size. To do this I used the following idea:
if planes are contiguous and have expected size so plane 'i' start address + plane 'i' expected size shall be equal to plane 'i + 1' start address.

I think it's enough to do the job, but please tell me if I'm wrong or if I'm miss
Please tell me if i missed something.

Review of attachment 294853 [details] [review]:

::: sys/v4l2/gstv4l2allocator.c
@@ +1167,3 @@
+        tmp = ((guint8 *) data[i]) + pix_fmt->bytesperline * pix_fmt->height;
+        if (tmp != data[i + 1])
+          goto non_contiguous_mem;

This logic seems right.

@@ -1164,0 +1180,5 @@
+
+      if ((i + 1) == n_planes) {
+        size = img_size - offset[i];
... 2 more ...

My fault, but this calculation of the plane size cannot be trusted. We'll need to pass the stride[] array instead of offset. Offset are relative to the buffer, there is no guaranty substracting offset give you plane size since data pointer may not be at the start of the memory.

The correct way is to calculate this way:
    size = stride[i] * height

A special case is needed for tiled formats. I think we should fix in a seperate patch, prior to this one.

Review of attachment 294853 [details] [review]:

::: sys/v4l2/gstv4l2allocator.c
@@ +1167,3 @@
+        tmp = ((guint8 *) data[i]) + pix_fmt->bytesperline * pix_fmt->height;
+        if (tmp != data[i + 1])
+          goto non_contiguous_mem;

Actually not. The stride (bytesperline) is not the same for all planes. It depends on the subsampling. So while it's correct for NV12 and NV21, it does not work for I420.

I wrote a method to interpolate the stride array from single bytesperline. We should make that available to the allocator and use that here, and then use the stride array.

(In reply to comment #7)
> Review of attachment 294853 [details] [review]:
> 
> ::: sys/v4l2/gstv4l2allocator.c
> @@ +1167,3 @@
> +        tmp = ((guint8 *) data[i]) + pix_fmt->bytesperline * pix_fmt->height;
> +        if (tmp != data[i + 1])
> +          goto non_contiguous_mem;
> 
> This logic seems right.
> 
> @@ -1164,0 +1180,5 @@
> +
> +      if ((i + 1) == n_planes) {
> +        size = img_size - offset[i];
> ... 2 more ...
> 
> My fault, but this calculation of the plane size cannot be trusted. We'll need
> to pass the stride[] array instead of offset. Offset are relative to the
> buffer, there is no guaranty substracting offset give you plane size since data
> pointer may not be at the start of the memory.
Indeed.

> 
> The correct way is to calculate this way:
>     size = stride[i] * height
> 
In fact it's 
    size = stride[i] * pheight[i]
no ? 

> A special case is needed for tiled formats. I think we should fix in a seperate
> patch, prior to this one.
I will propose a patch to fix this soon.

(In reply to comment #9)
> > 
> > The correct way is to calculate this way:
> >     size = stride[i] * height
> > 
> In fact it's 
>     size = stride[i] * pheight[i]
> no ? 

Yes you are right. The pool is more aware of video format, maybe we should replace the offset array into size[] array (and do the math in the pool) ?

> 
> > A special case is needed for tiled formats. I think we should fix in a seperate
> > patch, prior to this one.
> I will propose a patch to fix this soon.

Thanks.

Created attachment 295286 [details] [review]
v4l2allocator: let bufferpool calculate image size when importing userptr

Proposal patch to fix the size calculation as pointed out in comment 7.

As proposed, I move the calculation of size from allocation to bufferpool in order to have video information.

I handle the tiled format but I'm not sure I've done it correctly since I'm not familiar with them. And I have no hardware which support tiled format, so if someone can test it.

Created attachment 295289 [details] [review]
v4l2allocator: fix import_userptr() in single-planar API when n_planes > 1

Draft to check contiguity and size

As for the size calculation patch, we need video info to do our check, so I move the code from allocator to bufferpool. And I exported a function from gstv4l2object to extrapolate the v4l2 bytesperline to a specific stride.

Review of attachment 295286 [details] [review]:

This look good, nice work.

::: sys/v4l2/gstv4l2bufferpool.c
@@ +220,3 @@
   if (finfo && (finfo->format != GST_VIDEO_FORMAT_UNKNOWN &&
           finfo->format != GST_VIDEO_FORMAT_ENCODED)) {
+    gsize size[GST_VIDEO_MAX_COMPONENTS] = { 0, };

It's the same right now, and we assume all over the place that they are in same order, but let's use GST_VIDEO_MAX_PLANES here.

Created attachment 295292 [details] [review]
v4l2allocator: let bufferpool calculate image size when importing userptr

Change the MAX_COMPONENTS to MAX_PLANES.

Review of attachment 295289 [details] [review]:

::: sys/v4l2/gstv4l2allocator.c
@@ +1152,3 @@
   g_return_val_if_fail (allocator->memory == V4L2_MEMORY_USERPTR, FALSE);
 
   /* TODO Support passing N plane from 1 memory to MPLANE v4l2 format */

Actually, this TODO make no sense ;-P We already do that by mapping the buffer using gst_video_frame_map(). I'll drop it later, don't worry.

::: sys/v4l2/gstv4l2bufferpool.c
@@ +258,3 @@
+            pix_fmt->bytesperline);
+        guint eheight = GST_VIDEO_FORMAT_INFO_SCALE_HEIGHT (finfo, i,
+            pix_fmt->height);

I'm wondering if we could use a simplier check.

if (!V4L2_TYPE_IS_MULTIPLANAR (pool->obj->type)) {
  if (gst_buffer_n_memory (buffer) != 1)
    goto non_contiguous_mem;
  
  /* TODO check all strides */
  if (stride[0] != pix_fmt->bytesperline)
    goto not_aligned;

Would do you think ?

(In reply to comment #15)
> Review of attachment 295289 [details] [review]:
> 
> ::: sys/v4l2/gstv4l2bufferpool.c
> @@ +258,3 @@
> +            pix_fmt->bytesperline);
> +        guint eheight = GST_VIDEO_FORMAT_INFO_SCALE_HEIGHT (finfo, i,
> +            pix_fmt->height);
> 
> I'm wondering if we could use a simplier check.
> 
> if (!V4L2_TYPE_IS_MULTIPLANAR (pool->obj->type)) {
>   if (gst_buffer_n_memory (buffer) != 1)
>     goto non_contiguous_mem;
> 
>   /* TODO check all strides */
>   if (stride[0] != pix_fmt->bytesperline)
>     goto not_aligned;
> 
> Would do you think ?

If there are more than 1 memory, I think we can safely assume that memory won't be contiguous, otherwise we are lucky.

However checking the stride is not enough, since we could have padding at the bottom and in this case, plane are not contiguous. As a result, we will have padding pixel displayed as chroma sample in YUV.
Hence we should check that planes have the expected size. But currently, if I'm right, we have to deal with data pointer or frame offset calculate them in order to know if there is padding or not.

Maybe , the 'simpler/generic' way should be to let element know about padding by adding VideoAlignment information to VideoInfo, but there might be a good reason to not do so ?

Review of attachment 295289 [details] [review]:

Let's do that for now.

Review of attachment 295292 [details] [review]:

commit ac3cb8817e042bbed3d86d8f1cd1c45e49015a2c
Author: Aurélien Zanelli <aurelien.zanelli@parrot.com>
Date:   Fri Jan 23 10:15:46 2015 +0100

    v4l2allocator: let bufferpool calculate image size when importing userptr
    
    Offset are relative to the buffer and there is no guarantee substracting
    them will give us the plane size. So we let bufferpool make the math as
    it is more aware of video info than allocator and pass a size array to
    allocator import function.
    
    Pointed out by Nicolas Dufresne <nicolas.dufresne@collabora.com>
    
    https://bugzilla.gnome.org/show_bug.cgi?id=738013

Review of attachment 295289 [details] [review]:

commit 600027a1ee83ef595cc5598633812c15e6b30db2
Author: Aurélien Zanelli <aurelien.zanelli@parrot.com>
Date:   Mon Oct 6 17:30:06 2014 +0200

    v4l2bufferpool: fix import_userptr() in single-planar API when n_planes > 1
    
    In the V4L2 single-planar API, when format is semi-planar/planar,
    drivers expect the planes to be contiguous in memory.
    So this commit change the way we handle semi-planar/planar format
    (n_planes > 1) when we use the single-planar API (group->n_mem == 1).
    
    To check that planes are contiguous and have expected size, ie: no
    padding. We test the fact that plane 'i' start address + plane 'i'
    expected size equals to plane 'i + 1' start address. If not, we return
    in error.
    
    Math are done in bufferpool rather than in allocator because the
    former is aware of video info.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=738013