GNOME Bugzilla – Bug 712754
v4l2: add support for multi-planar V4L2 API
Last modified: 2013-12-04 09:41:11 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.
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.
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.
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
Created attachment 260380 [details] [review] v4l2object: Use space instead of tabs
Created attachment 260381 [details] [review] v4l2object: Split templates caps into RAW, CODEC and TRANSPORT type
Review of attachment 260381 [details] [review]: This one shall be better place for when the decoder is ready ?
Comment on attachment 260381 [details] [review] v4l2object: Split templates caps into RAW, CODEC and TRANSPORT type You are right.
Created attachment 260413 [details] [review] v4l2object: Fix header indentation so it's readable again
Created attachment 260414 [details] [review] v4l2object: Use space instead of tabs
Created attachment 260415 [details] [review] v4l2: add support for multi-planar V4L2 API
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.
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.
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?
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
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 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 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
(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 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
Created attachment 262842 [details] [review] v4l2: add support for multi-planar V4L2 API
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"
Created attachment 262844 [details] [review] v4l2bufferpool: add support for multi-planar V4l2 API in DMABUF mode
(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 :)
After addressing these comments this should be ready to be merged.
(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 ?
(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.
(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
Created attachment 263010 [details] [review] v4l2: add support for multi-planar V4L2 API
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.
Created attachment 263024 [details] [review] v4l2: add support for multi-planar V4L2 API
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
Created attachment 263027 [details] [review] v4l2: add support for multi-planar V4L2 API
Review of attachment 263027 [details] [review]: This version looks good to me.
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