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 712754 - v4l2: add support for multi-planar V4L2 API
v4l2: add support for multi-planar V4L2 API
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
http://cgit.collabora.com/git/user/ju...
Depends on:
Blocks:
 
 
Reported: 2013-11-20 17:13 UTC by Julien Isorce
Modified: 2013-12-04 09:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
add support for multi-planar V4L2 API (42.27 KB, patch)
2013-11-20 17:13 UTC, Julien Isorce
none Details | Review
v4l2: refactor MPLANE and not MPLANE mode (15.87 KB, patch)
2013-11-20 17:18 UTC, Julien Isorce
none Details | Review
v4l2: refactor by emulating one v4l2_plane in non-MPLANE mode (15.28 KB, patch)
2013-11-20 17:21 UTC, Julien Isorce
none Details | Review
v4l2object: Fix header indentation so it's readable again (9.73 KB, patch)
2013-11-20 19:06 UTC, Julien Isorce
none Details | Review
v4l2object: Use space instead of tabs (860 bytes, patch)
2013-11-20 19:07 UTC, Julien Isorce
none Details | Review
v4l2object: Split templates caps into RAW, CODEC and TRANSPORT type (7.41 KB, patch)
2013-11-20 19:08 UTC, Julien Isorce
none Details | Review
v4l2object: Fix header indentation so it's readable again (9.67 KB, patch)
2013-11-21 11:04 UTC, Julien Isorce
committed Details | Review
v4l2object: Use space instead of tabs (848 bytes, patch)
2013-11-21 11:05 UTC, Julien Isorce
committed Details | Review
v4l2: add support for multi-planar V4L2 API (42.18 KB, patch)
2013-11-21 11:06 UTC, Julien Isorce
needs-work Details | Review
v4l2: refactor MPLANE and not MPLANE mode (15.87 KB, patch)
2013-11-21 11:08 UTC, Julien Isorce
reviewed Details | Review
v4l2: refactor by emulating one v4l2_plane in non-MPLANE mode (15.28 KB, patch)
2013-11-21 11:10 UTC, Julien Isorce
reviewed Details | Review
v4l2: add support for multi-planar V4L2 API (42.72 KB, patch)
2013-11-26 12:02 UTC, Julien Isorce
reviewed Details | Review
v4l2: refactor by emulating one v4l2_plane in non-MPLANE mode (14.25 KB, patch)
2013-11-26 12:04 UTC, Julien Isorce
committed Details | Review
v4l2bufferpool: add support for multi-planar V4l2 API in DMABUF mode (2.29 KB, patch)
2013-11-26 12:06 UTC, Julien Isorce
committed Details | Review
v4l2: add support for multi-planar V4L2 API (42.80 KB, patch)
2013-11-28 10:03 UTC, Julien Isorce
reviewed Details | Review
v4l2: add support for multi-planar V4L2 API (41.77 KB, patch)
2013-11-28 14:15 UTC, Julien Isorce
reviewed Details | Review
v4l2: add support for multi-planar V4L2 API (42.29 KB, patch)
2013-11-28 14:42 UTC, Julien Isorce
committed Details | Review

Description Julien Isorce 2013-11-20 17:13:29 UTC
Created attachment 260343 [details] [review]
add support for multi-planar V4L2 API

