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