[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

Sebastian Dröge (slomo) <slomo> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #260415|none                        |needs-work
             status|                            |

--- Comment #13 from Sebastian Dröge (slomo) <slomo at coaxion.net> 2013-11-25 14:11:41 UTC ---
Review of attachment 260415:
 --> (https://bugzilla.gnome.org/review?bug=712754&attachment=260415)

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

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