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

[Hans Verkuil <hverkuil@xs4all.nl>]

On 2/20/19 7:53 AM, Tomasz Figa wrote:
> 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.

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