[PATCH weston v9 01/62] libweston: Add pixel-format helpers

[Emil Velikov]

On 10 March 2017 at 15:28, Daniel Stone <daniel at fooishbar.org> wrote:
> Hey Emil,
>
> On 9 March 2017 at 22:57, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> So was feeling a bit bored and decided to remind myself about all the
>> crazy formats out there :-)
>>
>> I believe you've got a couple of mistakes, plus there's a couple of
>> questions/ideas below.
>> Note that handful of those are inspired by now format info is stored in DRM.
>
> Great, thanks for taking a look!
>
>> On 3 March 2017 at 23:05, Daniel Stone <daniels at collabora.com> wrote:
>>> --- /dev/null
>>> +++ b/libweston/pixel-formats.c
>>
>>
>>  +#include <drm/drm_fourcc.h>
>> Err... please don't.
>> This will include the kernel copy of the header while we want to use
>> the libdrm one -> <drm_fourcc.h>
>
> Good point indeed.
>
>>> +static const struct pixel_format_info pixel_format_table[] = {
>>> +       {
>>> +               .format = DRM_FORMAT_VYUY,
>>> +               .sampler_type = EGL_TEXTURE_Y_XUXV_WL,
>>> +               .num_planes = 1,
>>> +               .luma_chroma_order = ORDER_CHROMA_LUMA,
>>> +               .chroma_order = ORDER_VU,
>> All of these four should all have hsub = 2, afaict. The only
>> difference between them is the order in which the components are
>> stored.
>
> Right you are, I think.
>
>>> +       },
>>> +       {
>>> +               /* our one format with an alpha channel and no opaque equiv;
>>> +                * also cannot be trivially converted to RGB without writing
>>> +                * a new shader in gl-renderer */
>>> +               .format = DRM_FORMAT_AYUV,
>>> +               .sampler_type = EGL_TEXTURE_Y_XUXV_WL,
>>> +               .num_planes = 1,
>>> +       },
>> According to WL_bind_wayland_display and the i965/i915 drivers
>> (afaict) above five should have two planes. Then again, the FOURCC and
>> DRM formats are pretty clear that we're talking about a packed format
>> in a single plain... Did you test these, do we have a spec bug, is the
>> i915/i965 iimplementation working/been tested ?
>
> No, they're all right, in their own way. ;) In terms of storage, the
> buffers are stored purely in one plane, containing (bytewise)
> YUYV/etc, as encoded in the FourCC. However, to use it from GL, you
> need to bind the one plane to two samplers: one for luma and one for
> chroma. Given that the luma and chroma have different subsampling
> values, you can't bind them to the same sampler. So I think this is
> correct here.
>
Right - I was confused there by the name "num_planes". Worth changing
to num_of_samplers ?

>> Seems like DRM_FORMAT_AYUV is going to act strange with the
>> pixel_format_is_opaque() helper. As-in latter will return true,
>> despite the format has alpha.
>
> Indeed. I'd be tempted to just delete AYUV tbh.
>
Emmanuel mentioned that some userspace (mpv iirc) uses AYUV. Not sure
if/who much that's applicable here.
Worst case - one can add it as a follow-up.

>>> +       {
>>> +               .format = DRM_FORMAT_NV16,
>>> +               .sampler_type = EGL_TEXTURE_Y_UV_WL,
>>> +               .num_planes = 2,
>>> +               .hsub = 2,
>>> +               .vsub = 1,
>>> +       },
>>> +       {
>>> +               .format = DRM_FORMAT_NV61,
>>> +               .sampler_type = EGL_TEXTURE_Y_UV_WL,
>>> +               .num_planes = 2,
>>> +               .chroma_order = ORDER_VU,
>>> +               .hsub = 2,
>>> +               .vsub = 1,
>>> +       },
>> Might be the only person, who finds the mixed use of 0 and 1 as
>> [hv]sub sampling confusing.
>> Strictly peaking 1x1 is equivalent no subsampling, hence why we have
>> 0's below (NV24/42). At the same time, for the single plain packed
>> formats we only specify the hsub, even though we should also mention
>> vsub [=1].
>>
>> For simplicity (?) the kernel provides 1, when there's no
>> {v,h}sub-sampling. Worth using same the approach here ?
>
> Yeah, consistency would probably be good here. I'll see what I can do.
>
>>> +struct pixel_format_info {
>>
>>> +       /** How the format should be sampled, expressed in terms of tokens
>>> +        *  from the EGL_WL_bind_wayland_display extension. If not set,
>>> +        *  assumed to be either RGB or RGBA, depending on whether or not
>>> +        *  the format contains an alpha channel. */
>>> +       uint32_t sampler_type;
>>> +
>> If we have special field say "is_rgb_format" one can correctly deduct
>> the sampler_type. I will also help with the "DRM_FORMAT_AYUV does not
>> have alpha?" question above.
>>  - RGB formats - has alpha - RGBA, RGB otherwise
>
> Yeah, that's good to have indeed.
>
>>  - YUV formats - 1/2/3 num_planes ->
>> EGL_TEXTURE_Y_XUXV_WL/EGL_TEXTURE_Y_XUXV_WL/EGL_TEXTURE_Y_XUXV_WL
>> respectively
>
> Well, you still need the chroma order to figure out if it's XUXV or
> XVXU; and did you mean to paste the same sampler type three times? ;)
Nope, they should read Y_U_V_WL, Y_UV_WL and Y_XUXV_WL.

> I get the point though. The idea was for the combination of
> ORDER_{LUMA_CHROMA,CHROMA_LUMA} and ORDER_{UV,VU}, as well as
> num_planes, to uniquely identify the sampler type.
>
No objection against the order - thinking that the num_sampler(s) can
imply the EGL_TEXTURE_FORMAT.

>> Considering DRM_IOCTL_MODE_ADDFB2 was introduced with kernel 3.3
>> (2012-03-18) do we really want to have the depth/bpp scruft ?
>> DRM_IOCTL_MODE_ADDFB supported only 5 formats from this table.
>
> Perhaps not, but I'd like to bin that in a follow-up patch to all
> these; doing it now would require removing features from the DRM
> backend which people may somehow still be using. Stranger things have
> happened. I'd be much more comfortable if I could do it standalone.
>
Good point. It's not a good idea to mix these.

> I'll send a respin of this next week.
>
Whenever really, I'll try to take a look at the rest in the next few days.

Thanks
Emil