Created attachment 318417 [details] [review]
rawparse: add a vfunc to enable subclass to add buffer metadata

First patch to add a vfunc to add buffer metadata in rawparse base class.

Created attachment 318419 [details] [review]
videoparse: cache video info in instance

This patch stored video info in video parser instance to avoid initializing one and filling one each time we use GstVideoInfo to make our life easier, ie calculate the framesize and the caps.

Good idea but top/bottom/left/right padding is different from stride and they should be provided separately. You might also have different stride per plane, and padding between planes (i.e. plane offsets).

Adding videometa is probably required in all these cases, you however have to check if downstream actually supports it and otherwise fail (or convert).

Created attachment 318420 [details] [review]
videoparse: add properties to set image padding

This is the patch which add the 4 properties to set padding. It also add in each output buffer video info metadata.

Note that I currently assume that downstream supports GstVideoMeta. We may check if it's the case or not. And if downstream doesn't support them, we should have to copy frame to remove padding.

I would prefer if we add two properties, strides and offset, a coma seperated list form to make it easy to use from gst-launch.

The problem with top/left/right/bottom is that you cannot specify arbitrary padding between planes, which I have seen from time to time. (and of course we need not to assume what downstream support, and copy if the GstVideoMeta is not supported).

(In reply to Nicolas Dufresne (stormer) from comment #5)
> I would prefer if we add two properties, strides and offset, a coma
> seperated list form to make it easy to use from gst-launch.

Not a bad idea, I will go for it with both expressed in bytes as it solve arbitrary padding between planes. I think we should also support padding between images, so we may have to provide a 'framesize' property.

If I'm right, with this set of property, we could handle all cases:
- padding before image
- padding after image
- padding between planes
- padding before and after image line per plane (with stride)

Oh good point about framesize, I agree. And offsets (with an s), correcting my typo. Btw, if downstream does not support GstVideoMeta, you can simply use gst_video_frame_copy(), it works for "normalizing" video frames.

Created attachment 318874 [details] [review]
[1/6] videoparse: cache video info in instance

Created attachment 318875 [details] [review]
[2/6] rawparse: rename 'set_buffer_flags' vfunc to 'pre_push_buffer'

Minor naming change to allow subclass to make other change in the buffer, not just the flags.

Created attachment 318876 [details] [review]
[3/6] videoparse: add properties to set framesize, strides and planes offsets

This patch only add the properties 'framesize', 'strides' and 'offsets' and copy frame when we have some as support of videometa is not present yet.

Created attachment 318878 [details] [review]
[4/6] rawparse: use size of buffer we got from adapter

When rawparse push a buffer, it increments its current offset using buffer size. But buffer size could have changed in pre_push_frame call causing wrong position update and so wrong data to be handled.

Created attachment 318879 [details] [review]
[5/6] rawparse: add 'decide_allocation' vfunc to let subclass parse an allocation query

This patch send an allocation query to downstream element and add a simple vfunc to let 'subclass' check result of it. It will be used to check if downstream supports various meta so base class doesn't handle any buffer pool or allocation parameters.

Created attachment 318881 [details] [review]
[6/6] videoparse: use decide_allocation to check if downstream supports videometa

Here is a new patch set to add discussed properties and needed vfunc in rawparse base class. I tried to split commits to make them easier to review and comment, especially the vfunc part and their usage.

Review of attachment 318874 [details] [review]:

Good.

Review of attachment 318875 [details] [review]:

Good.

Review of attachment 318876 [details] [review]:

::: gst/rawparse/gstvideoparse.c
@@ +31,3 @@
 
+#include <stdlib.h>
+#include <stdio.h>

Why ?

@@ +302,3 @@
+{
+  return info->stride[plane] *
+      GST_VIDEO_FORMAT_INFO_SCALE_HEIGHT (info->finfo, plane, info->height);

You need a special case for nv12-64z32 . In tiled format, the stride is an encoding of the number of tiles in x and y axis. So you extract those two values using the macros, and then you calculate the number of tiles, multiplied by the size of each tiles.

@@ +347,3 @@
+
+  /* 1. check that provided offsets are greaters than the default ones and is
+   * consistent with plane size */

For I420, GStreamer defaults add an empty line, which imho is acceptable no to have. It's a minor issue though.

Review of attachment 318878 [details] [review]:

Looks good.

Review of attachment 318879 [details] [review]:

Good.

Review of attachment 318881 [details] [review]:

Good.

Created attachment 319760 [details] [review]
[3/6] videoparse: add properties to set framesize, strides and planes offsets

(In reply to Nicolas Dufresne (stormer) from comment #17)
> Review of attachment 318876 [details] [review] [review]:
> 
> ::: gst/rawparse/gstvideoparse.c
> @@ +31,3 @@
>  
> +#include <stdlib.h>
> +#include <stdio.h>
> 
> Why ?
stdlib is needed for atoi function used to parse stride/offset string but stdio was not needed so I remove it.

> 
> @@ +302,3 @@
> +{
> +  return info->stride[plane] *
> +      GST_VIDEO_FORMAT_INFO_SCALE_HEIGHT (info->finfo, plane, info->height);
> 
> You need a special case for nv12-64z32 . In tiled format, the stride is an
> encoding of the number of tiles in x and y axis. So you extract those two
> values using the macros, and then you calculate the number of tiles,
> multiplied by the size of each tiles.

I've done the modification, I've also change the stride update check for tiled format. In case format is tiled I check number of tile in x and y is set by user is greater than the defaults one.

> 
> @@ +347,3 @@
> +
> +  /* 1. check that provided offsets are greaters than the default ones and
> is
> +   * consistent with plane size */
> 
> For I420, GStreamer defaults add an empty line, which imho is acceptable no
> to have. It's a minor issue though.
I'm not sure I understand what you mean here.
Do you mean the 'empty line' added by the round up of height when it's no a multiple of 2 ?
In this case, it seems right to do that, no ?

(In reply to Aurélien Zanelli from comment #21)
> Created attachment 319760 [details] [review] [review]
> [3/6] videoparse: add properties to set framesize, strides and planes offsets
> 
> (In reply to Nicolas Dufresne (stormer) from comment #17)
> > Review of attachment 318876 [details] [review] [review] [review]:
> > 
> > ::: gst/rawparse/gstvideoparse.c
> > @@ +31,3 @@
> >  
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > 
> > Why ?
> stdlib is needed for atoi function used to parse stride/offset string but
> stdio was not needed so I remove it.

Make sense. Would it possible to port to g_ascii_strtoll() or g_ascii_strtoull(), just to make sure we stay portable (maybe I'm over worried here ?)

> > 
> > @@ +347,3 @@
> > +
> > +  /* 1. check that provided offsets are greaters than the default ones and
> > is
> > +   * consistent with plane size */
> > 
> > For I420, GStreamer defaults add an empty line, which imho is acceptable no
> > to have. It's a minor issue though.
> I'm not sure I understand what you mean here.
> Do you mean the 'empty line' added by the round up of height when it's no a
> multiple of 2 ?
> In this case, it seems right to do that, no ?

So, what we do in GStreamer as a default for I420 and YV12 is:

  info->offset[1] = info->stride[0] * GST_ROUND_UP_2 (height);

This effectively skip a line. Skipping this line has no effect on memory alignment. Odd and even lines have the same memory alignment. I don't really know the origin of this. An offset 0 to 0 and offset 1 to (even if height is odd):

  info->offset[1] = info->stride[0] * height;

Would be completely valid. In fact this is what you get from GLMemory. So basically, this is one case where the GStreamer default offset is not the minimum acceptable offset. The minimum acceptable offset is (for i < (num_planes -1)):

  offset[i + 1] = offset[i] + height * stride[i]

(In reply to Nicolas Dufresne (stormer) from comment #22)
> (In reply to Aurélien Zanelli from comment #21)
> > stdlib is needed for atoi function used to parse stride/offset string but
> > stdio was not needed so I remove it.
> 
> Make sense. Would it possible to port to g_ascii_strtoll() or
> g_ascii_strtoull(), just to make sure we stay portable (maybe I'm over
> worried here ?)

No problem to port it, in fact I found we often use these functions to parse int. After some research, it seems they have more error checking that atoi.

> > > For I420, GStreamer defaults add an empty line, which imho is acceptable no
> > > to have. It's a minor issue though.
> > I'm not sure I understand what you mean here.
> > Do you mean the 'empty line' added by the round up of height when it's no a
> > multiple of 2 ?
> > In this case, it seems right to do that, no ?
> 
> So, what we do in GStreamer as a default for I420 and YV12 is:
> 
>   info->offset[1] = info->stride[0] * GST_ROUND_UP_2 (height);
> 
> This effectively skip a line. Skipping this line has no effect on memory
> alignment. Odd and even lines have the same memory alignment. I don't really
> know the origin of this.

In my current understanding, it's due to the subsampling of 4:2:0 formats:
If we have a NxN luma plane, U and V planes have a size of (N/2)x(N/2) since
1 chroma sample is mapped to a 2x2 squared luma sample.
So, for me, it implies that luma plane width and height should be at least multiple of two.

So I think we round up U/V plane offset to be more portable as some elements may not like an odd number of lines.

We may CC Wim here to have more details on that. And by the way, it is the same for NV12/NV21 formats.

> An offset 0 to 0 and offset 1 to (even if height is odd):
> 
>   info->offset[1] = info->stride[0] * height;
> 
> Would be completely valid. In fact this is what you get from GLMemory. So
> basically, this is one case where the GStreamer default offset is not the
> minimum acceptable offset. The minimum acceptable offset is (for i <
> (num_planes -1)):
> 
>   offset[i + 1] = offset[i] + height * stride[i]

I think, there is no issue to handle this special case in videoparse so I will do it.

Created attachment 319905 [details] [review]
[3/6] videoparse: add properties to set framesize, strides and planes offsets

Strides and offsets parsing has been ported to g_ascii_strtoll.

Review of attachment 319905 [details] [review]:

::: gst/rawparse/gstvideoparse.c
@@ -212,0 +276,135 @@
+static gboolean
+gst_video_parse_deserialize_int_array (const gchar * str, gint * dest,
+    guint n_values)
... 132 more ...

should be vp->offset[i] < min_offset

Created attachment 319923 [details] [review]
[3/6] videoparse: add properties to set framesize, strides and planes offsets

I fix update_offset function which was incorrect.
It now check user offset against the max of info->offset[i] and user_offset[i-1] + plane_size[i-1]

I also propose to make the 4:2:0 formats odd height handling after reviewing/merging this patch to keep it simple.

Review of attachment 319923 [details] [review]:

Looks good to me know. I'm fine with dealing with remaining details in separate patches.

(In reply to Aurélien Zanelli from comment #26)
> I also propose to make the 4:2:0 formats odd height handling after
> reviewing/merging this patch to keep it simple.

Yes, good idea.

I'm marking it fixed, we can carry the rest in seperate bugs of course.

I actually found a little bug here, this should work, but display artefact:

GST_DEBUG=2 gst-launch-1.0 videotestsrc ! videoparse format=i420 width=320 height=240 strides=320,160,160,0 ! xvimagesink

For some reason it recalculates the frame size wrong.

videoparse gstvideoparse.c:495:gst_video_parse_update_info:<videoparse0> video info: 320x240, format I420, size 192000, stride {320,160,160,0}, offset {0,76800,96000,0}

Ok, the problem is about adding the offset, you endup adding twice the same part of the memory. You also need to understand that with videometa, conversion from let's say I420 to YV12 can be done by flipping the two last offsets. So the minimum frame size is a bit more complex to calculate.

Created attachment 319957 [details] [review]
videoparse: Fix framesize calculation

When the framesize is not specified, we try and calculate a size from
the strides and offset information. This was done with the sum of
offsets + the size of the last frame. That is just wrong method. We also
need to account for video meta that may be flipping two planes. An
example is if you convert I420 to YV12 by flipping the two last offsets.

That fixes it for me, need more testing (I'm a bit in a hurry now). Another issue is that we have no equivalent for framesize_set, so if we set a stride or an offset, we can no longer also add a framesize (it get overwritten). This can be solved by overriding the property, and then proxying to the base class with the appropriate method.

Created attachment 319958 [details] [review]
videoparse: initialize update_size to FALSE when updating info

I also let update_size uninitialized which can cause bad behavior.
Here is a patch to initialize it.

(In reply to Nicolas Dufresne (stormer) from comment #33)
> Created attachment 319957 [details] [review] [review]
> videoparse: Fix framesize calculation
> 
> When the framesize is not specified, we try and calculate a size from
> the strides and offset information. This was done with the sum of
> offsets + the size of the last frame. That is just wrong method. We also
> need to account for video meta that may be flipping two planes. An
> example is if you convert I420 to YV12 by flipping the two last offsets.

I don't set the patch as accepted as I don't know if I'm allowed to do this but patch looks good. 
There is just 'planesize' which may be a confusion name though.

I tested it with I420 and Y42B with default and custom stride and it's ok.
However flipping offsets doesn't work at all since user provided offsets are checked against provided format defaults offsets which are sorted from small to large. Hence offsets will likely be ignored in this case.

(In reply to Nicolas Dufresne (stormer) from comment #34)
> That fixes it for me, need more testing (I'm a bit in a hurry now). Another
> issue is that we have no equivalent for framesize_set, so if we set a stride
> or an offset, we can no longer also add a framesize (it get overwritten).
> This can be solved by overriding the property, and then proxying to the base
> class with the appropriate method.

User provided framesize is set after size update from strides/offset to video info struct (only if bigger). Then when we set the base class framesize, we read back the value from info so it takes the user value if one has been set and is valid.
Confusing thing is that I updated directly vp->info.size instead of info->size (info point to &vp->info). I could make a patch to replace vp->info by info to make it clearer.

Ah ok, make sense, I didn't tested that framesize presumed issue, I fully trust you on that. Let's push those two then, we can improve the validation later (it's not a regression).

Attachment 319957 [details] pushed as acb7205 - videoparse: Fix framesize calculation
Attachment 319958 [details] pushed as 5fd3511 - videoparse: initialize update_size to FALSE when updating info