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

Tue Nov 26 04:19:42 PST 2013

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |slomo at coaxion.net

--- Comment #23 from Sebastian Dröge (slomo) <slomo at coaxion.net> 2013-11-26 12:19:39 UTC ---
(In reply to comment #18)
> (In reply to comment #13)
> > Review of attachment 260415 [details] [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 :)

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