[Bug 712754] v4l2: add support for multi-planar V4L2 API

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #18 from Julien Isorce <julien.isorce at gmail.com> 2013-11-26 11:23:43 UTC ---
(In reply to comment #13)
> Review of attachment 260415 [details]:
> 
> ::: sys/v4l2/gstv4l2bufferpool.c
> @@ +312,3 @@
> +        /* n_gst_planes is the number of planes in the meaning of gstreamer
> +         * it's not equivalent to the number of planes in the meaning of v4l2.
> +         * it means how a ONE GstMemory is organized
> 
> This comment is confusing. It's the number of planes, not the number of memory
> areas? Or what are you trying to say here? :)

You right :), is something like the following more clear:
/* n_gst_planes is the number of planes in the meaning of gstreamer
 * it's acutally the number of color space components. Which may be
 * greater than the number of v4l2 planes.
 */

> 
> @@ +318,2 @@
> +        /* the basic are common between MPLANE mode and non MPLANE mode
> +         * execpt a special case inside the loop at the end
> 
> Typo: except

ok

> 
> @@ +352,3 @@
> +             * And the next plane is after length bytes of the previous one
> from
> +             * the gst buffer point of view. */
> +            offs = meta->vplanes[i].length;
> 
> When is this different to stride*component_height? Also shouldn't it be +=
> instead of an assignment (and -= the above assignment)?

you right,  I tried to factorized but the result is wrong, the correct line
would be:
offs += meta->vplanes[i].length - stride[i] *
GST_VIDEO_FORMAT_INFO_SCALE_HEIGHT (finfo, i, height); 

but I will put a else with the previous offs +=

> 
> @@ +449,3 @@
> +    gint i = 0;
> +    for (i = 0; i < nb_checked_planes; i++) {
> +      /* we don't have video metadata, and we are not dealing with raw video,
> 
> and we *are* dealing with raw video

yup thx, actually just a typo before my patch

> 
> @@ +880,3 @@
> +   * element. So update our meta */
> +  if (obj->type == V4L2_BUF_TYPE_VIDEO_CAPTURE
> +      || obj->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> 
> || really? This should probably be &&

A buffer type cannot be both MPLANE and non MPLANE, then how do you interpret 
v4l2_buffer.length ?

Well actually it's a good point :) Here it's the obj type. Which is basically
the same from what I saw from gst-v4l2.

The fact is that when we instanciate a gstv4l2object through
gst_v4l2_object_new
we pass the buffer type. Which I think is now not really adapted since the add
of MPLANE in v4l2. Because we do not know in advance if it would be MPLANE or
not.
So we should pass our own enum type GST_CAPTURE/GST_OUTPUT or something like
that.
Here I choosed to let the default as before my patchs. So you have just
V4L2_BUF_TYPE_VIDEO_CAPTURE or V4L2_BUF_TYPE_VIDEO_OUTPUT.
Then when parsing the format and caps, the obj->type is actually switched to
V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE or keep V4L2_BUF_TYPE_VIDEO_CAPTURE.

I think adding enum type GST_V4L2_CAPTURE/GST_V4L2_OUTPUT can be added later in
another patch.

> 
> @@ +1179,3 @@
> +              for (i = 0; i < meta->vbuffer.length; i++)
> +                total_length += meta->vbuffer.m.planes[i].length;
> +              gst_buffer_resize (buffer, 0, total_length);
> 
> Isn't gst_buffer_get_size() getting the length of all memories? You might need
> to restore the sizes of the individual memories here and maybe also re-add them

So do you suggest gst_buffer_resize (buffer, 0, gst_buffer_get_size (buffer));
?

For now I have not seen the case where the length changed (I mean when
gst_buffer_get_size (buffer) != total_length) so for now I just imitate the non
MPLANE case. It could be done as you suggested if someone find that case.

I'll put a FIXME and a comment around.

> 
> ::: sys/v4l2/gstv4l2object.c
> @@ +61,3 @@
> +#ifndef V4L2_PIX_FMT_NV21M
> +#define V4L2_PIX_FMT_NV21M GST_MAKE_FOURCC ('N', 'M', '2', '1')
> +#endif
> 
> What about V4L2_PIX_FMT_YUV420M, V4L2_PIX_FMT_YVY420M? We can easily support
> them too. Please put the addition of new color formats into a separate patch

Well I choosed to add at least one MPLANE format inside the patch because I
think it actually need at least one MPLANE format to work :)
But you right I can add V4L2_PIX_FMT_YUV420M and V4L2_PIX_FMT_YVY420M and in a
separate patch

> 
> @@ +2516,3 @@
> +      format.fmt.pix_mp.height = height;
> +      format.fmt.pix_mp.field = field;
> +      format.fmt.pix_mp.num_planes = n_v4l_planes;
> 
> There's a lot of copy&paste here. Can't you better unify this with the
> non-mplane codepath here already?

I have tried and I do not see really how. Any idea ?


Updated patchs will follow based on this review, thx slomo!

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