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

[Tomasz Figa <tfiga@chromium.org>]

On Thu, Feb 7, 2019 at 1:36 AM Hans Verkuil <hverkuil@xs4all.nl> 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.

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

> >
> >>> + * @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.

> >>> + * @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?

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

Best regards,
Tomasz