From v4l2 doc (http://linuxtv.org/downloads/v4l-dvb-apis/planar-apis.html):
"Some devices require data for each input or output video frame to be placed in discontiguous memory buffers"

So the patch that follows add this support keeping the same code path than previously when the device does not support multi-planar.

A noticeable difference is that v4l2_buffer.length now means the number of planes. Also there are two new struct: 'struct v4l2_pix_format_mplane' and 'struct v4l2_plane'.

From what I found, this API is present in the 2.6 linux kernel since 2.6.39 and present in all v3.
So unless someone needs to keep compatibility with older kernel than 2.6.39, I think it's not usefull to add some #ifdef/#endif around the gst-v4l2 source code that uses this V4l2 multi-planar API.

Also I'm wondering if using gstreamer-1.3 implies to have at least kernel v3.

The following patch has been tested on Cotton-Candy / Exynos 4 device. And on this device, only v4l2sink has been tested and with the following format: NV21, NV12 and NV12-mplane.

About the compatibility of the previous code path, I tested the patch with v4l2src through a webcam that supports RGB, YUY2, I420 and JPEG.

So please test with your devices and do not hesitate to report any regression,
or any comment about those changes.
Comment 1 Julien Isorce 2013-11-20 17:18:54 UTC
Created attachment 260345 [details] [review]
v4l2: refactor MPLANE and not MPLANE mode

In order to easily maintain the code, because now there are 2 code path (v4l2 single planar and multi planar api), I suggest the attached approach to factorize the code. The idea is to add the planes fields in the GstV4l2Meta in order to access the attributs the same way.

Let me know what you think about it.

The patch applies just after 260343.
Comment 2 Julien Isorce 2013-11-20 17:21:48 UTC
Created attachment 260346 [details] [review]
v4l2: refactor by emulating one v4l2_plane in non-MPLANE mode

This patch applies just after the first one (260343).
This is a second approach to factorize the 2 paths. The idea here is too emulate a v4l2_plane when not using multi-planar. So that all fiels can be retrieved the same way.

I prefer this approach.
Comment 3 Julien Isorce 2013-11-20 19:06:52 UTC
Created attachment 260379 [details] [review]
v4l2object: Fix header indentation so it's readable again

I forgot to add those 3 initial patchs that you have to apply before 260343
Comment 4 Julien Isorce 2013-11-20 19:07:32 UTC
Created attachment 260380 [details] [review]
v4l2object: Use space instead of tabs
Comment 5 Julien Isorce 2013-11-20 19:08:16 UTC
Created attachment 260381 [details] [review]
v4l2object: Split templates caps into RAW, CODEC and TRANSPORT type
Comment 6 Nicolas Dufresne (ndufresne) 2013-11-20 20:12:06 UTC
Review of attachment 260381 [details] [review]:

This one shall be better place for when the decoder is ready ?
Comment 7 Julien Isorce 2013-11-21 10:50:13 UTC
Comment on attachment 260381 [details] [review]
v4l2object: Split templates caps into RAW, CODEC and TRANSPORT type

You are right.
Comment 8 Julien Isorce 2013-11-21 11:04:48 UTC
Created attachment 260413 [details] [review]
v4l2object: Fix header indentation so it's readable again
Comment 9 Julien Isorce 2013-11-21 11:05:24 UTC
Created attachment 260414 [details] [review]
v4l2object: Use space instead of tabs
Comment 10 Julien Isorce 2013-11-21 11:06:18 UTC
Created attachment 260415 [details] [review]
v4l2: add support for multi-planar V4L2 API
Comment 11 Julien Isorce 2013-11-21 11:08:18 UTC
Created attachment 260416 [details] [review]
v4l2: refactor MPLANE and not MPLANE mode

In order to easily maintain the code, because now there are 2 code path (v4l2
single planar and multi planar api), find attached a first approach to
factorize the code. The idea is to add the planes fields in the GstV4l2Meta in
order to access the attributes the same way.
Comment 12 Julien Isorce 2013-11-21 11:10:00 UTC
Created attachment 260417 [details] [review]
v4l2: refactor by emulating one v4l2_plane in non-MPLANE mode

This is a second approach to factorize the 2 paths. The idea here is too
emulate a v4l2_plane when not using multi-planar. So that all fields can be
retrieved the same way.

I prefer this approach.
Comment 13 Sebastian Dröge (slomo) 2013-11-25 14:11:41 UTC
Review of attachment 260415 [details] [review]:

::: 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? :)

@@ +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

@@ +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)?

@@ +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

@@ +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 &&

@@ +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

::: 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

@@ +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?
Comment 14 Sebastian Dröge (slomo) 2013-11-25 14:17:41 UTC
Review of attachment 260416 [details] [review]:

Basically ok, just related to a comment in the other patch

::: sys/v4l2/gstv4l2bufferpool.c
@@ +1191,3 @@
+            for (i = 0; i < meta->n_planes; i++)
+              total_length += meta->length[i];
+            gst_buffer_resize (buffer, 0, total_length);

Same comment as in the other patch
Comment 15 Sebastian Dröge (slomo) 2013-11-25 14:19:29 UTC
Review of attachment 260417 [details] [review]:

Generally ok, but as in the other patches

::: sys/v4l2/gstv4l2bufferpool.c
@@ +1144,3 @@
+            for (i = 0; i < meta->n_planes; i++)
+              total_length += meta->vplanes[i].length;
+            gst_buffer_resize (buffer, 0, total_length);

