[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 #26 from Sebastian Dröge (slomo) <slomo at coaxion.net> 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.

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