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

[Daniel Stone]

Hi,

On 14 December 2016 at 15:59, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Fri,  9 Dec 2016 19:57:16 +0000
> Daniel Stone <daniels at collabora.com> wrote:
>> +     {
>> +             .format = DRM_FORMAT_RGBX4444,
>> +             .gl_format = GL_RGBA,
>> +             .gl_type = GL_UNSIGNED_SHORT_4_4_4_4,
>
> Could there be any concern about sampling garbage as alpha?
> Should we have a flag 'ignore_alpha'?

I could add an explicit .you_must_ignore_alpha_no_im_serious = 1 to
opaque formats, but given that there's already a helper to query
whether or not the format is opaque, it seems a bit overkill.

>> +     {
>> +             .format = DRM_FORMAT_BGRX4444,
>> +             .gl_format = GL_BGRA_EXT,
>> +             .gl_type = GL_UNSIGNED_SHORT_4_4_4_4,
>> +     },
>> +     {
>> +             .format = DRM_FORMAT_BGRA4444,
>> +             .opaque_substitute = DRM_FORMAT_BGRX4444,
>> +             .gl_format = GL_BGRA_EXT,
>> +             .gl_type = GL_UNSIGNED_SHORT_4_4_4_4,
>> +     },

Turns out that BGRA_EXT is only considered valid for UNSIGNED_BYTE, so
I've axed all users of BGRA_EXT except for ARGB8888/XRGB8888.

>> +     {
>> +             .format = DRM_FORMAT_BGR888,
>> +             .gl_type = GL_RGB,
>> +             .gl_format = GL_UNSIGNED_BYTE,
>
> How do the 24-bpp formats actually work? Do we really imagine there is
> a 24-bit word and then address the bits of that?
>
> Yes, I'm thinking about big-endian.

Not a word as such. To the very best of my understanding,
UNSIGNED_BYTE loads each component in left-to-right order (in this
case, R then G then B) one byte at a time, starting from lower to
higher memory address. IOW, GL_RGB + GL_UNSIGNED_BYTE will always,
regardless of endianness, load DRM_FORMAT_BGR888 as defined to be
little-endian.

>> +     {
>> +             .format = DRM_FORMAT_XRGB8888,
>> +             .depth = 24,
>> +             .bpp = 32,
>> +             .gl_format = GL_BGRA_EXT,
>> +             .gl_type = GL_UNSIGNED_BYTE,
>
> GL info incorrect for big-endian.

As above, I don't believe that to be the case.

>> +     },
>> +     {
>> +             .format = DRM_FORMAT_ARGB8888,
>> +             .opaque_substitute = DRM_FORMAT_XRGB8888,
>> +             .depth = 32,
>> +             .bpp = 32,
>> +             .gl_format = GL_BGRA_EXT,
>> +             .gl_type = GL_UNSIGNED_BYTE,
>
> GL info incorrect for big-endian.
>
> Well, yeah, you know.
>
> < daniels> happy to drop a #if __BYTE_ORDER__ == BIG_ENDIAN #error
>            reverse literally every single one of these #endif in there
>
> Please do. :-)

I went with a different approach; see below. (Also note that
gl-renderer is absolutely broken _today_ if this is the case, since it
maps ARGB8888 -> GL_BGRA_EXT + GL_UNSIGNED_BYTE without regard to
endianness. But I don't think it is, as above.)

>> +     {
>> +             .format = DRM_FORMAT_ARGB2101010,
>> +             .opaque_substitute = DRM_FORMAT_XRGB2101010,
>> +             .gl_type = GL_BGRA_EXT,
>> +             .gl_format = GL_UNSIGNED_INT_2_10_10_10_REV_EXT,
>
> I suspect the GL format is not right...
>
> "The elements in a reversed packed pixel are ordered such that the
> first element is in the least-significant bits, ..."
>
> So, B would be the 2 LSB, G the next higher 10 bits, etc.
>
> The DRM format says A is the 2 MSB, right? That would make
> GL_UNSIGNED_INT_2_10_10_10_EXT for the bits, and GL_ARGB for the order
> (which might not exist?).
>
> Or, GL_UNSIGNED_INT_10_10_10_2_REV (does this exist?) and GL_BGRA_EXT.

Mind you, BGRA_EXT can't be used for anything other than
UNSIGNED_BYTE, so I just axed the GL equivalence for all the 10bpc
formats as there is none. You're right that the format definition was
confused even if this were the case though.

>> +     {
>> +             .format = DRM_FORMAT_YUYV,
>> +             .sampler_type = EGL_TEXTURE_Y_XUXV_WL,
>> +             .num_planes = 1,
>> +             .hsub = 2,
>
> Should chroma_order and luma_chroma_order be set? I suppose 0 happens
> to be a right value and it gets written implicitly.

Exactly.

>> +     },
>> +     {
>> +             .format = DRM_FORMAT_YVYU,
>> +             .sampler_type = EGL_TEXTURE_Y_XUXV_WL,
>> +             .num_planes = 1,
>> +             .chroma_order = ORDER_VU,
>> +             .hsub = 2,
>
> Should these entries have also all the information we now have in the
> yuv_formats table in gl-renderer.c?

What in particular do you feel it's missing? Per-plane subsampling values?

>> +WL_EXPORT bool
>> +pixel_format_is_opaque(const struct pixel_format_info *info)
>> +{
>> +     return !!info->opaque_substitute;
>
> Um, if there is an opaque substitute, you say the original format is
> opaque?
> Shouldn't it be the opposite? And even then I'm not sure it's accurate.

You're right that it's currently inverted, but when fixed, this only
returns incorrect information for AYUV (IIRC). But AYUV isn't actually
supported, so ...

>> +WL_EXPORT const struct pixel_format_info *
>> +pixel_format_get_opaque_substitute(const struct pixel_format_info *info)
>> +{
>> +     if (!info->opaque_substitute)
>> +             return info;
>
> What does this help? Intuitively I'd return NULL if there is no opaque
> substitute (and the format is not opaque itself?).

If we've already determined through the surface's opaque region that
we can address the entire buffer as opaque, then we would have to have
something like:
    if (surface_is_completely_opaque(surface)) {
        if (pixel_format_is_opaque(format))
            format = pixel_format_get_opaque_substitute(format);
        [...]
    }

This simplifies that slightly.

>> +/**
>> + * Contains information about pixel formats, mapping format codes from
>> + * wl_shm and drm_fourcc.h (which are deliberately identical) into various
>
> Except for WL_SHM_ARGB8888 and WL_SHM_XRGB8888. \o/

Ugh, that again.

>> +     /** GL format, if data can be natively/directly uploaded. Note that
>> +      *  whilst DRM formats are little-endian unless explicitly specified,
>> +      *  (i.e. DRM_FORMAT_ARGB8888 is stored BGRA as sequential bytes in
>> +      *  memory), GL uses the sequential byte order, so that format maps to
>> +      *  GL_BGRA_EXT plus GL_UNSIGNED_BYTE. To add to the confusion, the
>> +      *  explicitly-sized types (e.g. GL_UNSIGNED_SHORT_5_5_5_1) read in
>> +      *  this big-endian order, so for these types, the GL format descriptor
>> +      *  matches the order in the DRM format. */
>
> Do mention the fun on big-endian. ;-)

Explicitly? What should I add?

>> +     /** Horizontal subsampling; if non-zero, divide the height by this
>
> Should be vertical.

Yep.

> Sure there isn't something we could simply steal from?

There's drm_fourcc.c in the kernel, but that didn't include anything
about how YUV formats are organised, which is pretty important for us
if we want to convert. It also lacks any indication of the translation
to GL (useful for gl-renderer), or drmModeAddFB (not AddFB2) formats.
Given that, I just went with something separate.

> Btw. the functions are missing docs. Good that the struct has it already.

I tried to make them simple enough that any documentation would be
insulting, but fair enough. Will add for v2.

Cheers,
Daniel