And here too ;)
Comment 16 Nicolas Dufresne (ndufresne) 2013-11-25 18:45:41 UTC
Comment on attachment 260413 [details] [review]
v4l2object: Fix header indentation so it's readable again

commit f18290226a7a45ef09bb05f688844994b56d6c3c
Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>
Date:   Wed May 29 15:44:31 2013 -0400

    v4l2object: Fix header indentation so it's readable again
    
    It's unfortunate to have to do this, but with the mix of tabs and space, plus all the random
    indentation this header has become very hard to read.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=712754
Comment 17 Nicolas Dufresne (ndufresne) 2013-11-25 18:46:03 UTC
Comment on attachment 260414 [details] [review]
v4l2object: Use space instead of tabs

commit 90ac945dccebc36d9f8ae13e79ad75f151576dda
Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>
Date:   Wed May 29 15:57:09 2013 -0400

    v4l2object: Use space instead of tabs
    
    https://bugzilla.gnome.org/show_bug.cgi?id=712754
Comment 18 Julien Isorce 2013-11-26 11:23:43 UTC
(In reply to comment #13)
> Review of attachment 260415 [details] [review]:
> 
> ::: 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!
Comment 19 Julien Isorce 2013-11-26 12:01:11 UTC
Comment on attachment 260416 [details] [review]
v4l2: refactor MPLANE and not MPLANE mode

The other solution is better because it does not require to have a lot of fields in the meta and the diff lines removed - lines added is negative
Comment 20 Julien Isorce 2013-11-26 12:02:26 UTC
Created attachment 262842 [details] [review]
v4l2: add support for multi-planar V4L2 API
Comment 21 Julien Isorce 2013-11-26 12:04:27 UTC
Created attachment 262843 [details] [review]
v4l2: refactor by emulating one v4l2_plane in non-MPLANE mode

The idea here is too emulate a v4l2_plane when not using multi-planar. So that all fields can be retrieved the same way. Here "emulate" just means "manually fill in the plane"
Comment 22 Julien Isorce 2013-11-26 12:06:24 UTC
Created attachment 262844 [details] [review]
v4l2bufferpool: add support for multi-planar V4l2 API in DMABUF mode
Comment 23 Sebastian Dröge (slomo) 2013-11-26 12:19:39 UTC
(In reply to comment #18)
> (In reply to comment #13)
> > Review of attachment 260415 [details] [review] [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.
>  */

Really? So for ARGB that would be 4, for NV12 it would be 3 as it would be for I420 and RGB and UYVY?

> > @@ +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 +=

Ok, but when would meta->vplanes[i].length be not equal to GST_VIDEO_FORMAT_INFO_SCALE_HEIGHT (finfo, i, height)? Sounds to me like there's a potential problem if that ever happens.

> > @@ +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.

Ah sorry, makes sense yes :)

> > 
> > @@ +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));
> ?

No, that would have no effect at all. I mean that you need to iterate over the memories inside the buffer and make sure that they're all still the same as before and have the same sizes as before. Just using the buffer size should only be valid if you have a single memory in the buffer.

> > ::: 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

Ok

> > @@ +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 ?

No idea actually. Leave it as is, because the two structures have different names but the same fields it's a bit annoying :)
Comment 24 Sebastian Dröge (slomo) 2013-11-26 12:21:15 UTC
After addressing these comments this should be ready to be merged.
Comment 25 Julien Isorce 2013-11-26 12:39:55 UTC
(In reply to comment #23)
> > 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.
> >  */
> 
> Really? So for ARGB that would be 4, for NV12 it would be 3 as it would be for
> I420 and RGB and UYVY?

GST_VIDEO_INFO_N_PLANES (info) returns the values you said.

With V4L2_NV12 format (+type BUF_MPLANE) you are in MPLANE mode but there is still only one v4l2 plane that contains all the color components plane (as if it was not in MPLANE mode)

With V4L2_NV12M format you are in MPLANE mode but there is a v4l2 plane for each color component.


As we have no 2 format GST_NV12 vs GST_NV12M in gstreamer, I do the choice through the v4l2object->prefered_non_contiguous variable. See the comment around it in the patch

> > 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 +=
> 
> Ok, but when would meta->vplanes[i].length be not equal to
> GST_VIDEO_FORMAT_INFO_SCALE_HEIGHT (finfo, i, height)? Sounds to me like
> there's a potential problem if that ever happens.
> 

Well to be more precise, in "MPLANE mode + one v4l2 plane per color component" then  offset_i is always equal to meta->vplanes[i].length + offset_i_1

Is the way I wrote it in the updated patch ok ?

> > > 
> > > @@ +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));
> > ?
> 
> No, that would have no effect at all. I mean that you need to iterate over the
> memories inside the buffer and make sure that they're all still the same as
> before and have the same sizes as before. Just using the buffer size should
> only be valid if you have a single memory in the buffer.

