Re: [PATCH 01/10] media: Introduce helpers to fill pixel format structs

[Ezequiel Garcia <ezequiel-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>]

On Wed, 2019-02-20 at 09:39 +0100, Hans Verkuil wrote:
> On 2/20/19 7:53 AM, Tomasz Figa wrote:
> > On Thu, Feb 7, 2019 at 1:36 AM Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org> wrote:
> > > On 2/6/19 5:22 PM, Ezequiel Garcia wrote:
> > > > On Wed, 2019-02-06 at 11:43 +0100, Hans Verkuil wrote:
> > > > > Hi Ezequiel,
> > > > > 
> > > > > A quick review below. This looks really useful, BTW.
> > > > > 
> > > > > On 2/5/19 9:24 PM, Ezequiel Garcia wrote:
> > 
> > [snip]
> > > > > > +/**
> > > > > > + * struct v4l2_format_info - information about a V4L2 format
> > > > > > + * @format: 4CC format identifier (V4L2_PIX_FMT_*)
> > > > > > + * @header_size: Size of header, optional and used by compressed formats
> > > > > > + * @num_planes: Number of planes (1 to 3)
> > > > > 
> > > > > This is actually 1-4 since there may be an alpha channel as well. Not that we have
> > > > > such formats since the only formats with an alpha channel are interleaved formats,
> > > > > but it is possible.
> > 
> > How about 1 to VIDEO_MAX_PLANES to be a bit more consistent?
> > Tbh. I'm not sure why we have that defined to 8, but if we have such
> > constant already, it could make sense to use it here as well.
> 
> We didn't know at the time how many planes we would need. I think we
> chose 8 because 1) that fit inside struct v4l2_format and 2) it allowed
> room for planes carrying meta data.
> 
> In hindsight we probably should have chosen 4 instead of 8.
> 
> In any case, since this is an internal API I think chosing MAX_PLANES
> here would waste unnecessary memory.
> 

4 it is then.

> > [snip]
> > > > Also, note that drm-fourcc deprecates cpp, to support tile formats.
> > > > Hopefully we don't need that here?
> > > 
> > > We do have tile formats (V4L2_PIX_FMT_NV12MT_16X16), but it is up to the
> > > driver to align width/height accordingly.
> > > 
> > 
> > I'd still make these helpers align to the constraints defined by the
> > format itself (e.g. 16x16), since it doesn't cost us anything, and
> > have the driver do any further alignment only if they need so.
> 
> Yes, sorry, I should have said that: for tiled pixel formats this
> struct should give the alignments.
> 
> But those alignments differ from hsub/vsub: those values restrict the
> resolution, but the 'tiled' alignments are on top of that.
> 

Hm, OK. So the idea is support tiling formats on this helpers?

Might need to change the way we describe formats, as DRM does,
to incorporate some notion of pixel blocks.

> > > > > > + * @hsub: Horizontal chroma subsampling factor
> > > > > > + * @vsub: Vertical chroma subsampling factor
> > > > > 
> > > > > A bit too cryptic IMHO. I would prefer hdiv or hsubsampling. 'hsub' suggests
> > > > > subtraction :-)
> > > > > 
> > > > 
> > > > Ditto, this name follows drm-fourcc. I'm fine either way.
> > > > 
> > 
> > I personally like hsub and vsub too, but maybe I just spent too much
> > time with DRM code. *subsampling would make the initializers super
> > wide, so if we decide that we don't like *sub, I'd go with *div.
> > 

Note that there's a v2 patch, where I went with *div :-)

> > > > > > + * @multiplanar: Is it a multiplanar variant format? (e.g. NV12M)
> > > > > 
> > > > > This should, I think, be renamed to num_non_contig_planes to indicate how many
> > > > > non-contiguous planes there are in the format.
> > > > > 
> > > > > So this value is 1 for NV12 and 2 for NV12M. For V4L2_PIX_FMT_YUV444M it is 3.
> > > > > 
> > > > > You can stick this value directly into pixfmt_mp->num_planes.
> > > > > 
> > > > 
> > > > Fine by me, but I have to admit I don't see the value of adding the
> > > > number of non-contiguous planes. For multiplanar non-contiguous formats
> > > > the number of planes is equal to the number of planes.
> > > 
> > > Hmm, that's true. Choose whatever gives you the shortest code :-)
> > > 
> > > > Although maybe it will be clear this way for readers?
> > > > 
> > > > > As an aside: perhaps we should start calling the 'multiplanar API' the
> > > > > 'multiple non-contiguous planes API', at least in the documentation. It's the
> > 
> > To me, "multiple non-contiguous planes API" would suggest that the
> > planes themselves are non-contiguous.
> > 
> > Many drivers (especially Samsung ones) have a distinction between
> > "color planes" and "memory planes" internally, so maybe "Multiple
> > memory planes API" could make sense?
> 
> Huh, that's an idea. So _MPLANE should have been _MMPLANE?
> 
> > > > > first time that I found a description that actually covers the real meaning.
> > > > > 
> > > > 
> > > > Yes, indeed. In fact, my first version of this code had something like
> > > > "is_noncontiguous" instead of the "multiplanar" field.
> > > 
> > > I'm fine with that. Add a comment after it like: /* aka multiplanar */
> > > 
> > 
> > FWIW, some of the drivers have .num_cplanes and .num_mplanes in their
> > format descriptors.
> 
> 

This sounds nice.

To be clear, this means that instead of a boolean is_noncontigous, we describe
the number of (non-contiguous) memory planes and the number of component planes.

I really dislike the name "color planes", because it could be misleading
as chroma planes, so I think "component planes" makes it more clear.

Thanks for reviewing!
Eze