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

[Emil Velikov]

Hi Daniel,

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.

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>


> +static const struct pixel_format_info pixel_format_table[] = {


> +               .format = DRM_FORMAT_YUYV,
> +               .sampler_type = EGL_TEXTURE_Y_XUXV_WL,
> +               .num_planes = 1,
> +               .hsub = 2,
> +       },
> +       {
> +               .format = DRM_FORMAT_YVYU,
> +               .sampler_type = EGL_TEXTURE_Y_XUXV_WL,
> +               .num_planes = 1,
> +               .chroma_order = ORDER_VU,
> +               .hsub = 2,
> +       },
> +       {
> +               .format = DRM_FORMAT_UYVY,
> +               .sampler_type = EGL_TEXTURE_Y_XUXV_WL,
> +               .num_planes = 1,
> +               .luma_chroma_order = ORDER_CHROMA_LUMA,
> +       },
> +       {
> +               .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.

> +       },
> +       {
> +               /* 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 ?

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.


> +       {
> +               .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 ?

> +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
 - YUV formats - 1/2/3 num_planes ->
EGL_TEXTURE_Y_XUXV_WL/EGL_TEXTURE_Y_XUXV_WL/EGL_TEXTURE_Y_XUXV_WL
respectively


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.

Thanks
Emil