I added a FIXME/comment, is it ok ?
Comment 26 Sebastian Dröge (slomo) 2013-11-26 12:45:56 UTC
(In reply to comment #25)
> (In reply to comment #23)
> > > 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.
> > >  */
> > 
> > Really? So for ARGB that would be 4, for NV12 it would be 3 as it would be for
> > I420 and RGB and UYVY?
> 
> GST_VIDEO_INFO_N_PLANES (info) returns the values you said.

Actually it doesn't, it would return 1 (ARGB, RGB, UYVY), 2 (NV12) and 3 (I420). GST_VIDEO_INFO_N_COMPONENTS() returns the values I mentioned above.

Sorry if I'm a bit pedantic about that comment, I'm just afraid that there's some further confusion that could cause incorrect usage of the API in the code.

There are 3 different concepts here: the number of components, the number of planes and the number of memory areas. In this code you only care about the last two.

> With V4L2_NV12 format (+type BUF_MPLANE) you are in MPLANE mode but there is
> still only one v4l2 plane that contains all the color components plane (as if
> it was not in MPLANE mode)
> 
> With V4L2_NV12M format you are in MPLANE mode but there is a v4l2 plane for
> each color component.
>
> As we have no 2 format GST_NV12 vs GST_NV12M in gstreamer, I do the choice
> through the v4l2object->prefered_non_contiguous variable. See the comment
> around it in the patch

Ack, that makes sense. We don't care if it's in one memory completely, or one per plane, or even multiple per plane :)

> > > 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 +=
> > 
> > Ok, but when would meta->vplanes[i].length be not equal to
> > GST_VIDEO_FORMAT_INFO_SCALE_HEIGHT (finfo, i, height)? Sounds to me like
> > there's a potential problem if that ever happens.
> > 
> 
> Well to be more precise, in "MPLANE mode + one v4l2 plane per color component"
> then  offset_i is always equal to meta->vplanes[i].length + offset_i_1
> 
> Is the way I wrote it in the updated patch ok ?

Yes, I'm just wondering if we should print a warning if meta->vplanes[i].length != GST_VIDEO_FORMAT_INFO_SCALE_HEIGHT (finfo, i, height).

> > > > 
> > > > @@ +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));
> > > ?
> > 
> > No, that would have no effect at all. I mean that you need to iterate over the
> > memories inside the buffer and make sure that they're all still the same as
> > before and have the same sizes as before. Just using the buffer size should
> > only be valid if you have a single memory in the buffer.
> 
> I added a FIXME/comment, is it ok ?

Check with git blame who added that specific code and why, maybe they can tell you how to reproduce the reason for this code. It's probably uvch264 related :) If it seems safe to ignore in your case, a FIXME should be fine.
Comment 27 Julien Isorce 2013-11-26 14:49:09 UTC
(In reply to comment #26)

> Actually it doesn't, it would return 1 (ARGB, RGB, UYVY), 2 (NV12) and 3
> (I420). GST_VIDEO_INFO_N_COMPONENTS() returns the values I mentioned above.
> 
> Sorry if I'm a bit pedantic about that comment, I'm just afraid that there's
> some further confusion that could cause incorrect usage of the API in the code.
> 
> There are 3 different concepts here: the number of components, the number of
> planes and the number of memory areas. In this code you only care about the
> last two.

Sorry I confused myself :)
Is the following comment ok ?

/* n_gst_planes is the number of planes
 * (RGB: 1, YUY2: 1, NV12: 2, I420: 3)
 * It's greater or equal than the number of v4l2 planes. */

> Yes, I'm just wondering if we should print a warning if meta->vplanes[i].length
> != GST_VIDEO_FORMAT_INFO_SCALE_HEIGHT (finfo, i, height).
> 

You mean GST_VIDEO_FORMAT_INFO_SCALE_HEIGHT (finfo, i, height) * stride[i] ?

