On 2/20/19 7:53 AM, Tomasz Figa wrote: > On Thu, Feb 7, 2019 at 1:36 AM Hans Verkuil <hverkuil@xxxxxxxxx> 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. > > [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. > >>> >>>>> + * @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? 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. I think that makes sense. Good suggestion. Regards, Hans