Well not sure about the warning because it's a normal behaviuor. There is the .bytesused for that actually, that I set (the user has to set it for OUTPUT devices).
What I understand is that length >= bytesused because the device may use aligned memory or any other reason.


> > 
> > I added a FIXME/comment, is it ok ?
> 
> Check with git blame who added that specific code and why, maybe they can tell
> you how to reproduce the reason for this code. It's probably uvch264 related :)
> If it seems safe to ignore in your case, a FIXME should be fine.

ok. I let the FIXME.

For log:

http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/sys/v4l2/gstv4l2bufferpool.c?id=2e80c0d2c08d1fbda3286bfe397153ec65661322

http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/sys/v4l2/gstv4l2bufferpool.c?id=b1378f159accd4fd574c2a24883eef27196fac09
Comment 28 Julien Isorce 2013-11-28 10:03:22 UTC
Created attachment 263010 [details] [review]
v4l2: add support for multi-planar V4L2 API
Comment 29 Nicolas Dufresne (ndufresne) 2013-11-28 13:52:20 UTC
Review of attachment 263010 [details] [review]:

::: sys/v4l2/gstv4l2object.c
@@ +325,3 @@
+          "Device supports multi-planar video capture", "capture-mplane"},
+      {V4L2_CAP_VIDEO_OUTPUT_MPLANE,
+          "Device supports multi-planar video playback", "output-mplane"},

I would prefer not adding new flags. The detail of how we pack the buffers internally is no useful in the public application API.
Comment 30 Julien Isorce 2013-11-28 14:15:04 UTC
Created attachment 263024 [details] [review]
v4l2: add support for multi-planar V4L2 API
Comment 31 Sebastian Dröge (slomo) 2013-11-28 14:26:18 UTC
Comment on attachment 263024 [details] [review]
v4l2: add support for multi-planar V4L2 API

From IRC:

<slomo> capOM: from the comments in the bug, now that you removed the MPLANE stuff from that public enum... make sure that the public enum maps the MPLANE and non-MPLANE variant to its value
<slomo> capOM: otherwise it all was good already except for the comments i made, and you adressed them
Comment 32 Julien Isorce 2013-11-28 14:42:22 UTC
Created attachment 263027 [details] [review]
v4l2: add support for multi-planar V4L2 API
Comment 33 Nicolas Dufresne (ndufresne) 2013-12-03 14:39:30 UTC
Review of attachment 263027 [details] [review]:

This version looks good to me.
Comment 34 Julien Isorce 2013-12-04 09:31:25 UTC
commit 3c70741e45da94e76b7015c367857312861886ba
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Thu Nov 21 12:29:28 2013 +0000

    v4l2bufferpool: add support for multi-planar V4l2 API in DMABUF mode
    
    Fixes bug https://bugzilla.gnome.org/show_bug.cgi?id=712754

commit 303cec48db41f6e1ac422ac4faf979a26b51d277
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Tue Nov 19 17:16:27 2013 +0000

    v4l2: refactor by emulating one v4l2_plane in non-MPLANE mode
    
    so that the buffer informations can be retrieved the same way
    in both MPLANE and non-MPLANE mode.
    
    Here "emulating" means "manually fill in the plane".
    
    Fixes bug https://bugzilla.gnome.org/show_bug.cgi?id=712754

commit 61ae84b50dc319a53b513e5474c1578d7127c67c
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Wed Nov 13 12:05:40 2013 +0000

    v4l2: add support for multi-planar V4L2 API
    
    This api is in linux kernel since version 2.6.39,
    and present in all version 3.
    
    The commit that adds the API in master branch of the
    linux kernel source is:
    https://github.com/torvalds/linux/commit/f8f3914cf922f5f9e1d60e9e10f6fb92742907ad
    
    v4l2 doc: "Some devices require data for each input
    or output video frame to be placed in discontiguous
    memory buffers"
    
    There are newer structures 'struct v4l2_pix_format_mplane'
    and 'struct v4l2_plane'.
    So the pixel format is not setup with the same API when using
    multi-planar.
    
    Also for gst-v4l2, one of the difference is that in GstV4l2Meta
    there are now one mem pointer for each maped plane.
    
    When not using multi-planar, this commit takes care of keeping
    the same code path than previously. So that the 2 cases are
    in two different blocks triggered from V4L2_TYPE_IS_MULTIPLANAR.
    
    Fixes bug https://bugzilla.gnome.org/show_bug.cgi?id=712754